Re: [PATCH v3 for-4.14] pvcalls: Document correctly and explicitely the padding for all arches

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.06.2020 03:00, Stefano Stabellini wrote:
> On Sat, 13 Jun 2020, Julien Grall wrote:
>> From: Julien Grall <jgrall@xxxxxxxxxx>
>>
>> The documentation of pvcalls suggests there is padding for 32-bit x86
>> at the end of most the structure. However, they are not described in
>> in the public header.
>>
>> Because of that all the structures would be 32-bit aligned and not
>> 64-bit aligned for 32-bit x86.
>>
>> For all the other architectures supported (Arm and 64-bit x86), the
>> structure are aligned to 64-bit because they contain uint64_t field.
>> Therefore all the structures contain implicit padding.
>>
>> The paddings are now corrected for 32-bit x86 and written explicitly for
>> all the architectures.
>>
>> While the structure size between 32-bit and 64-bit x86 is different, it
>> shouldn't cause any incompatibility between a 32-bit and 64-bit
>> frontend/backend because the commands are always 56 bits and the padding
>> are at the end of the structure.
>>
>> As an aside, the padding sadly cannot be mandated to be 0 as they are
>> already present. So it is not going to be possible to use the padding
>> for extending a command in the future.
>>
>> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
>>
>> ---
>>     Changes in v3:
>>         - Use __i386__ rather than CONFIG_X86_32
>>
>>     Changes in v2:
>>         - It is not possible to use the same padding for 32-bit x86 and
>>         all the other supported architectures.
>> ---
>>  docs/misc/pvcalls.pandoc        | 18 ++++++++++--------
>>  xen/include/public/io/pvcalls.h | 14 ++++++++++++++
>>  2 files changed, 24 insertions(+), 8 deletions(-)
>>
>> diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc
>> index 665dad556c39..caa71b36d78b 100644
>> --- a/docs/misc/pvcalls.pandoc
>> +++ b/docs/misc/pvcalls.pandoc
>> @@ -246,9 +246,9 @@ The format is defined as follows:
>>                              uint32_t domain;
>>                              uint32_t type;
>>                              uint32_t protocol;
>> -                            #ifdef CONFIG_X86_32
>> +                    #ifndef __i386__
>>                              uint8_t pad[4];
>> -                            #endif
>> +                    #endif
> 
> 
> Hi Julien,
> 
> Thank you for doing this, and sorry for having missed v2 of this patch, I
> should have replied earlier.
> 
> The intention of the #ifdef blocks like:
> 
>   #ifdef CONFIG_X86_32
>     uint8_t pad[4];
>   #endif
> 
> in pvcalls.pandoc was to make sure that these structs would be 64bit
> aligned on x86_32 too.
> 
> I realize that the public header doesn't match, but the spec is the
> "master copy". We have been saying it for a while (Andy in particular)
> that the specification documents are the one that define the protocol,
> not the public headers. This is the very first time we get to act on
> that statement. What a special occasion this is :-)
> 
> So I think we should keep the specification as is and fix the public
> header instead. Something like v1 of this patch.

But you've read the communication on v1 and v2? A public header
can't be "fixed" if it may already be in use by anyone. We can
only do as Andrew and you suggest (mandate textual descriptions
to be "the ABI") when we do so for _new_ interfaces from the
very beginning, making clear that the public header (if any)
exists just for reference.

I'm not convinced btw that expressing at least the majority of
an ABI by way of a C reference implementation is a bad or
unsuitable thing. It's just that aspects like underlying type
properties need to be very clear and unambiguous.

Jan