Re: [Xen-devel] Re: Paravirtualizing bits of acpi access

[Jeremy Fitzhardinge <jeremy@xxxxxxxx>]

Tian, Kevin wrote:
> The reason why those useless code are prevented, is in case of
> clobberring 0-1M area which if contains valid Xen content. in
> acpi_save_state_mem, dom0's wakeup code is copied to area
> allocated in acpi_reserve_bootmem. If every Xen's usage on 0-1M
> is based on copy-on-use style, such as trampoline code for AP
> boot, it's ok. But I'm not sure whether Xen puts some persistent
> content there. IIRC, that boot time allocation usually returns sth
> like 0x90000 since wakeup stub is first run in real mode. But if
> for dom0 alloc_bootmem_low never gives <1M page, then this
> prevention could be skipped as your thought.
>   
Yes, but the memory allocated is only in 0-1M in dom0's pseudo-physical 
space, not in Xen's machine space.  It allocates the memory and does a 
virt_to_phys on it, but there's no special effort to remap it as mfns or 
anything.  There should be no possible conflict with Xen's use of the 
real 0-1M region.
Not that I've actually tried to execute that patch yet, so I could well 
be overlooking something...
   J

>   
> > The result is below.  Does this look reasonable?
> >
> > Thanks,
> >    J
> >
> > diff --git a/arch/x86/kernel/acpi/sleep.c 
> > b/arch/x86/kernel/acpi/sleep.c
> > index 7c243a2..8ebda00 100644
> > --- a/arch/x86/kernel/acpi/sleep.c
> > +++ b/arch/x86/kernel/acpi/sleep.c
> > @@ -12,6 +12,9 @@
> > #include <asm/segment.h>
> > #include <asm/desc.h>
> >
> > +#include <xen/acpi.h>
> > +#include <asm/xen/hypervisor.h>
> > +
> > #include "realmode/wakeup.h"
> > #include "sleep.h"
> >
> > @@ -25,6 +28,34 @@ static unsigned long acpi_realmode;
> > static char temp_stack[4096];
> > #endif
> >
> > +/* 
> > + * Override final register write when entering sleep state so we can
> > + * direct it to a hypercall in the Xen case.
> > + */
> > +acpi_status arch_acpi_enter_sleep_state(u8 sleep_state, u32
> > +                                       PM1Acontrol, u32 PM1Bcontrol)
> > +{
> > +       acpi_status ret;
> > +
> > +       if (xen_pv_acpi()) {
> > +               int err;
> > +
> > +               err = acpi_notify_hypervisor_state(sleep_state,
> > +						   PM1Acontrol, 
> > PM1Bcontrol);
> > +
> > +               ret = AE_OK;
> > +               if (err) {
> > +                       ACPI_DEBUG_PRINT((ACPI_DB_INIT,
> > +					  "Hypervisor failure 
> > [%d]\n", err));
> > +                       ret = AE_ERROR;
> > +               }
> > +       } else
> > +               ret = default_acpi_enter_sleep_state(sleep_state,
> > +						     
> > PM1Acontrol, PM1Bcontrol);
> > +
> > +       return ret;
> > +}
> > +
> > /**
> >  * acpi_save_state_mem - save kernel state
> >  *
> > diff --git a/drivers/acpi/acpica/hwsleep.c 
> > b/drivers/acpi/acpica/hwsleep.c
> > index a2af2a4..6196a7e 100644
> > --- a/drivers/acpi/acpica/hwsleep.c
> > +++ b/drivers/acpi/acpica/hwsleep.c
> > @@ -209,6 +209,83 @@ acpi_status 
> > acpi_enter_sleep_state_prep(u8 sleep_state)
> > ACPI_EXPORT_SYMBOL(acpi_enter_sleep_state_prep)
> >
> > +/*
> > + * Default implementation of final register write to enter sleep
> > + * state, available for architecture versions to call if necessary.
> > + */
> > +acpi_status default_acpi_enter_sleep_state(u8 sleep_state, u32
> > +                                          PM1Acontrol, u32 PM1Bcontrol)
> > +{
> > +       acpi_status status;
> > +       u32 in_value;
> > +
> > +       status = acpi_hw_register_write(ACPI_REGISTER_PM1A_CONTROL,
> > +                                       PM1Acontrol);
> > +       if (ACPI_FAILURE(status)) {
> > +               return_ACPI_STATUS(status);
> > +       }
> > +
> > +       status = acpi_hw_register_write(ACPI_REGISTER_PM1B_CONTROL,
> > +                                       PM1Bcontrol);
> > +
> > +       if (ACPI_FAILURE(status)) {
> > +               return_ACPI_STATUS(status);
> > +       }
> > +
> > +       if (sleep_state > ACPI_STATE_S3) {
> > +               struct acpi_bit_register_info *sleep_enable_reg_info;
> > +
> > +               /*
> > +		 * We wanted to sleep > S3, but it didn't 
> > happen (by virtue of the
> > +                * fact that we are still executing!)
> > +                *
> > +		 * Wait ten seconds, then try again. This is to 
> > get S4/S5 to work on
> > +                * all machines.
> > +                *
> > +		 * We wait so long to allow chipsets that poll 
> > this reg very slowly to
> > +		 * still read the right value. Ideally, this 
> > block would go
> > +                * away entirely.
> > +                */
> > +               acpi_os_stall(10000000);
> > +
> > +               sleep_enable_reg_info =
> > +                       
> > acpi_hw_get_bit_register_info(ACPI_BITREG_SLEEP_ENABLE);
> > +
> > +		status = 
> > acpi_hw_register_write(ACPI_REGISTER_PM1_CONTROL,
> > +                                               sleep_enable_reg_info->
> > +                                               access_bit_mask);
> > +               if (ACPI_FAILURE(status)) {
> > +                       return_ACPI_STATUS(status);
> > +               }
> > +       }
> > +
> > +       /* Wait until we enter sleep state */
> > +
> > +       do {
> > +		status = 
> > acpi_get_register_unlocked(ACPI_BITREG_WAKE_STATUS,
> > +                                                   &in_value);
> > +               if (ACPI_FAILURE(status)) {
> > +                       return_ACPI_STATUS(status);
> > +               }
> > +
> > +               /* Spin until we wake */
> > +
> > +       } while (!in_value);
> > +
> > +       return_ACPI_STATUS(AE_OK);
> > +}
> > +
> > +/* 
> > + * Weak version of final write to enter sleep state, so that
> > + * architectures can override it.
> > + */
> > +acpi_status __weak arch_acpi_enter_sleep_state(u8 sleep_state,
> > +					    u32 PM1Acontrol, 
> > u32 PM1Bcontrol)
> > +{
> > +       return default_acpi_enter_sleep_state(sleep_state,
> > +                                             PM1Acontrol, PM1Bcontrol);
> > +}
> > +
> >
> > /**************************************************************
> > *****************
> >  *
> >  * FUNCTION:    acpi_enter_sleep_state
> > @@ -227,7 +304,6 @@ acpi_status asmlinkage 
> > acpi_enter_sleep_state(u8 sleep_state)
> >         u32 PM1Bcontrol;
> >         struct acpi_bit_register_info *sleep_type_reg_info;
> >         struct acpi_bit_register_info *sleep_enable_reg_info;
> > -       u32 in_value;
> >         struct acpi_object_list arg_list;
> >         union acpi_object arg;
> >         acpi_status status;
> > @@ -337,54 +413,7 @@ acpi_status asmlinkage 
> > acpi_enter_sleep_state(u8 sleep_state)
> >         ACPI_FLUSH_CPU_CACHE();
> >
> > -       status = acpi_hw_register_write(ACPI_REGISTER_PM1A_CONTROL,
> > -                                       PM1Acontrol);
> > -       if (ACPI_FAILURE(status)) {
> > -               return_ACPI_STATUS(status);
> > -       }
> > -
> > -       status = acpi_hw_register_write(ACPI_REGISTER_PM1B_CONTROL,
> > -                                       PM1Bcontrol);
> > -       if (ACPI_FAILURE(status)) {
> > -               return_ACPI_STATUS(status);
> > -       }
> > -
> > -       if (sleep_state > ACPI_STATE_S3) {
> > -               /*
> > -		 * We wanted to sleep > S3, but it didn't 
> > happen (by virtue of the
> > -                * fact that we are still executing!)
> > -                *
> > -		 * Wait ten seconds, then try again. This is to 
> > get S4/S5 to work on
> > -                * all machines.
> > -                *
> > -		 * We wait so long to allow chipsets that poll 
> > this reg very slowly to
> > -		 * still read the right value. Ideally, this 
> > block would go
> > -                * away entirely.
> > -                */
> > -               acpi_os_stall(10000000);
> > -
> > -		status = 
> > acpi_hw_register_write(ACPI_REGISTER_PM1_CONTROL,
> > -                                               sleep_enable_reg_info->
> > -                                               access_bit_mask);
> > -               if (ACPI_FAILURE(status)) {
> > -                       return_ACPI_STATUS(status);
> > -               }
> > -       }
> > -
> > -       /* Wait until we enter sleep state */
> > -
> > -       do {
> > -		status = 
> > acpi_get_register_unlocked(ACPI_BITREG_WAKE_STATUS,
> > -                                                   &in_value);
> > -               if (ACPI_FAILURE(status)) {
> > -                       return_ACPI_STATUS(status);
> > -               }
> > -
> > -               /* Spin until we wake */
> > -
> > -       } while (!in_value);
> > -
> > -       return_ACPI_STATUS(AE_OK);
> > +	return arch_acpi_enter_sleep_state(sleep_state, 
> > PM1Acontrol, PM1Bcontrol);
> > }
> >
> > ACPI_EXPORT_SYMBOL(acpi_enter_sleep_state)
> > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> > index 5192666..bd1cc1a 100644
> > --- a/drivers/acpi/sleep.c
> > +++ b/drivers/acpi/sleep.c
> > @@ -19,6 +19,8 @@
> >
> > #include <asm/io.h>
> >
> > +#include <xen/acpi.h>
> > +
> > #include <acpi/acpi_bus.h>
> > #include <acpi/acpi_drivers.h>
> > #include "sleep.h"
> > @@ -209,6 +211,21 @@ static int 
> > acpi_suspend_begin(suspend_state_t pm_state)
> >         return error;
> > }
> >
> > +static void do_suspend(void)
> > +{
> > +       if (!xen_pv_acpi()) {
> > +               do_suspend_lowlevel();
> > +               return;
> > +       }
> > +
> > +       /*
> > +        * Xen will save and restore CPU context, so
> > +        * we can skip that and just go straight to
> > +        * the suspend.
> > +        */
> > +       acpi_enter_sleep_state(ACPI_STATE_S3);
> > +}
> > +
> > /**
> >  *      acpi_suspend_enter - Actually enter a sleep state.
> >  *      @pm_state: ignored
> > @@ -242,7 +259,7 @@ static int 
> > acpi_suspend_enter(suspend_state_t pm_state)
> >                 break;
> >
> >         case ACPI_STATE_S3:
> > -               do_suspend_lowlevel();
> > +               do_suspend();
> >                 break;
> >         }
> >
> > diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> > index 51cbaa5..0138113 100644
> > --- a/drivers/xen/Kconfig
> > +++ b/drivers/xen/Kconfig
> > @@ -76,3 +76,7 @@ config XEN_COMPAT_XENFS
> >
> > config XEN_XENBUS_FRONTEND
> >        tristate
> > +
> > +config XEN_S3
> > +       def_bool y
> > +       depends on XEN_DOM0 && ACPI
> > \ No newline at end of file
> > diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> > index bb88673..4b01fc8 100644
> > --- a/drivers/xen/Makefile
> > +++ b/drivers/xen/Makefile
> > @@ -7,4 +7,6 @@ obj-$(CONFIG_XEN_BALLOON)               += balloon.o
> > obj-$(CONFIG_XEN_DEV_EVTCHN)    += evtchn.o
> > obj-$(CONFIG_XEN_BLKDEV_BACKEND)        += blkback/
> > obj-$(CONFIG_XEN_NETDEV_BACKEND)        += netback/
> > -obj-$(CONFIG_XENFS)                    += xenfs/
> > \ No newline at end of file
> > +obj-$(CONFIG_XENFS)                    += xenfs/
> > +
> > +obj-$(CONFIG_XEN_S3)                   += acpi.o
> > \ No newline at end of file
> > diff --git a/drivers/xen/acpi.c b/drivers/xen/acpi.c
> > new file mode 100644
> > index 0000000..e6d3d0e
> > --- /dev/null
> > +++ b/drivers/xen/acpi.c
> > @@ -0,0 +1,23 @@
> > +#include <xen/acpi.h>
> > +
> > +#include <xen/interface/platform.h>
> > +#include <asm/xen/hypercall.h>
> > +#include <asm/xen/hypervisor.h>
> > +
> > +int acpi_notify_hypervisor_state(u8 sleep_state,
> > +                                u32 pm1a_cnt, u32 pm1b_cnt)
> > +{
> > +       struct xen_platform_op op = {
> > +               .cmd = XENPF_enter_acpi_sleep,
> > +               .interface_version = XENPF_INTERFACE_VERSION,
> > +               .u = {
> > +                       .enter_acpi_sleep = {
> > +                               .pm1a_cnt_val = (u16)pm1a_cnt,
> > +                               .pm1b_cnt_val = (u16)pm1b_cnt,
> > +                               .sleep_state = sleep_state,
> > +                       },
> > +               },
> > +       };
> > +
> > +       return HYPERVISOR_dom0_op(&op);
> > +}
> > diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> > index cc40102..f39f396 100644
> > --- a/include/acpi/acpixf.h
> > +++ b/include/acpi/acpixf.h
> > @@ -372,6 +372,12 @@ acpi_status 
> > acpi_enter_sleep_state_prep(u8 sleep_state);
> > acpi_status asmlinkage acpi_enter_sleep_state(u8 sleep_state);
> >
> > +acpi_status __weak arch_acpi_enter_sleep_state(u8 sleep_state,
> > +					       u32 PM1Acontrol, 
> > u32 PM1Bcontrol);
> > +
> > +acpi_status default_acpi_enter_sleep_state(u8 sleep_state,
> > +					   u32 PM1Acontrol, u32 
> > PM1Bcontrol);
> > +
> > acpi_status asmlinkage acpi_enter_sleep_state_s4bios(void);
> >
> > acpi_status acpi_leave_sleep_state_prep(u8 sleep_state);
> > diff --git a/include/xen/acpi.h b/include/xen/acpi.h
> > new file mode 100644
> > index 0000000..fea4cfb
> > --- /dev/null
> > +++ b/include/xen/acpi.h
> > @@ -0,0 +1,23 @@
> > +#ifndef _XEN_ACPI_H
> > +#define _XEN_ACPI_H
> > +
> > +#include <linux/types.h>
> > +
> > +#ifdef CONFIG_XEN_S3
> > +#include <asm/xen/hypervisor.h>
> > +
> > +static inline bool xen_pv_acpi(void)
> > +{
> > +       return xen_pv_domain();
> > +}
> > +#else
> > +static inline bool xen_pv_acpi(void)
> > +{
> > +       return false;
> > +}
> > +#endif
> > +
> > +int acpi_notify_hypervisor_state(u8 sleep_state,
> > +                                u32 pm1a_cnt, u32 pm1b_cnd);
> > +
> > +#endif /* _XEN_ACPI_H */
> >
> >
> >     
> >
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel