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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 20 Apr 2020, Jan Beulich wrote:
> On 20.04.2020 15:34, Julien Grall wrote:
> > Hi Jan,
> > 
> > On 20/04/2020 09:04, Jan Beulich wrote:
> >> 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. 
> > 
> > This is a bit confusing, each structure contain a 64-bit field so
> > I would have thought it the structure would be 8-byte aligned (as
> > on 32-bit Arm). But looking at the spec, a uint64_t will only
> > aligned to 4-byte.
> > 
> > However, I am not sure why sizeof() matters here. I understand
> > the value would be different, but AFAICT, this is not used as part
> > of the protocol.
> 
> Two independent components of a consumer of our interface could
> have a function taking (pointer to) struct xen_pvcalls_connect.
> If one component gets re-built with the new definition and the
> other doesn't, they'll disagree on what range of memory needs
> to be accessible. The instantiating side (using the old header)
> may have ended up placing the struct immediately ahead of a
> page boundary. The consuming side (using the changed header)
> would then encounter a fault if it wanted to access the struct
> as a whole (assignment, memcpy()).

Even if it was possible to use the sub-structs defined in the header
that way, keep in mind that we also wrote:

        /* dummy member to force sizeof(struct xen_pvcalls_request)
         * to match across archs */
        struct xen_pvcalls_dummy {
            uint8_t dummy[56];
        } dummy;

And the spec also clarifies that the size of each specific request is
always 56 bytes. So I think that if we wanted, we could make the changes
Julien is suggesting without worrying about breaking anything.

Speaking of the patch, I think it would be good to have to make things
clearer.