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(+)

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
-- 
2.34.1

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