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

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

On Tue, 16 Jun 2020 at 21:57, Stefano Stabellini <sstabellini@xxxxxxxxxx> 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@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".
> >
> > 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,

-- 
Julien Grall