[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] pvcalls: Document explicitly the padding for all arches
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |