| Message ID | 20190729153944.24239-13-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Specific platform to run OVMF in Xen PVH and HVM guests | expand |
On Mon, Jul 29, 2019 at 04:39:21PM +0100, Anthony PERARD wrote: > Check if there's a start of the day struct provided to PVH guest, save > the ACPI RSDP address for later. > > This patch import import arch-x86/hvm/start_info.h from xen.git. You seem to change the types when importing start_info.h, is that absolutely necessary? From my experience working with different projects when importing such headers it's better to keep them verbatim, this makes importing future versions from upstream easier. I have a comment below, but it's more like a question. > diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c > index 5c7d7ddc1c..b366139a0a 100644 > --- a/OvmfPkg/XenPlatformPei/Xen.c > +++ b/OvmfPkg/XenPlatformPei/Xen.c > @@ -25,6 +25,7 @@ > #include <IndustryStandard/E820.h> > #include <Library/ResourcePublicationLib.h> > #include <Library/MtrrLib.h> > +#include <IndustryStandard/Xen/arch-x86/hvm/start_info.h> > > #include "Platform.h" > #include "Xen.h" > @@ -86,6 +87,7 @@ XenConnect ( > UINT32 XenVersion; > EFI_XEN_OVMF_INFO *Info; > CHAR8 Sig[sizeof (Info->Signature) + 1]; > + UINT32 *PVHResetVectorData; > > AsmCpuid (XenLeaf + 2, &TransferPages, &TransferReg, NULL, NULL); > mXenInfo.HyperPages = AllocatePages (TransferPages); > @@ -121,6 +123,29 @@ XenConnect ( > mXenHvmloaderInfo = NULL; > } > > + mXenInfo.RsdpPvh = NULL; > + > + // > + // Locate and use information from the start of day structure if we have > + // booted via the PVH entry point. > + // > + > + PVHResetVectorData = (VOID *)(UINTN) PcdGet32 (PcdXenPvhStartOfDayStructPtr); > + // > + // That magic value is written in XenResetVector/Ia32/XenPVHMain.asm > + // > + if (PVHResetVectorData[1] == SIGNATURE_32 ('X', 'P', 'V', 'H')) { > + struct hvm_start_info *HVMStartInfo; > + > + HVMStartInfo = (VOID *)(UINTN) PVHResetVectorData[0]; > + if (HVMStartInfo->magic == XEN_HVM_START_MAGIC_VALUE) { > + ASSERT (HVMStartInfo->rsdp_paddr != 0); > + if (HVMStartInfo->rsdp_paddr != 0) { > + mXenInfo.RsdpPvh = (VOID *)(UINTN)HVMStartInfo->rsdp_paddr; I guess you can do this because OVMF has an identity map of virtual addresses to physical addresses? I wonder the size of such identity map, and whether you need to check that the rsdp address is indeed inside of that region before converting it to a pointer. The same would apply to PcdXenPvhStartOfDayStructPtr, but that's likely safe because it's always < 4GB, which I assume OVMF always has identity mapped? Thanks, Roger.
On Wed, Aug 07, 2019 at 04:35:58PM +0200, Roger Pau Monné wrote: > On Mon, Jul 29, 2019 at 04:39:21PM +0100, Anthony PERARD wrote: > > Check if there's a start of the day struct provided to PVH guest, save > > the ACPI RSDP address for later. > > > > This patch import import arch-x86/hvm/start_info.h from xen.git. > > You seem to change the types when importing start_info.h, is that > absolutely necessary? I guess one could try to map xen's types to EDKII's type with typedefs, but I'm not sur how well that would work. Importing the xen headers is documented so changing the types is fairly easy, see https://github.com/tianocore/edk2/blob/master/OvmfPkg/Include/IndustryStandard/Xen/README Also, changing the header further might have been something useful to do, we could have match EDKII's naming convention and source files would have looked a bit less weird. > From my experience working with different projects when importing such > headers it's better to keep them verbatim, this makes importing future > versions from upstream easier. > > I have a comment below, but it's more like a question. > > > diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c > > index 5c7d7ddc1c..b366139a0a 100644 > > --- a/OvmfPkg/XenPlatformPei/Xen.c > > +++ b/OvmfPkg/XenPlatformPei/Xen.c > > @@ -25,6 +25,7 @@ > > #include <IndustryStandard/E820.h> > > #include <Library/ResourcePublicationLib.h> > > #include <Library/MtrrLib.h> > > +#include <IndustryStandard/Xen/arch-x86/hvm/start_info.h> > > > > #include "Platform.h" > > #include "Xen.h" > > @@ -86,6 +87,7 @@ XenConnect ( > > UINT32 XenVersion; > > EFI_XEN_OVMF_INFO *Info; > > CHAR8 Sig[sizeof (Info->Signature) + 1]; > > + UINT32 *PVHResetVectorData; > > > > AsmCpuid (XenLeaf + 2, &TransferPages, &TransferReg, NULL, NULL); > > mXenInfo.HyperPages = AllocatePages (TransferPages); > > @@ -121,6 +123,29 @@ XenConnect ( > > mXenHvmloaderInfo = NULL; > > } > > > > + mXenInfo.RsdpPvh = NULL; > > + > > + // > > + // Locate and use information from the start of day structure if we have > > + // booted via the PVH entry point. > > + // > > + > > + PVHResetVectorData = (VOID *)(UINTN) PcdGet32 (PcdXenPvhStartOfDayStructPtr); > > + // > > + // That magic value is written in XenResetVector/Ia32/XenPVHMain.asm > > + // > > + if (PVHResetVectorData[1] == SIGNATURE_32 ('X', 'P', 'V', 'H')) { > > + struct hvm_start_info *HVMStartInfo; > > + > > + HVMStartInfo = (VOID *)(UINTN) PVHResetVectorData[0]; > > + if (HVMStartInfo->magic == XEN_HVM_START_MAGIC_VALUE) { > > + ASSERT (HVMStartInfo->rsdp_paddr != 0); > > + if (HVMStartInfo->rsdp_paddr != 0) { > > + mXenInfo.RsdpPvh = (VOID *)(UINTN)HVMStartInfo->rsdp_paddr; > > I guess you can do this because OVMF has an identity map of virtual > addresses to physical addresses? I think so, yes. I know that early code does created page table like that, but I don't know if later code rework those page table or not. > I wonder the size of such identity map, and whether you need to check > that the rsdp address is indeed inside of that region before > converting it to a pointer. The same would apply to > PcdXenPvhStartOfDayStructPtr, but that's likely safe because it's > always < 4GB, which I assume OVMF always has identity mapped? PcdXenPvhStartOfDayStructPtr is safe because OVMF owns it. As for the rspd_paddr* and the HVMStartInfo*, I need to check. As you say, it's probably fine as long as it's <4GB. I've looked at the comment here: https://github.com/tianocore/edk2/blob/master/OvmfPkg/ResetVector/Ia32/PageTables64.asm#L94 This mean that the code executed in the patch (accessing the hvm start info struct) is executed while the id map is setup up to 4GB. So as long as the struct is below 4G, it's fine. As for the RSDP, I think that pointer is accessed much later, when a different page table is setup, I think that would be that code: https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c But I'm not sure how much is setup. But I'm guessing that whatever is pointed by RSDP, it will be in the 1:1, because we tell the UEFI about it while parsing the e820. Thanks,
On Thu, Aug 08, 2019 at 11:26:41AM +0100, Anthony PERARD wrote: > On Wed, Aug 07, 2019 at 04:35:58PM +0200, Roger Pau Monné wrote: > > On Mon, Jul 29, 2019 at 04:39:21PM +0100, Anthony PERARD wrote: > > > Check if there's a start of the day struct provided to PVH guest, save > > > the ACPI RSDP address for later. > > > > > > This patch import import arch-x86/hvm/start_info.h from xen.git. > > > > You seem to change the types when importing start_info.h, is that > > absolutely necessary? > > I guess one could try to map xen's types to EDKII's type with typedefs, > but I'm not sur how well that would work. Importing the xen headers is > documented so changing the types is fairly easy, see > https://github.com/tianocore/edk2/blob/master/OvmfPkg/Include/IndustryStandard/Xen/README > > Also, changing the header further might have been something useful to > do, we could have match EDKII's naming convention and source files would > have looked a bit less weird. Ack, didn't know there was a README for this procedure. > > From my experience working with different projects when importing such > > headers it's better to keep them verbatim, this makes importing future > > versions from upstream easier. > > > > I have a comment below, but it's more like a question. > > > > > diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c > > > index 5c7d7ddc1c..b366139a0a 100644 > > > --- a/OvmfPkg/XenPlatformPei/Xen.c > > > +++ b/OvmfPkg/XenPlatformPei/Xen.c > > > @@ -25,6 +25,7 @@ > > > #include <IndustryStandard/E820.h> > > > #include <Library/ResourcePublicationLib.h> > > > #include <Library/MtrrLib.h> > > > +#include <IndustryStandard/Xen/arch-x86/hvm/start_info.h> > > > > > > #include "Platform.h" > > > #include "Xen.h" > > > @@ -86,6 +87,7 @@ XenConnect ( > > > UINT32 XenVersion; > > > EFI_XEN_OVMF_INFO *Info; > > > CHAR8 Sig[sizeof (Info->Signature) + 1]; > > > + UINT32 *PVHResetVectorData; > > > > > > AsmCpuid (XenLeaf + 2, &TransferPages, &TransferReg, NULL, NULL); > > > mXenInfo.HyperPages = AllocatePages (TransferPages); > > > @@ -121,6 +123,29 @@ XenConnect ( > > > mXenHvmloaderInfo = NULL; > > > } > > > > > > + mXenInfo.RsdpPvh = NULL; > > > + > > > + // > > > + // Locate and use information from the start of day structure if we have > > > + // booted via the PVH entry point. > > > + // > > > + > > > + PVHResetVectorData = (VOID *)(UINTN) PcdGet32 (PcdXenPvhStartOfDayStructPtr); > > > + // > > > + // That magic value is written in XenResetVector/Ia32/XenPVHMain.asm > > > + // > > > + if (PVHResetVectorData[1] == SIGNATURE_32 ('X', 'P', 'V', 'H')) { > > > + struct hvm_start_info *HVMStartInfo; > > > + > > > + HVMStartInfo = (VOID *)(UINTN) PVHResetVectorData[0]; > > > + if (HVMStartInfo->magic == XEN_HVM_START_MAGIC_VALUE) { > > > + ASSERT (HVMStartInfo->rsdp_paddr != 0); > > > + if (HVMStartInfo->rsdp_paddr != 0) { > > > + mXenInfo.RsdpPvh = (VOID *)(UINTN)HVMStartInfo->rsdp_paddr; > > > > I guess you can do this because OVMF has an identity map of virtual > > addresses to physical addresses? > > I think so, yes. I know that early code does created page table like > that, but I don't know if later code rework those page table or not. > > > I wonder the size of such identity map, and whether you need to check > > that the rsdp address is indeed inside of that region before > > converting it to a pointer. The same would apply to > > PcdXenPvhStartOfDayStructPtr, but that's likely safe because it's > > always < 4GB, which I assume OVMF always has identity mapped? > > PcdXenPvhStartOfDayStructPtr is safe because OVMF owns it. As for the > rspd_paddr* and the HVMStartInfo*, I need to check. As you say, it's > probably fine as long as it's <4GB. > > I've looked at the comment here: > https://github.com/tianocore/edk2/blob/master/OvmfPkg/ResetVector/Ia32/PageTables64.asm#L94 > This mean that the code executed in the patch (accessing the hvm start > info struct) is executed while the id map is setup up to 4GB. So as long > as the struct is below 4G, it's fine. Yes, the start_info struct is guaranteed to be below 4GB because the physical memory address of it is passed on a register when starting the kernel in 32bit protected mode, so the address cannot be greater than 4GB or it would be truncated. > As for the RSDP, I think that pointer is accessed much later, when a > different page table is setup, I think that would be that code: > https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c > But I'm not sure how much is setup. But I'm guessing that whatever is > pointed by RSDP, it will be in the 1:1, because we tell the UEFI about > it while parsing the e820. OK, as long as we know it's safe to access. Note it's quite likely the rsdp is also below 4GB anyway, but better be safe than sorry. Thanks, Roger.
diff --git a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf index d1265c365a..4d00206d09 100644 --- a/OvmfPkg/XenPlatformPei/XenPlatformPei.inf +++ b/OvmfPkg/XenPlatformPei/XenPlatformPei.inf @@ -84,6 +84,9 @@ [Pcd] gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress + gUefiOvmfPkgTokenSpaceGuid.PcdXenPvhStartOfDayStructPtr + gUefiOvmfPkgTokenSpaceGuid.PcdXenPvhStartOfDayStructPtrSize + [FixedPcd] gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress diff --git a/OvmfPkg/Include/Guid/XenInfo.h b/OvmfPkg/Include/Guid/XenInfo.h index b052d618fd..25743b3884 100644 --- a/OvmfPkg/Include/Guid/XenInfo.h +++ b/OvmfPkg/Include/Guid/XenInfo.h @@ -25,6 +25,10 @@ typedef struct { /// Hypervisor minor version. /// UINT16 VersionMinor; + /// + /// Pointer to the RSDP found in the hvm_start_info provided to a PVH guest + /// + VOID *RsdpPvh; } EFI_XEN_INFO; extern EFI_GUID gEfiXenInfoGuid; diff --git a/OvmfPkg/Include/IndustryStandard/Xen/arch-x86/hvm/start_info.h b/OvmfPkg/Include/IndustryStandard/Xen/arch-x86/hvm/start_info.h new file mode 100644 index 0000000000..15708d6dd5 --- /dev/null +++ b/OvmfPkg/Include/IndustryStandard/Xen/arch-x86/hvm/start_info.h @@ -0,0 +1,143 @@ +/* + * SPDX-License-Identifier: MIT + * + * Copyright (c) 2016, Citrix Systems, Inc. + */ + +#ifndef __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ +#define __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ + +/* + * Start of day structure passed to PVH guests and to HVM guests in %ebx. + * + * NOTE: nothing will be loaded at physical address 0, so a 0 value in any + * of the address fields should be treated as not present. + * + * 0 +----------------+ + * | magic | Contains the magic value XEN_HVM_START_MAGIC_VALUE + * | | ("xEn3" with the 0x80 bit of the "E" set). + * 4 +----------------+ + * | version | Version of this structure. Current version is 1. New + * | | versions are guaranteed to be backwards-compatible. + * 8 +----------------+ + * | flags | SIF_xxx flags. + * 12 +----------------+ + * | nr_modules | Number of modules passed to the kernel. + * 16 +----------------+ + * | modlist_paddr | Physical address of an array of modules + * | | (layout of the structure below). + * 24 +----------------+ + * | cmdline_paddr | Physical address of the command line, + * | | a zero-terminated ASCII string. + * 32 +----------------+ + * | rsdp_paddr | Physical address of the RSDP ACPI data structure. + * 40 +----------------+ + * | memmap_paddr | Physical address of the (optional) memory map. Only + * | | present in version 1 and newer of the structure. + * 48 +----------------+ + * | memmap_entries | Number of entries in the memory map table. Zero + * | | if there is no memory map being provided. Only + * | | present in version 1 and newer of the structure. + * 52 +----------------+ + * | reserved | Version 1 and newer only. + * 56 +----------------+ + * + * The layout of each entry in the module structure is the following: + * + * 0 +----------------+ + * | paddr | Physical address of the module. + * 8 +----------------+ + * | size | Size of the module in bytes. + * 16 +----------------+ + * | cmdline_paddr | Physical address of the command line, + * | | a zero-terminated ASCII string. + * 24 +----------------+ + * | reserved | + * 32 +----------------+ + * + * The layout of each entry in the memory map table is as follows: + * + * 0 +----------------+ + * | addr | Base address + * 8 +----------------+ + * | size | Size of mapping in bytes + * 16 +----------------+ + * | type | Type of mapping as defined between the hypervisor + * | | and guest. See XEN_HVM_MEMMAP_TYPE_* values below. + * 20 +----------------| + * | reserved | + * 24 +----------------+ + * + * The address and sizes are always a 64bit little endian unsigned integer. + * + * NB: Xen on x86 will always try to place all the data below the 4GiB + * boundary. + * + * Version numbers of the hvm_start_info structure have evolved like this: + * + * Version 0: Initial implementation. + * + * Version 1: Added the memmap_paddr/memmap_entries fields (plus 4 bytes of + * padding) to the end of the hvm_start_info struct. These new + * fields can be used to pass a memory map to the guest. The + * memory map is optional and so guests that understand version 1 + * of the structure must check that memmap_entries is non-zero + * before trying to read the memory map. + */ +#define XEN_HVM_START_MAGIC_VALUE 0x336ec578 + +/* + * The values used in the type field of the memory map table entries are + * defined below and match the Address Range Types as defined in the "System + * Address Map Interfaces" section of the ACPI Specification. Please refer to + * section 15 in version 6.2 of the ACPI spec: http://uefi.org/specifications + */ +#define XEN_HVM_MEMMAP_TYPE_RAM 1 +#define XEN_HVM_MEMMAP_TYPE_RESERVED 2 +#define XEN_HVM_MEMMAP_TYPE_ACPI 3 +#define XEN_HVM_MEMMAP_TYPE_NVS 4 +#define XEN_HVM_MEMMAP_TYPE_UNUSABLE 5 +#define XEN_HVM_MEMMAP_TYPE_DISABLED 6 +#define XEN_HVM_MEMMAP_TYPE_PMEM 7 + +/* + * C representation of the x86/HVM start info layout. + * + * The canonical definition of this layout is above, this is just a way to + * represent the layout described there using C types. + */ +struct hvm_start_info { + UINT32 magic; /* Contains the magic value 0x336ec578 */ + /* ("xEn3" with the 0x80 bit of the "E" set).*/ + UINT32 version; /* Version of this structure. */ + UINT32 flags; /* SIF_xxx flags. */ + UINT32 nr_modules; /* Number of modules passed to the kernel. */ + UINT64 modlist_paddr; /* Physical address of an array of */ + /* hvm_modlist_entry. */ + UINT64 cmdline_paddr; /* Physical address of the command line. */ + UINT64 rsdp_paddr; /* Physical address of the RSDP ACPI data */ + /* structure. */ + /* All following fields only present in version 1 and newer */ + UINT64 memmap_paddr; /* Physical address of an array of */ + /* hvm_memmap_table_entry. */ + UINT32 memmap_entries; /* Number of entries in the memmap table. */ + /* Value will be zero if there is no memory */ + /* map being provided. */ + UINT32 reserved; /* Must be zero. */ +}; + +struct hvm_modlist_entry { + UINT64 paddr; /* Physical address of the module. */ + UINT64 size; /* Size of the module in bytes. */ + UINT64 cmdline_paddr; /* Physical address of the command line. */ + UINT64 reserved; +}; + +struct hvm_memmap_table_entry { + UINT64 addr; /* Base address of the memory region */ + UINT64 size; /* Size of the memory region in bytes */ + UINT32 type; /* Mapping type */ + UINT32 reserved; /* Must be zero for Version 1. */ +}; + +#endif /* __XEN_PUBLIC_ARCH_X86_HVM_START_INFO_H__ */ diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c index 5c7d7ddc1c..b366139a0a 100644 --- a/OvmfPkg/XenPlatformPei/Xen.c +++ b/OvmfPkg/XenPlatformPei/Xen.c @@ -25,6 +25,7 @@ #include <IndustryStandard/E820.h> #include <Library/ResourcePublicationLib.h> #include <Library/MtrrLib.h> +#include <IndustryStandard/Xen/arch-x86/hvm/start_info.h> #include "Platform.h" #include "Xen.h" @@ -86,6 +87,7 @@ XenConnect ( UINT32 XenVersion; EFI_XEN_OVMF_INFO *Info; CHAR8 Sig[sizeof (Info->Signature) + 1]; + UINT32 *PVHResetVectorData; AsmCpuid (XenLeaf + 2, &TransferPages, &TransferReg, NULL, NULL); mXenInfo.HyperPages = AllocatePages (TransferPages); @@ -121,6 +123,29 @@ XenConnect ( mXenHvmloaderInfo = NULL; } + mXenInfo.RsdpPvh = NULL; + + // + // Locate and use information from the start of day structure if we have + // booted via the PVH entry point. + // + + PVHResetVectorData = (VOID *)(UINTN) PcdGet32 (PcdXenPvhStartOfDayStructPtr); + // + // That magic value is written in XenResetVector/Ia32/XenPVHMain.asm + // + if (PVHResetVectorData[1] == SIGNATURE_32 ('X', 'P', 'V', 'H')) { + struct hvm_start_info *HVMStartInfo; + + HVMStartInfo = (VOID *)(UINTN) PVHResetVectorData[0]; + if (HVMStartInfo->magic == XEN_HVM_START_MAGIC_VALUE) { + ASSERT (HVMStartInfo->rsdp_paddr != 0); + if (HVMStartInfo->rsdp_paddr != 0) { + mXenInfo.RsdpPvh = (VOID *)(UINTN)HVMStartInfo->rsdp_paddr; + } + } + } + BuildGuidDataHob ( &gEfiXenInfoGuid, &mXenInfo,