Re: [edk2-devel] [PATCH v4 12/35] OvmfPkg/XenPlatformPei: Grab RSDP from PVH guest start of day struct

["Roger Pau Monné" <roger.pau@citrix.com>]

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.