Re: [Qemu-devel] [RFC PATCH] OvmfPkg/AcpiPlatformDxe: lift 4 GB alloc limit for modern ACPI systems

[Igor Mammedov]

On Thu, 1 Jun 2017 14:25:48 +0200
Laszlo Ersek <address@hidden> wrote:

> On 06/01/17 13:22, Ard Biesheuvel wrote:
> > ACPI supports architectures such as arm64, which did not exist when the
> > original 32-bit ACPI 1.0 was introduced. These days, ACPI tables can all
> > support 64-bit memory addresses, and so QEMU has been updated to emit
> > 64-bit table and entry point types on arm64/mach-virt.  
> 
> Do you have commit cb51ac2ffe36 ("hw/arm/virt: generate 64-bit
> addressable ACPI objects", 2017-04-10) in mind?
My understanding was that OVMF discards RSDP/[RX]SDT so that commit
probably irrelevant.

Now about FADT or any other blob, do we really need to extend
loader protocol? Couldn't firmware decide where to allocate
table based on size in add_pointer commands?

That might be a bit complicated but on the bright side is that
it is firmware only change and it should work both on old and
new qemu without breaking anything.

> 
> > 
> > For the UEFI side, this means we no longer have to serve allocation
> > requests from the 32-bit addressable region. So lift this restriction
> > when the platform has been configured without ACPI 1.0b support.
> > 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Ard Biesheuvel <address@hidden>
> > ---
> > 
> > This is an RFC because this change breaks compatibility with older versions
> > of QEMU. At the least, this means I should sit on this patch for another
> > while, but perhaps it also means we need some runtime logic to detect which
> > QEMU we are dealing with? Comments welcome.
> > 
> >  OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf          |  3 +++
> >  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c              | 13 +++++++++++--
> >  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf |  3 +++
> >  3 files changed, 17 insertions(+), 2 deletions(-)  
> 
> I guess by compat breakage, you mean the case when new ArmVirtQemu (with
> this patch) is booted on old QEMU (which lacks commit cb51ac2ffe36) --
> the 64-bit allocation addresses might not fit into the 4-byte fields
> generated by QEMU.
> 
> Since your QEMU patch (which I did ACK), I've given more thought to the
> QEMU_LOADER_ALLOCATE linker/loader command. See bullet (68) in:
> 
> 
> http://mid.mail-archive.com/address@hidden
> 
> There are now two reasons to extend the Zone field's meaning:
> 
> - The first reason is that the most significant bit in Zone could be
> repurposed to suppress the SDT header probing implemented in
> OvmfPkg/AcpiPlatformDxe, in the Process2ndPassCmdAddPointer() function.
> 
> - The second reason is that a new value in Zone (with the msb masked
> off), say QemuLoaderAlloc64Bit=3, could be used to lift the 32-bit
> allocation limit explicitly.
> 
> In QEMU, we could tie both of these extensions to new machine types.
> 
> The result would be:
> 
>   firmware  QEMU  QEMU machine type  result
>   --------  ----  -----------------  -----------------------------------
>   old       new   old                allocate blobs under 4GB
>   old       new   new                breakage, but that's OK, we can
>                                        require refreshed firmware for
>                                        new machine types
>   new       old   old                allocate blobs under 4GB
>   new       new   old                allocate blobs under 4GB
>   new       new   new                allocate blobs from 64-bit space
> 
> ... I guess my proposal might be a bit unpolished (and certainly
> diverges from your original QEMU commit cb51ac2ffe36), but at this
> point, the need for further *explicit* hints in the ALLOC command is
> just too obvious to pass them up.
> 
> Again, the first extension would be setting aside bit 7, to signify
> "this blob contains no ACPI tables", and the second extension would be a
> new zone value (3) to mean "allocate from 64-bit address space".
> 
> The first extension would be used by both x86 and arm machine types
> (2.10+ only), and the second extension would only be used by 2.10+ arm
> machine types.
> 
> What do you think? Can we discuss both of these Zone extensions on
> qemu-devel as well? Adding qemu-devel, Shannon, Michael, Igor, Drew, and
> Dongjiu Geng.
>
> Thanks
> Laszlo
> 
> > diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf 
> > b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
> > index 9a9b2e6bb2e5..9b883871bc23 100644
> > --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
> > +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
> > @@ -73,5 +73,8 @@ [Pcd]
> >    gPcAtChipsetPkgTokenSpaceGuid.Pcd8259LegacyModeEdgeLevel
> >    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress
> >  
> > +[FixedPcd]
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions
> > +
> >  [Depex]
> >    gEfiAcpiTableProtocolGuid
> > diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c 
> > b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> > index 1bc5fe297a96..97632bc636c0 100644
> > --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> > +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> > @@ -24,6 +24,7 @@
> >  #include <Library/PcdLib.h>
> >  #include <Library/OrderedCollectionLib.h>
> >  #include <IndustryStandard/Acpi.h>
> > +#include <Protocol/AcpiSystemDescriptionTable.h>
> >  
> >  
> >  //
> > @@ -173,6 +174,7 @@ ProcessCmdAllocate (
> >    UINTN                NumPages;
> >    EFI_PHYSICAL_ADDRESS Address;
> >    BLOB                 *Blob;
> > +  EFI_ALLOCATE_TYPE    AllocType;
> >  
> >    if (Allocate->File[QEMU_LOADER_FNAME_SIZE - 1] != '\0') {
> >      DEBUG ((EFI_D_ERROR, "%a: malformed file name\n", __FUNCTION__));
> > @@ -192,9 +194,16 @@ ProcessCmdAllocate (
> >      return Status;
> >    }
> >  
> > +  if ((FixedPcdGet32 (PcdAcpiExposedTableVersions) &
> > +       EFI_ACPI_TABLE_VERSION_1_0B) != 0) {
> > +    Address = 0xFFFFFFFF;
> > +    AllocType = AllocateMaxAddress;
> > +  } else {
> > +    AllocType = AllocateAnyPages;
> > +  }
> > +
> >    NumPages = EFI_SIZE_TO_PAGES (FwCfgSize);
> > -  Address = 0xFFFFFFFF;
> > -  Status = gBS->AllocatePages (AllocateMaxAddress, EfiACPIMemoryNVS, 
> > NumPages,
> > +  Status = gBS->AllocatePages (AllocType, EfiACPIMemoryNVS, NumPages,
> >                    &Address);
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> > diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf 
> > b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
> > index adc50cfd9f76..64db80dd9cbc 100644
> > --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
> > +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
> > @@ -58,5 +58,8 @@ [Guids]
> >  [Pcd]
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
> >  
> > +[FixedPcd]
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions
> > +
> >  [Depex]
> >    gEfiAcpiTableProtocolGuid
> >   
>