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

[Julien Grall <julien@xxxxxxx>]

Hi,

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