Message ID | 4F65016F6CB04E49BFFA15D4F7B798D9944E2524@orsmsx506.amr.corp.intel.com (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers | show |
> > The two changes this patch makes are, unfortunately, unavoidable: > > > > 1. Turn the final "enter sleep" into a hypercall, so that Xen can do > > all the low-level context save/restore. The normal baremetal case > > is still localized in acpica/hwsleep.c, but it (may) make an > > excursion into arch code to see if it should do something else, and > > 2. Directly enter the sleep state, rather than save cpu context > > (which Xen does) > > > > Though, come to think of it, perhaps there's no harm in letting the > > kernel do its own state-saving. I'll check. Certainly the less different than bare metal you can be, the better. The last thing we need is to complicate something that in Linux has only been sort of working for > diff -r 855cb34ca992 drivers/acpi/acpica/hwsleep.c > --- a/drivers/acpi/acpica/hwsleep.c Tue Mar 17 19:53:17 2009 -0400 > +++ b/drivers/acpi/acpica/hwsleep.c Tue Mar 24 09:37:22 2009 -0700 > @@ -45,6 +45,7 @@ > #include <acpi/acpi.h> > #include "accommon.h" > #include "actables.h" > +#include <asm/tboot.h> > > #define _COMPONENT ACPI_HARDWARE > ACPI_MODULE_NAME("hwsleep") > @@ -332,6 +333,39 @@ acpi_status asmlinkage acpi_enter_sleep_ > > PM1Acontrol |= sleep_enable_reg_info->access_bit_mask; > PM1Bcontrol |= sleep_enable_reg_info->access_bit_mask; > + > +#ifdef CONFIG_TXT > +#define TB_COPY_GAS(tbg, g) \ > + tbg.space_id = g.space_id; \ > + tbg.bit_width = g.bit_width; \ > + tbg.bit_offset = g.bit_offset; \ > + tbg.access_width = g.access_width; \ > + tbg.address = g.address; > + > + if (tboot_in_measured_env()) { > + TB_COPY_GAS(tboot_shared->acpi_sinfo.pm1a_cnt_blk, > + acpi_gbl_FADT.xpm1a_control_block); > + TB_COPY_GAS(tboot_shared->acpi_sinfo.pm1b_cnt_blk, > + acpi_gbl_FADT.xpm1b_control_block); Who'd a thunk that suddently everybody would want to scribble on acpi_enter_sleep_state()? Note that acpica/hwsleep.c is a file from ACPICA that we share with BSD etc. Yes, we manage local changes in Linux, but we try to reduce them to zero over time, else we create a big maintenace headache. perhaps tboot_in_measured_env() could compile in as 0 for !CONFIG_TXT and you can get rid of the #ifdefs? Jeremy, I'm not excited about a proposed change to acpixf.h -- this is the API to ACPICA... thanks, -Len Brown, Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Len Brown wrote: >> diff -r 855cb34ca992 drivers/acpi/acpica/hwsleep.c >> --- a/drivers/acpi/acpica/hwsleep.c Tue Mar 17 19:53:17 2009 -0400 >> +++ b/drivers/acpi/acpica/hwsleep.c Tue Mar 24 09:37:22 2009 -0700 >> @@ -45,6 +45,7 @@ >> #include <acpi/acpi.h> >> #include "accommon.h" >> #include "actables.h" >> +#include <asm/tboot.h> >> >> #define _COMPONENT ACPI_HARDWARE >> ACPI_MODULE_NAME("hwsleep") >> @@ -332,6 +333,39 @@ acpi_status asmlinkage acpi_enter_sleep_ >> >> PM1Acontrol |= sleep_enable_reg_info->access_bit_mask; >> PM1Bcontrol |= sleep_enable_reg_info->access_bit_mask; >> + >> +#ifdef CONFIG_TXT >> +#define TB_COPY_GAS(tbg, g) \ >> + tbg.space_id = g.space_id; \ >> + tbg.bit_width = g.bit_width; \ >> + tbg.bit_offset = g.bit_offset; \ >> + tbg.access_width = g.access_width; \ >> + tbg.address = g.address; >> + >> + if (tboot_in_measured_env()) { >> + TB_COPY_GAS(tboot_shared->acpi_sinfo.pm1a_cnt_blk, >> + acpi_gbl_FADT.xpm1a_control_block); >> + TB_COPY_GAS(tboot_shared->acpi_sinfo.pm1b_cnt_blk, >> + acpi_gbl_FADT.xpm1b_control_block); >> > > Who'd a thunk that suddently everybody would want to scribble > on acpi_enter_sleep_state()? > > Note that acpica/hwsleep.c is a file from ACPICA that we share > with BSD etc. Yes, we manage local changes in Linux, but we > try to reduce them to zero over time, else we create a big > maintenace headache. > > perhaps tboot_in_measured_env() could compile in as 0 > for !CONFIG_TXT and you can get rid of the #ifdefs? > > Jeremy, I'm not excited about a proposed change to acpixf.h -- > this is the API to ACPICA... > Do you have an issue with the mechanism (using weak function, etc), or just the placement of the prototypes in that header? Would there be a better header to put them in? Or would you prefer some other mechanism? It certainly seems like Xen and tboot should be able to share the same hook, given that they're doing similar things for similar reasons. (I don't really understand the structure of all the acpi stuff; I'm just wading in and making a mess of things until I can close the lid of laptop successfully.) J -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Jeremy, I'm not excited about a proposed change to acpixf.h -- > > this is the API to ACPICA... > > > Do you have an issue with the mechanism (using weak function, etc), or just > the placement of the prototypes in that header? Would there be a better > header to put them in? Or would you prefer some other mechanism? > > It certainly seems like Xen and tboot should be able to share the same hook, > given that they're doing similar things for similar reasons. > > (I don't really understand the structure of all the acpi stuff; I'm just > wading in and making a mess of things until I can close the lid of laptop > successfully.) Everything in acpi/acpica/ is source code that we share with BSD via the ACPICA project http://acpica.org/ ACPICA also supplies a couple of the headers outside that directory, eg. acpixf.h, which is the API between the kernel and ACPICA. We can, and do, change that API when it makes sense. However, we want to think carefully before changing it, for we either cause Linux to diverge, or we have to sell the same change to several other operating systems. So ideally we wouuld need to make no Linux-specific, or Xen-specific changes in that directory. One possibility is to have this called via function pointer from ASM and scribble over the default function pointer for the Xen case. In that case, the Xen version of the routine would live someplace other than acpi/acpica/ - someplace with the word xen in the path. If using _weak can effectively do that at link time, then fine, if we can do it w/o changing the API -- a _weak annotation is an easy ACPICA/Linux divergencen to manage. I don't know if Xen and TXT are exclusive, or if we'd ever need to handle both cases at the same time. I guess that will influence if the TXT and Xen use the same approach or something different. thanks, Len Brown, Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>From: Len Brown [mailto:lenb@kernel.org] >Sent: 2009年3月28日 9:02 >> > Jeremy, I'm not excited about a proposed change to acpixf.h -- >> > this is the API to ACPICA... >> > >> Do you have an issue with the mechanism (using weak >function, etc), or just >> the placement of the prototypes in that header? Would there >be a better >> header to put them in? Or would you prefer some other mechanism? >> >> It certainly seems like Xen and tboot should be able to >share the same hook, >> given that they're doing similar things for similar reasons. >> >> (I don't really understand the structure of all the acpi >stuff; I'm just >> wading in and making a mess of things until I can close the >lid of laptop >> successfully.) > >Everything in acpi/acpica/ is source code that we share with BSD >via the ACPICA project http://acpica.org/ > >ACPICA also supplies a couple of the headers outside that directory, >eg. acpixf.h, which is the API between the kernel and ACPICA. > >We can, and do, change that API when it makes sense. >However, we want to think carefully before changing it, >for we either cause Linux to diverge, or we have to sell >the same change to several other operating systems. >So ideally we wouuld need to make no Linux-specific, or Xen-specific >changes in that directory. > >One possibility is to have this called via >function pointer from ASM and scribble over the default >function pointer for the Xen case. In that case, the Xen >version of the routine would live someplace other than acpi/acpica/ - >someplace with the word xen in the path. If using _weak can >effectively >do that at link time, then fine, if we can do it w/o changing >the API -- >a _weak annotation is an easy ACPICA/Linux divergencen to manage. > >I don't know if Xen and TXT are exclusive, or if we'd ever need >to handle both cases at the same time. I guess that will influence >if the TXT and Xen use the same approach or something different. > When only Xen exists, S3 flow is: dom0 S3 -> Xen S3 When only TXT is enabled, it's: dom0 S3 -> TXT S3 When both Xen and TXT exist, TXT is not exposed to dom0 and thus the S3 flow is: dom0 S3 -> Xen S3 -> TXT S3 I.e, dom0 only needs to care one case at given time. Transition to TXT is only required if system software is the lowest level on top of hardware. Thanks Kevin
Len Brown wrote: >>> Jeremy, I'm not excited about a proposed change to acpixf.h -- >>> this is the API to ACPICA... >>> >>> >> Do you have an issue with the mechanism (using weak function, etc), or just >> the placement of the prototypes in that header? Would there be a better >> header to put them in? Or would you prefer some other mechanism? >> >> It certainly seems like Xen and tboot should be able to share the same hook, >> given that they're doing similar things for similar reasons. >> >> (I don't really understand the structure of all the acpi stuff; I'm just >> wading in and making a mess of things until I can close the lid of laptop >> successfully.) >> > > Everything in acpi/acpica/ is source code that we share with BSD > via the ACPICA project http://acpica.org/ > > ACPICA also supplies a couple of the headers outside that directory, > eg. acpixf.h, which is the API between the kernel and ACPICA. > > We can, and do, change that API when it makes sense. > However, we want to think carefully before changing it, > for we either cause Linux to diverge, or we have to sell > the same change to several other operating systems. > So ideally we wouuld need to make no Linux-specific, or Xen-specific > changes in that directory. > > One possibility is to have this called via > function pointer from ASM and scribble over the default > function pointer for the Xen case. In that case, the Xen > version of the routine would live someplace other than acpi/acpica/ - > someplace with the word xen in the path. Yes, that would be easy enough to do; we could overwrite it only when actually running under Xen. However, we don't need to replace the whole of acpi_enter_sleep_state(); there are two options: we could duplicate the early part of the function in the Xen version of it, or break just the differing part out via function pointer. The former has the disadvantage of duplicating code, but it does allow the same function pointer to be used by the tboot version. > If using _weak can effectively > do that at link time, then fine, if we can do it w/o changing the API -- > a _weak annotation is an easy ACPICA/Linux divergencen to manage. > The weak approach is what my proposed patch does: * the default native-hardware version of the sleep-entry register writes is broken out into default_acpi_enter_sleep_state() * it introduces a weak arch_acpi_enter_sleep_state() which just calls the default case, leaving the normal function unchanged * in arch/x86/kernel/acpi/sleep.c, it adds an override arch_acpi_enter_sleep_state(), which checks to see if we're running under Xen; if not, then it simply calls default_acpi_enter_sleep_state() as usual; if it does, it calls xen_acpi_enter_sleep_state() * xen_acpi_enter_sleep-state() is defined in arch/x86/xen/acpi.c (Actually it didn't break the Xen version out separately, but it easily could.) On the whole, using a function pointer would be a bit cleaner, as it removes the need for the weak glue functions, at the cost of some (possible) code duplication. > I don't know if Xen and TXT are exclusive, or if we'd ever need > to handle both cases at the same time. I guess that will influence > if the TXT and Xen use the same approach or something different. > As Kevin said, they're exclusive, so they could share the same function pointer. J -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 28 March 2009, Jeremy Fitzhardinge wrote: > Len Brown wrote: > >>> Jeremy, I'm not excited about a proposed change to acpixf.h -- > >>> this is the API to ACPICA... > >>> > >>> > >> Do you have an issue with the mechanism (using weak function, etc), or just > >> the placement of the prototypes in that header? Would there be a better > >> header to put them in? Or would you prefer some other mechanism? > >> > >> It certainly seems like Xen and tboot should be able to share the same hook, > >> given that they're doing similar things for similar reasons. > >> > >> (I don't really understand the structure of all the acpi stuff; I'm just > >> wading in and making a mess of things until I can close the lid of laptop > >> successfully.) > >> > > > > Everything in acpi/acpica/ is source code that we share with BSD > > via the ACPICA project http://acpica.org/ > > > > ACPICA also supplies a couple of the headers outside that directory, > > eg. acpixf.h, which is the API between the kernel and ACPICA. > > > > We can, and do, change that API when it makes sense. > > However, we want to think carefully before changing it, > > for we either cause Linux to diverge, or we have to sell > > the same change to several other operating systems. > > So ideally we wouuld need to make no Linux-specific, or Xen-specific > > changes in that directory. > > > > One possibility is to have this called via > > function pointer from ASM and scribble over the default > > function pointer for the Xen case. In that case, the Xen > > version of the routine would live someplace other than acpi/acpica/ - > > someplace with the word xen in the path. > > Yes, that would be easy enough to do; we could overwrite it only when > actually running under Xen. > > However, we don't need to replace the whole of acpi_enter_sleep_state(); > there are two options: we could duplicate the early part of the function > in the Xen version of it, or break just the differing part out via > function pointer. The former has the disadvantage of duplicating code, > but it does allow the same function pointer to be used by the tboot version. > > > If using _weak can effectively > > do that at link time, then fine, if we can do it w/o changing the API -- > > a _weak annotation is an easy ACPICA/Linux divergencen to manage. > > > > The weak approach is what my proposed patch does: > > * the default native-hardware version of the sleep-entry register > writes is broken out into default_acpi_enter_sleep_state() > * it introduces a weak arch_acpi_enter_sleep_state() which just > calls the default case, leaving the normal function unchanged > * in arch/x86/kernel/acpi/sleep.c, it adds an override > arch_acpi_enter_sleep_state(), which checks to see if we're > running under Xen; if not, then it simply calls > default_acpi_enter_sleep_state() as usual; if it does, it calls > xen_acpi_enter_sleep_state() > * xen_acpi_enter_sleep-state() is defined in arch/x86/xen/acpi.c > > (Actually it didn't break the Xen version out separately, but it easily > could.) > > On the whole, using a function pointer would be a bit cleaner, as it > removes the need for the weak glue functions, at the cost of some > (possible) code duplication. > > > I don't know if Xen and TXT are exclusive, or if we'd ever need > > to handle both cases at the same time. I guess that will influence > > if the TXT and Xen use the same approach or something different. > > > > As Kevin said, they're exclusive, so they could share the same function > pointer. FWIW, if you only care about suspen to RAM (S3). I'm still thinking that do_suspend_lowlevel() is the place to work on. After all acpi_enter_sleep_state() is called from there. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff -r 855cb34ca992 arch/x86/kernel/reboot.c --- a/arch/x86/kernel/reboot.c Tue Mar 17 19:53:17 2009 -0400 +++ b/arch/x86/kernel/reboot.c Tue Mar 24 09:37:22 2009 -0700 @@ -22,6 +22,8 @@ #else # include <asm/iommu.h> #endif + +#include <asm/tboot.h> #include <mach_ipi.h> @@ -436,6 +438,8 @@ static void native_machine_emergency_res if (reboot_emergency) emergency_vmx_disable_all(); + tboot_shutdown(TB_SHUTDOWN_REBOOT); + /* Tell the BIOS if we want cold or warm reboot */ *((unsigned short *)__va(0x472)) = reboot_mode; @@ -501,11 +505,19 @@ static void native_machine_emergency_res void native_machine_shutdown(void) { +#ifdef CONFIG_SMP + /* The boot cpu is always logical cpu 0 */ + int reboot_cpu_id = 0; +#endif + + /* TXT requires VMX to be off for all shutdowns */ + if (tboot_in_measured_env()) { + emergency_vmx_disable_all(); + local_irq_enable(); + } + /* Stop the cpus and apics */ #ifdef CONFIG_SMP - - /* The boot cpu is always logical cpu 0 */ - int reboot_cpu_id = 0; #ifdef CONFIG_X86_32 /* See if there has been given a command line override */ @@ -562,6 +574,8 @@ static void native_machine_halt(void) /* stop other cpus and apics */ machine_shutdown(); + tboot_shutdown(TB_SHUTDOWN_HALT); + /* stop this cpu */ stop_this_cpu(NULL); } @@ -573,6 +587,8 @@ static void native_machine_power_off(voi machine_shutdown(); pm_power_off(); } + /* a fallback in case there is no PM info available */ + tboot_shutdown(TB_SHUTDOWN_HALT); } struct machine_ops machine_ops = { diff -r 855cb34ca992 arch/x86/kernel/smpboot.c --- a/arch/x86/kernel/smpboot.c Tue Mar 17 19:53:17 2009 -0400 +++ b/arch/x86/kernel/smpboot.c Tue Mar 24 09:37:22 2009 -0700 @@ -63,6 +63,7 @@ #include <asm/vmi.h> #include <asm/genapic.h> #include <asm/setup.h> +#include <asm/tboot.h> #include <linux/mc146818rtc.h> #include <mach_apic.h> @@ -1436,7 +1437,10 @@ void native_play_dead(void) void native_play_dead(void) { play_dead_common(); - wbinvd_halt(); + if (tboot_in_measured_env()) + tboot_shutdown(TB_SHUTDOWN_WFS); + else + wbinvd_halt(); } #else /* ... !CONFIG_HOTPLUG_CPU */ diff -r 855cb34ca992 drivers/acpi/acpica/hwsleep.c --- a/drivers/acpi/acpica/hwsleep.c Tue Mar 17 19:53:17 2009 -0400 +++ b/drivers/acpi/acpica/hwsleep.c Tue Mar 24 09:37:22 2009 -0700 @@ -45,6 +45,7 @@ #include <acpi/acpi.h> #include "accommon.h" #include "actables.h" +#include <asm/tboot.h> #define _COMPONENT ACPI_HARDWARE ACPI_MODULE_NAME("hwsleep") @@ -332,6 +333,39 @@ acpi_status asmlinkage acpi_enter_sleep_ PM1Acontrol |= sleep_enable_reg_info->access_bit_mask; PM1Bcontrol |= sleep_enable_reg_info->access_bit_mask; + +#ifdef CONFIG_TXT +#define TB_COPY_GAS(tbg, g) \ + tbg.space_id = g.space_id; \ + tbg.bit_width = g.bit_width; \ + tbg.bit_offset = g.bit_offset; \ + tbg.access_width = g.access_width; \ + tbg.address = g.address; + + if (tboot_in_measured_env()) { + TB_COPY_GAS(tboot_shared->acpi_sinfo.pm1a_cnt_blk, + acpi_gbl_FADT.xpm1a_control_block); + TB_COPY_GAS(tboot_shared->acpi_sinfo.pm1b_cnt_blk, + acpi_gbl_FADT.xpm1b_control_block); + TB_COPY_GAS(tboot_shared->acpi_sinfo.pm1a_evt_blk, + acpi_gbl_FADT.xpm1a_event_block); + TB_COPY_GAS(tboot_shared->acpi_sinfo.pm1b_evt_blk, + acpi_gbl_FADT.xpm1b_event_block); + tboot_shared->acpi_sinfo.pm1a_cnt_val = PM1Acontrol; + tboot_shared->acpi_sinfo.pm1b_cnt_val = PM1Bcontrol; + /* we need phys addr of waking vector, but can't use + virt_to_phys() on &acpi_gbl_FACS because it is ioremap'ed, + so calc from FACS phys addr */ + tboot_shared->acpi_sinfo.wakeup_vector = acpi_gbl_FADT.facs + + ((void *)&acpi_gbl_FACS->firmware_waking_vector - + (void *)acpi_gbl_FACS); + tboot_shared->acpi_sinfo.vector_width = 32; + tboot_shared->acpi_sinfo.kernel_s3_resume_vector = + acpi_wakeup_address; + + tboot_sleep(sleep_state); + } +#endif /* Write #2: SLP_TYP + SLP_EN */