diff mbox series

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

Message ID 20200921180214.4842-2-julien@xen.org (mailing list archive)
State New, archived
Headers show
Series xen/arm: Properly disable M2P on Arm | expand

Commit Message

Julien Grall Sept. 21, 2020, 6:02 p.m. UTC 
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(+)

Comments

Andrew Cooper Sept. 21, 2020, 7:46 p.m. UTC | #1 
On 21/09/2020 19:02, Julien Grall wrote:
> 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?

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@citrix.com>, 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,
Jan Beulich Sept. 22, 2020, 7:35 a.m. UTC | #2 
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
Julien Grall Sept. 22, 2020, 5:53 p.m. UTC | #3 
Hi Andrew,

On 21/09/2020 20:46, Andrew Cooper wrote:
> On 21/09/2020 19:02, Julien Grall wrote:
>> 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?
> 
> 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@citrix.com>, 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 Sept. 22, 2020, 6:05 p.m. UTC | #4 
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,
diff mbox series

Patch

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,