Wei, Gang
2010-Feb-25 05:49 UTC
[Xen-devel] [PATCH]ACPI: workaround for S3 fail in two facs tables case
ACPI: workaround for S3 fail in two facs tables case Some legacy BIOS which support ACPI2.0+ may expose two FACS tables via both FADT->FIRMWARE_CTRL and FADT->X_FIRMWARE_CTRL, but only lookup S3 waking_vector in the first one. So enhance the X_FIRMWARE_CTRL selection condition by adding FADT->FIRMWARE_CTRL == 0. Signed-off-by: Wei Gang <gang.wei@intel.com> diff -r e11c8dcbf690 xen/arch/x86/acpi/boot.c --- a/xen/arch/x86/acpi/boot.c Wed Feb 24 11:00:11 2010 +0800 +++ b/xen/arch/x86/acpi/boot.c Thu Feb 25 20:47:37 2010 +0800 @@ -365,7 +365,7 @@ acpi_fadt_parse_sleep_info(struct acpi_t acpi_sinfo.pm1b_evt_blk.address); /* Now FACS... */ - if (fadt->header.revision >= FADT2_REVISION_ID) + if (fadt->header.revision >= FADT2_REVISION_ID && fadt->facs == 0) facs_pa = fadt->Xfacs; else facs_pa = (uint64_t)fadt->facs; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2010-Feb-25 08:38 UTC
Re: [Xen-devel] [PATCH]ACPI: workaround for S3 fail in two facs tables case
>>> "Wei, Gang" <gang.wei@intel.com> 25.02.10 06:49 >>> >ACPI: workaround for S3 fail in two facs tables case > >Some legacy BIOS which support ACPI2.0+ may expose two FACS tables via both FADT->FIRMWARE_CTRL and FADT->X_FIRMWARE_CTRL, but only lookup S3 waking_vector in the first one. So enhance the X_FIRMWARE_CTRL selection condition by adding FADT->FIRMWARE_CTRL == 0. > >Signed-off-by: Wei Gang <gang.wei@intel.com> > >diff -r e11c8dcbf690 xen/arch/x86/acpi/boot.c >--- a/xen/arch/x86/acpi/boot.c Wed Feb 24 11:00:11 2010 +0800 >+++ b/xen/arch/x86/acpi/boot.c Thu Feb 25 20:47:37 2010 +0800 >@@ -365,7 +365,7 @@ acpi_fadt_parse_sleep_info(struct acpi_t > acpi_sinfo.pm1b_evt_blk.address); > > /* Now FACS... */ >- if (fadt->header.revision >= FADT2_REVISION_ID) >+ if (fadt->header.revision >= FADT2_REVISION_ID && fadt->facs == 0) > facs_pa = fadt->Xfacs; > else > facs_pa = (uint64_t)fadt->facs;Wouldn''t that generally suppress using fadt->Xfacs, since I think in order to be pre-2.0-OS compatible a BIOS would not normally set facs to zero when Xfacs is non-zero? Or is that a requirement by the spec? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Feb-25 10:39 UTC
Re: [Xen-devel] [PATCH]ACPI: workaround for S3 fail in two facs tables case
On 25/02/2010 08:38, "Jan Beulich" <JBeulich@novell.com> wrote:>> /* Now FACS... */ >> - if (fadt->header.revision >= FADT2_REVISION_ID) >> + if (fadt->header.revision >= FADT2_REVISION_ID && fadt->facs == 0) >> facs_pa = fadt->Xfacs; >> else >> facs_pa = (uint64_t)fadt->facs; > > Wouldn''t that generally suppress using fadt->Xfacs, since I think in > order to be pre-2.0-OS compatible a BIOS would not normally set > facs to zero when Xfacs is non-zero? Or is that a requirement by the > spec?Good question would be: What does Linux do here? -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Feb-25 11:58 UTC
Re: [Xen-devel] [PATCH]ACPI: workaround for S3 fail in two facs tables case
On 25/02/2010 05:49, "Wei, Gang" <gang.wei@intel.com> wrote:> /* Now FACS... */ > - if (fadt->header.revision >= FADT2_REVISION_ID) > + if (fadt->header.revision >= FADT2_REVISION_ID && fadt->facs == 0) > facs_pa = fadt->Xfacs; > else > facs_pa = (uint64_t)fadt->facs;I think Linux in this case falls back to facs if Xfacs is zero, rather than always using facs if facs is non-zero. So more like: if (fadt->header.revisions >= FADT2_REVISION_ID && fadt->Xfacs) ... What do you think? And I was looking at Linux 2.6.27 -- has behaviour there chanegd since then? -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2010-Feb-25 13:17 UTC
Re: [Xen-devel] [PATCH]ACPI: workaround for S3 fail in two facs tables case
>>> Keir Fraser <keir.fraser@eu.citrix.com> 25.02.10 12:58 >>> >What do you think? And I was looking at Linux 2.6.27 -- has behaviour there >chanegd since then?Not much, but it got improved - 2.6.33 has /* * Expand the 32-bit FACS and DSDT addresses to 64-bit as necessary. * Later code will always use the X 64-bit field. Also, check for an * address mismatch between the 32-bit and 64-bit address fields * (FIRMWARE_CTRL/X_FIRMWARE_CTRL, DSDT/X_DSDT) which would indicate * the presence of two FACS or two DSDT tables. */ if (!acpi_gbl_FADT.Xfacs) { acpi_gbl_FADT.Xfacs = (u64) acpi_gbl_FADT.facs; } else if (acpi_gbl_FADT.facs && (acpi_gbl_FADT.Xfacs != (u64) acpi_gbl_FADT.facs)) { ACPI_WARNING((AE_INFO, "32/64 FACS address mismatch in FADT - two FACS tables!")); } and /* * Check for FACS and DSDT address mismatches. An address mismatch between * the 32-bit and 64-bit address fields (FIRMWARE_CTRL/X_FIRMWARE_CTRL and * DSDT/X_DSDT) would indicate the presence of two FACS or two DSDT tables. */ if (acpi_gbl_FADT.facs && (acpi_gbl_FADT.Xfacs != (u64) acpi_gbl_FADT.facs)) { ACPI_WARNING((AE_INFO, "32/64X FACS address mismatch in FADT - " "%8.8X/%8.8X%8.8X, using 32", acpi_gbl_FADT.facs, ACPI_FORMAT_UINT64(acpi_gbl_FADT.Xfacs))); acpi_gbl_FADT.Xfacs = (u64) acpi_gbl_FADT.facs; } (and as the comments say, each repeated for the DSDT). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Feb-25 13:24 UTC
Re: [Xen-devel] [PATCH]ACPI: workaround for S3 fail in two facs tables case
On 25/02/2010 13:17, "Jan Beulich" <JBeulich@novell.com> wrote:> /* > * Check for FACS and DSDT address mismatches. An address mismatch between > * the 32-bit and 64-bit address fields (FIRMWARE_CTRL/X_FIRMWARE_CTRL and > * DSDT/X_DSDT) would indicate the presence of two FACS or two DSDT tables. > */ > if (acpi_gbl_FADT.facs && > (acpi_gbl_FADT.Xfacs != (u64) acpi_gbl_FADT.facs)) { > ACPI_WARNING((AE_INFO, > "32/64X FACS address mismatch in FADT - " > "%8.8X/%8.8X%8.8X, using 32", > acpi_gbl_FADT.facs, > ACPI_FORMAT_UINT64(acpi_gbl_FADT.Xfacs))); > > acpi_gbl_FADT.Xfacs = (u64) acpi_gbl_FADT.facs; > }Okay, well I guess that is basically what Gang Wei''s patch implements, although we don''t print a warning and perhaps we should. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Feb-25 20:57 UTC
Re: [Xen-devel] [PATCH]ACPI: workaround for S3 fail in two facs tables case
On 25/02/2010 05:49, "Wei, Gang" <gang.wei@intel.com> wrote:> ACPI: workaround for S3 fail in two facs tables case > > Some legacy BIOS which support ACPI2.0+ may expose two FACS tables via both > FADT->FIRMWARE_CTRL and FADT->X_FIRMWARE_CTRL, but only lookup S3 > waking_vector in the first one. So enhance the X_FIRMWARE_CTRL selection > condition by adding FADT->FIRMWARE_CTRL == 0. > > Signed-off-by: Wei Gang <gang.wei@intel.com>I reworked the patch to warn just the same as a native Linux kernel, and applied as changeset xen-unstable:20981. Thanks, Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei, Gang
2010-Feb-26 01:24 UTC
RE: [Xen-devel] [PATCH]ACPI: workaround for S3 fail in two facs tables case
Keir Fraser wrote:> On 25/02/2010 05:49, "Wei, Gang" <gang.wei@intel.com> wrote: > >> ACPI: workaround for S3 fail in two facs tables case >> >> Some legacy BIOS which support ACPI2.0+ may expose two FACS tables >> via both FADT->FIRMWARE_CTRL and FADT->X_FIRMWARE_CTRL, but only >> lookup S3 waking_vector in the first one. So enhance the >> X_FIRMWARE_CTRL selection condition by adding FADT->FIRMWARE_CTRL =>> 0. >> >> Signed-off-by: Wei Gang <gang.wei@intel.com> > > I reworked the patch to warn just the same as a native Linux kernel, > and applied as changeset xen-unstable:20981.Your change is fine. Thanks --Jimmy _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei, Gang
2010-Feb-26 01:43 UTC
RE: [Xen-devel] [PATCH]ACPI: workaround for S3 fail in two facs tables case
Jan Beulich wrote:>>>> "Wei, Gang" <gang.wei@intel.com> 25.02.10 06:49 >>> >> ACPI: workaround for S3 fail in two facs tables case >> >> Some legacy BIOS which support ACPI2.0+ may expose two FACS tables >> via both FADT->FIRMWARE_CTRL and FADT->X_FIRMWARE_CTRL, but only >> lookup S3 waking_vector in the first one. So enhance the >> X_FIRMWARE_CTRL selection condition by adding FADT->FIRMWARE_CTRL =>> 0. >> >> Signed-off-by: Wei Gang <gang.wei@intel.com> >> > Wouldn''t that generally suppress using fadt->Xfacs, since I think in > order to be pre-2.0-OS compatible a BIOS would not normally set > facs to zero when Xfacs is non-zero? Or is that a requirement by the > spec?ACPI 4.0 required that if Xfacs is non-zero facs should be zero. Pointed by either Xfacs or facs, the FACS table should be the same. The major reason using Xfacs should be just making FACS capable to be located above 4GB. To be pre-2.0-OS compatible a BIOS is better to allocate the FACS table under 4GB and pass the single address in both facs & Xfacs. Jimmy _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel