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 */
> >
> >
> >     
> >

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html