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

[Julien Grall <julien@xxxxxxx>]

Hi Andrew,

On 21/09/2020 20:46, Andrew Cooper wrote:
> On 21/09/2020 19:02, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > XENMEM_exchange can only be used by PV guest but the check is well
> > hidden in steal_page(). This is because paging_model_external() will
> > return false only for PV domain.
> >
> > To make clearer this is PV only, add a check at the beginning of the
> > implementation. Take the opportunity to compile out the code if
> > CONFIG_PV is not set.
> >
> > This change will also help a follow-up patch where the gmfn_mfn() will
> > be completely removed on arch not supporting the M2P.
> >
> > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > ---
> >
> > Jan suggested to #ifdef anything after the check to is_pv_domain().
> > However, it means to have two block of #ifdef as we can't mix
> > declaration and code.
> >
> > I am actually thinking to move the implementation outside of mm.c in
> > possibly arch/x86 or a pv specific directory under common. Any opinion?
> arch/x86/pv/mm.c, with the case XENMEM_exchange: moving into
> arch_memory_op().
>
> However, making this happen is incredibly tangled, and we're years
> overdue a fix here.
>
> Lets go with this for now, and tidying up can come later.
>
> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, however...
Thanks!

> >      Changes in v4:
> >          - Patch added
> > ---
> >   xen/common/memory.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/xen/common/memory.c b/xen/common/memory.c
> > index 714077c1e597..9300104943b0 100644
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -504,6 +504,7 @@ static bool propagate_node(unsigned int xmf, unsigned int 
> > *memflags)
> >   
> >   static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
> >   {
> > +#ifdef CONFIG_PV
> >       struct xen_memory_exchange exch;
> >       PAGE_LIST_HEAD(in_chunk_list);
> >       PAGE_LIST_HEAD(out_chunk_list);
> > @@ -516,6 +517,9 @@ static long 
> > memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
> >       struct domain *d;
> >       struct page_info *page;
> >   
> > +    if ( !is_pv_domain(d) )
> > +        return -EOPNOTSUPP;
> > +
> >       if ( copy_from_guest(&exch, arg, 1) )
> >           return -EFAULT;
> >   
> > @@ -797,6 +801,9 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
> ... there are now a load of #ifdef CONFIG_X86 between these two hunks
> which can be dropped.
I didn't drop them because I wasn't sure whether we wanted to cater 
future arch.
Anyway, I am happy to do the cleanup :).

Cheers,

--
Julien Grall