Re: [PATCH v5 1/4] xen: XENMEM_exchange should only be used/compiled for arch supporting PV guest

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.07.2021 15:39, Julien Grall wrote:
> On 05/07/2021 09:41, Jan Beulich wrote:
>> On 03.07.2021 19:11, Julien Grall wrote:
>>> Changes in v5:
>>>      - Removed the #ifdef CONFIG_X86 as they are not necessary anymore
>>>      - Used paging_mode_translate() rather than is_pv_domain()
>>
>> Is there a particular reason you use this in favor of steal_page()'s
>> paging_mode_external()?
> 
> This is what you suggested in v4 [1]. I can switch to 
> paging_mode_external() if this is what you now prefer.

Well, I did say this would be better than is_pv_*(). I probably didn't
pay enough attention to you already pointing out paging_mode_external()
in the description; I'm sorry. On x86 both are in sync anyway, and I
have to admit I don't see clearly enough which of the two would be the
right one to use here, partly because I'm afraid I also don't see why
steal_page() has such a restriction in the first place (which you now
build upon).

>>> @@ -815,6 +812,9 @@ static long 
>>> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>>>       if ( __copy_field_to_guest(arg, &exch, nr_exchanged) )
>>>           rc = -EFAULT;
>>
>> I'm afraid that for correctness of the interface you need to keep
>> this part even in the !PV case.
> 
> Xen never initializes the field nr_exchanged. Instead, it expects the 
> guest to set to 0. So I am not quite to sure why we would need to keep 
> this line.

Hmm, the public header is wrong then, as it documents the field as
[OUT] only _despite_ the shouting warning in point 5 of the comment.
I guess I never really understood why this sub-op differs from
others in where the continuation indicator lives.

Never mind then, indeed no code adjustment needed:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan