Re: [PATCH 3/6] x86/mem-paging: use guest handle for XENMEM_paging_op_prep

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.04.2020 19:13, Julien Grall wrote:
> On 17/04/2020 10:44, Jan Beulich wrote:
>> On 17.04.2020 10:37, Julien Grall wrote:
>>> On 16/04/2020 16:46, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/mm/p2m.c
>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>> @@ -1779,7 +1779,8 @@ void p2m_mem_paging_populate(struct doma
>>>>     * mfn if populate was called for  gfn which was nominated but not 
>>>> evicted. In
>>>>     * this case only the p2mt needs to be forwarded.
>>>>     */
>>>> -int p2m_mem_paging_prep(struct domain *d, unsigned long gfn_l, uint64_t 
>>>> buffer)
>>>> +int p2m_mem_paging_prep(struct domain *d, unsigned long gfn_l,
>>>> +                        XEN_GUEST_HANDLE_PARAM(const_uint8) buffer)
>>>
>>> Shouldn't this technically be XEN_GUEST_HANDLE_64() to match the field?
>>
>> I think an argument can be made for going either way - as a function
>> parameter it should have the type chosen. Do you see any (possibly
>> just latent) breakage from using _PARAM() rather than _64()?
> I know they are the same on x86, but from an abstract PoV they are 
> fundamentally different.
> 
> XEN_GUEST_HANDLE_PARAM() represents a guest pointer, when pased as an
> hypercall argument.
> 
> XEN_GUEST_HANDLE() represents a guest pointer, when passed as a field
> in a struct in memory.
> 
> In this case, the guest pointer was part of a structure. So I think
> you want to use XEN_GUEST_HANDLE().

Hmm, looks like I was confused about what the two mean. So far I was
under the impression that _PARAM() was to be used for function
parameters in general, not just hypercall ones. While the text near
the macro definitions is quite clear in this regard, I'm afraid
Stefano's original series (first and foremost commit abf06ea91d12's
playing with e.g. handle_iomem_range()) was rather confusing than
helpful - it looks to me as if quite a bit of the "casting" could
actually be dropped (I'll see about doing some cleanup there). Plus
I'm afraid other mixing of plain vs param has been introduced on
x86, at least for dm.c:track_dirty_vram()'s calls to
{hap,shadow}_track_dirty_vram(); this is just the first instance
I've found - there may be more.

> FWIW, the different matters on Arm. Although, it looks like the
> compiler will not warn you if you are using the wrong handler :(.

I find this highly suspicious, but can't check myself until back
in the office - these are distinct compound types after all, so
this shouldn't just be a warning, but an error. Or did you merely
mean there's no warning on x86?

Jan