Re: [RFC PATCH v2 1/2] xen/pvh: Add memory map pointer to hvm_start_info struct

[Jan Beulich]

>>> On 08.12.17 at 20:05, <maran.wilson@xxxxxxxxxx> wrote:
> On 12/8/2017 12:49 AM, Jan Beulich wrote:
>> I'm not convinced of re-using E820 types here. I can see that this
>> might ease the consumption in Linux, but I don't think there should
>> be any connection to x86 aspects here - the data being supplied is
>> x86-agnostic, and Linux'es placement of the header is also making
>> no connection to x86 (oddly enough, the current placement in the
>> Xen tree does, for a reason which escapes me).
>>
>> I could also imagine reasons to add new types without them being
>> sanctioned by whoever maintains E820 type assignments.
> 
> So there are three aspects to discuss here.
> 
> 1) The addition of the "E820_TYPE_xxx" comment. I am fine with just 
> changing that to "mapping type" and leaving it as something to be 
> coordinated between the hypervisor and the guest OS being started by 
> that hypervisor.
> 
> 2) x86 vs x86-agnostic. While I'm trying to keep this interface generic 
> in terms of guest OS (like Linux, FreeBSD, possible other guests in the 
> future) and hypervisor type (Xen, QEMU/KVM, etc), I was actually under 
> the impression that we are dealing with an ABI that is very much x86 
> specific.
> 
> The canonical document describing the ABI 
> (https://xenbits.xen.org/docs/unstable/misc/pvh.html) is titled "x86/HVM 
> direct boot ABI" and goes on to describe an interface in very 
> x86-specific terms. i.e. The ebx register must contain a pointer, cs, 
> ds, es must be set a certain way, etc.
> 
> That is probably why Xen's placement of the header file is in a x86 
> section of the tree. And also why there already exist a number of "x86" 
> references in the existing header file. A quick grep of the existing 
> header file will show lines like:
> 
>     "C representation of the x86/HVM start info layout"
>     "Start of day structure passed to PVH guests and to HVM guests in %ebx"
>     "Xen on x86 will always try to place all the data below the 4GiB"
> 
> If at some point in the future someone decides to implement a similar 
> ABI for a different CPU architecture while re-using this same 
> hvm_start_info struct, then this header will have to be redone a bit 
> anyway. But I'm not aware of any other such ABI that exists or is 
> currently in the works.

Anything like this being in the works elsewhere doesn't mean much.
Otherwise you'd advocate for anyone introducing something new
on a single architecture only to ignore all portability concerns. My
fundamental point here is that within the currently defined
structures there's nothing x86 specific, no matter that a few
comments are mentioning x86 (and in at least two of the three
cases for obvious [implementation] reasons rather than to
explain why this interface can/should be x86 only).

> 3) The (packed) layout of the hvm_memmap_table_entry struct. I did 
> initially consider just making this a new structure that did not 
> necessarily match struct e820_entry in its array layout. But, it's not 
> just the consumer that has an easier time digesting it in the e820_entry 
> array format. It's also the producer side (QEMU for instance) where code 
> already exists to lay out this information in e820_entry array format. 
> And since this is all x86 specific anyway, it just seemed like I would 
> be needlessly making more work for both ends by inventing a completely 
> new memory map layout just for the sake of being different. Especially 
> when there doesn't seem to be anything terribly broken about the 
> existing e820_entry array format as a general purpose memory map.

The brokenness is the mis-alignment of every other array member.
This doesn't matter on x86, but it would matter on any architecture
requiring strict alignment. Furthermore please note that in the
canonical headers you can't even express what you want: Use of
#pragma pack or the packed attribute is prohibited there - we
demand that the headers can be used by any C89-compatible
compiler (lately there have been a few extensions requiring C99,
but these need to be opted in for by consumers, which imo is not
an option here).

Jan