| Message ID | 20200419104948.31200-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pvcalls: Document explicitly the padding for all arches | expand |
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
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,
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
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.
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
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,
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
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,
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
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,
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
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,
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
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;