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 96B0081786 for ; Tue, 10 Jan 2017 08:54:52 -0800 (PST) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2C905627DE; Tue, 10 Jan 2017 16:54:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-126.phx2.redhat.com [10.3.116.126]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v0AGspQE000384; Tue, 10 Jan 2017 11:54:52 -0500 To: Anthony PERARD References: <20161208153340.2285-1-anthony.perard@citrix.com> <20161208153340.2285-7-anthony.perard@citrix.com> <8cc382af-45b2-c836-2907-98f066cca05b@redhat.com> <20170110161848.GF1903@perard.uk.xensource.com> Cc: edk2-devel@ml01.01.org, xen-devel@lists.xenproject.org From: Laszlo Ersek Message-ID: <87b475e5-ae74-c606-cfca-226aac71aeb8@redhat.com> Date: Tue, 10 Jan 2017 17:54:49 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <20170110161848.GF1903@perard.uk.xensource.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Tue, 10 Jan 2017 16:54:53 +0000 (UTC) Subject: Re: [PATCH RFC 06/14] OvmfPkg/XenPlatformPei: Add xen PVH specific code X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Jan 2017 16:54:52 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 01/10/17 17:18, Anthony PERARD wrote: > On Thu, Jan 05, 2017 at 11:30:32AM +0100, Laszlo Ersek wrote: >> On 12/08/16 16:33, Anthony PERARD wrote: >>> - learn about memory size from the E820 >>> - ignore error if host bridge devid is 0xffff, PVH does not have PCI and >>> reading from unexisting device return -1. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Anthony PERARD >>> --- >>> OvmfPkg/XenPlatformPei/MemDetect.c | 71 ++++++++++++++++++++++++++++++++++++++ >>> OvmfPkg/XenPlatformPei/Platform.c | 5 +++ >>> OvmfPkg/XenPlatformPei/Platform.h | 10 ++++++ >>> 3 files changed, 86 insertions(+) >>> >>> diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c b/OvmfPkg/XenPlatformPei/MemDetect.c >>> index 4ecdf5e..0d80775 100644 >>> --- a/OvmfPkg/XenPlatformPei/MemDetect.c >>> +++ b/OvmfPkg/XenPlatformPei/MemDetect.c >>> @@ -42,6 +42,70 @@ UINT8 mPhysMemAddressWidth; >>> STATIC UINT32 mS3AcpiReservedMemoryBase; >>> STATIC UINT32 mS3AcpiReservedMemorySize; >>> >>> +STATIC UINT32 mXenLowerMemorySize = 0; >>> +STATIC UINT64 mXenHighMemorySize = 0; >>> + >>> +VOID >>> +XenReadMemorySizes ( >>> + VOID >>> + ) >>> +{ >>> + EFI_E820_ENTRY64 *E820Map; >>> + UINT32 E820EntriesCount; >>> + EFI_STATUS Status; >>> + >>> + Status = XenGetE820Map (&E820Map, &E820EntriesCount); >>> + ASSERT_EFI_ERROR (Status); >>> + >>> + mXenLowerMemorySize = 0; >>> + mXenHighMemorySize = 0; >>> + >>> + if (E820EntriesCount > 0) { >>> + EFI_E820_ENTRY64 *Entry; >>> + UINT32 Loop; >>> + >>> + for (Loop = 0; Loop < E820EntriesCount; Loop++) { >>> + Entry = E820Map + Loop; >>> + >>> + // >>> + // Only care about RAM >>> + // >>> + if (Entry->Type != EfiAcpiAddressRangeMemory) { >>> + continue; >>> + } >>> + >>> + if ((Entry->BaseAddr + Entry->Length) <= SIZE_16MB) { >>> + continue; >>> + } >>> + >>> + if (Entry->BaseAddr < SIZE_16MB) { >>> + UINT64 bottom = Entry->BaseAddr; >>> + UINT64 size = Entry->Length; >>> + size -= SIZE_16MB - bottom; >>> + bottom = SIZE_16MB; >>> + mXenLowerMemorySize += size; >>> + continue; >>> + } >>> + if (Entry->BaseAddr <= SIZE_4GB) { >>> + UINT64 size = Entry->Length; >>> + mXenLowerMemorySize += size; >>> + continue; >>> + } >>> + >>> + if (Entry->BaseAddr == SIZE_4GB) { >>> + mXenHighMemorySize = Entry->Length; >>> + continue; >>> + } >>> + >>> + DEBUG ((EFI_D_INFO, "%a: ignored XenE820 entry (0x%llx, 0x%llx)\n", >>> + __FUNCTION__, >>> + Entry->BaseAddr, >>> + Entry->Length)); >>> + } >>> + mXenLowerMemorySize += SIZE_16MB; >>> + } >>> +} >>> + >>> UINT32 >>> GetSystemMemorySizeBelow4gb ( >>> VOID >>> @@ -50,6 +114,9 @@ GetSystemMemorySizeBelow4gb ( >>> UINT8 Cmos0x34; >>> UINT8 Cmos0x35; >>> >>> + if (mXen && mXenLowerMemorySize) >>> + return mXenLowerMemorySize; >>> + >>> // >>> // CMOS 0x34/0x35 specifies the system memory above 16 MB. >>> // * CMOS(0x35) is the high byte >>> @@ -74,6 +141,10 @@ GetSystemMemorySizeAbove4gb ( >>> UINT32 Size; >>> UINTN CmosIndex; >>> >>> + // if lower memory is specified that way, return also high memory >>> + if (mXen && mXenLowerMemorySize) >>> + return mXenHighMemorySize; >>> + >>> // >>> // CMOS 0x5b-0x5d specifies the system memory above 4GB MB. >>> // * CMOS(0x5d) is the most significant size byte >>> diff --git a/OvmfPkg/XenPlatformPei/Platform.c b/OvmfPkg/XenPlatformPei/Platform.c >>> index bf78878..9fc713c 100644 >>> --- a/OvmfPkg/XenPlatformPei/Platform.c >>> +++ b/OvmfPkg/XenPlatformPei/Platform.c >>> @@ -362,6 +362,10 @@ MiscInitialization ( >>> AcpiCtlReg = POWER_MGMT_REGISTER_Q35 (ICH9_ACPI_CNTL); >>> AcpiEnBit = ICH9_ACPI_CNTL_ACPI_EN; >>> break; >>> + case 0xffff: >>> + // xen PVH, ignore >>> + PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, mHostBridgeDevId); >>> + return; >> >> Can we create a new macro for 0xFFFF in >> "OvmfPkg/Include/OvmfPlatforms.h", and use that here? >> >> Normally I would suggest a separate header file under "OvmfPkg/Include", >> similar to "Q35MchIch9.h" and "I440FxPiix4.h", and to include that new >> file in "OvmfPlatforms.h", but here we only need a single "chipset >> identifier", so I guess that can go directly into "OvmfPlatforms.h". >> >> I mainly suggest this because in patch 08/14, the same 0xFFFF case label >> is being added to code shared with QEMU, and using a verbose macro there >> is much better than a magic number. In turn, we should use the same >> macro here, I think. > > This value is just the result of a read to an incorrect location, and > the return value is -1. There is no PCI when running in Xen PVH, at > least for now. I think I should try harder to find out if OVMF is > running in PVH, and so I would not need this case 0xffff at all. > Right, that would be best. In fact, this affects two modules, (Xen)PlatformPei and PlatformBootManagerLib. In both, we already have the XenDetected() function. Maybe you can add a XenPvhDetected() function as well (which would return TRUE only for PVH, while the original XenDetected() would continue returning TRUE for both HVM and PVH). Then the PCI host bridge ID checking could be entirely short-circuited if XenPvhDetected() returned TRUE first. Thanks Laszlo