| Message ID | 20200613184132.11880-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,for-4.14] pvcalls: Document correctly and explicitely the padding for all arches | expand |
> -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: 13 June 2020 19:42 > To: xen-devel@lists.xenproject.org > Cc: paul@xen.org; Julien Grall <jgrall@amazon.com>; Andrew Cooper <andrew.cooper3@citrix.com>; George > Dunlap <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Jan Beulich > <jbeulich@suse.com>; Julien Grall <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Wei > Liu <wl@xen.org>; Juergen Gross <jgross@suse.com> > Subject: [PATCH v3 for-4.14] pvcalls: Document correctly and explicitely the padding for all arches > > From: Julien Grall <jgrall@amazon.com> > > The documentation of pvcalls suggests there is padding for 32-bit x86 > at the end of most the structure. However, they are not described in > in the public header. > > Because of that all the structures would be 32-bit aligned and not > 64-bit aligned for 32-bit x86. > > For all the other architectures supported (Arm and 64-bit x86), the > structure are aligned to 64-bit because they contain uint64_t field. > Therefore all the structures contain implicit padding. > > The paddings are now corrected for 32-bit x86 and written explicitly for > all the architectures. > > While the structure size between 32-bit and 64-bit x86 is different, it > shouldn't cause any incompatibility between a 32-bit and 64-bit > frontend/backend because the commands are always 56 bits and the padding > are at the end of the structure. > > As an aside, the padding sadly cannot be mandated to be 0 as they are > already present. So it is not going to be possible to use the padding > for extending a command in the future. Ugh. > > Signed-off-by: Julien Grall <jgrall@amazon.com> Release-acked-by: Paul Durrant <paul@xen.org> > > --- > Changes in v3: > - Use __i386__ rather than CONFIG_X86_32 > > Changes in v2: > - It is not possible to use the same padding for 32-bit x86 and > all the other supported architectures. > --- > docs/misc/pvcalls.pandoc | 18 ++++++++++-------- > xen/include/public/io/pvcalls.h | 14 ++++++++++++++ > 2 files changed, 24 insertions(+), 8 deletions(-) > > diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc > index 665dad556c39..caa71b36d78b 100644 > --- a/docs/misc/pvcalls.pandoc > +++ b/docs/misc/pvcalls.pandoc > @@ -246,9 +246,9 @@ The format is defined as follows: > uint32_t domain; > uint32_t type; > uint32_t protocol; > - #ifdef CONFIG_X86_32 > + #ifndef __i386__ > uint8_t pad[4]; > - #endif > + #endif > } socket; > struct xen_pvcalls_connect { > uint64_t id; > @@ -257,16 +257,18 @@ The format is defined as follows: > uint32_t flags; > grant_ref_t ref; > uint32_t evtchn; > - #ifdef CONFIG_X86_32 > + #ifndef __i386__ > uint8_t pad[4]; > - #endif > + #endif > } connect; > struct xen_pvcalls_release { > uint64_t id; > uint8_t reuse; > - #ifdef CONFIG_X86_32 > + #ifndef __i386__ > uint8_t pad[7]; > - #endif > + #else > + uint8_t pad[3]; > + #endif > } release; > struct xen_pvcalls_bind { > uint64_t id; > @@ -276,9 +278,9 @@ The format is defined as follows: > struct xen_pvcalls_listen { > uint64_t id; > uint32_t backlog; > - #ifdef CONFIG_X86_32 > + #ifndef __i386__ > uint8_t pad[4]; > - #endif > + #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 cb8171275c13..28374a82f410 100644 > --- a/xen/include/public/io/pvcalls.h > +++ b/xen/include/public/io/pvcalls.h > @@ -65,6 +65,9 @@ struct xen_pvcalls_request { > uint32_t domain; > uint32_t type; > uint32_t protocol; > +#ifndef __i386__ > + uint8_t pad[4]; > +#endif > } socket; > struct xen_pvcalls_connect { > uint64_t id; > @@ -73,10 +76,18 @@ struct xen_pvcalls_request { > uint32_t flags; > grant_ref_t ref; > uint32_t evtchn; > +#ifndef __i386__ > + uint8_t pad[4]; > +#endif > } connect; > struct xen_pvcalls_release { > uint64_t id; > uint8_t reuse; > +#ifndef __i386__ > + uint8_t pad[7]; > +#else > + uint8_t pad[3]; > +#endif > } release; > struct xen_pvcalls_bind { > uint64_t id; > @@ -86,6 +97,9 @@ struct xen_pvcalls_request { > struct xen_pvcalls_listen { > uint64_t id; > uint32_t backlog; > +#ifndef __i386__ > + uint8_t pad[4]; > +#endif > } listen; > struct xen_pvcalls_accept { > uint64_t id; > -- > 2.17.1
On Sat, 13 Jun 2020, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > The documentation of pvcalls suggests there is padding for 32-bit x86 > at the end of most the structure. However, they are not described in > in the public header. > > Because of that all the structures would be 32-bit aligned and not > 64-bit aligned for 32-bit x86. > > For all the other architectures supported (Arm and 64-bit x86), the > structure are aligned to 64-bit because they contain uint64_t field. > Therefore all the structures contain implicit padding. > > The paddings are now corrected for 32-bit x86 and written explicitly for > all the architectures. > > While the structure size between 32-bit and 64-bit x86 is different, it > shouldn't cause any incompatibility between a 32-bit and 64-bit > frontend/backend because the commands are always 56 bits and the padding > are at the end of the structure. > > As an aside, the padding sadly cannot be mandated to be 0 as they are > already present. So it is not going to be possible to use the padding > for extending a command in the future. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > > --- > Changes in v3: > - Use __i386__ rather than CONFIG_X86_32 > > Changes in v2: > - It is not possible to use the same padding for 32-bit x86 and > all the other supported architectures. > --- > docs/misc/pvcalls.pandoc | 18 ++++++++++-------- > xen/include/public/io/pvcalls.h | 14 ++++++++++++++ > 2 files changed, 24 insertions(+), 8 deletions(-) > > diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc > index 665dad556c39..caa71b36d78b 100644 > --- a/docs/misc/pvcalls.pandoc > +++ b/docs/misc/pvcalls.pandoc > @@ -246,9 +246,9 @@ The format is defined as follows: > uint32_t domain; > uint32_t type; > uint32_t protocol; > - #ifdef CONFIG_X86_32 > + #ifndef __i386__ > uint8_t pad[4]; > - #endif > + #endif Hi Julien, Thank you for doing this, and sorry for having missed v2 of this patch, I should have replied earlier. The intention of the #ifdef blocks like: #ifdef CONFIG_X86_32 uint8_t pad[4]; #endif in pvcalls.pandoc was to make sure that these structs would be 64bit aligned on x86_32 too. I realize that the public header doesn't match, but the spec is the "master copy". We have been saying it for a while (Andy in particular) that the specification documents are the one that define the protocol, not the public headers. This is the very first time we get to act on that statement. What a special occasion this is :-) So I think we should keep the specification as is and fix the public header instead. Something like v1 of this patch.
On 16.06.2020 03:00, Stefano Stabellini wrote: > On Sat, 13 Jun 2020, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> The documentation of pvcalls suggests there is padding for 32-bit x86 >> at the end of most the structure. However, they are not described in >> in the public header. >> >> Because of that all the structures would be 32-bit aligned and not >> 64-bit aligned for 32-bit x86. >> >> For all the other architectures supported (Arm and 64-bit x86), the >> structure are aligned to 64-bit because they contain uint64_t field. >> Therefore all the structures contain implicit padding. >> >> The paddings are now corrected for 32-bit x86 and written explicitly for >> all the architectures. >> >> While the structure size between 32-bit and 64-bit x86 is different, it >> shouldn't cause any incompatibility between a 32-bit and 64-bit >> frontend/backend because the commands are always 56 bits and the padding >> are at the end of the structure. >> >> As an aside, the padding sadly cannot be mandated to be 0 as they are >> already present. So it is not going to be possible to use the padding >> for extending a command in the future. >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> >> --- >> Changes in v3: >> - Use __i386__ rather than CONFIG_X86_32 >> >> Changes in v2: >> - It is not possible to use the same padding for 32-bit x86 and >> all the other supported architectures. >> --- >> docs/misc/pvcalls.pandoc | 18 ++++++++++-------- >> xen/include/public/io/pvcalls.h | 14 ++++++++++++++ >> 2 files changed, 24 insertions(+), 8 deletions(-) >> >> diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc >> index 665dad556c39..caa71b36d78b 100644 >> --- a/docs/misc/pvcalls.pandoc >> +++ b/docs/misc/pvcalls.pandoc >> @@ -246,9 +246,9 @@ The format is defined as follows: >> uint32_t domain; >> uint32_t type; >> uint32_t protocol; >> - #ifdef CONFIG_X86_32 >> + #ifndef __i386__ >> uint8_t pad[4]; >> - #endif >> + #endif > > > Hi Julien, > > Thank you for doing this, and sorry for having missed v2 of this patch, I > should have replied earlier. > > The intention of the #ifdef blocks like: > > #ifdef CONFIG_X86_32 > uint8_t pad[4]; > #endif > > in pvcalls.pandoc was to make sure that these structs would be 64bit > aligned on x86_32 too. > > I realize that the public header doesn't match, but the spec is the > "master copy". We have been saying it for a while (Andy in particular) > that the specification documents are the one that define the protocol, > not the public headers. This is the very first time we get to act on > that statement. What a special occasion this is :-) > > So I think we should keep the specification as is and fix the public > header instead. Something like v1 of this patch. But you've read the communication on v1 and v2? A public header can't be "fixed" if it may already be in use by anyone. We can only do as Andrew and you suggest (mandate textual descriptions to be "the ABI") when we do so for _new_ interfaces from the very beginning, making clear that the public header (if any) exists just for reference. I'm not convinced btw that expressing at least the majority of an ABI by way of a C reference implementation is a bad or unsuitable thing. It's just that aspects like underlying type properties need to be very clear and unambiguous. Jan
On 13.06.2020 20:41, Julien Grall wrote: > @@ -73,10 +76,18 @@ struct xen_pvcalls_request { > uint32_t flags; > grant_ref_t ref; > uint32_t evtchn; > +#ifndef __i386__ > + uint8_t pad[4]; > +#endif Where possible I think uint32_t would be slightly better to use. > } connect; > struct xen_pvcalls_release { > uint64_t id; > uint8_t reuse; > +#ifndef __i386__ > + uint8_t pad[7]; > +#else > + uint8_t pad[3]; > +#endif For this I'd recommend uniform "uint8_t pad[3];" (i.e. outside of any #ifdef) followed by a uint32_t again inside the #ifdef. Expressing everything through e.g. alignof() would seem even better, but I can't currently think of a way to do so cleanly. In any event - I'm not the maintainer of these headers, so it may be just me ... Jan
On 16/06/2020 09:26, Jan Beulich wrote: > On 13.06.2020 20:41, Julien Grall wrote: >> @@ -73,10 +76,18 @@ struct xen_pvcalls_request { >> uint32_t flags; >> grant_ref_t ref; >> uint32_t evtchn; >> +#ifndef __i386__ >> + uint8_t pad[4]; >> +#endif > > Where possible I think uint32_t would be slightly better to use. OOI, why? > >> } connect; >> struct xen_pvcalls_release { >> uint64_t id; >> uint8_t reuse; >> +#ifndef __i386__ >> + uint8_t pad[7]; >> +#else >> + uint8_t pad[3]; >> +#endif > > For this I'd recommend uniform "uint8_t pad[3];" (i.e. outside > of any #ifdef) followed by a uint32_t again inside the #ifdef. Same question here. The more the padding cannot be re-used. > > Expressing everything through e.g. alignof() would seem even > better, but I can't currently think of a way to do so cleanly. I am afraid I don't have time to look at how alignof() can work nicely. Feel free to send a follow-up or pick-up the patch is you really want alignof(). Cheers,
On 16.06.2020 11:19, Julien Grall wrote: > > > On 16/06/2020 09:26, Jan Beulich wrote: >> On 13.06.2020 20:41, Julien Grall wrote: >>> @@ -73,10 +76,18 @@ struct xen_pvcalls_request { >>> uint32_t flags; >>> grant_ref_t ref; >>> uint32_t evtchn; >>> +#ifndef __i386__ >>> + uint8_t pad[4]; >>> +#endif >> >> Where possible I think uint32_t would be slightly better to use. > > OOI, why? Because everything else here uses the wider type, plus the question of why use a compound type (array) when a simple one does. >> >>> } connect; >>> struct xen_pvcalls_release { >>> uint64_t id; >>> uint8_t reuse; >>> +#ifndef __i386__ >>> + uint8_t pad[7]; >>> +#else >>> + uint8_t pad[3]; >>> +#endif >> >> For this I'd recommend uniform "uint8_t pad[3];" (i.e. outside >> of any #ifdef) followed by a uint32_t again inside the #ifdef. > > Same question here. The more the padding cannot be re-used. > >> >> Expressing everything through e.g. alignof() would seem even >> better, but I can't currently think of a way to do so cleanly. > > I am afraid I don't have time to look at how alignof() can work nicely. > Feel free to send a follow-up or pick-up the patch is you really want > alignof(). I didn't mean to ask that you find a solution. I merely pointed out that expressing things properly rather than using hard coded numbers would be nice. Jan
Hi, On 16/06/2020 02:00, Stefano Stabellini wrote: > On Sat, 13 Jun 2020, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> The documentation of pvcalls suggests there is padding for 32-bit x86 >> at the end of most the structure. However, they are not described in >> in the public header. >> >> Because of that all the structures would be 32-bit aligned and not >> 64-bit aligned for 32-bit x86. >> >> For all the other architectures supported (Arm and 64-bit x86), the >> structure are aligned to 64-bit because they contain uint64_t field. >> Therefore all the structures contain implicit padding. >> >> The paddings are now corrected for 32-bit x86 and written explicitly for >> all the architectures. >> >> While the structure size between 32-bit and 64-bit x86 is different, it >> shouldn't cause any incompatibility between a 32-bit and 64-bit >> frontend/backend because the commands are always 56 bits and the padding >> are at the end of the structure. >> >> As an aside, the padding sadly cannot be mandated to be 0 as they are >> already present. So it is not going to be possible to use the padding >> for extending a command in the future. >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> >> --- >> Changes in v3: >> - Use __i386__ rather than CONFIG_X86_32 >> >> Changes in v2: >> - It is not possible to use the same padding for 32-bit x86 and >> all the other supported architectures. >> --- >> docs/misc/pvcalls.pandoc | 18 ++++++++++-------- >> xen/include/public/io/pvcalls.h | 14 ++++++++++++++ >> 2 files changed, 24 insertions(+), 8 deletions(-) >> >> diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc >> index 665dad556c39..caa71b36d78b 100644 >> --- a/docs/misc/pvcalls.pandoc >> +++ b/docs/misc/pvcalls.pandoc >> @@ -246,9 +246,9 @@ The format is defined as follows: >> uint32_t domain; >> uint32_t type; >> uint32_t protocol; >> - #ifdef CONFIG_X86_32 >> + #ifndef __i386__ >> uint8_t pad[4]; >> - #endif >> + #endif > > > Hi Julien, > > Thank you for doing this, and sorry for having missed v2 of this patch, I > should have replied earlier. > > The intention of the #ifdef blocks like: > > #ifdef CONFIG_X86_32 > uint8_t pad[4]; > #endif > > in pvcalls.pandoc was to make sure that these structs would be 64bit > aligned on x86_32 too. > > I realize that the public header doesn't match, but the spec is the > "master copy". So far, the public headers are the defacto official ABI. So did you mark the pvcall header as just a reference? > We have been saying it for a while (Andy in particular) > that the specification documents are the one that define the protocol, > not the public headers. This is the very first time we get to act on > that statement. What a special occasion this is :-) > > So I think we should keep the specification as is and fix the public > header instead. Something like v1 of this patch. Well, the header is now part of multiple open-source projects (don't know about private). So adding padding will result to incompatibility between two x86 32-bit entities that may disagree on the ABI if they are using the per-command structure. TBH, I don't think the issue is worth the breakage here. You will never be able to use those paddings in any case as they are not reserved as 0. Cheers,
On 16/06/2020 10:36, Jan Beulich wrote: > On 16.06.2020 11:19, Julien Grall wrote: >> >> >> On 16/06/2020 09:26, Jan Beulich wrote: >>> On 13.06.2020 20:41, Julien Grall wrote: >>>> @@ -73,10 +76,18 @@ struct xen_pvcalls_request { >>>> uint32_t flags; >>>> grant_ref_t ref; >>>> uint32_t evtchn; >>>> +#ifndef __i386__ >>>> + uint8_t pad[4]; >>>> +#endif >>> >>> Where possible I think uint32_t would be slightly better to use. >> >> OOI, why? > > Because everything else here uses the wider type, plus the > question of why use a compound type (array) when a simple > one does. This is pretty much a matter of taste. In this case, I decided to specify the padding the same way accross all the structure. Cheers,
On Tue, 16 Jun 2020, Julien Grall wrote: > On 16/06/2020 02:00, Stefano Stabellini wrote: > > On Sat, 13 Jun 2020, Julien Grall wrote: > > > From: Julien Grall <jgrall@amazon.com> > > > > > > The documentation of pvcalls suggests there is padding for 32-bit x86 > > > at the end of most the structure. However, they are not described in > > > in the public header. > > > > > > Because of that all the structures would be 32-bit aligned and not > > > 64-bit aligned for 32-bit x86. > > > > > > For all the other architectures supported (Arm and 64-bit x86), the > > > structure are aligned to 64-bit because they contain uint64_t field. > > > Therefore all the structures contain implicit padding. > > > > > > The paddings are now corrected for 32-bit x86 and written explicitly for > > > all the architectures. > > > > > > While the structure size between 32-bit and 64-bit x86 is different, it > > > shouldn't cause any incompatibility between a 32-bit and 64-bit > > > frontend/backend because the commands are always 56 bits and the padding > > > are at the end of the structure. > > > > > > As an aside, the padding sadly cannot be mandated to be 0 as they are > > > already present. So it is not going to be possible to use the padding > > > for extending a command in the future. > > > > > > Signed-off-by: Julien Grall <jgrall@amazon.com> > > > > > > --- > > > Changes in v3: > > > - Use __i386__ rather than CONFIG_X86_32 > > > > > > Changes in v2: > > > - It is not possible to use the same padding for 32-bit x86 and > > > all the other supported architectures. > > > --- > > > docs/misc/pvcalls.pandoc | 18 ++++++++++-------- > > > xen/include/public/io/pvcalls.h | 14 ++++++++++++++ > > > 2 files changed, 24 insertions(+), 8 deletions(-) > > > > > > diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc > > > index 665dad556c39..caa71b36d78b 100644 > > > --- a/docs/misc/pvcalls.pandoc > > > +++ b/docs/misc/pvcalls.pandoc > > > @@ -246,9 +246,9 @@ The format is defined as follows: > > > uint32_t domain; > > > uint32_t type; > > > uint32_t protocol; > > > - #ifdef CONFIG_X86_32 > > > + #ifndef __i386__ > > > uint8_t pad[4]; > > > - #endif > > > + #endif > > > > > > Hi Julien, > > > > Thank you for doing this, and sorry for having missed v2 of this patch, I > > should have replied earlier. > > > > The intention of the #ifdef blocks like: > > > > #ifdef CONFIG_X86_32 > > uint8_t pad[4]; > > #endif > > > > in pvcalls.pandoc was to make sure that these structs would be 64bit > > aligned on x86_32 too. > > > > I realize that the public header doesn't match, but the spec is the > > "master copy". > > So far, the public headers are the defacto official ABI. So did you mark the > pvcall header as just a reference? No, there is no document that says that the canonical copy of the interface is pvcalls.pandoc. However, it was clearly spelled out from the start on xen-devel (see below.) In fact, if you notice, this is the first document under docs/misc that goes into this level of details in describing a new PV protocol. Also note the title of the document which is "PV Calls Protocol version 1". In reply to Jan: > A public header can't be "fixed" if it may already be in use by > anyone. We can only do as Andrew and you suggest (mandate textual > descriptions to be "the ABI") when we do so for _new_ interfaces from > the very beginning, making clear that the public header (if any) > exists just for reference. What if somebody took the specification of the interface from pvcalls.pandoc and wrote their own headers and code? It is definitely possible. At the time, it was clarified that the purpose of writing such a detailed specification document was that the document was the specification :-) See for instance this email (there are others): https://marc.info/?l=linux-kernel&m=149815619513930&w=2 """ This was done differently in the past, but now that we have a formal process, a person in charge of new PV drivers reviews, and design documents with clearly spelled out ABIs, I consider the design docs under docs/misc as the official specification. We don't need headers anymore, they are redundant. In fact, we cannot have two specifications, and the design docs are certainly the official ones (we don't want the specs to be written as header files in C). To me, the headers under xen/include/public/io/ are optional helpers. It doesn't matter what's in there, or if frontends and backends use them or not. There is really an argument for removing those headers, because they might get out of sync with the spec by mistake, and in those cases, then we really end up with two specifications for the same protocol. I would be in favor of `git rm'ing all files under xen/include/public/io/ for which we have a complete design doc under docs/misc. """ (Andy and Roger agreed on the thread if you look at the follow-up emails.)
On Tue, 16 Jun 2020 at 21:57, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Tue, 16 Jun 2020, Julien Grall wrote: > > On 16/06/2020 02:00, Stefano Stabellini wrote: > > > On Sat, 13 Jun 2020, Julien Grall wrote: > > > > From: Julien Grall <jgrall@amazon.com> > > > > > > > > The documentation of pvcalls suggests there is padding for 32-bit x86 > > > > at the end of most the structure. However, they are not described in > > > > in the public header. > > > > > > > > Because of that all the structures would be 32-bit aligned and not > > > > 64-bit aligned for 32-bit x86. > > > > > > > > For all the other architectures supported (Arm and 64-bit x86), the > > > > structure are aligned to 64-bit because they contain uint64_t field. > > > > Therefore all the structures contain implicit padding. > > > > > > > > The paddings are now corrected for 32-bit x86 and written explicitly for > > > > all the architectures. > > > > > > > > While the structure size between 32-bit and 64-bit x86 is different, it > > > > shouldn't cause any incompatibility between a 32-bit and 64-bit > > > > frontend/backend because the commands are always 56 bits and the padding > > > > are at the end of the structure. > > > > > > > > As an aside, the padding sadly cannot be mandated to be 0 as they are > > > > already present. So it is not going to be possible to use the padding > > > > for extending a command in the future. > > > > > > > > Signed-off-by: Julien Grall <jgrall@amazon.com> > > > > > > > > --- > > > > Changes in v3: > > > > - Use __i386__ rather than CONFIG_X86_32 > > > > > > > > Changes in v2: > > > > - It is not possible to use the same padding for 32-bit x86 and > > > > all the other supported architectures. > > > > --- > > > > docs/misc/pvcalls.pandoc | 18 ++++++++++-------- > > > > xen/include/public/io/pvcalls.h | 14 ++++++++++++++ > > > > 2 files changed, 24 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc > > > > index 665dad556c39..caa71b36d78b 100644 > > > > --- a/docs/misc/pvcalls.pandoc > > > > +++ b/docs/misc/pvcalls.pandoc > > > > @@ -246,9 +246,9 @@ The format is defined as follows: > > > > uint32_t domain; > > > > uint32_t type; > > > > uint32_t protocol; > > > > - #ifdef CONFIG_X86_32 > > > > + #ifndef __i386__ > > > > uint8_t pad[4]; > > > > - #endif > > > > + #endif > > > > > > > > > Hi Julien, > > > > > > Thank you for doing this, and sorry for having missed v2 of this patch, I > > > should have replied earlier. > > > > > > The intention of the #ifdef blocks like: > > > > > > #ifdef CONFIG_X86_32 > > > uint8_t pad[4]; > > > #endif > > > > > > in pvcalls.pandoc was to make sure that these structs would be 64bit > > > aligned on x86_32 too. > > > > > > I realize that the public header doesn't match, but the spec is the > > > "master copy". > > > > So far, the public headers are the defacto official ABI. So did you mark the > > pvcall header as just a reference? > > No, there is no document that says that the canonical copy of the > interface is pvcalls.pandoc. However, it was clearly spelled out from > the start on xen-devel (see below.) > In fact, if you notice, this is the > first document under docs/misc that goes into this level of details in > describing a new PV protocol. Also note the title of the document which > is "PV Calls Protocol version 1". While I understand this may have been the original intention, you can't expect a developer to go through the archive to check whether he/she should trust the header of the document. > > > In reply to Jan: > > A public header can't be "fixed" if it may already be in use by > > anyone. We can only do as Andrew and you suggest (mandate textual > > descriptions to be "the ABI") when we do so for _new_ interfaces from > > the very beginning, making clear that the public header (if any) > > exists just for reference. > > What if somebody took the specification of the interface from > pvcalls.pandoc and wrote their own headers and code? It is definitely > possible. As it is possible for someone to have picked the headers from Xen as in the past public/ has always been the authority. > At the time, it was clarified that the purpose of writing such > a detailed specification document was that the document was the > specification :-) At the risk of being pedantic, if it is not written in xen.git it doesn't exist ;). Anyway, no matter the decision you take here, you are going to potentially break one set of the users. I am leaning towards the header as authoritative because this has always been the case in the past and nothing in xen.git says otherwise. However I am not a user of pvcalls, so I don't really have any big incentive to go either way. For the future, I would highly suggest writing down the support decision in xen.git. This would avoid such debate on what is the authority... Cheers,
On Tue, 16 Jun 2020, Julien Grall wrote: > On Tue, 16 Jun 2020 at 21:57, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > > On Tue, 16 Jun 2020, Julien Grall wrote: > > > On 16/06/2020 02:00, Stefano Stabellini wrote: > > > > On Sat, 13 Jun 2020, Julien Grall wrote: > > > > > From: Julien Grall <jgrall@amazon.com> > > > > > > > > > > The documentation of pvcalls suggests there is padding for 32-bit x86 > > > > > at the end of most the structure. However, they are not described in > > > > > in the public header. > > > > > > > > > > Because of that all the structures would be 32-bit aligned and not > > > > > 64-bit aligned for 32-bit x86. > > > > > > > > > > For all the other architectures supported (Arm and 64-bit x86), the > > > > > structure are aligned to 64-bit because they contain uint64_t field. > > > > > Therefore all the structures contain implicit padding. > > > > > > > > > > The paddings are now corrected for 32-bit x86 and written explicitly for > > > > > all the architectures. > > > > > > > > > > While the structure size between 32-bit and 64-bit x86 is different, it > > > > > shouldn't cause any incompatibility between a 32-bit and 64-bit > > > > > frontend/backend because the commands are always 56 bits and the padding > > > > > are at the end of the structure. > > > > > > > > > > As an aside, the padding sadly cannot be mandated to be 0 as they are > > > > > already present. So it is not going to be possible to use the padding > > > > > for extending a command in the future. > > > > > > > > > > Signed-off-by: Julien Grall <jgrall@amazon.com> > > > > > > > > > > --- > > > > > Changes in v3: > > > > > - Use __i386__ rather than CONFIG_X86_32 > > > > > > > > > > Changes in v2: > > > > > - It is not possible to use the same padding for 32-bit x86 and > > > > > all the other supported architectures. > > > > > --- > > > > > docs/misc/pvcalls.pandoc | 18 ++++++++++-------- > > > > > xen/include/public/io/pvcalls.h | 14 ++++++++++++++ > > > > > 2 files changed, 24 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc > > > > > index 665dad556c39..caa71b36d78b 100644 > > > > > --- a/docs/misc/pvcalls.pandoc > > > > > +++ b/docs/misc/pvcalls.pandoc > > > > > @@ -246,9 +246,9 @@ The format is defined as follows: > > > > > uint32_t domain; > > > > > uint32_t type; > > > > > uint32_t protocol; > > > > > - #ifdef CONFIG_X86_32 > > > > > + #ifndef __i386__ > > > > > uint8_t pad[4]; > > > > > - #endif > > > > > + #endif > > > > > > > > > > > > Hi Julien, > > > > > > > > Thank you for doing this, and sorry for having missed v2 of this patch, I > > > > should have replied earlier. > > > > > > > > The intention of the #ifdef blocks like: > > > > > > > > #ifdef CONFIG_X86_32 > > > > uint8_t pad[4]; > > > > #endif > > > > > > > > in pvcalls.pandoc was to make sure that these structs would be 64bit > > > > aligned on x86_32 too. > > > > > > > > I realize that the public header doesn't match, but the spec is the > > > > "master copy". > > > > > > So far, the public headers are the defacto official ABI. So did you mark the > > > pvcall header as just a reference? > > > > No, there is no document that says that the canonical copy of the > > interface is pvcalls.pandoc. However, it was clearly spelled out from > > the start on xen-devel (see below.) > > In fact, if you notice, this is the > > first document under docs/misc that goes into this level of details in > > describing a new PV protocol. Also note the title of the document which > > is "PV Calls Protocol version 1". > > While I understand this may have been the original intention, you > can't expect a developer to go through the archive to check whether > he/she should trust the header of the document. > > > > > > > In reply to Jan: > > > A public header can't be "fixed" if it may already be in use by > > > anyone. We can only do as Andrew and you suggest (mandate textual > > > descriptions to be "the ABI") when we do so for _new_ interfaces from > > > the very beginning, making clear that the public header (if any) > > > exists just for reference. > > > > What if somebody took the specification of the interface from > > pvcalls.pandoc and wrote their own headers and code? It is definitely > > possible. > > As it is possible for someone to have picked the headers from Xen as > in the past public/ has always been the authority. We never had documents under docs/ before specifying the interfaces before pvcalls. It is not written anywhere that the headers under public/ are the authoritative interfaces either, it is just that it was the only thing available before. If you are new to the project you might go to docs/ first. > > At the time, it was clarified that the purpose of writing such > > a detailed specification document was that the document was the > > specification :-) > > At the risk of being pedantic, if it is not written in xen.git it > doesn't exist ;). > > Anyway, no matter the decision you take here, you are going to > potentially break one set of the users. > > I am leaning towards the header as authoritative because this has > always been the case in the past and nothing in xen.git says > otherwise. However I am not a user of pvcalls, so I don't really have > any big incentive to go either way. Yeah, we are risking breaking one set of users either way :-/ In reality, we are using pvcalls on arm64 in a new project (but it is still very green). I am not aware of anybody using pvcalls on x86 (especially x86_32). I would prefer to honor the pvcalls.pandoc specification because that is what it was meant to be, and also leads to a better protocol specification. > For the future, I would highly suggest writing down the support > decision in xen.git. This would avoid such debate on what is the > authority... Yes that's the way to go
(+ Committers) On 18/06/2020 02:34, Stefano Stabellini wrote: > On Tue, 16 Jun 2020, Julien Grall wrote: >> On Tue, 16 Jun 2020 at 21:57, Stefano Stabellini <sstabellini@kernel.org> wrote: >>> >>> On Tue, 16 Jun 2020, Julien Grall wrote: >>>> On 16/06/2020 02:00, Stefano Stabellini wrote: >>>>> On Sat, 13 Jun 2020, Julien Grall wrote: >>>>>> From: Julien Grall <jgrall@amazon.com> >>>>>> >>>>>> The documentation of pvcalls suggests there is padding for 32-bit x86 >>>>>> at the end of most the structure. However, they are not described in >>>>>> in the public header. >>>>>> >>>>>> Because of that all the structures would be 32-bit aligned and not >>>>>> 64-bit aligned for 32-bit x86. >>>>>> >>>>>> For all the other architectures supported (Arm and 64-bit x86), the >>>>>> structure are aligned to 64-bit because they contain uint64_t field. >>>>>> Therefore all the structures contain implicit padding. >>>>>> >>>>>> The paddings are now corrected for 32-bit x86 and written explicitly for >>>>>> all the architectures. >>>>>> >>>>>> While the structure size between 32-bit and 64-bit x86 is different, it >>>>>> shouldn't cause any incompatibility between a 32-bit and 64-bit >>>>>> frontend/backend because the commands are always 56 bits and the padding >>>>>> are at the end of the structure. >>>>>> >>>>>> As an aside, the padding sadly cannot be mandated to be 0 as they are >>>>>> already present. So it is not going to be possible to use the padding >>>>>> for extending a command in the future. >>>>>> >>>>>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>>>>> >>>>>> --- >>>>>> Changes in v3: >>>>>> - Use __i386__ rather than CONFIG_X86_32 >>>>>> >>>>>> Changes in v2: >>>>>> - It is not possible to use the same padding for 32-bit x86 and >>>>>> all the other supported architectures. >>>>>> --- >>>>>> docs/misc/pvcalls.pandoc | 18 ++++++++++-------- >>>>>> xen/include/public/io/pvcalls.h | 14 ++++++++++++++ >>>>>> 2 files changed, 24 insertions(+), 8 deletions(-) >>>>>> >>>>>> diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc >>>>>> index 665dad556c39..caa71b36d78b 100644 >>>>>> --- a/docs/misc/pvcalls.pandoc >>>>>> +++ b/docs/misc/pvcalls.pandoc >>>>>> @@ -246,9 +246,9 @@ The format is defined as follows: >>>>>> uint32_t domain; >>>>>> uint32_t type; >>>>>> uint32_t protocol; >>>>>> - #ifdef CONFIG_X86_32 >>>>>> + #ifndef __i386__ >>>>>> uint8_t pad[4]; >>>>>> - #endif >>>>>> + #endif >>>>> >>>>> >>>>> Hi Julien, >>>>> >>>>> Thank you for doing this, and sorry for having missed v2 of this patch, I >>>>> should have replied earlier. >>>>> >>>>> The intention of the #ifdef blocks like: >>>>> >>>>> #ifdef CONFIG_X86_32 >>>>> uint8_t pad[4]; >>>>> #endif >>>>> >>>>> in pvcalls.pandoc was to make sure that these structs would be 64bit >>>>> aligned on x86_32 too. >>>>> >>>>> I realize that the public header doesn't match, but the spec is the >>>>> "master copy". >>>> >>>> So far, the public headers are the defacto official ABI. So did you mark the >>>> pvcall header as just a reference? >>> >>> No, there is no document that says that the canonical copy of the >>> interface is pvcalls.pandoc. However, it was clearly spelled out from >>> the start on xen-devel (see below.) >>> In fact, if you notice, this is the >>> first document under docs/misc that goes into this level of details in >>> describing a new PV protocol. Also note the title of the document which >>> is "PV Calls Protocol version 1". >> >> While I understand this may have been the original intention, you >> can't expect a developer to go through the archive to check whether >> he/she should trust the header of the document. >> >>> >>> >>> In reply to Jan: >>>> A public header can't be "fixed" if it may already be in use by >>>> anyone. We can only do as Andrew and you suggest (mandate textual >>>> descriptions to be "the ABI") when we do so for _new_ interfaces from >>>> the very beginning, making clear that the public header (if any) >>>> exists just for reference. >>> >>> What if somebody took the specification of the interface from >>> pvcalls.pandoc and wrote their own headers and code? It is definitely >>> possible. >> >> As it is possible for someone to have picked the headers from Xen as >> in the past public/ has always been the authority. > > We never had documents under docs/ before specifying the interfaces > before pvcalls. It is not written anywhere that the headers under > public/ are the authoritative interfaces either, it is just that it was > the only thing available before. If you are new to the project you might > go to docs/ first. > > >>> At the time, it was clarified that the purpose of writing such >>> a detailed specification document was that the document was the >>> specification :-) >> >> At the risk of being pedantic, if it is not written in xen.git it >> doesn't exist ;). >> >> Anyway, no matter the decision you take here, you are going to >> potentially break one set of the users. >> >> I am leaning towards the header as authoritative because this has >> always been the case in the past and nothing in xen.git says >> otherwise. However I am not a user of pvcalls, so I don't really have >> any big incentive to go either way. > > Yeah, we are risking breaking one set of users either way :-/ > In reality, we are using pvcalls on arm64 in a new project (but it is > still very green). I am not aware of anybody using pvcalls on x86 > (especially x86_32). > > I would prefer to honor the pvcalls.pandoc specification because that is > what it was meant to be, and also leads to a better protocol > specification. As Jan and you disagree on the approach, I would like to get more input. To summarize the discussion, the document for PV calls and the public headers don't match when describing the padding. There is a disagreement on which of the two are the authority and therefore which one to fix. Does anyone else have a preference on the approach? > > >> For the future, I would highly suggest writing down the support >> decision in xen.git. This would avoid such debate on what is the >> authority... > > Yes that's the way to go >
Hi Gentle ping. It would be good to get this resolved for Xen 4.14. On 18/06/2020 16:00, Julien Grall wrote: > (+ Committers) > > On 18/06/2020 02:34, Stefano Stabellini wrote: >> On Tue, 16 Jun 2020, Julien Grall wrote: >>> On Tue, 16 Jun 2020 at 21:57, Stefano Stabellini >>> <sstabellini@kernel.org> wrote: >>>> >>>> On Tue, 16 Jun 2020, Julien Grall wrote: >>>>> On 16/06/2020 02:00, Stefano Stabellini wrote: >>>>>> On Sat, 13 Jun 2020, Julien Grall wrote: >>>>>>> From: Julien Grall <jgrall@amazon.com> >>>>>>> >>>>>>> The documentation of pvcalls suggests there is padding for 32-bit >>>>>>> x86 >>>>>>> at the end of most the structure. However, they are not described in >>>>>>> in the public header. >>>>>>> >>>>>>> Because of that all the structures would be 32-bit aligned and not >>>>>>> 64-bit aligned for 32-bit x86. >>>>>>> >>>>>>> For all the other architectures supported (Arm and 64-bit x86), the >>>>>>> structure are aligned to 64-bit because they contain uint64_t field. >>>>>>> Therefore all the structures contain implicit padding. >>>>>>> >>>>>>> The paddings are now corrected for 32-bit x86 and written >>>>>>> explicitly for >>>>>>> all the architectures. >>>>>>> >>>>>>> While the structure size between 32-bit and 64-bit x86 is >>>>>>> different, it >>>>>>> shouldn't cause any incompatibility between a 32-bit and 64-bit >>>>>>> frontend/backend because the commands are always 56 bits and the >>>>>>> padding >>>>>>> are at the end of the structure. >>>>>>> >>>>>>> As an aside, the padding sadly cannot be mandated to be 0 as they >>>>>>> are >>>>>>> already present. So it is not going to be possible to use the >>>>>>> padding >>>>>>> for extending a command in the future. >>>>>>> >>>>>>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>>>>>> >>>>>>> --- >>>>>>> Changes in v3: >>>>>>> - Use __i386__ rather than CONFIG_X86_32 >>>>>>> >>>>>>> Changes in v2: >>>>>>> - It is not possible to use the same padding for 32-bit >>>>>>> x86 and >>>>>>> all the other supported architectures. >>>>>>> --- >>>>>>> docs/misc/pvcalls.pandoc | 18 ++++++++++-------- >>>>>>> xen/include/public/io/pvcalls.h | 14 ++++++++++++++ >>>>>>> 2 files changed, 24 insertions(+), 8 deletions(-) >>>>>>> >>>>>>> diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc >>>>>>> index 665dad556c39..caa71b36d78b 100644 >>>>>>> --- a/docs/misc/pvcalls.pandoc >>>>>>> +++ b/docs/misc/pvcalls.pandoc >>>>>>> @@ -246,9 +246,9 @@ The format is defined as follows: >>>>>>> uint32_t domain; >>>>>>> uint32_t type; >>>>>>> uint32_t protocol; >>>>>>> - #ifdef CONFIG_X86_32 >>>>>>> + #ifndef __i386__ >>>>>>> uint8_t pad[4]; >>>>>>> - #endif >>>>>>> + #endif >>>>>> >>>>>> >>>>>> Hi Julien, >>>>>> >>>>>> Thank you for doing this, and sorry for having missed v2 of this >>>>>> patch, I >>>>>> should have replied earlier. >>>>>> >>>>>> The intention of the #ifdef blocks like: >>>>>> >>>>>> #ifdef CONFIG_X86_32 >>>>>> uint8_t pad[4]; >>>>>> #endif >>>>>> >>>>>> in pvcalls.pandoc was to make sure that these structs would be 64bit >>>>>> aligned on x86_32 too. >>>>>> >>>>>> I realize that the public header doesn't match, but the spec is the >>>>>> "master copy". >>>>> >>>>> So far, the public headers are the defacto official ABI. So did you >>>>> mark the >>>>> pvcall header as just a reference? >>>> >>>> No, there is no document that says that the canonical copy of the >>>> interface is pvcalls.pandoc. However, it was clearly spelled out from >>>> the start on xen-devel (see below.) >>>> In fact, if you notice, this is the >>>> first document under docs/misc that goes into this level of details in >>>> describing a new PV protocol. Also note the title of the document which >>>> is "PV Calls Protocol version 1". >>> >>> While I understand this may have been the original intention, you >>> can't expect a developer to go through the archive to check whether >>> he/she should trust the header of the document. >>> >>>> >>>> >>>> In reply to Jan: >>>>> A public header can't be "fixed" if it may already be in use by >>>>> anyone. We can only do as Andrew and you suggest (mandate textual >>>>> descriptions to be "the ABI") when we do so for _new_ interfaces from >>>>> the very beginning, making clear that the public header (if any) >>>>> exists just for reference. >>>> >>>> What if somebody took the specification of the interface from >>>> pvcalls.pandoc and wrote their own headers and code? It is definitely >>>> possible. >>> >>> As it is possible for someone to have picked the headers from Xen as >>> in the past public/ has always been the authority. >> >> We never had documents under docs/ before specifying the interfaces >> before pvcalls. It is not written anywhere that the headers under >> public/ are the authoritative interfaces either, it is just that it was >> the only thing available before. If you are new to the project you might >> go to docs/ first. >> >> >>>> At the time, it was clarified that the purpose of writing such >>>> a detailed specification document was that the document was the >>>> specification :-) >>> >>> At the risk of being pedantic, if it is not written in xen.git it >>> doesn't exist ;). >>> >>> Anyway, no matter the decision you take here, you are going to >>> potentially break one set of the users. >>> >>> I am leaning towards the header as authoritative because this has >>> always been the case in the past and nothing in xen.git says >>> otherwise. However I am not a user of pvcalls, so I don't really have >>> any big incentive to go either way. >> >> Yeah, we are risking breaking one set of users either way :-/ >> In reality, we are using pvcalls on arm64 in a new project (but it is >> still very green). I am not aware of anybody using pvcalls on x86 >> (especially x86_32). >> >> I would prefer to honor the pvcalls.pandoc specification because that is >> what it was meant to be, and also leads to a better protocol >> specification. > > As Jan and you disagree on the approach, I would like to get more input. > > To summarize the discussion, the document for PV calls and the public > headers don't match when describing the padding. There is a disagreement > on which of the two are the authority and therefore which one to fix. > > Does anyone else have a preference on the approach? > >> >> >>> For the future, I would highly suggest writing down the support >>> decision in xen.git. This would avoid such debate on what is the >>> authority... >> >> Yes that's the way to go >> >
Julien Grall writes ("Re: [PATCH v3 for-4.14] pvcalls: Document correctly and explicitely the padding for all arches"):
> (+ Committers)
...
> As Jan and you disagree on the approach, I would like to get more input.
>
> To summarize the discussion, the document for PV calls and the public
> headers don't match when describing the padding. There is a disagreement
> on which of the two are the authority and therefore which one to fix.
>
> Does anyone else have a preference on the approach?
Hi.
>[Jan:]
>> I am leaning towards the header as authoritative because this has
>> always been the case in the past and nothing in xen.git says
>> otherwise. However I am not a user of pvcalls, so I don't really have
>> any big incentive to go either way.
I think the practice of using headers as protocol specs is not a
particularly good one. Certainly my expectations anywhere outside the
Xen Project is that if there's a doc, that is at the very least on par
with any header file. Of course there are possible compatibility
implications:
> Yeah, we are risking breaking one set of users either way :-/
> In reality, we are using pvcalls on arm64 in a new project (but it is
> still very green). I am not aware of anybody using pvcalls on x86
> (especially x86_32).
>
> I would prefer to honor the pvcalls.pandoc specification because that is
> what it was meant to be, and also leads to a better protocol
> specification.
pvcalls in Linux is Tech Preview / Experimental AFAICT ? I think that
means we can de jure change things like this.
And it seems that we don't think there are any actual users who would
experience compatibility problems.
So I don't think the compatibility concerns are a reason not to change
the header rather than the document.
So I think my conclusion is the same as Julien's: we should change the
header to match the doc.
> >> For the future, I would highly suggest writing down the support
> >> decision in xen.git. This would avoid such debate on what is the
> >> authority...
> >
> > Yes that's the way to go
Maybe it would be worth putting a note somewhere in the headers saying
the headers are provided for convenience but that the ABIs and
protocols are as specified in the docs (at least where docs exist).
Ian.
Julien Grall writes ("Re: [INPUT REQUESTED][PATCH v3 for-4.14] pvcalls: Document correctly and explicitely the padding for all arches"):
> Gentle ping. It would be good to get this resolved for Xen 4.14.
Oh and I agree that this should be changed for 4.14.
Ian.
Hi Ian, Thank you for your input! On 24/06/2020 13:04, Ian Jackson wrote: > Julien Grall writes ("Re: [PATCH v3 for-4.14] pvcalls: Document correctly and explicitely the padding for all arches"): >> (+ Committers) > ... >> As Jan and you disagree on the approach, I would like to get more input. >> >> To summarize the discussion, the document for PV calls and the public >> headers don't match when describing the padding. There is a disagreement >> on which of the two are the authority and therefore which one to fix. >> >> Does anyone else have a preference on the approach? > > Hi. > >> [Jan:] >>> I am leaning towards the header as authoritative because this has >>> always been the case in the past and nothing in xen.git says >>> otherwise. However I am not a user of pvcalls, so I don't really have >>> any big incentive to go either way. > > I think the practice of using headers as protocol specs is not a > particularly good one. Certainly my expectations anywhere outside the > Xen Project is that if there's a doc, that is at the very least on par > with any header file. Of course there are possible compatibility > implications: > >> Yeah, we are risking breaking one set of users either way :-/ >> In reality, we are using pvcalls on arm64 in a new project (but it is >> still very green). I am not aware of anybody using pvcalls on x86 >> (especially x86_32). >> >> I would prefer to honor the pvcalls.pandoc specification because that is >> what it was meant to be, and also leads to a better protocol >> specification. > > pvcalls in Linux is Tech Preview / Experimental AFAICT ? I think that > means we can de jure change things like this. SUPPORT.md suggests this is a Tech Preview, so I agree that we could still change the interface. > > And it seems that we don't think there are any actual users who would > experience compatibility problems. Right, that's what Stefano suggested. > > So I don't think the compatibility concerns are a reason not to change > the header rather than the document. > > So I think my conclusion is the same as Julien's: we should change the > header to match the doc. Ok, so you are on the same page as Stefano. I will revert to the v1 change and rework the commit message then. > >>>> For the future, I would highly suggest writing down the support >>>> decision in xen.git. This would avoid such debate on what is the >>>> authority... >>> >>> Yes that's the way to go > > Maybe it would be worth putting a note somewhere in the headers saying > the headers are provided for convenience but that the ABIs and > protocols are as specified in the docs (at least where docs exist). I will write a patch for it. Cheers,
diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc index 665dad556c39..caa71b36d78b 100644 --- a/docs/misc/pvcalls.pandoc +++ b/docs/misc/pvcalls.pandoc @@ -246,9 +246,9 @@ The format is defined as follows: uint32_t domain; uint32_t type; uint32_t protocol; - #ifdef CONFIG_X86_32 + #ifndef __i386__ uint8_t pad[4]; - #endif + #endif } socket; struct xen_pvcalls_connect { uint64_t id; @@ -257,16 +257,18 @@ The format is defined as follows: uint32_t flags; grant_ref_t ref; uint32_t evtchn; - #ifdef CONFIG_X86_32 + #ifndef __i386__ uint8_t pad[4]; - #endif + #endif } connect; struct xen_pvcalls_release { uint64_t id; uint8_t reuse; - #ifdef CONFIG_X86_32 + #ifndef __i386__ uint8_t pad[7]; - #endif + #else + uint8_t pad[3]; + #endif } release; struct xen_pvcalls_bind { uint64_t id; @@ -276,9 +278,9 @@ The format is defined as follows: struct xen_pvcalls_listen { uint64_t id; uint32_t backlog; - #ifdef CONFIG_X86_32 + #ifndef __i386__ uint8_t pad[4]; - #endif + #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 cb8171275c13..28374a82f410 100644 --- a/xen/include/public/io/pvcalls.h +++ b/xen/include/public/io/pvcalls.h @@ -65,6 +65,9 @@ struct xen_pvcalls_request { uint32_t domain; uint32_t type; uint32_t protocol; +#ifndef __i386__ + uint8_t pad[4]; +#endif } socket; struct xen_pvcalls_connect { uint64_t id; @@ -73,10 +76,18 @@ struct xen_pvcalls_request { uint32_t flags; grant_ref_t ref; uint32_t evtchn; +#ifndef __i386__ + uint8_t pad[4]; +#endif } connect; struct xen_pvcalls_release { uint64_t id; uint8_t reuse; +#ifndef __i386__ + uint8_t pad[7]; +#else + uint8_t pad[3]; +#endif } release; struct xen_pvcalls_bind { uint64_t id; @@ -86,6 +97,9 @@ struct xen_pvcalls_request { struct xen_pvcalls_listen { uint64_t id; uint32_t backlog; +#ifndef __i386__ + uint8_t pad[4]; +#endif } listen; struct xen_pvcalls_accept { uint64_t id;