From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9BCA021AE30CE for ; Thu, 1 Jun 2017 08:15:58 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5E8CF80C2B; Thu, 1 Jun 2017 15:16:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 5E8CF80C2B Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=imammedo@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 5E8CF80C2B Received: from nial.brq.redhat.com (dhcp-1-118.brq.redhat.com [10.34.1.118]) by smtp.corp.redhat.com (Postfix) with ESMTP id EAB375DC18; Thu, 1 Jun 2017 15:16:53 +0000 (UTC) Date: Thu, 1 Jun 2017 17:16:50 +0200 From: Igor Mammedov To: Laszlo Ersek Cc: Ard Biesheuvel , edk2-devel@lists.01.org, leif.lindholm@linaro.org, jordan.l.justen@intel.com, Shannon Zhao , "Michael S. Tsirkin" , qemu devel list , gengdongjiu , Drew Jones Message-ID: <20170601171650.4a196f36@nial.brq.redhat.com> In-Reply-To: References: <20170601112241.2580-1-ard.biesheuvel@linaro.org> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Thu, 01 Jun 2017 15:16:59 +0000 (UTC) Subject: Re: [RFC PATCH] OvmfPkg/AcpiPlatformDxe: lift 4 GB alloc limit for modern ACPI systems X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 01 Jun 2017 15:15:58 -0000 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Thu, 1 Jun 2017 14:25:48 +0200 Laszlo Ersek 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 > > --- > > > > 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 > > #include > > #include > > +#include > > > > > > // > > @@ -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 > > >