[PATCH]ACPI: workaround for S3 fail in two facs tables case

[PATCH]ACPI: workaround for S3 fail in two facs tables case

[Wei, Gang]

[-- Attachment #1: Type: text/plain, Size: 861 bytes --]

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;

[-- Attachment #2: two_facs_workaround.patch --]
[-- Type: application/octet-stream, Size: 845 bytes --]

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;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH]ACPI: workaround for S3 fail in two facs tables case

[Jan Beulich]

>>> "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

Re: [PATCH]ACPI: workaround for S3 fail in two facs tables case

[Keir Fraser]

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

RE: [PATCH]ACPI: workaround for S3 fail in two facs tables case

[Wei, Gang]

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

Re: [PATCH]ACPI: workaround for S3 fail in two facs tables case

[Keir Fraser]

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

Re: [PATCH]ACPI: workaround for S3 fail in two facs tables case

[Jan Beulich]

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

Re: [PATCH]ACPI: workaround for S3 fail in two facs tables case

[Keir Fraser]

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

Re: [PATCH]ACPI: workaround for S3 fail in two facs tables case

[Keir Fraser]

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

RE: [PATCH]ACPI: workaround for S3 fail in two facs tables case

[Wei, Gang]

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