From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Thu, 11 Apr 2019 05:31:40 -0700 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B2244C04B94F; Thu, 11 Apr 2019 12:31:39 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-225.rdu2.redhat.com [10.10.120.225]) by smtp.corp.redhat.com (Postfix) with ESMTP id C416E183F0; Thu, 11 Apr 2019 12:31:37 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 14/31] OvmfPkg/AcpiPlatformDxe: Use PVH RSDP if exist From: "Laszlo Ersek" To: devel@edk2.groups.io, anthony.perard@citrix.com Cc: Jordan Justen , Ard Biesheuvel , Julien Grall , xen-devel@lists.xenproject.org References: <20190409110844.14746-1-anthony.perard@citrix.com> <20190409110844.14746-15-anthony.perard@citrix.com> <972252b3-4de1-acaa-03d3-5a7d27a68630@redhat.com> Message-ID: <10338b3d-fb1b-34b0-f7f4-d1debf70bfec@redhat.com> Date: Thu, 11 Apr 2019 14:31:36 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <972252b3-4de1-acaa-03d3-5a7d27a68630@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 11 Apr 2019 12:31:39 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/11/19 14:23, Laszlo Ersek wrote: > On 04/11/19 14:20, Laszlo Ersek wrote: >> On 04/09/19 13:08, Anthony PERARD wrote: >>> If the firmware have been started via the PVH entry point, a RSDP >>> pointer would have been provided. Use it. >>> >>> Also, use XenDetect() from the new XenPlatformLib. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Anthony PERARD >>> --- >>> OvmfPkg/OvmfPkgIa32.dsc | 1 + >>> OvmfPkg/OvmfPkgIa32X64.dsc | 1 + >>> OvmfPkg/OvmfPkgX64.dsc | 1 + >>> OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 2 +- >>> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h | 6 +-- >>> OvmfPkg/AcpiPlatformDxe/Xen.c | 41 ++++++++------------ >>> 6 files changed, 22 insertions(+), 30 deletions(-) >>> >>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc >>> index f55ab5a3d2..cab9764cab 100644 >>> --- a/OvmfPkg/OvmfPkgIa32.dsc >>> +++ b/OvmfPkg/OvmfPkgIa32.dsc >>> @@ -205,6 +205,7 @@ [LibraryClasses] >>> SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf >>> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf >>> XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf >>> + XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf >>> >>> !if $(TPM2_ENABLE) == TRUE >>> Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf >>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc >>> index 5c9bdf034e..a156358690 100644 >>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc >>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc >>> @@ -210,6 +210,7 @@ [LibraryClasses] >>> SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf >>> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf >>> XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf >>> + XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf >>> >>> !if $(TPM2_ENABLE) == TRUE >>> Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf >>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >>> index 2943e9e8af..9d8dc442b4 100644 >>> --- a/OvmfPkg/OvmfPkgX64.dsc >>> +++ b/OvmfPkg/OvmfPkgX64.dsc >>> @@ -210,6 +210,7 @@ [LibraryClasses] >>> SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf >>> OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf >>> XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf >>> + XenPlatformLib|OvmfPkg/Library/XenPlatformLib/XenPlatformLib.inf >>> >>> !if $(TPM2_ENABLE) == TRUE >>> Tpm2CommandLib|SecurityPkg/Library/Tpm2CommandLib/Tpm2CommandLib.inf >>> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf >>> index 8440e7b343..ca54a3bd9e 100644 >>> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf >>> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf >>> @@ -51,13 +51,13 @@ [LibraryClasses] >>> DebugLib >>> UefiBootServicesTableLib >>> UefiDriverEntryPoint >>> - HobLib >>> QemuFwCfgLib >>> QemuFwCfgS3Lib >>> MemoryAllocationLib >>> BaseLib >>> DxeServicesTableLib >>> OrderedCollectionLib >>> + XenPlatformLib >>> >>> [Protocols] >>> gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED >>> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h >>> index 83b981ee00..85f37128dd 100644 >>> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h >>> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h >>> @@ -25,6 +25,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> #include >>> >>> @@ -58,11 +59,6 @@ QemuInstallAcpiTable ( >>> OUT UINTN *TableKey >>> ); >>> >>> -BOOLEAN >>> -XenDetected ( >>> - VOID >>> - ); >>> - >>> EFI_STATUS >>> EFIAPI >>> InstallXenTables ( >>> diff --git a/OvmfPkg/AcpiPlatformDxe/Xen.c b/OvmfPkg/AcpiPlatformDxe/Xen.c >>> index 618ac58b42..7e9a7797d7 100644 >>> --- a/OvmfPkg/AcpiPlatformDxe/Xen.c >>> +++ b/OvmfPkg/AcpiPlatformDxe/Xen.c >>> @@ -15,8 +15,6 @@ >>> **/ >>> >>> #include "AcpiPlatform.h" >>> -#include >>> -#include >>> #include >>> >>> #define XEN_ACPI_PHYSICAL_ADDRESS 0x000EA020 >>> @@ -24,28 +22,6 @@ >>> >>> EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER *XenAcpiRsdpStructurePtr = NULL; >>> >>> -/** >>> - This function detects if OVMF is running on Xen. >>> - >>> -**/ >>> -BOOLEAN >>> -XenDetected ( >>> - VOID >>> - ) >>> -{ >>> - EFI_HOB_GUID_TYPE *GuidHob; >>> - >>> - // >>> - // See if a XenInfo HOB is available >>> - // >>> - GuidHob = GetFirstGuidHob (&gEfiXenInfoGuid); >>> - if (GuidHob == NULL) { >>> - return FALSE; >>> - } >>> - >>> - return TRUE; >>> -} >>> - >>> /** >>> Get the address of Xen ACPI Root System Description Pointer (RSDP) >>> structure. >>> @@ -66,10 +42,27 @@ GetXenAcpiRsdp ( >>> EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER *RsdpStructurePtr; >>> UINT8 *XenAcpiPtr; >>> UINT8 Sum; >>> + EFI_XEN_INFO *XenInfo; >>> >>> // >>> // Detect the RSDP structure >>> // >>> + >>> + // >>> + // First look for PVH one >>> + // >>> + XenInfo = XenGetInfoHOB (); >>> + ASSERT (XenInfo != NULL); >>> + if (XenInfo->RsdpPvh != NULL) { >>> + DEBUG ((DEBUG_INFO, "AcpiPlatformDxe: Use ACPI RSDP table at 0x%08x\n", >>> + XenInfo->RsdpPvh)); > > (4) sorry, missed this one: please > > - either print pointers with %p, > > - or convert them with > > (UINT64)(UINTN)XenInfo->RsdpPvh > > and log them with "0x%016Lx". Sigh, I clearly need to cease reviewing this series for today. Here's another remark: (5) if you want to print the name of the module (such as "AcpiPlatformDxe") in a log message, please use the "%a" format specifier (for "ASCII string"), and use the following auto-generated variable: gEfiCallerBaseName It will contain the BASE_NAME string from the module INF file. This variable is particularly useful in files that are built into multiple modules: - for example, a module directory may contain two INF files, with different BASE_NAMEs, and include a common .c file. If that .c file produces log messages, gEfiCallerBaseName is helpful. - the same applies even more to log messages generated from libraries -- you'll want to see the name of the driver or application that the library is linked into. In that case, gEfiCallerBaseName refers to the BASE_NAME of the driver/app INF file, and not the lib instance INF file. Thanks Laszlo >>> + *RsdpPtr = XenInfo->RsdpPvh; >>> + return EFI_SUCCESS; >>> + } >>> + >>> + // >>> + // Otherwise, look for the HVM one >>> + // >>> for (XenAcpiPtr = (UINT8*)(UINTN) XEN_ACPI_PHYSICAL_ADDRESS; >>> XenAcpiPtr < (UINT8*)(UINTN) XEN_BIOS_PHYSICAL_END; >>> XenAcpiPtr += 0x10) { >>> >> >> Can you please split this patch in two: >> >> (1) the first patch should only rebase AcpiPlatformDxe to the new >> library, without functional changes. >> >> (2) the second patch should add the new feature, of looking for the PVH >> RSDP. >> >> (3) In the 2nd patch, pls mention Xen in the subject line, such as: >> >> OvmfPkg/AcpiPlatformDxe: Use Xen PVH RSDP if it exist >> >> Thanks! >> Laszlo >> >