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

[Julien Grall <julien@xxxxxxx>]

(+ 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@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.
> 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