pvcalls: Document explicitly the padding for all arches
diff mbox series

Message ID 20200419104948.31200-1-julien@xen.org
State New
Headers show
Series
  • pvcalls: Document explicitly the padding for all arches
Related show

Commit Message

Julien Grall April 19, 2020, 10:49 a.m. UTC
From: Julien Grall <jgrall@amazon.com>

The documentation of pvcalls only describes the padding for i386. This
is a bit odd as there are some implicit padding for 64-bit (e.g in
xen_pvcalls_release) and this doesn't cater other 32-bit arch.

Remove the #ifdef in the documentation to show the padding is present on
all arches and take the opportunity to describe the padding in the
public header as well.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---

AFAICT, we cannot mandate the padding to be 0 as they are already
present.
---
 docs/misc/pvcalls.pandoc        | 8 --------
 xen/include/public/io/pvcalls.h | 4 ++++
 2 files changed, 4 insertions(+), 8 deletions(-)

Comments

Jan Beulich April 20, 2020, 8:04 a.m. UTC | #1
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
Julien Grall April 20, 2020, 1:34 p.m. UTC | #2
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,
Jan Beulich April 20, 2020, 1:45 p.m. UTC | #3
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()).

Jan
Stefano Stabellini April 21, 2020, 11:27 p.m. UTC | #4
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.
Jan Beulich April 22, 2020, 9:20 a.m. UTC | #5
On 22.04.2020 01:27, Stefano Stabellini wrote:
> 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;

This has nothing to do with how a consumer may use the structs.

> And the spec also clarifies that the size of each specific request is
> always 56 bytes.

Sure, and I didn't mean to imply that a consumer would be allowed
to break this requirement. Still something like this

int pvcall_new_socket(struct xen_pvcalls_socket *s) {
    struct xen_pvcalls_request req = {
        .req_id = REQ_ID,
        .cmd = PVCALLS_SOCKET,
        .u.socket = *s,
    };

    return pvcall(&req);
}

may break.

Jan
Julien Grall April 29, 2020, 2:01 p.m. UTC | #6
Hi,

On 22/04/2020 10:20, Jan Beulich wrote:
>> 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;
> 
> This has nothing to do with how a consumer may use the structs.
> 
>> And the spec also clarifies that the size of each specific request is
>> always 56 bytes.
> 
> Sure, and I didn't mean to imply that a consumer would be allowed
> to break this requirement. Still something like this
> 
> int pvcall_new_socket(struct xen_pvcalls_socket *s) {
>      struct xen_pvcalls_request req = {
>          .req_id = REQ_ID,
>          .cmd = PVCALLS_SOCKET,
>          .u.socket = *s,
>      };
> 
>      return pvcall(&req);
> }
> 
> may break.

I think I understand your concern now. So yes I agree this would break 
32-bit consumer.

As the padding is at the end of the structure, I think a 32-bit frontend 
and 64-bit backend (or vice-versa) should currently work without any 
trouble. The problem would come later if we decide to extend a command.

I will document the padding only for non 32-bit x86 guest and rework the 
documentation.

Cheers,
Jan Beulich April 29, 2020, 2:05 p.m. UTC | #7
On 29.04.2020 16:01, Julien Grall wrote:
> Hi,
> 
> On 22/04/2020 10:20, Jan Beulich wrote:
>>> 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;
>>
>> This has nothing to do with how a consumer may use the structs.
>>
>>> And the spec also clarifies that the size of each specific request is
>>> always 56 bytes.
>>
>> Sure, and I didn't mean to imply that a consumer would be allowed
>> to break this requirement. Still something like this
>>
>> int pvcall_new_socket(struct xen_pvcalls_socket *s) {
>>      struct xen_pvcalls_request req = {
>>          .req_id = REQ_ID,
>>          .cmd = PVCALLS_SOCKET,
>>          .u.socket = *s,
>>      };
>>
>>      return pvcall(&req);
>> }
>>
>> may break.
> 
> I think I understand your concern now. So yes I agree this would break 32-bit consumer.
> 
> As the padding is at the end of the structure, I think a 32-bit frontend and 64-bit backend (or vice-versa) should currently work without any trouble. The problem would come later if we decide to extend a command.

Can commands be extended at all, i.e. don't extensions require new
commands? The issue I've described has nothing to do with future
extending of any of the affected structures.

Jan
Julien Grall April 29, 2020, 2:14 p.m. UTC | #8
Hi Jan,

On 29/04/2020 15:05, Jan Beulich wrote:
> On 29.04.2020 16:01, Julien Grall wrote:
>> Hi,
>>
>> On 22/04/2020 10:20, Jan Beulich wrote:
>>>> 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;
>>>
>>> This has nothing to do with how a consumer may use the structs.
>>>
>>>> And the spec also clarifies that the size of each specific request is
>>>> always 56 bytes.
>>>
>>> Sure, and I didn't mean to imply that a consumer would be allowed
>>> to break this requirement. Still something like this
>>>
>>> int pvcall_new_socket(struct xen_pvcalls_socket *s) {
>>>       struct xen_pvcalls_request req = {
>>>           .req_id = REQ_ID,
>>>           .cmd = PVCALLS_SOCKET,
>>>           .u.socket = *s,
>>>       };
>>>
>>>       return pvcall(&req);
>>> }
>>>
>>> may break.
>>
>> I think I understand your concern now. So yes I agree this would break 32-bit consumer.
>>
>> As the padding is at the end of the structure, I think a 32-bit frontend and 64-bit backend (or vice-versa) should currently work without any trouble. The problem would come later if we decide to extend a command.
> 
> Can commands be extended at all, i.e. don't extensions require new
> commands? The issue I've described has nothing to do with future
> extending of any of the affected structures.

I think my point wasn't conveyed correctly. The implicit padding is at 
the end of the structure for all the consumers but 32-bit x86. So 
without any modification, I think 32-bit frontend can still communicate 
with 64-bit backend (or vice-versa).

Therefore I suggest to rework the documentation and add the implicit 
padding just for all the architectures but 32-bit x86.

Cheers,
Jan Beulich April 29, 2020, 2:56 p.m. UTC | #9
On 29.04.2020 16:14, Julien Grall wrote:
> Hi Jan,
> 
> On 29/04/2020 15:05, Jan Beulich wrote:
>> On 29.04.2020 16:01, Julien Grall wrote:
>>> Hi,
>>>
>>> On 22/04/2020 10:20, Jan Beulich wrote:
>>>>> 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;
>>>>
>>>> This has nothing to do with how a consumer may use the structs.
>>>>
>>>>> And the spec also clarifies that the size of each specific request is
>>>>> always 56 bytes.
>>>>
>>>> Sure, and I didn't mean to imply that a consumer would be allowed
>>>> to break this requirement. Still something like this
>>>>
>>>> int pvcall_new_socket(struct xen_pvcalls_socket *s) {
>>>>       struct xen_pvcalls_request req = {
>>>>           .req_id = REQ_ID,
>>>>           .cmd = PVCALLS_SOCKET,
>>>>           .u.socket = *s,
>>>>       };
>>>>
>>>>       return pvcall(&req);
>>>> }
>>>>
>>>> may break.
>>>
>>> I think I understand your concern now. So yes I agree this would break 32-bit consumer.
>>>
>>> As the padding is at the end of the structure, I think a 32-bit frontend and 64-bit backend (or vice-versa) should currently work without any trouble. The problem would come later if we decide to extend a command.
>>
>> Can commands be extended at all, i.e. don't extensions require new
>> commands? The issue I've described has nothing to do with future
>> extending of any of the affected structures.
> 
> I think my point wasn't conveyed correctly. The implicit padding is at
> the end of the structure for all the consumers but 32-bit x86. So
> without any modification, I think 32-bit frontend can still communicate
> with 64-bit backend (or vice-versa).

There's no issue communicating afaics, as for communication
you wouldn't use the sub-structures, but the single container
one. The problem is, as described, with possible uses internal
to one side of the communication.

Jan
Julien Grall April 29, 2020, 3:06 p.m. UTC | #10
On 29/04/2020 15:56, Jan Beulich wrote:
> On 29.04.2020 16:14, Julien Grall wrote:
>> Hi Jan,
>>
>> On 29/04/2020 15:05, Jan Beulich wrote:
>>> On 29.04.2020 16:01, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 22/04/2020 10:20, Jan Beulich wrote:
>>>>>> 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;
>>>>>
>>>>> This has nothing to do with how a consumer may use the structs.
>>>>>
>>>>>> And the spec also clarifies that the size of each specific request is
>>>>>> always 56 bytes.
>>>>>
>>>>> Sure, and I didn't mean to imply that a consumer would be allowed
>>>>> to break this requirement. Still something like this
>>>>>
>>>>> int pvcall_new_socket(struct xen_pvcalls_socket *s) {
>>>>>        struct xen_pvcalls_request req = {
>>>>>            .req_id = REQ_ID,
>>>>>            .cmd = PVCALLS_SOCKET,
>>>>>            .u.socket = *s,
>>>>>        };
>>>>>
>>>>>        return pvcall(&req);
>>>>> }
>>>>>
>>>>> may break.
>>>>
>>>> I think I understand your concern now. So yes I agree this would break 32-bit consumer.
>>>>
>>>> As the padding is at the end of the structure, I think a 32-bit frontend and 64-bit backend (or vice-versa) should currently work without any trouble. The problem would come later if we decide to extend a command.
>>>
>>> Can commands be extended at all, i.e. don't extensions require new
>>> commands? The issue I've described has nothing to do with future
>>> extending of any of the affected structures.
>>
>> I think my point wasn't conveyed correctly. The implicit padding is at
>> the end of the structure for all the consumers but 32-bit x86. So
>> without any modification, I think 32-bit frontend can still communicate
>> with 64-bit backend (or vice-versa).
> 
> There's no issue communicating afaics, as for communication
> you wouldn't use the sub-structures, but the single container
> one. The problem is, as described, with possible uses internal
> to one side of the communication.

I am sorry but I can't figure out how this is an issue. The problem you 
described would only happen if you are calling a 64-bit library from a 
32-bit software. Is it even possible?

Cheers,
Jan Beulich April 29, 2020, 3:23 p.m. UTC | #11
On 29.04.2020 17:06, Julien Grall wrote:
> 
> 
> On 29/04/2020 15:56, Jan Beulich wrote:
>> On 29.04.2020 16:14, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 29/04/2020 15:05, Jan Beulich wrote:
>>>> On 29.04.2020 16:01, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 22/04/2020 10:20, Jan Beulich wrote:
>>>>>>> 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;
>>>>>>
>>>>>> This has nothing to do with how a consumer may use the structs.
>>>>>>
>>>>>>> And the spec also clarifies that the size of each specific request is
>>>>>>> always 56 bytes.
>>>>>>
>>>>>> Sure, and I didn't mean to imply that a consumer would be allowed
>>>>>> to break this requirement. Still something like this
>>>>>>
>>>>>> int pvcall_new_socket(struct xen_pvcalls_socket *s) {
>>>>>>        struct xen_pvcalls_request req = {
>>>>>>            .req_id = REQ_ID,
>>>>>>            .cmd = PVCALLS_SOCKET,
>>>>>>            .u.socket = *s,
>>>>>>        };
>>>>>>
>>>>>>        return pvcall(&req);
>>>>>> }
>>>>>>
>>>>>> may break.
>>>>>
>>>>> I think I understand your concern now. So yes I agree this would break 32-bit consumer.
>>>>>
>>>>> As the padding is at the end of the structure, I think a 32-bit frontend and 64-bit backend (or vice-versa) should currently work without any trouble. The problem would come later if we decide to extend a command.
>>>>
>>>> Can commands be extended at all, i.e. don't extensions require new
>>>> commands? The issue I've described has nothing to do with future
>>>> extending of any of the affected structures.
>>>
>>> I think my point wasn't conveyed correctly. The implicit padding is at
>>> the end of the structure for all the consumers but 32-bit x86. So
>>> without any modification, I think 32-bit frontend can still communicate
>>> with 64-bit backend (or vice-versa).
>>
>> There's no issue communicating afaics, as for communication
>> you wouldn't use the sub-structures, but the single container
>> one. The problem is, as described, with possible uses internal
>> to one side of the communication.
> 
> I am sorry but I can't figure out how this is an issue. The
> problem you described would only happen if you are calling a
> 64-bit library from a 32-bit software.

Why? The example given doesn't require such.

> Is it even possible?

In principle yes, I think. I don't think OSes like Linux allow this,
though.

Jan
Julien Grall April 29, 2020, 3:30 p.m. UTC | #12
Hi Jan,

On 29/04/2020 16:23, Jan Beulich wrote:
> On 29.04.2020 17:06, Julien Grall wrote:
>>
>>
>> On 29/04/2020 15:56, Jan Beulich wrote:
>>> On 29.04.2020 16:14, Julien Grall wrote:
>>>> Hi Jan,
>>>>
>>>> On 29/04/2020 15:05, Jan Beulich wrote:
>>>>> On 29.04.2020 16:01, Julien Grall wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 22/04/2020 10:20, Jan Beulich wrote:
>>>>>>>> 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;
>>>>>>>
>>>>>>> This has nothing to do with how a consumer may use the structs.
>>>>>>>
>>>>>>>> And the spec also clarifies that the size of each specific request is
>>>>>>>> always 56 bytes.
>>>>>>>
>>>>>>> Sure, and I didn't mean to imply that a consumer would be allowed
>>>>>>> to break this requirement. Still something like this
>>>>>>>
>>>>>>> int pvcall_new_socket(struct xen_pvcalls_socket *s) {
>>>>>>>         struct xen_pvcalls_request req = {
>>>>>>>             .req_id = REQ_ID,
>>>>>>>             .cmd = PVCALLS_SOCKET,
>>>>>>>             .u.socket = *s,
>>>>>>>         };
>>>>>>>
>>>>>>>         return pvcall(&req);
>>>>>>> }
>>>>>>>
>>>>>>> may break.
>>>>>>
>>>>>> I think I understand your concern now. So yes I agree this would break 32-bit consumer.
>>>>>>
>>>>>> As the padding is at the end of the structure, I think a 32-bit frontend and 64-bit backend (or vice-versa) should currently work without any trouble. The problem would come later if we decide to extend a command.
>>>>>
>>>>> Can commands be extended at all, i.e. don't extensions require new
>>>>> commands? The issue I've described has nothing to do with future
>>>>> extending of any of the affected structures.
>>>>
>>>> I think my point wasn't conveyed correctly. The implicit padding is at
>>>> the end of the structure for all the consumers but 32-bit x86. So
>>>> without any modification, I think 32-bit frontend can still communicate
>>>> with 64-bit backend (or vice-versa).
>>>
>>> There's no issue communicating afaics, as for communication
>>> you wouldn't use the sub-structures, but the single container
>>> one. The problem is, as described, with possible uses internal
>>> to one side of the communication.
>>
>> I am sorry but I can't figure out how this is an issue. The
>> problem you described would only happen if you are calling a
>> 64-bit library from a 32-bit software.
> 
> Why? The example given doesn't require such.

Your example is only valid if we change the padding. In my previous 
e-mail I wrote "without modification" (i.e existing code) and marking 
the implicit padding only for 64-bit x86 and Arm. So there is no change 
in the size of the structure and therefore there is no issue to 
recompile as the size would not change.

>> Is it even possible?
> 
> In principle yes, I think. I don't think OSes like Linux allow this,
> though.
Do we really have to care about this?

Cheers,
Jan Beulich April 29, 2020, 3:57 p.m. UTC | #13
On 29.04.2020 17:30, Julien Grall wrote:
> Hi Jan,
> 
> On 29/04/2020 16:23, Jan Beulich wrote:
>> On 29.04.2020 17:06, Julien Grall wrote:
>>>
>>>
>>> On 29/04/2020 15:56, Jan Beulich wrote:
>>>> On 29.04.2020 16:14, Julien Grall wrote:
>>>>> Hi Jan,
>>>>>
>>>>> On 29/04/2020 15:05, Jan Beulich wrote:
>>>>>> On 29.04.2020 16:01, Julien Grall wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 22/04/2020 10:20, Jan Beulich wrote:
>>>>>>>>> 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;
>>>>>>>>
>>>>>>>> This has nothing to do with how a consumer may use the structs.
>>>>>>>>
>>>>>>>>> And the spec also clarifies that the size of each specific request is
>>>>>>>>> always 56 bytes.
>>>>>>>>
>>>>>>>> Sure, and I didn't mean to imply that a consumer would be allowed
>>>>>>>> to break this requirement. Still something like this
>>>>>>>>
>>>>>>>> int pvcall_new_socket(struct xen_pvcalls_socket *s) {
>>>>>>>>         struct xen_pvcalls_request req = {
>>>>>>>>             .req_id = REQ_ID,
>>>>>>>>             .cmd = PVCALLS_SOCKET,
>>>>>>>>             .u.socket = *s,
>>>>>>>>         };
>>>>>>>>
>>>>>>>>         return pvcall(&req);
>>>>>>>> }
>>>>>>>>
>>>>>>>> may break.
>>>>>>>
>>>>>>> I think I understand your concern now. So yes I agree this would break 32-bit consumer.
>>>>>>>
>>>>>>> As the padding is at the end of the structure, I think a 32-bit frontend and 64-bit backend (or vice-versa) should currently work without any trouble. The problem would come later if we decide to extend a command.
>>>>>>
>>>>>> Can commands be extended at all, i.e. don't extensions require new
>>>>>> commands? The issue I've described has nothing to do with future
>>>>>> extending of any of the affected structures.
>>>>>
>>>>> I think my point wasn't conveyed correctly. The implicit padding is at
>>>>> the end of the structure for all the consumers but 32-bit x86. So
>>>>> without any modification, I think 32-bit frontend can still communicate
>>>>> with 64-bit backend (or vice-versa).
>>>>
>>>> There's no issue communicating afaics, as for communication
>>>> you wouldn't use the sub-structures, but the single container
>>>> one. The problem is, as described, with possible uses internal
>>>> to one side of the communication.
>>>
>>> I am sorry but I can't figure out how this is an issue. The
>>> problem you described would only happen if you are calling a
>>> 64-bit library from a 32-bit software.
>>
>> Why? The example given doesn't require such.
> 
> Your example is only valid if we change the padding. In my previous
> e-mail I wrote "without modification" (i.e existing code) and
> marking the implicit padding only for 64-bit x86 and Arm. So there
> is no change in the size of the structure and therefore there is no
> issue to recompile as the size would not change.

Oh, sorry, yes. I was mislead by "I think 32-bit frontend can still
communicate with 64-bit backend (or vice-versa)", because I never
said there would be such an issue.

>>> Is it even possible?
>>
>> In principle yes, I think. I don't think OSes like Linux allow this,
>> though.
> Do we really have to care about this?

I don't think we do, but this is a moot point anyway.

Jan

Patch
diff mbox series

diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc
index 665dad556c..971cd8f4b1 100644
--- 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;
diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
index cb8171275c..f6048ddc63 100644
--- 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;
         struct xen_pvcalls_accept {
             uint64_t id;