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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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


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