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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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...

>
>     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.

~Andrew

>      if ( __copy_field_to_guest(arg, &exch, nr_exchanged) )
>          rc = -EFAULT;
>      return rc;
> +#else /* !CONFIG_PV */
> +    return -EOPNOTSUPP;
> +#endif
>  }
>  
>  int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp,