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

[Julien Grall <julien@xxxxxxx>]

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.
IIUC the request should always be 56-bytes, so at worse you will read 
unknown bytes. Those bytes are at the end of the structure, so it should 
not matter.
> 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.
I don't think this is necessary because of the way a request has been 
defined.
Cheers,

--
Julien Grall