[v3,for-4.14] pvcalls: Document correctly and explicitely the padding for all arches
diff mbox series

Message ID 20200613184132.11880-1-julien@xen.org
State New
Headers show
Series
  • [v3,for-4.14] pvcalls: Document correctly and explicitely the padding for all arches
Related show

Commit Message

Julien Grall June 13, 2020, 6:41 p.m. UTC
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(-)

Comments

Paul Durrant June 15, 2020, 8:26 a.m. UTC | #1
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 13 June 2020 19:42
> To: xen-devel@lists.xenproject.org
> Cc: paul@xen.org; Julien Grall <jgrall@amazon.com>; Andrew Cooper <andrew.cooper3@citrix.com>; George
> Dunlap <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Jan Beulich
> <jbeulich@suse.com>; Julien Grall <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Wei
> Liu <wl@xen.org>; Juergen Gross <jgross@suse.com>
> Subject: [PATCH v3 for-4.14] pvcalls: Document correctly and explicitely the padding for all arches
> 
> 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.

Ugh.

> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Release-acked-by: Paul Durrant <paul@xen.org>

> 
> ---
>     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
>      		} socket;
>      		struct xen_pvcalls_connect {
>      			uint64_t id;
> @@ -257,16 +257,18 @@ The format is defined as follows:
>      			uint32_t flags;
>      			grant_ref_t ref;
>      			uint32_t evtchn;
> -    			#ifdef CONFIG_X86_32
> +			#ifndef __i386__
>      			uint8_t pad[4];
> -    			#endif
> +			#endif
>      		} connect;
>      		struct xen_pvcalls_release {
>      			uint64_t id;
>      			uint8_t reuse;
> -    			#ifdef CONFIG_X86_32
> +			#ifndef __i386__
>      			uint8_t pad[7];
> -    			#endif
> +			#else
> +			uint8_t pad[3];
> +			#endif
>      		} release;
>      		struct xen_pvcalls_bind {
>      			uint64_t id;
> @@ -276,9 +278,9 @@ The format is defined as follows:
>      		struct xen_pvcalls_listen {
>      			uint64_t id;
>      			uint32_t backlog;
> -    			#ifdef CONFIG_X86_32
> +			#ifndef __i386__
>      			uint8_t pad[4];
> -    			#endif
> +			#endif
>      		} listen;
>      		struct xen_pvcalls_accept {
>      			uint64_t id;
> diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
> index cb8171275c13..28374a82f410 100644
> --- a/xen/include/public/io/pvcalls.h
> +++ b/xen/include/public/io/pvcalls.h
> @@ -65,6 +65,9 @@ struct xen_pvcalls_request {
>              uint32_t domain;
>              uint32_t type;
>              uint32_t protocol;
> +#ifndef __i386__
> +            uint8_t pad[4];
> +#endif
>          } socket;
>          struct xen_pvcalls_connect {
>              uint64_t id;
> @@ -73,10 +76,18 @@ struct xen_pvcalls_request {
>              uint32_t flags;
>              grant_ref_t ref;
>              uint32_t evtchn;
> +#ifndef __i386__
> +            uint8_t pad[4];
> +#endif
>          } connect;
>          struct xen_pvcalls_release {
>              uint64_t id;
>              uint8_t reuse;
> +#ifndef __i386__
> +            uint8_t pad[7];
> +#else
> +            uint8_t pad[3];
> +#endif
>          } release;
>          struct xen_pvcalls_bind {
>              uint64_t id;
> @@ -86,6 +97,9 @@ struct xen_pvcalls_request {
>          struct xen_pvcalls_listen {
>              uint64_t id;
>              uint32_t backlog;
> +#ifndef __i386__
> +            uint8_t pad[4];
> +#endif
>          } listen;
>          struct xen_pvcalls_accept {
>              uint64_t id;
> --
> 2.17.1
Stefano Stabellini June 16, 2020, 1 a.m. UTC | #2
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". 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.
Jan Beulich June 16, 2020, 7:13 a.m. UTC | #3
On 16.06.2020 03: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". 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
Jan Beulich June 16, 2020, 8:26 a.m. UTC | #4
On 13.06.2020 20:41, Julien Grall wrote:
> @@ -73,10 +76,18 @@ struct xen_pvcalls_request {
>              uint32_t flags;
>              grant_ref_t ref;
>              uint32_t evtchn;
> +#ifndef __i386__
> +            uint8_t pad[4];
> +#endif

Where possible I think uint32_t would be slightly better to use.

>          } connect;
>          struct xen_pvcalls_release {
>              uint64_t id;
>              uint8_t reuse;
> +#ifndef __i386__
> +            uint8_t pad[7];
> +#else
> +            uint8_t pad[3];
> +#endif

For this I'd recommend uniform "uint8_t pad[3];" (i.e. outside
of any #ifdef) followed by a uint32_t again inside the #ifdef.

Expressing everything through e.g. alignof() would seem even
better, but I can't currently think of a way to do so cleanly.

In any event - I'm not the maintainer of these headers, so it
may be just me ...

Jan
Julien Grall June 16, 2020, 9:19 a.m. UTC | #5
On 16/06/2020 09:26, Jan Beulich wrote:
> On 13.06.2020 20:41, Julien Grall wrote:
>> @@ -73,10 +76,18 @@ struct xen_pvcalls_request {
>>               uint32_t flags;
>>               grant_ref_t ref;
>>               uint32_t evtchn;
>> +#ifndef __i386__
>> +            uint8_t pad[4];
>> +#endif
> 
> Where possible I think uint32_t would be slightly better to use.

OOI, why?

> 
>>           } connect;
>>           struct xen_pvcalls_release {
>>               uint64_t id;
>>               uint8_t reuse;
>> +#ifndef __i386__
>> +            uint8_t pad[7];
>> +#else
>> +            uint8_t pad[3];
>> +#endif
> 
> For this I'd recommend uniform "uint8_t pad[3];" (i.e. outside
> of any #ifdef) followed by a uint32_t again inside the #ifdef.

Same question here. The more the padding cannot be re-used.

> 
> Expressing everything through e.g. alignof() would seem even
> better, but I can't currently think of a way to do so cleanly.

I am afraid I don't have time to look at how alignof() can work nicely. 
Feel free to send a follow-up or pick-up the patch is you really want 
alignof().

Cheers,
Jan Beulich June 16, 2020, 9:36 a.m. UTC | #6
On 16.06.2020 11:19, Julien Grall wrote:
> 
> 
> On 16/06/2020 09:26, Jan Beulich wrote:
>> On 13.06.2020 20:41, Julien Grall wrote:
>>> @@ -73,10 +76,18 @@ struct xen_pvcalls_request {
>>>               uint32_t flags;
>>>               grant_ref_t ref;
>>>               uint32_t evtchn;
>>> +#ifndef __i386__
>>> +            uint8_t pad[4];
>>> +#endif
>>
>> Where possible I think uint32_t would be slightly better to use.
> 
> OOI, why?

Because everything else here uses the wider type, plus the
question of why use a compound type (array) when a simple
one does.

>>
>>>           } connect;
>>>           struct xen_pvcalls_release {
>>>               uint64_t id;
>>>               uint8_t reuse;
>>> +#ifndef __i386__
>>> +            uint8_t pad[7];
>>> +#else
>>> +            uint8_t pad[3];
>>> +#endif
>>
>> For this I'd recommend uniform "uint8_t pad[3];" (i.e. outside
>> of any #ifdef) followed by a uint32_t again inside the #ifdef.
> 
> Same question here. The more the padding cannot be re-used.
> 
>>
>> Expressing everything through e.g. alignof() would seem even
>> better, but I can't currently think of a way to do so cleanly.
> 
> I am afraid I don't have time to look at how alignof() can work nicely. 
> Feel free to send a follow-up or pick-up the patch is you really want 
> alignof().

I didn't mean to ask that you find a solution. I merely pointed
out that expressing things properly rather than using hard coded
numbers would be nice.

Jan
Julien Grall June 16, 2020, 9:39 a.m. UTC | #7
Hi,

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?

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

Well, the header is now part of multiple open-source projects (don't 
know about private). So adding padding will result to incompatibility 
between two x86 32-bit entities that may disagree on the ABI if they are 
using the per-command structure.

TBH, I don't think the issue is worth the breakage here. You will never 
be able to use those paddings in any case as they are not reserved as 0.

Cheers,
Julien Grall June 16, 2020, 9:42 a.m. UTC | #8
On 16/06/2020 10:36, Jan Beulich wrote:
> On 16.06.2020 11:19, Julien Grall wrote:
>>
>>
>> On 16/06/2020 09:26, Jan Beulich wrote:
>>> On 13.06.2020 20:41, Julien Grall wrote:
>>>> @@ -73,10 +76,18 @@ struct xen_pvcalls_request {
>>>>                uint32_t flags;
>>>>                grant_ref_t ref;
>>>>                uint32_t evtchn;
>>>> +#ifndef __i386__
>>>> +            uint8_t pad[4];
>>>> +#endif
>>>
>>> Where possible I think uint32_t would be slightly better to use.
>>
>> OOI, why?
> 
> Because everything else here uses the wider type, plus the
> question of why use a compound type (array) when a simple
> one does.
This is pretty much a matter of taste. In this case, I decided to 
specify the padding the same way accross all the structure.

Cheers,
Stefano Stabellini June 16, 2020, 8:57 p.m. UTC | #9
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".


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. At the time, it was clarified that the purpose of writing such
a detailed specification document was that the document was the
specification :-)

See for instance this email (there are others):

https://marc.info/?l=linux-kernel&m=149815619513930&w=2

"""
This was done differently in the past, but now that we have a formal
process, a person in charge of new PV drivers reviews, and design
documents with clearly spelled out ABIs, I consider the design docs
under docs/misc as the official specification. We don't need headers
anymore, they are redundant. In fact, we cannot have two specifications,
and the design docs are certainly the official ones (we don't want the
specs to be written as header files in C). To me, the headers under
xen/include/public/io/ are optional helpers. It doesn't matter what's in
there, or if frontends and backends use them or not.

There is really an argument for removing those headers, because they
might get out of sync with the spec by mistake, and in those cases, then
we really end up with two specifications for the same protocol. I would
be in favor of `git rm'ing all files under xen/include/public/io/ for
which we have a complete design doc under docs/misc.
"""

(Andy and Roger agreed on the thread if you look at the follow-up
emails.)
Julien Grall June 16, 2020, 9:31 p.m. UTC | #10
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.

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

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

Cheers,
Stefano Stabellini June 18, 2020, 1:34 a.m. UTC | #11
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.


> 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 June 18, 2020, 3 p.m. UTC | #12
(+ 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 June 24, 2020, 11:29 a.m. UTC | #13
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
>>
>
Ian Jackson June 24, 2020, 12:04 p.m. UTC | #14
Julien Grall writes ("Re: [PATCH v3 for-4.14] pvcalls: Document correctly and explicitely the padding for all arches"):
> (+ Committers)
...
> 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?

Hi.

>[Jan:]
>> 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.

I think the practice of using headers as protocol specs is not a
particularly good one.  Certainly my expectations anywhere outside the
Xen Project is that if there's a doc, that is at the very least on par
with any header file.  Of course there are possible compatibility
implications:

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

pvcalls in Linux is Tech Preview / Experimental AFAICT ?  I think that
means we can de jure change things like this.

And it seems that we don't think there are any actual users who would
experience compatibility problems.

So I don't think the compatibility concerns are a reason not to change
the header rather than the document.

So I think my conclusion is the same as Julien's: we should change the
header to match the doc.

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

Maybe it would be worth putting a note somewhere in the headers saying
the headers are provided for convenience but that the ABIs and
protocols are as specified in the docs (at least where docs exist).

Ian.
Ian Jackson June 24, 2020, 12:05 p.m. UTC | #15
Julien Grall writes ("Re: [INPUT REQUESTED][PATCH v3 for-4.14] pvcalls: Document correctly and explicitely the padding for all arches"):
> Gentle ping. It would be good to get this resolved for Xen 4.14.

Oh and I agree that this should be changed for 4.14.

Ian.
Julien Grall June 26, 2020, 5:46 p.m. UTC | #16
Hi Ian,

Thank you for your input!

On 24/06/2020 13:04, Ian Jackson wrote:
> Julien Grall writes ("Re: [PATCH v3 for-4.14] pvcalls: Document correctly and explicitely the padding for all arches"):
>> (+ Committers)
> ...
>> 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?
> 
> Hi.
> 
>> [Jan:]
>>> 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.
> 
> I think the practice of using headers as protocol specs is not a
> particularly good one.  Certainly my expectations anywhere outside the
> Xen Project is that if there's a doc, that is at the very least on par
> with any header file.  Of course there are possible compatibility
> implications:
> 
>> 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.
> 
> pvcalls in Linux is Tech Preview / Experimental AFAICT ?  I think that
> means we can de jure change things like this.

SUPPORT.md suggests this is a Tech Preview, so I agree that we could 
still change the interface.

> 
> And it seems that we don't think there are any actual users who would
> experience compatibility problems.

Right, that's what Stefano suggested.

> 
> So I don't think the compatibility concerns are a reason not to change
> the header rather than the document.
> 
> So I think my conclusion is the same as Julien's: we should change the
> header to match the doc.

Ok, so you are on the same page as Stefano. I will revert to the v1 
change and rework the commit message then.

> 
>>>> 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
> 
> Maybe it would be worth putting a note somewhere in the headers saying
> the headers are provided for convenience but that the ABIs and
> protocols are as specified in the docs (at least where docs exist).

I will write a patch for it.

Cheers,

Patch
diff mbox series

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
     		} socket;
     		struct xen_pvcalls_connect {
     			uint64_t id;
@@ -257,16 +257,18 @@  The format is defined as follows:
     			uint32_t flags;
     			grant_ref_t ref;
     			uint32_t evtchn;
-    			#ifdef CONFIG_X86_32
+			#ifndef __i386__
     			uint8_t pad[4];
-    			#endif
+			#endif
     		} connect;
     		struct xen_pvcalls_release {
     			uint64_t id;
     			uint8_t reuse;
-    			#ifdef CONFIG_X86_32
+			#ifndef __i386__
     			uint8_t pad[7];
-    			#endif
+			#else
+			uint8_t pad[3];
+			#endif
     		} release;
     		struct xen_pvcalls_bind {
     			uint64_t id;
@@ -276,9 +278,9 @@  The format is defined as follows:
     		struct xen_pvcalls_listen {
     			uint64_t id;
     			uint32_t backlog;
-    			#ifdef CONFIG_X86_32
+			#ifndef __i386__
     			uint8_t pad[4];
-    			#endif
+			#endif
     		} listen;
     		struct xen_pvcalls_accept {
     			uint64_t id;
diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
index cb8171275c13..28374a82f410 100644
--- a/xen/include/public/io/pvcalls.h
+++ b/xen/include/public/io/pvcalls.h
@@ -65,6 +65,9 @@  struct xen_pvcalls_request {
             uint32_t domain;
             uint32_t type;
             uint32_t protocol;
+#ifndef __i386__
+            uint8_t pad[4];
+#endif
         } socket;
         struct xen_pvcalls_connect {
             uint64_t id;
@@ -73,10 +76,18 @@  struct xen_pvcalls_request {
             uint32_t flags;
             grant_ref_t ref;
             uint32_t evtchn;
+#ifndef __i386__
+            uint8_t pad[4];
+#endif
         } connect;
         struct xen_pvcalls_release {
             uint64_t id;
             uint8_t reuse;
+#ifndef __i386__
+            uint8_t pad[7];
+#else
+            uint8_t pad[3];
+#endif
         } release;
         struct xen_pvcalls_bind {
             uint64_t id;
@@ -86,6 +97,9 @@  struct xen_pvcalls_request {
         struct xen_pvcalls_listen {
             uint64_t id;
             uint32_t backlog;
+#ifndef __i386__
+            uint8_t pad[4];
+#endif
         } listen;
         struct xen_pvcalls_accept {
             uint64_t id;