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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 13 June 2020 19:42
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: paul@xxxxxxx; Julien Grall <jgrall@xxxxxxxxxx>; Andrew Cooper 
> <andrew.cooper3@xxxxxxxxxx>; George
> Dunlap <george.dunlap@xxxxxxxxxx>; Ian Jackson <ian.jackson@xxxxxxxxxxxxx>; 
> Jan Beulich
> <jbeulich@xxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>; Wei
> Liu <wl@xxxxxxx>; Juergen Gross <jgross@xxxxxxxx>
> Subject: [PATCH v3 for-4.14] pvcalls: Document correctly and explicitely the 
> padding for all arches
> 
> 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.

Ugh.

> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

Release-acked-by: Paul Durrant <paul@xxxxxxx>

> 
> ---
>     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
>               } socket;
>               struct xen_pvcalls_connect {
>                       uint64_t id;
> @@ -257,16 +257,18 @@ The format is defined as follows:
>                       uint32_t flags;
>                       grant_ref_t ref;
>                       uint32_t evtchn;
> -                     #ifdef CONFIG_X86_32
> +                     #ifndef __i386__
>                       uint8_t pad[4];
> -                     #endif
> +                     #endif
>               } connect;
>               struct xen_pvcalls_release {
>                       uint64_t id;
>                       uint8_t reuse;
> -                     #ifdef CONFIG_X86_32
> +                     #ifndef __i386__
>                       uint8_t pad[7];
> -                     #endif
> +                     #else
> +                     uint8_t pad[3];
> +                     #endif
>               } release;
>               struct xen_pvcalls_bind {
>                       uint64_t id;
> @@ -276,9 +278,9 @@ The format is defined as follows:
>               struct xen_pvcalls_listen {
>                       uint64_t id;
>                       uint32_t backlog;
> -                     #ifdef CONFIG_X86_32
> +                     #ifndef __i386__
>                       uint8_t pad[4];
> -                     #endif
> +                     #endif
>               } listen;
>               struct xen_pvcalls_accept {
>                       uint64_t id;
> diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
> index cb8171275c13..28374a82f410 100644
> --- a/xen/include/public/io/pvcalls.h
> +++ b/xen/include/public/io/pvcalls.h
> @@ -65,6 +65,9 @@ struct xen_pvcalls_request {
>              uint32_t domain;
>              uint32_t type;
>              uint32_t protocol;
> +#ifndef __i386__
> +            uint8_t pad[4];
> +#endif
>          } socket;
>          struct xen_pvcalls_connect {
>              uint64_t id;
> @@ -73,10 +76,18 @@ struct xen_pvcalls_request {
>              uint32_t flags;
>              grant_ref_t ref;
>              uint32_t evtchn;
> +#ifndef __i386__
> +            uint8_t pad[4];
> +#endif
>          } connect;
>          struct xen_pvcalls_release {
>              uint64_t id;
>              uint8_t reuse;
> +#ifndef __i386__
> +            uint8_t pad[7];
> +#else
> +            uint8_t pad[3];
> +#endif
>          } release;
>          struct xen_pvcalls_bind {
>              uint64_t id;
> @@ -86,6 +97,9 @@ struct xen_pvcalls_request {
>          struct xen_pvcalls_listen {
>              uint64_t id;
>              uint32_t backlog;
> +#ifndef __i386__
> +            uint8_t pad[4];
> +#endif
>          } listen;
>          struct xen_pvcalls_accept {
>              uint64_t id;
> --
> 2.17.1