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 E702C21AE30EC for ; Thu, 1 Jun 2017 05:24:52 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9B9A583F42; Thu, 1 Jun 2017 12:25:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9B9A583F42 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 9B9A583F42 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-77.phx2.redhat.com [10.3.116.77]) by smtp.corp.redhat.com (Postfix) with ESMTP id A4B697FE87; Thu, 1 Jun 2017 12:25:49 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org Cc: leif.lindholm@linaro.org, jordan.l.justen@intel.com, Shannon Zhao , "Michael S. Tsirkin" , Igor Mammedov , qemu devel list , gengdongjiu , Drew Jones References: <20170601112241.2580-1-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: Date: Thu, 1 Jun 2017 14:25:48 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <20170601112241.2580-1-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 01 Jun 2017 12:25:53 +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 12:24:53 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > --- > > 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 >