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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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