Re: [PATCH] pvcalls: Document explicitly the padding for all arches

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.04.2020 12:49, Julien Grall wrote:
> --- a/docs/misc/pvcalls.pandoc
> +++ b/docs/misc/pvcalls.pandoc
> @@ -246,9 +246,7 @@ The format is defined as follows:
>                       uint32_t domain;
>                       uint32_t type;
>                       uint32_t protocol;
> -                     #ifdef CONFIG_X86_32
>                       uint8_t pad[4];
> -                     #endif
>               } socket;
>               struct xen_pvcalls_connect {
>                       uint64_t id;
> @@ -257,16 +255,12 @@ The format is defined as follows:
>                       uint32_t flags;
>                       grant_ref_t ref;
>                       uint32_t evtchn;
> -                     #ifdef CONFIG_X86_32
>                       uint8_t pad[4];
> -                     #endif
>               } connect;
>               struct xen_pvcalls_release {
>                       uint64_t id;
>                       uint8_t reuse;
> -                     #ifdef CONFIG_X86_32
>                       uint8_t pad[7];
> -                     #endif
>               } release;
>               struct xen_pvcalls_bind {
>                       uint64_t id;
> @@ -276,9 +270,7 @@ The format is defined as follows:
>               struct xen_pvcalls_listen {
>                       uint64_t id;
>                       uint32_t backlog;
> -                     #ifdef CONFIG_X86_32
>                       uint8_t pad[4];
> -                     #endif
>               } listen;
>               struct xen_pvcalls_accept {
>                       uint64_t id;

I wonder on what grounds these #ifdef-s had been there - they're
plain wrong with the types used in the public header.

> --- a/xen/include/public/io/pvcalls.h
> +++ b/xen/include/public/io/pvcalls.h
> @@ -65,6 +65,7 @@ struct xen_pvcalls_request {
>              uint32_t domain;
>              uint32_t type;
>              uint32_t protocol;
> +            uint8_t pad[4];
>          } socket;
>          struct xen_pvcalls_connect {
>              uint64_t id;
> @@ -73,10 +74,12 @@ struct xen_pvcalls_request {
>              uint32_t flags;
>              grant_ref_t ref;
>              uint32_t evtchn;
> +            uint8_t pad[4];
>          } connect;
>          struct xen_pvcalls_release {
>              uint64_t id;
>              uint8_t reuse;
> +            uint8_t pad[7];
>          } release;
>          struct xen_pvcalls_bind {
>              uint64_t id;
> @@ -86,6 +89,7 @@ struct xen_pvcalls_request {
>          struct xen_pvcalls_listen {
>              uint64_t id;
>              uint32_t backlog;
> +            uint8_t pad[4];
>          } listen;

I'm afraid we can't change these in such a way - your additions
change sizeof() for the respective sub-structures on 32-bit x86,
and hence this is not a backwards compatible adjustment. The
best I can think of right now that we could do is make the
difference explicit, by putting the padding fields inside
#ifndef __i386__. But of course this is awkward at least when
thinking about a 32-bit / 64-bit pair of communication ends on
an x86-64 host.

Jan