diff mbox series

[v4,4/4] xen/mm: Provide dummy M2P-related helpers when !CONFIG_HAVE_M2P

Message ID 20200921180214.4842-5-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 <julien.grall@arm.com>

At the moment, Arm is providing a dummy implementation for the M2P
helpers used in common code. However, they are quite isolated and could
be used by other architecture in the future. So move all the helpers in
xen/mm.h.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
    Changes in v4:
        - The tags were dropped as the previous version was sent a long
        time ago.

    Changes in v3:
        - Add Stefano's reviewed-by
        - Add George's acked-by

    Changes in v2:
        - Patch added
---
 xen/include/asm-arm/mm.h | 11 -----------
 xen/include/xen/mm.h     | 13 +++++++++++++
 2 files changed, 13 insertions(+), 11 deletions(-)

Comments

Andrew Cooper Sept. 21, 2020, 8:37 p.m. UTC | #1 
On 21/09/2020 19:02, Julien Grall wrote:
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 4536a62940a1..15bb0aa30d22 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -685,4 +685,17 @@ static inline void put_page_alloc_ref(struct page_info *page)
>      }
>  }
>  
> +/*
> + * Dummy implementation of M2P-related helpers for common code when
> + * the architecture doesn't have an M2P.
> + */
> +#ifndef CONFIG_HAS_M2P
> +
> +#define INVALID_M2P_ENTRY        (~0UL)
> +#define SHARED_M2P(_e)           false

((void)_e, false)

Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> +
> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) {}
> +
> +#endif
> +
>  #endif /* __XEN_MM_H__ */
Jan Beulich Sept. 22, 2020, 8:02 a.m. UTC | #2 
On 21.09.2020 20:02, Julien Grall wrote:
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -685,4 +685,17 @@ static inline void put_page_alloc_ref(struct page_info *page)
>      }
>  }
>  
> +/*
> + * Dummy implementation of M2P-related helpers for common code when
> + * the architecture doesn't have an M2P.
> + */
> +#ifndef CONFIG_HAS_M2P
> +
> +#define INVALID_M2P_ENTRY        (~0UL)
> +#define SHARED_M2P(_e)           false
> +
> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) {}

While I think this would better BUG() or at least ASSERT_UNREACHABLE(),
I realize its use in page_alloc.c prevents this. However, if this was a
macro, I think the need for having INVALID_P2M_ENTRY would vanish, as
long as the stub macro didn't evaluate its 2nd argument.

I'm feeling somewhat uneasy with the SHARED_M2P() definition: This
would seem to better be tied to CONFIG_MEM_SHARING rather than M2P
existence.

Jan
Julien Grall Sept. 22, 2020, 6:39 p.m. UTC | #3 
Hi Jan,

On 22/09/2020 09:02, Jan Beulich wrote:
> On 21.09.2020 20:02, Julien Grall wrote:
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -685,4 +685,17 @@ static inline void put_page_alloc_ref(struct page_info *page)
>>       }
>>   }
>>   
>> +/*
>> + * Dummy implementation of M2P-related helpers for common code when
>> + * the architecture doesn't have an M2P.
>> + */
>> +#ifndef CONFIG_HAS_M2P
>> +
>> +#define INVALID_M2P_ENTRY        (~0UL)
>> +#define SHARED_M2P(_e)           false
>> +
>> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) {}
> 
> While I think this would better BUG() or at least ASSERT_UNREACHABLE(),
> I realize its use in page_alloc.c prevents this. However, if this was a
> macro, I think the need for having INVALID_P2M_ENTRY would vanish, as
> long as the stub macro didn't evaluate its 2nd argument.
This is not very future proof... The cost of defining INVALID_M2P_ENTRY 
is very minimal compare to the damage that may result from this choice.

> I'm feeling somewhat uneasy with the SHARED_M2P() definition: This
> would seem to better be tied to CONFIG_MEM_SHARING rather than M2P
> existence.

I can see pros and cons in both solution. To me it contains the word 
"M2P" so it makes sense to be protected by HAS_M2P.

If someone else think that it should be protected by CONFIG_MEM_SHARING, 
then I will do the change.

I have added Tamas to give him an opportunity to share his view.

Cheers,
Andrew Cooper Sept. 22, 2020, 6:59 p.m. UTC | #4 
On 22/09/2020 19:39, Julien Grall wrote:
> Hi Jan,
>
> On 22/09/2020 09:02, Jan Beulich wrote:
>> On 21.09.2020 20:02, Julien Grall wrote:
>>> --- a/xen/include/xen/mm.h
>>> +++ b/xen/include/xen/mm.h
>>> @@ -685,4 +685,17 @@ static inline void put_page_alloc_ref(struct
>>> page_info *page)
>>>       }
>>>   }
>>>   +/*
>>> + * Dummy implementation of M2P-related helpers for common code when
>>> + * the architecture doesn't have an M2P.
>>> + */
>>> +#ifndef CONFIG_HAS_M2P
>>> +
>>> +#define INVALID_M2P_ENTRY        (~0UL)
>>> +#define SHARED_M2P(_e)           false
>>> +
>>> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned
>>> long pfn) {}
>>
>> While I think this would better BUG() or at least ASSERT_UNREACHABLE(),
>> I realize its use in page_alloc.c prevents this. However, if this was a
>> macro, I think the need for having INVALID_P2M_ENTRY would vanish, as
>> long as the stub macro didn't evaluate its 2nd argument.
> This is not very future proof... The cost of defining
> INVALID_M2P_ENTRY is very minimal compare to the damage that may
> result from this choice.
>
>> I'm feeling somewhat uneasy with the SHARED_M2P() definition: This
>> would seem to better be tied to CONFIG_MEM_SHARING rather than M2P
>> existence.
>
> I can see pros and cons in both solution. To me it contains the word
> "M2P" so it makes sense to be protected by HAS_M2P.
>
> If someone else think that it should be protected by
> CONFIG_MEM_SHARING, then I will do the change.
>
> I have added Tamas to give him an opportunity to share his view.

This is clearly guarded by HAS_M2P first first and foremost.

However, the work to actually let MEM_SHARING be turned off in this
regard is rather larger, and not appropriate to delay this series with.

~Andrew
Tamas K Lengyel Sept. 22, 2020, 8:35 p.m. UTC | #5 
> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: Tuesday, September 22, 2020 3:00 PM
> To: Julien Grall <julien@xen.org>; Jan Beulich <jbeulich@suse.com>
> Cc: xen-devel@lists.xenproject.org; Julien Grall <julien.grall@arm.com>;
> Stefano Stabellini <sstabellini@kernel.org>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; George Dunlap
> <george.dunlap@citrix.com>; Ian Jackson <iwj@xenproject.org>; Wei Liu
> <wl@xen.org>; Lengyel, Tamas <tamas.lengyel@intel.com>
> Subject: Re: [PATCH v4 4/4] xen/mm: Provide dummy M2P-related helpers
> when !CONFIG_HAVE_M2P
> 
> On 22/09/2020 19:39, Julien Grall wrote:
> > Hi Jan,
> >
> > On 22/09/2020 09:02, Jan Beulich wrote:
> >> On 21.09.2020 20:02, Julien Grall wrote:
> >>> --- a/xen/include/xen/mm.h
> >>> +++ b/xen/include/xen/mm.h
> >>> @@ -685,4 +685,17 @@ static inline void put_page_alloc_ref(struct
> >>> page_info *page)
> >>>       }
> >>>   }
> >>>   +/*
> >>> + * Dummy implementation of M2P-related helpers for common code
> when
> >>> + * the architecture doesn't have an M2P.
> >>> + */
> >>> +#ifndef CONFIG_HAS_M2P
> >>> +
> >>> +#define INVALID_M2P_ENTRY        (~0UL) #define SHARED_M2P(_e)
> >>> +false
> >>> +
> >>> +static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned
> >>> long pfn) {}
> >>
> >> While I think this would better BUG() or at least
> >> ASSERT_UNREACHABLE(), I realize its use in page_alloc.c prevents
> >> this. However, if this was a macro, I think the need for having
> >> INVALID_P2M_ENTRY would vanish, as long as the stub macro didn't
> evaluate its 2nd argument.
> > This is not very future proof... The cost of defining
> > INVALID_M2P_ENTRY is very minimal compare to the damage that may
> > result from this choice.
> >
> >> I'm feeling somewhat uneasy with the SHARED_M2P() definition: This
> >> would seem to better be tied to CONFIG_MEM_SHARING rather than M2P
> >> existence.
> >
> > I can see pros and cons in both solution. To me it contains the word
> > "M2P" so it makes sense to be protected by HAS_M2P.
> >
> > If someone else think that it should be protected by
> > CONFIG_MEM_SHARING, then I will do the change.
> >
> > I have added Tamas to give him an opportunity to share his view.
> 
> This is clearly guarded by HAS_M2P first first and foremost.
> 
> However, the work to actually let MEM_SHARING be turned off in this regard is
> rather larger, and not appropriate to delay this series with.

I don't see any issue with making CONFIG_MEM_SHARING also depend on CONFIG_HAS_M2P, so IMHO it would be enough to put both behind that.

Tamas
diff mbox series

Patch

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 29489a3e1076..5929201d0299 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -318,17 +318,6 @@  static inline void *page_to_virt(const struct page_info *pg)
 struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
                                     unsigned long flags);
 
-/*
- * Arm does not have an M2P, but common code expects a handful of
- * M2P-related defines and functions. Provide dummy versions of these.
- */
-#define INVALID_M2P_ENTRY        (~0UL)
-#define SHARED_M2P_ENTRY         (~0UL - 1UL)
-#define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
-
-/* We don't have a M2P on Arm */
-#define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
-
 /* Arch-specific portion of memory_op hypercall. */
 long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
 
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 4536a62940a1..15bb0aa30d22 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -685,4 +685,17 @@  static inline void put_page_alloc_ref(struct page_info *page)
     }
 }
 
+/*
+ * Dummy implementation of M2P-related helpers for common code when
+ * the architecture doesn't have an M2P.
+ */
+#ifndef CONFIG_HAS_M2P
+
+#define INVALID_M2P_ENTRY        (~0UL)
+#define SHARED_M2P(_e)           false
+
+static inline void set_gpfn_from_mfn(unsigned long mfn, unsigned long pfn) {}
+
+#endif
+
 #endif /* __XEN_MM_H__ */