Re: [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion

[Laszlo Ersek <lersek@redhat.com>]

On 03/14/17 09:33, Zeng, Star wrote:
> In original code for < 4G table,
> Dsdt and XDsdt will be both assigned if FADT is installed before DSDT, but
> Dsdt and XDsdt will have mutual exclusion if FADT is installed after DSDT.
> 
> They are inconsistent.

That's right. The revert would not solve this original problem.

> 
> Is there any negative impact found to assign both Dsdt and XDsdt for < 4G table except the spec volation?

No, I don't think so; the spec violation is the only impact to my knowledge.

Are you suggesting that we:
- delete RequireDsdtXDsdtExclusion(),
- replace all invocations of RequireDsdtXDsdtExclusion() with FALSE,
- and then simplify the resultant code (that is, eliminate the
  always-FALSE branches, and un-indent the always-TRUE branches)?

This would be [VARIANT A], regardless of ACPI spec version.

I think that could work (as long as we don't mind breaking the ACPI spec
versions that require mutual exclusion).

Thanks
Laszlo

> 
> Thanks,
> Star
> -----Original Message-----
> From: Fan, Jeff 
> Sent: Tuesday, March 14, 2017 3:57 PM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@ml01.01.org>
> Cc: Tian, Feng <feng.tian@intel.com>; Michael Tsirkin <mtsirkin@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Phil Dennis-Jordan <phil@philjordan.eu>; Leo Duran <leo.duran@amd.com>; Yao, Jiewen <jiewen.yao@intel.com>; Al Stone <ahs3@redhat.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
> 
> Laszlo,
> 
> Basically, I agree with this is OS assumption. I did not find better fix to handle such compatibility issue.
> 
> I agree to revert this patch 2/2 to fix Windows 2012 R2 boot issue.
> 
> I don't know if the other guys have other suggestions. :-)
> 
> Thanks!
> Jeff
> 
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, March 13, 2017 10:44 PM
> To: Fan, Jeff; edk2-devel-01
> Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan; Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star
> Subject: Re: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
> 
> On 03/13/17 04:07, Fan, Jeff wrote:
>> Laszlo,
>>
>> We found one Windows Server 2012 R2 blue screen issue with ACPI 6.1 FADT table.
>>
>> We did the following configuration test with DSDT under 4GB.
>> .DSDT     .X_DSDT         Window Server 2012 R2
>> ----------   ------------       -------------------------------
>> set            clear             Failed            // current implementation
>> clear         set                Succeed
>> set            set                Succeed
> 
> That looks like a Windows bug. The above configuration satisfies ACPI 6.1:
> 
> DSDT -- Physical memory address of the DSDT. If the X_DSDT field contains a non-zero value then this field must be zero.
> 
> X_DSDT -- Extended physical address of the DSDT. If the DSDT field contains a non-zero value then this field must be zero.
> 
> Michael told me that "6.1 errata will specify X_DSDT takes preference over DSDT but both can be present legaly", however, here X_DSDT cannot take precedence because it is zero.
> 
> Based on past experience, I don't expect that Microsoft will ever fix this ACPI bug in Windows Server 2012 R2. I don't even expect that they would share with us a list of ACPI spec versions that should be exempted from RequireDsdtXDsdtExclusion() -- despite the spec clearly requiring DSDT / X_DSDT exclusion --, for bug compatibility.
> 
> That leaves us with trial and error, to see what works and what doesn't.
> Unfortunately, I don't have ACPI tables for several ACPI spec versions; I don't think I can experiment with this. If you find a workaround, that would be great, but if we can't, I guess the patch should be reverted.
> (Note however that the BSOD will remain possible to trigger, with the DSDT, FADT installation order.)
> 
> Thanks
> Laszlo
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
>> Laszlo Ersek
>> Sent: Thursday, March 09, 2017 3:59 AM
>> To: edk2-devel-01
>> Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan; 
>> Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star
>> Subject: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve 
>> FADT.{DSDT, X_DSDT} mutual exclusion
>>
>> The ACPI specification, up to and including revision 5.1 Errata A, 
>> allows the DSDT and X_DSDT fields to be both set in the FADT.
>> (Obviously, this only makes sense if the DSDT address is representable 
>> in 4 bytes.)
>>
>> Starting with 5.1 Errata B, specifically for Mantis 1393 <https://mantis.uefi.org/mantis/view.php?id=1393>, the spec requires at most one of DSDT and X_DSDT to be set to a nonzero value.
>>
>> MdeModulePkg/AcpiTableDxe handles this mutual exclusion somewhat inconsistently.
>>
>> - If the caller of EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() installs the
>>   tables in "DSDT, FADT" order, then we enforce the exclusion between the
>>   DSDT and X_DSDT fields:
>>
>>   DSDT under 4GB  FADT.DSDT  FADT.X_DSDT    [VARIANT B]
>>   --------------  ---------  -----------
>>   yes             set        clear
>>   no              clear      set
>>
>>   This behavior conforms to 5.1 Errata B. (And it's not required by
>>   earlier versions of the spec.)
>>
>> - If the caller passes in the tables in "FADT, DSDT" relative order, then
>>   we do not enforce the exclusion:
>>
>>   DSDT under 4GB  FADT.DSDT  FADT.X_DSDT    [VARIANT A]
>>   --------------  ---------  -----------
>>   yes             set        set
>>   no              clear      set
>>
>>   This satisfies 5.1 Errata A and earlier, but breaks 5.1 Errata B and
>>   later.
>>
>> Unify the handling of both relative orders. In particular, check the major and minor version numbers in the FADT. If the FADT version is strictly before 5.1, then implement [VARIANT A]. If the FADT version is equal to or larger than 5.1, then implement [VARIANT B].
>>
>> We make three observations:
>>
>> - We can't check the FADT table version precisely against "5.1 Errata B";
>>   erratum levels are not captured in the table. We err in the safe
>>   direction, namely we enforce the exclusion for "5.1" and "5.1 Errata A".
>>
>> - The same applies to "6.0" versus "6.0 Errata A". Because we cannot
>>   distinguish these two, we consider "6.0" to be "equal to or larger than
>>   5.1", and apply [VARIANT B], enforcing the exclusion.
>>
>> - While a blanket [VARIANT B] would be simpler, there is a significant
>>   benefit to [VARIANT A], under the spec versions that permit it:
>>   compatibility with a wider range of OSPMs (typically, older ones).
>>
>>   For example, Igor reported about a "DELL R430 system with rev4 FADT
>>   where DSDT and X_DSDT are pointing to the same address". Michael also
>>   reported about several systems that exhibit the same.
>>
>> Regression tested with the following KVM guests (QEMU built at ata0def594286d, "Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging", 2017-01-30):
>>
>> - OVMF: boot and S3 suspend/resume
>>   - Ia32, Q35, SMM
>>     - Fedlet 20141209
>>   - Ia32X64, Q35, SMM
>>     - Fedora 22
>>     - Windows 7
>>     - Windows 8.1
>>     - Windows 10
>>     - Windows Server 2008 R2
>>     - Windows Server 2012 R2
>>     - Windows Server 2016 Tech Preview 4
>>   - X64, I440FX, no SMM
>>     - Fedora 24
>>     - RHEL-6.7
>>     - RHEL-7.2-ish
>> - ArmVirtQemu: boot test with virtio-gpu
>>   - AARCH64
>>     - Fedora 24
>>     - RHELSA-7.3
>>     - openSUSE Tumbleweed (4.8.4-based)
>>
>> This change is connected to ASWG ticket 
>> <https://mantis.uefi.org/mantis/view.php?id=1757>, which is now closed/fixed.
>>
>> Cc: Al Stone <ahs3@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Feng Tian <feng.tian@intel.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Leo Duran <leo.duran@amd.com>
>> Cc: Michael Tsirkin <mtsirkin@redhat.com>
>> Cc: Phil Dennis-Jordan <phil@philjordan.eu>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Reported-by: Phil Dennis-Jordan <phil@philjordan.eu>
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
>> ---
>>
>> Notes:
>>     v2:
>>     - simplify logic in RequireDsdtXDsdtExclusion() [Jiewen]
>>     - pick up Phil's R-b nonetheless (the above change is a minimal
>>       reformulation of code, with no behavioral difference)
>>     - add reference to Mantis#1757 to the commit message
>>     
>>     v1:
>>     NOTE for people on the CC list:
>>     
>>     If you are not presently subscribed to edk2-devel and wish to comment on
>>     this patch publicly, you need to subscribe first, and wait for the
>>     subscription request to *complete* (see your inbox), *before* sending
>>     your followup. This is not ideal, but edk2-devel requires subscription
>>     before reflecting messages from someone.
>>     
>>     Subscribe at <https://lists.01.org/mailman/listinfo/edk2-devel>. Thanks.
>>
>>  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 62
>> +++++++++++++++++++-
>>  1 file changed, 59 insertions(+), 3 deletions(-)
>>
>> diff --git
>> a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>> index 7795ff7269ca..4bb848df5203 100644
>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>> @@ -430,6 +430,51 @@ ReallocateAcpiTableBuffer (
>>    mEfiAcpiMaxNumTables = NewMaxTableNumber;
>>    return EFI_SUCCESS;
>>  }
>> +
>> +/**
>> +  Determine whether the FADT table passed in as parameter requires 
>> +mutual
>> +  exclusion between the DSDT and X_DSDT fields. (That is, whether 
>> +there exists
>> +  an explicit requirement that at most one of those fields is 
>> +permitted to be
>> +  nonzero.)
>> +
>> +  @param[in] Fadt  The EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE object to
>> +                   check.
>> +
>> +  @retval TRUE     Fadt requires mutual exclusion between DSDT and X_DSDT.
>> +  @retval FALSE    Otherwise.
>> +**/
>> +BOOLEAN
>> +RequireDsdtXDsdtExclusion (
>> +  IN EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt
>> +  )
>> +{
>> +  //
>> +  // Mantis ticket #1393 was addressed in ACPI 5.1 Errata B. 
>> +Unfortunately, we
>> +  // can't tell apart 5.1 Errata A and 5.1 Errata B just from looking 
>> +at the
>> +  // FADT table. Therefore let's require exclusion for table versions >= 5.1.
>> +  //
>> +  // While this needlessly covers 5.1 and 5.1A too, it is safer to 
>> +require
>> +  // DSDT<->X_DSDT exclusion for lax (5.1, 5.1A) versions of the spec 
>> +than to
>> +  // permit DSDT<->X_DSDT duplication for strict (5.1B) versions of the spec.
>> +  //
>> +  // The same applies to 6.0 vs. 6.0A. While 6.0 does not require the
>> +  // exclusion, 6.0A and 6.1 do. Since we cannot distinguish 6.0 from 
>> +6.0A
>> +  // based on just the FADT, we lump 6.0 in with the rest of >= 5.1.
>> +  //
>> +  if ((Fadt->Header.Revision < 5) ||
>> +      ((Fadt->Header.Revision == 5) &&
>> +       (((EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE *)Fadt)->MinorVersion == 0))) {
>> +    //
>> +    // version <= 5.0
>> +    //
>> +    return FALSE;
>> +  }
>> +  //
>> +  // version >= 5.1
>> +  //
>> +  return TRUE;
>> +}
>> +
>>  /**
>>    This function adds an ACPI table to the table list.  It will detect FACS and
>>    allocate the correct type of memory and properly align the table.
>> @@ -647,12 +692,16 @@ AddTableToList (
>>        }
>>        if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
>>          AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN) AcpiTableInstance->Dsdt3;
>> -        ZeroMem (&AcpiTableInstance->Fadt3->XDsdt, sizeof (UINT64));
>> +        if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
>> +          Buffer64 = 0;
>> +        } else {
>> +          Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
>> +        }
>>        } else {
>>          AcpiTableInstance->Fadt3->Dsdt = 0;
>>          Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
>> -        CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT64));
>>        }
>> +      CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof 
>> + (UINT64));
>>  
>>        //
>>        // RSDP OEM information is updated to match the FADT OEM information @@ -847,8 +896,15 @@ AddTableToList (
>>        if (AcpiTableInstance->Fadt3 != NULL) {
>>          if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
>>            AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN)
>> AcpiTableInstance->Dsdt3;
>> +          if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
>> +            Buffer64 = 0;
>> +          } else {
>> +            Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
>> +          }
>> +        } else {
>> +          AcpiTableInstance->Fadt3->Dsdt = 0;
>> +          Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
>>          }
>> -        Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
>>          CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof 
>> (UINT64));
>>  
>>          //
>> --
>> 2.9.3
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>