Re: [edk2-devel] [PATCH v2 07/31] OvmfPkg/XenResetVector: Saving start of day pointer for PVH guests

["Laszlo Ersek" <lersek@redhat.com>]

On 04/09/19 13:08, Anthony PERARD wrote:
> As described in the Xen PVH documentation [1], "ebx: contains the
> physical memory address where the loader has placed the boot start info
> structure". To have this pointer saved to be able to use it later in the
> PEI phase, we allocate some space in the MEMFD for it. We use 'XPVH' as
> a signature (for "Xen PVH").
> 
> [1] https://xenbits.xenproject.org/docs/unstable/misc/pvh.html
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
>  OvmfPkg/OvmfPkg.dec                         | 4 ++++
>  OvmfPkg/XenOvmf.fdf                         | 4 ++++
>  OvmfPkg/XenResetVector/XenResetVector.inf   | 3 +++
>  OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm  | 4 ++++
>  OvmfPkg/XenResetVector/XenResetVector.nasmb | 2 ++
>  5 files changed, 17 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index e50c6179a2..1cbbc49a6b 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -127,6 +127,10 @@ [PcdsFixedAtBuild]
>    gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize|0x0|UINT32|0x1a
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecompressionScratchEnd|0x0|UINT32|0x1f
>  
> +  # Used by XenOvmf

(1) I suggest dropping this comment (it'd have to be updated to OvmfXen
anyway), and...

> +  gUefiOvmfPkgTokenSpaceGuid.PcdXenStartOfDayStructPtr|0x0|UINT32|0x30
> +  gUefiOvmfPkgTokenSpaceGuid.PcdXenStartOfDayStructPtrSize|0x0|UINT32|0x31
> +

(2) calling these PcdXenPvh* rather than just PcdXen*

(my understanding is that they are specific to PVH)

(3) Please use token values 0x17 and 0x28. That should decrease the
token space fragmentation.

With those addressed:

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

>  [PcdsDynamic, PcdsDynamicEx]
>    gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
> diff --git a/OvmfPkg/XenOvmf.fdf b/OvmfPkg/XenOvmf.fdf
> index 20ebacd673..f9e58befd6 100644
> --- a/OvmfPkg/XenOvmf.fdf
> +++ b/OvmfPkg/XenOvmf.fdf
> @@ -159,6 +159,10 @@ [FD.MEMFD]
>  0x007000|0x001000
>  gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>  
> +0x008000|0x001000
> +# Used by XenResetVector to communicate with XenPlatformPei
> +gUefiOvmfPkgTokenSpaceGuid.PcdXenStartOfDayStructPtr|gUefiOvmfPkgTokenSpaceGuid.PcdXenStartOfDayStructPtrSize
> +
>  0x010000|0x010000
>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>  
> diff --git a/OvmfPkg/XenResetVector/XenResetVector.inf b/OvmfPkg/XenResetVector/XenResetVector.inf
> index 5c05f02285..ec98c17876 100644
> --- a/OvmfPkg/XenResetVector/XenResetVector.inf
> +++ b/OvmfPkg/XenResetVector/XenResetVector.inf
> @@ -41,3 +41,6 @@ [BuildOptions]
>  [Pcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
> +
> +  gUefiOvmfPkgTokenSpaceGuid.PcdXenStartOfDayStructPtr
> +  gUefiOvmfPkgTokenSpaceGuid.PcdXenStartOfDayStructPtrSize
> diff --git a/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> index c4802bf4d1..4e55b0ac1f 100644
> --- a/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> +++ b/OvmfPkg/XenResetVector/Ia32/XenPVHMain.asm
> @@ -22,6 +22,10 @@ xenPVHMain:
>      ; ESP -  Initial value of the EAX register (BIST: Built-in Self Test)
>      mov     esp, eax
>  
> +    ;; Store "Start of day" struct pointer for later use
> +    mov     dword[PVH_SPACE (0)], ebx
> +    mov     dword[PVH_SPACE (4)], 'XPVH'
> +
>      cli
>  
>      mov     ebx, ADDR_OF(gdtr)
> diff --git a/OvmfPkg/XenResetVector/XenResetVector.nasmb b/OvmfPkg/XenResetVector/XenResetVector.nasmb
> index d5a791c139..50cb81fcd1 100644
> --- a/OvmfPkg/XenResetVector/XenResetVector.nasmb
> +++ b/OvmfPkg/XenResetVector/XenResetVector.nasmb
> @@ -41,6 +41,8 @@
>  
>  %include "CommonMacros.inc"
>  
> +%define PVH_SPACE(Offset) (FixedPcdGet32 (PcdXenStartOfDayStructPtr) + (Offset))
> +
>  %include "PostCodes.inc"
>  
>  %ifdef DEBUG_PORT80
>