From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from SMTP.CITRIX.COM (smtp.citrix.com [66.165.176.89]) (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 AB82C81976 for ; Tue, 10 Jan 2017 08:30:09 -0800 (PST) X-IronPort-AV: E=Sophos;i="5.33,344,1477958400"; d="scan'208";a="398977231" Date: Tue, 10 Jan 2017 16:18:48 +0000 From: Anthony PERARD To: Laszlo Ersek CC: , Message-ID: <20170110161848.GF1903@perard.uk.xensource.com> References: <20161208153340.2285-1-anthony.perard@citrix.com> <20161208153340.2285-7-anthony.perard@citrix.com> <8cc382af-45b2-c836-2907-98f066cca05b@redhat.com> MIME-Version: 1.0 In-Reply-To: <8cc382af-45b2-c836-2907-98f066cca05b@redhat.com> User-Agent: Mutt/1.7.2 (2016-11-26) 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:30:10 -0000 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline 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. -- Anthony PERARD