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

[Laszlo Ersek <lersek@redhat.com>]

On 03/15/17 02:22, Zeng, Star wrote:
> On 2017/3/14 21:13, Laszlo Ersek wrote:
>> 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).
> 
> Yes. Then the code could have maximum compatibility and the behavior
> could be consistent, sure more comments about code behavior and ACPI
> spec could be added.

Do you want me to submit the patch, or can one of you guys do it? I'm
happy to review the patch, but I would be slower to post it.

Thanks!
Laszlo

> 
> Thanks,
> Star
> 
>>
>> 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
>>>>
>>>
>