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

[Stefano Stabellini <sstabellini@kernel.org>]

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