| Message ID | 20220224152414.27948-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] xen/public: add comment to struct xen_mem_acquire_resource | expand |
On 24.02.2022 16:24, Juergen Gross wrote: > --- a/xen/include/public/memory.h > +++ b/xen/include/public/memory.h > @@ -662,6 +662,13 @@ struct xen_mem_acquire_resource { > * two calls. > */ > uint32_t nr_frames; > + /* > + * Padding field, must be zero on input. > + * In a previous version this was an output field with the lowest > + * bit named XENMEM_rsrc_acq_caller_owned. Future versions of this > + * interface will not reuse this bit with the field being zero on > + * input. > + */ > uint32_t pad; Did you mean "... being non-zero ..." and "bit" and "field" changing positions? Jan
On 24.02.22 16:37, Jan Beulich wrote: > On 24.02.2022 16:24, Juergen Gross wrote: >> --- a/xen/include/public/memory.h >> +++ b/xen/include/public/memory.h >> @@ -662,6 +662,13 @@ struct xen_mem_acquire_resource { >> * two calls. >> */ >> uint32_t nr_frames; >> + /* >> + * Padding field, must be zero on input. >> + * In a previous version this was an output field with the lowest >> + * bit named XENMEM_rsrc_acq_caller_owned. Future versions of this >> + * interface will not reuse this bit with the field being zero on >> + * input. >> + */ >> uint32_t pad; > > Did you mean "... being non-zero ..." and "bit" and "field" changing > positions? No, why? The current Linux kernel will set pad (the "field") to zero when doing the hypercall, and it expects the bit to be set or not on return. This means that the bit is reserved for the case that pad was zero on input. Juergen
On 24.02.2022 16:41, Juergen Gross wrote: > On 24.02.22 16:37, Jan Beulich wrote: >> On 24.02.2022 16:24, Juergen Gross wrote: >>> --- a/xen/include/public/memory.h >>> +++ b/xen/include/public/memory.h >>> @@ -662,6 +662,13 @@ struct xen_mem_acquire_resource { >>> * two calls. >>> */ >>> uint32_t nr_frames; >>> + /* >>> + * Padding field, must be zero on input. >>> + * In a previous version this was an output field with the lowest >>> + * bit named XENMEM_rsrc_acq_caller_owned. Future versions of this >>> + * interface will not reuse this bit with the field being zero on >>> + * input. >>> + */ >>> uint32_t pad; >> >> Did you mean "... being non-zero ..." and "bit" and "field" changing >> positions? > > No, why? The current Linux kernel will set pad (the "field") to zero > when doing the hypercall, and it expects the bit to be set or not on > return. This means that the bit is reserved for the case that pad > was zero on input. Hmm, maybe I got confused (but only in part by the wording). The bit is fine to use as input. This will mean the field is not zero on input, but the way this is worded is somewhat confusing. How about making things explicit: "... will not reuse this bit as an output with the field being zero on input"? Then Acked-by: Jan Beulich <jbeulich@suse.com> and I'd be fine making the adjustment while committing. Jan
On 24.02.22 17:23, Jan Beulich wrote: > On 24.02.2022 16:41, Juergen Gross wrote: >> On 24.02.22 16:37, Jan Beulich wrote: >>> On 24.02.2022 16:24, Juergen Gross wrote: >>>> --- a/xen/include/public/memory.h >>>> +++ b/xen/include/public/memory.h >>>> @@ -662,6 +662,13 @@ struct xen_mem_acquire_resource { >>>> * two calls. >>>> */ >>>> uint32_t nr_frames; >>>> + /* >>>> + * Padding field, must be zero on input. >>>> + * In a previous version this was an output field with the lowest >>>> + * bit named XENMEM_rsrc_acq_caller_owned. Future versions of this >>>> + * interface will not reuse this bit with the field being zero on >>>> + * input. >>>> + */ >>>> uint32_t pad; >>> >>> Did you mean "... being non-zero ..." and "bit" and "field" changing >>> positions? >> >> No, why? The current Linux kernel will set pad (the "field") to zero >> when doing the hypercall, and it expects the bit to be set or not on >> return. This means that the bit is reserved for the case that pad >> was zero on input. > > Hmm, maybe I got confused (but only in part by the wording). The bit is > fine to use as input. This will mean the field is not zero on input, but > the way this is worded is somewhat confusing. How about making things > explicit: "... will not reuse this bit as an output with the field being > zero on input"? Then Fine with me. > Acked-by: Jan Beulich <jbeulich@suse.com> > and I'd be fine making the adjustment while committing. Thanks, Juergen
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h index 383a9468c3..2d0817eab1 100644 --- a/xen/include/public/memory.h +++ b/xen/include/public/memory.h @@ -662,6 +662,13 @@ struct xen_mem_acquire_resource { * two calls. */ uint32_t nr_frames; + /* + * Padding field, must be zero on input. + * In a previous version this was an output field with the lowest + * bit named XENMEM_rsrc_acq_caller_owned. Future versions of this + * interface will not reuse this bit with the field being zero on + * input. + */ uint32_t pad; /* * IN - the index of the initial frame to be mapped. This parameter
Commit 7c7f7e8fba01 changed xen/include/public/memory.h in an incompatible way. Unfortunately the changed parts were already in use in the Linux kernel, so an update of the header in the kernel would result in a build breakage. As the change of above commit was in a section originally meant to be not stable, it was the usage in the kernel which was wrong. Add a comment to the modified struct for not reusing the now removed bit, in order to avoid kernels using it stumbling over a possible new meaning of the bit. In case the kernel is updating to a new version of the header, the wrong use case must be removed first. Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - only add comment instead of reverting commit 7c7f7e8fba01 (Jan Beulich) V3: - make pad field comment more specific --- xen/include/public/memory.h | 7 +++++++ 1 file changed, 7 insertions(+)