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

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

[Laszlo Ersek]

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?

> 
> 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 <ard.biesheuvel@linaro.org>
> ---
> 
> 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/accf8880-f4b0-85f1-9f33-4103c8c26d7a@redhat.com

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
> 

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 <lersek@redhat.com> 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 <ard.biesheuvel@linaro.org>
> > ---
> > 
> > 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/accf8880-f4b0-85f1-9f33-4103c8c26d7a@redhat.com
> 
> 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
> >   
> 

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

[Laszlo Ersek]

On 06/01/17 17:16, Igor Mammedov wrote:
> On Thu, 1 Jun 2017 14:25:48 +0200
> Laszlo Ersek <lersek@redhat.com> 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.

OVMF does not install these root tables, that's correct, but it does
execute the commands that refer to the blob(s) that contain these
tables. So for example pointers and checksums are updated, it's only the
last heuristics phase that detects if a pointer points to RSDT / XSDT
and then those tables are not installed.

OVMF catches pointer field overflows, and rolls back the entire
processing if one occurs. So the commit in question is certainly
relevant; without it, a >4GB allocation address would be added to an
RSDT entry (which is 4-byte wide), and that would fail the entire process.

> 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 this I'd have to pre-scan the ADD_POINTER commands, and for target
blob ever pointed-at by them, find the minimum referring pointer width.
If that minimum width is smaller than 8 bytes, the pointed-at blob must
be allocated under 4GB.

I think this is too complex (it would add a third pass to OVMF's
processing), especially given that we already have a dedicated Zone
field that specifically tells the firmware where the blobs should be
allocated. The goal is to decrease the dirty tricks in OVMF, not
increase them.

Furthermore, this wouldn't help us turn off the SDT header probing in
OVMF (the feature that currently requires those strange AML-level offset
increments, into blobs that don't host ACPI tables). Suppressing the
probe in a nice way surely needs a specific hint, so we might as well
handle both cases with Zone extensions.

Laszlo

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

[Laszlo Ersek]

On 06/01/17 14:25, Laszlo Ersek wrote:

> 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 think the situation is easier than this. We don't have to tie the
extensions to machine types.

The reason is that old firmware is allowed to fail on new QEMU
(regardless of machine type). Example: the WRITE_POINTER command,
originally introduced for VMGENID. If you run a SeaBIOS binary without
WRITE_POINTER support, in a QEMU VM with "-device vmgenid", the device
will not work. And QEMU doesn't try to prevent that by binding vmgenid
to machine types. Instead, QEMU bundled a SeaBIOS binary with
WRITE_POINTER support, for the release that introduced VMGENID.

(There's no reason for not bundling OVMF and ArmVirtQemu binaries with
QEMU releases now. Gerd already has a build service up and running, at
<http://www.kraxel.org/repos/>.)

The scenario that we *should* avoid is new firmware failing on old QEMU.
And this patch is actually that case, because the new fw would allocate
blobs with such 8-byte addresses that might not fit into 32-bit blob
fields. So, the extensions are necessary, but tying them to machine
types isn't.

  firmware  QEMU  result
  --------  ----  ------------------------------------------------------
  old       new   breakage, but that's OK; we can require refreshed
                    firmware for new QEMU releases
  new       old   allocate blobs under 4GB (alloc zone extension is
                    necessary)
  new       new   allocate blobs from any address range

Thanks
Laszlo

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

[Gerd Hoffmann]

  Hi,

> The reason is that old firmware is allowed to fail on new QEMU
> (regardless of machine type). Example: the WRITE_POINTER command,
> originally introduced for VMGENID. If you run a SeaBIOS binary
> without
> WRITE_POINTER support, in a QEMU VM with "-device vmgenid", the
> device
> will not work.

Old seabios will work just fine on new qemu as long as you don't use
the vmgenid device.

Only when using new features (which require firmware support) new
seabios is needed, and usually we update seabios for that reason.  We
want the new features be usable of course.  But in general there are no
lockstep updates for qemu and seabios (any more).

> And this patch is actually that case, because the new fw would
> allocate
> blobs with such 8-byte addresses that might not fit into 32-bit blob
> fields.

I think ovmf simply checking whenever the pointer fields are 32bit or
64bit, then doing allocations accordingly (as Igor suggested) would be
the best.  Why ask qemu for a hint when ovmf can figure on its own? 
That adds a new interface and new config knobs for IMO no good reason.

cheers,
  Gerd

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

[Laszlo Ersek]

On 06/02/17 16:56, Gerd Hoffmann wrote:
>   Hi,
> 
>>  The reason is that old firmware is allowed to fail on new QEMU
>> (regardless of machine type). Example: the WRITE_POINTER command,
>> originally introduced for VMGENID. If you run a SeaBIOS binary
>> without
>> WRITE_POINTER support, in a QEMU VM with "-device vmgenid", the
>> device
>> will not work.
> 
> Old seabios will work just fine on new qemu as long as you don't use
> the vmgenid device.
> 
> Only when using new features (which require firmware support) new
> seabios is needed, and usually we update seabios for that reason.  We
> want the new features be usable of course.  But in general there are no
> lockstep updates for qemu and seabios (any more).
> 
>> And this patch is actually that case, because the new fw would
>> allocate
>> blobs with such 8-byte addresses that might not fit into 32-bit blob
>> fields.
> 
> I think ovmf simply checking whenever the pointer fields are 32bit or
> 64bit, then doing allocations accordingly (as Igor suggested) would be
> the best.  Why ask qemu for a hint when ovmf can figure on its own?
> That adds a new interface and new config knobs for IMO no good reason.

There is a good reason: the reason is that OVMF should not be forced to
reverse-engineer such information with a third, separate pass over the
linker/loader script, adding yet more complexity.

The linker/loader interface was designed for SeaBIOS, without any regard
to OVMF / ArmVirtQemu. For example, SeaBIOS gets hints like HIGH vs.
FSEG (which distinction is irrelevant for OVMF / ArmVirtQemu) despite
the fact that SeaBIOS could also figure that out one way or another if
it really wanted to. OVMF / ArmVirtQemu need different kinds of hints,
and I think they should get them at this point.

Even now, OVMF deduces the starts of individual ACPI tables in the blobs
by using somewhat messy heuristics. (OVMF *must* know where the tables
start.) The heuristics incur no false negatives, but a chance exists for
false positives, unless QEMU takes extra precautions. But Igor happens
to hate those precautions, and the TCPA and NVDIMM authors have
completely missed them. So we should have fewer of these heuristics and
workarounds, not more.

And, as I replied to Igor earlier, the other blob allocation hint (the
"no ACPI content" hint that allows QEMU to drop precisely the above
obscure precautions, i.e. the "ACPI SDT header probe suppressor"),
necessitates extending the ALLOCATE command anyway.

Would it help if I made these extensions dependent on 2.10+ machine
types? (Obviously existing generators like TCPA, NVDIMM and VMGENID
would not be modified then, because the old stuff would have to be
preserved for old machine types anyway.)

Thanks
Laszlo