Re: [Xen-devel] [edk2] [PATCH RFC v2 3/7] OvmfPkg: define EFI_XEN_OVMF_INFO and extend XenInfo

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Sun, Nov 24, 2013 at 05:46:42PM -0800, Jordan Justen wrote:
> On Tue, Nov 19, 2013 at 12:38 PM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> > EFI_XEN_OVMF_INFO is defined to accept configurations from hvmloader. It
> > must match the definition on Xen side.
> >
> > XenInfo is extended to include those bits as well. Currently only E820
> > map is in use.
> >
> > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> >  OvmfPkg/Include/Guid/XenInfo.h |   27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/OvmfPkg/Include/Guid/XenInfo.h b/OvmfPkg/Include/Guid/XenInfo.h
> > index d512b0b..eaeab1a 100644
> > --- a/OvmfPkg/Include/Guid/XenInfo.h
> > +++ b/OvmfPkg/Include/Guid/XenInfo.h
> > @@ -18,6 +18,28 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, 
> > EITHER EXPRESS OR IMPLIED.
> >  #define EFI_XEN_INFO_GUID \
> >      { 0xd3b46f3b, 0xd441, 0x1244, {0x9a, 0x12, 0x0, 0x12, 0x27, 0x3f, 
> > 0xc1, 0x4d } }
> >
> > +#pragma pack(1)
> > +typedef struct {
> > +  CHAR8 Signature[11]; /* XenHVMOVMF\0 */
> > +  CHAR8 Padding[3];
> 
> Does this follow a convention used by Xen? Normally EDK II would use
> either a 4 or 8 byte integer signature with SIGNATURE_32/64.
> 
> This information does not seem all that OVMF specific, but I guess
> from Xen's perspective it is?
> 
> > +  UINT8 Length;     /* Length of this struct */
> > +  UINT8 Checksum;   /* Set such that the sum over bytes 0..length == 0 */
> > +  /*
> > +   * Physical address of an array of tables_nr elements.
> 
> tables_nr?
> 

I modified the actual members of the structure but I forgot to modify
comments. Sorry about this. I will fix them.

> > +   *
> > +   * Each element is a 32 bit value contianing the physical address
> > +   * of a BIOS table.
> > +   */
> > +  UINT32 Tables;
> 
> 64-bit EFI_PHYSICAL_ADDRESS type?
> 

I don't think I can change this. As this structure needs to be
identical to the one on Xen side. Hvmloader runs in 32 bit mode so the
address allocated should be UINT32.

> > +  UINT32 TablesNr;
> 
> Could this be TableCount?
> 
> Is "Tables" unused in this patch series? The purpose doesn't seem
> clear. (Perhaps an updated comment or commit message could help?)
> 

No, not yet. We need to reserve places for future usage though.
Otherwise we need to break this interface when we find out we need to
pass more information.

> > +  /*
> > +   * Physical address of the e820 table, contains e820_nr entries.
> 
> e820_nr?
> 
> > +   */
> > +  UINT32 E820;
> 
> 64-bit EFI_PHYSICAL_ADDRESS type?
> 
> > +  UINT32 E820Nr;
> 
> Maybe E820EntriesCount instead?
> 
> > +} EFI_XEN_OVMF_INFO;
> > +#pragma pack()
> 
> Since this struct defines a Xen <=> OVMF data transfer, maybe we
> should consider a new include file? Or, maybe just a good comment on
> the structure?
> 

As this is one time and one way information transfer so that I don't
think we need dedicated header. I will add comments.

> > +
> >  typedef struct {
> >    ///
> >    /// Beginning of the hypercall page.
> > @@ -35,6 +57,11 @@ typedef struct {
> >    /// Hypervisor minor version.
> >    ///
> >    UINT16 VersionMinor;
> > +  ///
> > +  /// E820 map
> > +  ///
> > +  VOID *E820;
> > +  UINT16 E820EntryCount;
> 
> I think we (previously) made a mistake with this structure. It is a
> HOB, and therefore produced by PEI. HOBs can be consumed by DXE code.
> This means that the VOID* members will be different between PEI and
> DXE when the Ia32X64 build is used. Right now, I think only PEI looks
> at the HOB. This struct is internal to OVMF, so less of a concern than
> the Xen <=> OVMF interface.
> 

Oh, then this one should be EFI_PHYSICAL_ADDRESS. That should fix the
problem, right?

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel