Date: Mon, 4 Apr 2022 08:34:48 +0200 From: =?UTF-8?q?Corvin=20K=C3=B6hne?= <c.koehne@beckhoff.com> Cc: <virtualization@FreeBSD.org>, =?UTF-8?q?Corvin=20K=C3=B6hne?= <c.koehne@beckhoff.com>, =?UTF-8?q?Corvin=20K=C3=B6hne?= <CorvinK@beckhoff.com>, Ard Biesheuvel <ardb+tianocore@kernel.org>, Jiewen Yao <jiewen.yao@intel.com>, Jordan Justen <jordan.l.justen@intel.com>, Gerd Hoffmann <kraxel@redhat.com>, Rebecca Cran <rebecca@bsdio.com>, Peter Grehan <grehan@freebsd.org>, <devel@edk2.groups.io> Subject: [PATCH] OvmfPkg: reserve igd memory by E820 Message-ID: <20220404063448.280-1-c.koehne@beckhoff.com>
next in thread | raw e-mail | index | archive | help
=EF=BB=BFHi all, I'd like to discuss the following patch. At the moment, it's bhyve=20 specific but I'd like to merge it into stock OVMF too. I'm working=20 on GPU passthrough support for Intels integrated graphics devices=20 (Intel calls it GVT-d). In order to get GVT-d working properly,=20 the guest bios should allocate two memory areas (Graphics Stolen=20 Memory and OpRegion). I'm trying to create a platform independent=20 patch which allows to allocate those regions. At the moment, I'm using an E820 table that bhyve provides to the=20 guest. Another approach would be to use the QemuLoader which is=20 used for loader Qemu's ACPI tables. While this has the advantage=20 that the guest bios allocates the memory by it's own, the=20 hypervisor has less control over the address. E.g. the ACRN=20 hypervisor uses an indentical mapping for the Graphics Stolen=20 Memory on Tiger Lake platforms and above (see=20 https://github.com/projectacrn/acrn-hypervisor/blob/master/devicemodel/hw/p= ci/passthrough.c#L519-L523). What do you think about this patch? Do you prefer using the E820 table=20 or the QemuLoader? Do you have any other ideas? Btw: I'm using the appended patch with an own bhyve implementation for=20 several month now without any issues. Here's the patch: Detecting Graphics Stolen Memory and OpRegion address and length is platform specific. OVMF shouldn't be platform specific. Move the platform specific part to bhyve. Bhyve detects Graphics Stolen Memory and OpRegion address and length and passes these information as E820 table to the firmware. This will additionally allow us to pass other platform specific memory ranges to the guest in the future. Signed-off-by: Corvin K=C3=B6hne <CorvinK@beckhoff.com> Reviewed-by: Patrick Bruenn <p.bruenn@beckhoff.com> --- OvmfPkg/Bhyve/BhyveBhfX64.dsc | 1 + OvmfPkg/Bhyve/PlatformPei/MemDetect.c | 78 ++++++++++++++++++ OvmfPkg/Bhyve/PlatformPei/Platform.c | 127 ++++++++++++++++++++++++++= ++++ OvmfPkg/Bhyve/PlatformPei/PlatformPei.inf | 1 + 4 files changed, 207 insertions(+) diff --git a/OvmfPkg/Bhyve/BhyveBhfX64.dsc b/OvmfPkg/Bhyve/BhyveBhfX64.dsc index 0a81b1de75..03d1fb50d9 100644 --- a/OvmfPkg/Bhyve/BhyveBhfX64.dsc +++ b/OvmfPkg/Bhyve/BhyveBhfX64.dsc @@ -284,6 +284,7 @@ !endif CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuE= xceptionHandlerLib.inf PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf + QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiLib.inf =20 MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLi= b.inf =20 diff --git a/OvmfPkg/Bhyve/PlatformPei/MemDetect.c b/OvmfPkg/Bhyve/Platform= Pei/MemDetect.c index 1949e586a0..0681586927 100644 --- a/OvmfPkg/Bhyve/PlatformPei/MemDetect.c +++ b/OvmfPkg/Bhyve/PlatformPei/MemDetect.c @@ -30,6 +30,7 @@ Module Name: #include <Library/PcdLib.h> #include <Library/PciLib.h> #include <Library/PeimEntryPoint.h> +#include <Library/QemuFwCfgLib.h> #include <Library/ResourcePublicationLib.h> #include <Library/MtrrLib.h> =20 @@ -357,6 +358,9 @@ PublishPeiMemory ( VOID ) { + EFI_E820_ENTRY64 E820Entry; + FIRMWARE_CONFIG_ITEM Item; + UINTN ItemSize; EFI_STATUS Status; EFI_PHYSICAL_ADDRESS MemoryBase; UINT64 MemorySize; @@ -418,6 +422,80 @@ PublishPeiMemory ( } =20 // + // Bhyve uses an E820 table to reserve certain kinds of memory like Grap= hics + // Stolen Memory. These reserved memory regions could overlap with the P= EI + // core memory which leads to undefined behaviour. Check the E820 table = for + // a memory region starting at or below MemoryBase which is equal or lar= ger + // than MemorySize. + // + if (!EFI_ERROR (QemuFwCfgFindFile ("etc/e820", &Item, &ItemSize))) { + // + // Set a new base address based on E820 table. + // + + EFI_PHYSICAL_ADDRESS MaxAddress; + UINT64 EntryEnd; + UINT64 EndAddr; + + if (ItemSize % sizeof (E820Entry) !=3D 0) { + DEBUG ((DEBUG_INFO, "%a: invalid E820 size\n", __FUNCTION__)); + return EFI_PROTOCOL_ERROR; + } + + QemuFwCfgSelectItem (Item); + + MaxAddress =3D MemoryBase + MemorySize; + MemoryBase =3D 0; + + for (UINTN i =3D 0; i < ItemSize; i +=3D sizeof (E820Entry)) { + QemuFwCfgReadBytes (sizeof (E820Entry), &E820Entry); + + if (E820Entry.BaseAddr > MaxAddress) { + // + // E820 is always sorted in ascending order. For that reason, Base= Addr + // will always be larger than MaxAddress for the following entries= . + // + break; + } else if (E820Entry.Type !=3D EfiAcpiAddressRangeMemory) { + continue; + } + + // + // Since MemoryBase should be at top of the memory, calculate the en= d + // address of the entry. Additionally, MemoryBase and MemorySize are= page + // aligned. For that reason, page align EntryEnd too. + // + EntryEnd =3D (E820Entry.BaseAddr + E820Entry.Length) & ~EFI_PAGE_MAS= K; + if (E820Entry.BaseAddr > EntryEnd) { + // + // Either BaseAddr + Length overflows or the entry is smaller than= a + // page. In both cases, we can't use it. + // + continue; + } + + EndAddr =3D MIN (EntryEnd, MaxAddress); + if (EndAddr - E820Entry.BaseAddr < MemorySize) { + // + // E820 entry is too small for the Pei core memory. + // + continue; + } + + MemoryBase =3D EndAddr - MemorySize; + } + + if (MemoryBase =3D=3D 0) { + DEBUG (( + DEBUG_ERROR, + "%a: Unable to find suitable PeiCore address\n", + __FUNCTION__ + )); + return EFI_OUT_OF_RESOURCES; + } + } + + // // Publish this memory to the PEI Core // Status =3D PublishSystemMemory (MemoryBase, MemorySize); diff --git a/OvmfPkg/Bhyve/PlatformPei/Platform.c b/OvmfPkg/Bhyve/PlatformP= ei/Platform.c index eba7c60fce..81062dbf44 100644 --- a/OvmfPkg/Bhyve/PlatformPei/Platform.c +++ b/OvmfPkg/Bhyve/PlatformPei/Platform.c @@ -27,6 +27,7 @@ #include <Library/PciLib.h> #include <Library/PeimEntryPoint.h> #include <Library/PeiServicesLib.h> +#include <Library/QemuFwCfgLib.h> #include <Library/ResourcePublicationLib.h> #include <Guid/MemoryTypeInformation.h> #include <Ppi/MasterBootMode.h> @@ -491,6 +492,118 @@ DebugDumpCmos ( } } =20 +STATIC +EFI_STATUS +E820ReserveMemory ( + VOID + ) +{ + EFI_E820_ENTRY64 E820Entry; + FIRMWARE_CONFIG_ITEM Item; + UINTN Size; + EFI_STATUS Status; + + Status =3D QemuFwCfgFindFile ("etc/e820", &Item, &Size); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_INFO, "%a: E820 not found: %r\n", __FUNCTION__, Status))= ; + return Status; + } + + if (Size % sizeof (E820Entry) !=3D 0) { + DEBUG ((DEBUG_INFO, "%a: invalid E820 size\n", __FUNCTION__)); + return EFI_PROTOCOL_ERROR; + } + + QemuFwCfgSelectItem (Item); + for (UINTN i =3D 0; i < Size; i +=3D sizeof (E820Entry)) { + UINT64 Base; + UINT64 End; + + QemuFwCfgReadBytes (sizeof (E820Entry), &E820Entry); + + DEBUG_CODE ( + CHAR8 *E820TypeName; + switch (E820Entry.Type) { + case EfiAcpiAddressRangeMemory: + E820TypeName =3D "RAM "; + break; + case EfiAcpiAddressRangeReserved: + E820TypeName =3D "Reserved"; + break; + case EfiAcpiAddressRangeACPI: + E820TypeName =3D "ACPI "; + break; + case EfiAcpiAddressRangeNVS: + E820TypeName =3D "NVS "; + break; + default: + E820TypeName =3D "Unknown "; + break; + } + + DEBUG (( + DEBUG_INFO, + "E820 entry [ %16lx, %16lx] (%a)\n", + E820Entry.BaseAddr, + E820Entry.BaseAddr + E820Entry.Length, + E820TypeName + )); + ); + + // + // Round down base and round up length to page boundary + // + Base =3D E820Entry.BaseAddr & ~(UINT64)EFI_PAGE_MASK; + End =3D ALIGN_VALUE ( + E820Entry.BaseAddr + E820Entry.Length, + (UINT64)EFI_PAGE_SIZE + ); + + switch (E820Entry.Type) { + case EfiAcpiAddressRangeReserved: + BuildMemoryAllocationHob ( + Base, + End - Base, + EfiReservedMemoryType + ); + break; + + case EfiAcpiAddressRangeACPI: + BuildMemoryAllocationHob ( + Base, + End - Base, + EfiACPIReclaimMemory + ); + break; + + case EfiAcpiAddressRangeNVS: + BuildMemoryAllocationHob ( + Base, + End - Base, + EfiACPIMemoryNVS + ); + break; + + case EfiAcpiAddressRangeMemory: + // + // EfiAcpiAddressRangeMemory is usable. Do not build an Allocation= HOB. + // + break; + + default: + DEBUG (( + DEBUG_ERROR, + "%a: Unknown E820 type %d\n", + __FUNCTION__, + E820Entry.Type + )); + return EFI_PROTOCOL_ERROR; + } + } + + return EFI_SUCCESS; +} + VOID S3Verification ( VOID @@ -579,6 +692,10 @@ InitializePlatform ( IN CONST EFI_PEI_SERVICES **PeiServices ) { + EFI_STATUS Status; + FIRMWARE_CONFIG_ITEM Item; + UINTN Size; + DEBUG ((DEBUG_INFO, "Platform PEIM Loaded\n")); =20 // @@ -591,6 +708,16 @@ InitializePlatform ( =20 DebugDumpCmos (); =20 + // + // If an E820 table exists, Memory should be reserved by E820. + // + if (!EFI_ERROR (QemuFwCfgFindFile ("etc/e820", &Item, &Size))) { + Status =3D E820ReserveMemory (); + if (EFI_ERROR (Status)) { + return Status; + } + } + BootModeInitialization (); AddressWidthInitialization (); MaxCpuCountInitialization (); diff --git a/OvmfPkg/Bhyve/PlatformPei/PlatformPei.inf b/OvmfPkg/Bhyve/Plat= formPei/PlatformPei.inf index 739d63098b..557a8f890e 100644 --- a/OvmfPkg/Bhyve/PlatformPei/PlatformPei.inf +++ b/OvmfPkg/Bhyve/PlatformPei/PlatformPei.inf @@ -59,6 +59,7 @@ PeiServicesLib PeiServicesTablePointerLib PcdLib + QemuFwCfgLib ResourcePublicationLib =20 [Pcd] --=20 2.11.0 Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Bec= khoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20220404063448.280-1-c.koehne>