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

[Jordan Justen <jljusten@xxxxxxxxx>]

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?

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

64-bit EFI_PHYSICAL_ADDRESS type?

> +  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?)

> +  /*
> +   * 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?

> +
>  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.

-Jordan

>  } EFI_XEN_INFO;
>
>  extern EFI_GUID gEfiXenInfoGuid;
> --
> 1.7.10.4
>
>
> ------------------------------------------------------------------------------
> Shape the Mobile Experience: Free Subscription
> Software experts and developers: Be at the forefront of tech innovation.
> Intel(R) Software Adrenaline delivers strategic insight and game-changing
> conversations that shape the rapidly evolving mobile landscape. Sign up now.
> http://pubads.g.doubleclick.net/gampad/clk?id=63431311&iu=/4140/ostg.clktrk
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.sourceforge.net/lists/listinfo/edk2-devel

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