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

[Jordan Justen <jljusten@xxxxxxxxx>]

On Mon, Nov 25, 2013 at 3:41 AM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> 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:
>> > +   *
>> > +   * 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.

This sounds like an implementation detail on the hvmloader side. It
doesn't seem like a good idea to bake that into the data structure.

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

What about when the next blob type needs to be transferred? How about
something that E820 can fit into, and yet allow for more new blobs to
be passed?

How about something like:
#define XEN_HVM_INFO_SIGNATURE SIGNATURE_32 ('X', 'E', 'N', 'H')
#define XEN_HVM_INFO_ADDRESS BASE_4KB
#define XEN_HVM_INFO_VERSION 0

#define XEN_HVM_DATA_TYPE_E820 SIGNATURE_32 ('E', '8', '2', '0')

#pragma pack(1)
typedef struct {
  UINT32 Type;
  UINT32 Reserved;
  EFI_PHYSICAL_ADDRESS Address;
  UINT64 Length;
} XEN_HVM_INFO_ITEM;

typedef struct {
  UINT32 Signature;
  UINT8 Length;
  UINT8 Checksum;
  UINT8 Version;
  UINT8 Reserved;
  EFI_PHYSICAL_ADDRESS InfoItems;
  UINT32 InfoItemCount;
} XEN_HVM_INFO;
#pragma pack()

(The Reserved fields are for 64-bit alignment.)

You could also consider an E820 alternative:
#define XEN_HVM_DATA_TYPE_RAM_RANGE SIGNATURE_32 ('R', 'A', 'M', 'R')

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

That seems a good reason to put it into a separate header. (Perhaps a
header only present within the one consuming driver.)

-Jordan

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