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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

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:
> > > While it should have been this way from the beginning, not doing so will
> > > become an actual problem with PVH Dom0.
> > I think the current code is also buggy on PV dom0 because the buffer
> > is not locked in memory. So you have no promise the buffer will be
> > present when calling the hypercall.
> Quite possibly; I didn't looks at that aspect at all.
>
> > > --- a/tools/libxc/xc_mem_paging.c
> > > +++ b/tools/libxc/xc_mem_paging.c
> > > @@ -26,15 +26,33 @@ static int xc_mem_paging_memop(xc_interf
> > >                                   unsigned int op, uint64_t gfn, void *buffer)
> > NIT: As you switch the handle to use const, would it make to also
> > make the buffer const?
> A separate change, I would say, but if the tool stack maintainers
> agree with doing so at the same time, I certainly can.
Ok.

> > > --- 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().
FWIW, the different matters on Arm. Although, it looks like the compiler 
will not warn you if you are using the wrong handler :(.
Cheers,

--
Julien Grall