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

[Julien Grall <julien@xen.org>]

Hi

Gentle ping. It would be good to get this resolved for Xen 4.14.

On 18/06/2020 16:00, Julien Grall wrote:
> (+ Committers)
> 
> On 18/06/2020 02:34, Stefano Stabellini wrote:
>> On Tue, 16 Jun 2020, Julien Grall wrote:
>>> On Tue, 16 Jun 2020 at 21:57, Stefano Stabellini 
>>> <sstabellini@kernel.org> wrote:
>>>>
>>>> On Tue, 16 Jun 2020, Julien Grall wrote:
>>>>> On 16/06/2020 02:00, Stefano Stabellini wrote:
>>>>>> On Sat, 13 Jun 2020, Julien Grall wrote:
>>>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>>>>
>>>>>>> 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@amazon.com>
>>>>>>>
>>>>>>> ---
>>>>>>>       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".
>>>>>
>>>>> So far, the public headers are the defacto official ABI. So did you 
>>>>> mark the
>>>>> pvcall header as just a reference?
>>>>
>>>> No, there is no document that says that the canonical copy of the
>>>> interface is pvcalls.pandoc. However, it was clearly spelled out from
>>>> the start on xen-devel (see below.)
>>>> In fact, if you notice, this is the
>>>> first document under docs/misc that goes into this level of details in
>>>> describing a new PV protocol. Also note the title of the document which
>>>> is "PV Calls Protocol version 1".
>>>
>>> While I understand this may have been the original intention, you
>>> can't expect a developer to go through the archive to check whether
>>> he/she should trust the header of the document.
>>>
>>>>
>>>>
>>>> In reply to Jan:
>>>>> 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.
>>>>
>>>> What if somebody took the specification of the interface from
>>>> pvcalls.pandoc and wrote their own headers and code? It is definitely
>>>> possible.
>>>
>>> As it is possible for someone to have picked the headers from Xen as
>>> in the past public/ has always been the authority.
>>
>> We never had documents under docs/ before specifying the interfaces
>> before pvcalls. It is not written anywhere that the headers under
>> public/ are the authoritative interfaces either, it is just that it was
>> the only thing available before. If you are new to the project you might
>> go to docs/ first.
>>
>>
>>>> At the time, it was clarified that the purpose of writing such
>>>> a detailed specification document was that the document was the
>>>> specification :-)
>>>
>>> At the risk of being pedantic, if it is not written in xen.git it
>>> doesn't exist ;).
>>>
>>> Anyway, no matter the decision you take here, you are going to
>>> potentially break one set of the users.
>>>
>>> I am leaning towards the header as authoritative because this has
>>> always been the case in the past and nothing in xen.git says
>>> otherwise. However I am not a user of pvcalls, so I don't really have
>>> any big incentive to go either way.
>>
>> Yeah, we are risking breaking one set of users either way :-/
>> In reality, we are using pvcalls on arm64 in a new project (but it is
>> still very green). I am not aware of anybody using pvcalls on x86
>> (especially x86_32).
>>
>> I would prefer to honor the pvcalls.pandoc specification because that is
>> what it was meant to be, and also leads to a better protocol
>> specification.
> 
> As Jan and you disagree on the approach, I would like to get more input.
> 
> To summarize the discussion, the document for PV calls and the public 
> headers don't match when describing the padding. There is a disagreement 
> on which of the two are the authority and therefore which one to fix.
> 
> Does anyone else have a preference on the approach?
> 
>>
>>
>>> For the future, I would highly suggest writing down the support
>>> decision in xen.git. This would avoid such debate on what is the
>>> authority...
>>
>> Yes that's the way to go
>>
> 

-- 
Julien Grall