* [PATCH] pvcalls: Document explicitly the padding for all arches @ 2020-04-19 10:49 Julien Grall 2020-04-20 8:04 ` Jan Beulich 0 siblings, 1 reply; 14+ messages in thread From: Julien Grall @ 2020-04-19 10:49 UTC (permalink / raw) To: xen-devel Cc: Juergen Gross, Stefano Stabellini, julien, Wei Liu, Andrew Cooper, Julien Grall, Ian Jackson, George Dunlap, Jan Beulich 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(-) 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; -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] pvcalls: Document explicitly the padding for all arches 2020-04-19 10:49 [PATCH] pvcalls: Document explicitly the padding for all arches Julien Grall @ 2020-04-20 8:04 ` Jan Beulich 2020-04-20 13:34 ` Julien Grall 0 siblings, 1 reply; 14+ messages in thread From: Jan Beulich @ 2020-04-20 8:04 UTC (permalink / raw) To: Julien Grall Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall, Ian Jackson, George Dunlap, xen-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pvcalls: Document explicitly the padding for all arches 2020-04-20 8:04 ` Jan Beulich @ 2020-04-20 13:34 ` Julien Grall 2020-04-20 13:45 ` Jan Beulich 0 siblings, 1 reply; 14+ messages in thread From: Julien Grall @ 2020-04-20 13:34 UTC (permalink / raw) To: Jan Beulich Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall, Ian Jackson, George Dunlap, xen-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pvcalls: Document explicitly the padding for all arches 2020-04-20 13:34 ` Julien Grall @ 2020-04-20 13:45 ` Jan Beulich 2020-04-21 23:27 ` Stefano Stabellini 0 siblings, 1 reply; 14+ messages in thread From: Jan Beulich @ 2020-04-20 13:45 UTC (permalink / raw) To: Julien Grall Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall, Ian Jackson, George Dunlap, xen-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pvcalls: Document explicitly the padding for all arches 2020-04-20 13:45 ` Jan Beulich @ 2020-04-21 23:27 ` Stefano Stabellini 2020-04-22 9:20 ` Jan Beulich 0 siblings, 1 reply; 14+ messages in thread From: Stefano Stabellini @ 2020-04-21 23:27 UTC (permalink / raw) To: Jan Beulich Cc: Juergen Gross, Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper, Julien Grall, Ian Jackson, George Dunlap, xen-devel [-- Attachment #1: Type: text/plain, Size: 5559 bytes --] 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pvcalls: Document explicitly the padding for all arches 2020-04-21 23:27 ` Stefano Stabellini @ 2020-04-22 9:20 ` Jan Beulich 2020-04-29 14:01 ` Julien Grall 0 siblings, 1 reply; 14+ messages in thread From: Jan Beulich @ 2020-04-22 9:20 UTC (permalink / raw) To: Stefano Stabellini Cc: Juergen Gross, Julien Grall, Wei Liu, Andrew Cooper, Julien Grall, Ian Jackson, George Dunlap, xen-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pvcalls: Document explicitly the padding for all arches 2020-04-22 9:20 ` Jan Beulich @ 2020-04-29 14:01 ` Julien Grall 2020-04-29 14:05 ` Jan Beulich 0 siblings, 1 reply; 14+ messages in thread From: Julien Grall @ 2020-04-29 14:01 UTC (permalink / raw) To: Jan Beulich, Stefano Stabellini Cc: Juergen Gross, Wei Liu, Andrew Cooper, Julien Grall, Ian Jackson, George Dunlap, xen-devel 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, -- Julien Grall ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pvcalls: Document explicitly the padding for all arches 2020-04-29 14:01 ` Julien Grall @ 2020-04-29 14:05 ` Jan Beulich 2020-04-29 14:14 ` Julien Grall 0 siblings, 1 reply; 14+ messages in thread From: Jan Beulich @ 2020-04-29 14:05 UTC (permalink / raw) To: Julien Grall Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall, Ian Jackson, George Dunlap, xen-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pvcalls: Document explicitly the padding for all arches 2020-04-29 14:05 ` Jan Beulich @ 2020-04-29 14:14 ` Julien Grall 2020-04-29 14:56 ` Jan Beulich 0 siblings, 1 reply; 14+ messages in thread From: Julien Grall @ 2020-04-29 14:14 UTC (permalink / raw) To: Jan Beulich Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall, Ian Jackson, George Dunlap, xen-devel 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, -- Julien Grall ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pvcalls: Document explicitly the padding for all arches 2020-04-29 14:14 ` Julien Grall @ 2020-04-29 14:56 ` Jan Beulich 2020-04-29 15:06 ` Julien Grall 0 siblings, 1 reply; 14+ messages in thread From: Jan Beulich @ 2020-04-29 14:56 UTC (permalink / raw) To: Julien Grall Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall, Ian Jackson, George Dunlap, xen-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pvcalls: Document explicitly the padding for all arches 2020-04-29 14:56 ` Jan Beulich @ 2020-04-29 15:06 ` Julien Grall 2020-04-29 15:23 ` Jan Beulich 0 siblings, 1 reply; 14+ messages in thread From: Julien Grall @ 2020-04-29 15:06 UTC (permalink / raw) To: Jan Beulich Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall, Ian Jackson, George Dunlap, xen-devel 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, -- Julien Grall ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pvcalls: Document explicitly the padding for all arches 2020-04-29 15:06 ` Julien Grall @ 2020-04-29 15:23 ` Jan Beulich 2020-04-29 15:30 ` Julien Grall 0 siblings, 1 reply; 14+ messages in thread From: Jan Beulich @ 2020-04-29 15:23 UTC (permalink / raw) To: Julien Grall Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall, Ian Jackson, George Dunlap, xen-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pvcalls: Document explicitly the padding for all arches 2020-04-29 15:23 ` Jan Beulich @ 2020-04-29 15:30 ` Julien Grall 2020-04-29 15:57 ` Jan Beulich 0 siblings, 1 reply; 14+ messages in thread From: Julien Grall @ 2020-04-29 15:30 UTC (permalink / raw) To: Jan Beulich Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall, Ian Jackson, George Dunlap, xen-devel 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, -- Julien Grall ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] pvcalls: Document explicitly the padding for all arches 2020-04-29 15:30 ` Julien Grall @ 2020-04-29 15:57 ` Jan Beulich 0 siblings, 0 replies; 14+ messages in thread From: Jan Beulich @ 2020-04-29 15:57 UTC (permalink / raw) To: Julien Grall Cc: Juergen Gross, Stefano Stabellini, Wei Liu, Andrew Cooper, Julien Grall, Ian Jackson, George Dunlap, xen-devel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-04-29 15:58 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-19 10:49 [PATCH] pvcalls: Document explicitly the padding for all arches Julien Grall 2020-04-20 8:04 ` Jan Beulich 2020-04-20 13:34 ` Julien Grall 2020-04-20 13:45 ` Jan Beulich 2020-04-21 23:27 ` Stefano Stabellini 2020-04-22 9:20 ` Jan Beulich 2020-04-29 14:01 ` Julien Grall 2020-04-29 14:05 ` Jan Beulich 2020-04-29 14:14 ` Julien Grall 2020-04-29 14:56 ` Jan Beulich 2020-04-29 15:06 ` Julien Grall 2020-04-29 15:23 ` Jan Beulich 2020-04-29 15:30 ` Julien Grall 2020-04-29 15:57 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).