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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 20/04/2020 08:26, Jan Beulich wrote:
> 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.
I agree the commit you mention above is confusing. If we follow the 
definition, then the conversion between the two internally should never 
have been done.
Maybe Stefano can clarify the intention?

> > 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?
I mean on Arm 32-bit. I have changed one of the function to use 
XEN_GUEST_HANDLE_PARAM() rather than XEN_GUEST_HANDLE() but not changing 
the caller.
It is probably because they are both defined using an union. Interestly, 
the type will also not be checked, so the code a function will happily 
accept a XEN_GUEST_HANDLE_PARAM(uint8) even if the prototype requested 
XEN_GUEST_HANDLE_PARAM(uint64).
This looks rather messy, maybe we should use a structure (and some 
alignment) to add more safety.
Cheers,

--
Julien Grall