Re: [edk2-devel] [PATCH v4 4/7] OvmfPkg: Generate CloudHv as a PVH ELF binary

["Boeuf, Sebastien" <sebastien.boeuf@intel.com>]

On Fri, 2022-02-25 at 14:00 +0100, Gerd Hoffmann wrote:
> > +++ b/OvmfPkg/CloudHv/CloudHvX64.fdf
> > @@ -14,8 +14,8 @@
> >  !include OvmfPkg/OvmfPkgDefines.fdf.inc
> >  
> >  #
> > -# Build the variable store and the firmware code as one unified
> > flash device
> > -# image.
> > +# This will allow the flash device image to be recognize as an
> > ELF, with first
> > +# an ELF headers, then the firmware code.
> >  #
> >  [FD.CLOUDHV]
> >  BaseAddress   = $(FW_BASE_ADDRESS)
> > @@ -24,7 +24,9 @@ ErasePolarity = 1
> >  BlockSize     = $(BLOCK_SIZE)
> >  NumBlocks     = $(FW_BLOCKS)
> >  
> > +DEFINE PVH_HEADER_CLOUDHV = TRUE
> 
> I'd place the define in CloudHvX64.dsc, in the [Defines] section,
> where
> all the other DEFINE statements are too.

I wanted to make sure it would only be enabled for this very specific
case.

> 
> >  !include OvmfPkg/VarStore.fdf.inc
> > +DEFINE PVH_HEADER_CLOUDHV = FALSE
> 
> No need to flip that back to FALSE.

Well if I don't flip it back, the [FD.CLOUDHV_VARS] section is build as
a PVH ELF binary as well.
 
> 
> > +!if $(PVH_HEADER_CLOUDHV) == TRUE
> > +!include OvmfPkg/CloudHv/CloudHvElfHeader.fdf.inc
> > +!else
> >  DATA = {
> >    ## This is the EFI_FIRMWARE_VOLUME_HEADER
> >    # ZeroVector []
> > @@ -79,6 +83,7 @@ DATA = {
> >    # FORMATTED: 0x5A #HEALTHY: 0xFE #Reserved: UINT16 #Reserved1:
> > UINT32
> >    0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> >  }
> > +!endif
> 
> Oh.  So the ELF header *replaces* the firmware volume header.
> Didn't notice that before.
> 
> Hmm.  With that the varstore initialization most likely fails.
> There is a fallback path though, with ovmf storing variables
> elsewhere (in normal ram instead of firmware flash I think).
> Persistent variables don't work in that case.
> 
> When the persistent varstore doesn't work anyway (and you are
> ok with that) you probably can drop the space reserved for it
> from the firmware image and use just a single page for the pvh
> elf header (and sharing VarStore.fdf doesn't make sense then).

Sorry it's not entirely clear to me what should be done here. Should I
remove the VARS section since we're not using it anyway?
I'm lacking some knowledge here to understand what this is used for,
and how I actually broke it. I mean with the define and the include, I
ended up modifying exclusively the [FD.CLOUDHV] which is exactly what
was done for Xen. Do you think the Xen VARS section is broken as well?

> 
> take care,
>   Gerd
> 
> 
> 
> 
> 
> 

---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.