From: Julien Grall <jgrall@amazon.com>

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@amazon.com>

---

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?

    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)
     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,
-- 
2.17.1

On 21.09.2020 20:02, Julien Grall wrote:
> --- 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;

I think "paging_mode_translate(d)" would be more correct, at which
point the use later in the function ought to be dropped (as that's
precisely one of the things making this function not really
sensible to use on translated guests).

I also wonder whether the #ifdef wouldn't better be left out. Yes,
that'll mean retaining a stub mfn_to_gmfn(), but it could expand
to simply BUG() then, for example.

Jan

Hi Jan,

On 22/09/2020 08:35, Jan Beulich wrote:
> On 21.09.2020 20:02, Julien Grall wrote:
>> --- 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;
> 
> I think "paging_mode_translate(d)" would be more correct, at which
> point the use later in the function ought to be dropped (as that's
> precisely one of the things making this function not really
> sensible to use on translated guests).

I have used paging_mode_translate(d) and remove the later use.

> 
> I also wonder whether the #ifdef wouldn't better be left out. Yes,
> that'll mean retaining a stub mfn_to_gmfn(), but it could expand
> to simply BUG() then, for example.

Keeping mfn_to_gmfn() will not discourage developper to add more use of 
it. The risk is we don't notice it when reviewing  and this would lead 
to hit a BUG_ON().

Given this will be the only place where mfn_to_gmfn() is used, I think 
it is best to stub the code. Bear in mind that long term, this function 
should leave outside of common/mm.c (see Andrew's e-mail).

Cheers,

-- 
Julien Grall