[03/17] xen/mm: Move the MM types in a separate header
diff mbox series

Message ID 20200322161418.31606-4-julien@xen.org
State New
Headers show
Series
  • Bunch of typesafe conversion
Related show

Commit Message

Julien Grall March 22, 2020, 4:14 p.m. UTC
From: Julien Grall <jgrall@amazon.com>

It is getting incredibly difficult to use typesafe GFN/MFN/PFN in the
headers because of circular dependency. For instance, asm-x86/page.h
cannot include xen/mm.h.

In order to convert more code to use typesafe, the types are now moved
in a separate header that requires only a few dependencies.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/include/xen/mm.h       | 134 +-------------------------------
 xen/include/xen/mm_types.h | 155 +++++++++++++++++++++++++++++++++++++
 2 files changed, 156 insertions(+), 133 deletions(-)
 create mode 100644 xen/include/xen/mm_types.h

Comments

Jan Beulich March 25, 2020, 3 p.m. UTC | #1
On 22.03.2020 17:14, julien@xen.org wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> It is getting incredibly difficult to use typesafe GFN/MFN/PFN in the
> headers because of circular dependency. For instance, asm-x86/page.h
> cannot include xen/mm.h.
> 
> In order to convert more code to use typesafe, the types are now moved
> in a separate header that requires only a few dependencies.

We definitely need to do this, so thanks for investing the
time. I think though that we want to settle up front (and
perhaps record in a comment in the new header) what is or
is not suitable to go into the new header. After all you're
moving not just type definitions, but also simple helper
functions.

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -1,50 +1,7 @@
>  /******************************************************************************
>   * include/xen/mm.h
>   *
> - * Definitions for memory pages, frame numbers, addresses, allocations, etc.
> - *
>   * Copyright (c) 2002-2006, K A Fraser <keir@xensource.com>
> - *
> - *                         +---------------------+
> - *                          Xen Memory Management
> - *                         +---------------------+
> - *
> - * Xen has to handle many different address spaces.  It is important not to
> - * get these spaces mixed up.  The following is a consistent terminology which
> - * should be adhered to.
> - *
> - * mfn: Machine Frame Number
> - *   The values Xen puts into its own pagetables.  This is the host physical
> - *   memory address space with RAM, MMIO etc.
> - *
> - * gfn: Guest Frame Number
> - *   The values a guest puts in its own pagetables.  For an auto-translated
> - *   guest (hardware assisted with 2nd stage translation, or shadowed), gfn !=
> - *   mfn.  For a non-translated guest which is aware of Xen, gfn == mfn.
> - *
> - * pfn: Pseudophysical Frame Number
> - *   A linear idea of a guest physical address space. For an auto-translated
> - *   guest, pfn == gfn while for a non-translated guest, pfn != gfn.
> - *
> - * dfn: Device DMA Frame Number (definitions in include/xen/iommu.h)
> - *   The linear frame numbers of device DMA address space. All initiators for
> - *   (i.e. all devices assigned to) a guest share a single DMA address space
> - *   and, by default, Xen will ensure dfn == pfn.
> - *
> - * WARNING: Some of these terms have changed over time while others have been
> - * used inconsistently, meaning that a lot of existing code does not match the
> - * definitions above.  New code should use these terms as described here, and
> - * over time older code should be corrected to be consistent.
> - *
> - * An incomplete list of larger work area:
> - * - Phase out the use of 'pfn' from the x86 pagetable code.  Callers should
> - *   know explicitly whether they are talking about mfns or gfns.
> - * - Phase out the use of 'pfn' from the ARM mm code.  A cursory glance
> - *   suggests that 'mfn' and 'pfn' are currently used interchangeably, where
> - *   'mfn' is the appropriate term to use.
> - * - Phase out the use of gpfn/gmfn where pfn/mfn are meant.  This excludes
> - *   the x86 shadow code, which uses gmfn/smfn pairs with different,
> - *   documented, meanings.
>   */
>  
>  #ifndef __XEN_MM_H__
> @@ -54,100 +11,11 @@
>  #include <xen/types.h>
>  #include <xen/list.h>
>  #include <xen/spinlock.h>
> -#include <xen/typesafe.h>
>  #include <xen/kernel.h>
> +#include <xen/mm_types.h>

Is there anything left in the header here which requires the
explicit inclusion of xen/kernel.h?

Jan
Julien Grall March 25, 2020, 6:09 p.m. UTC | #2
Hi Jan,

On 25/03/2020 15:00, Jan Beulich wrote:
> On 22.03.2020 17:14, julien@xen.org wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> It is getting incredibly difficult to use typesafe GFN/MFN/PFN in the
>> headers because of circular dependency. For instance, asm-x86/page.h
>> cannot include xen/mm.h.
>>
>> In order to convert more code to use typesafe, the types are now moved
>> in a separate header that requires only a few dependencies.
> 
> We definitely need to do this, so thanks for investing the
> time. I think though that we want to settle up front (and
> perhaps record in a comment in the new header) what is or
> is not suitable to go into the new header. After all you're
> moving not just type definitions, but also simple helper
> functions.

I am expecting headers to use the typesafe helpers (such mfn_add) in the 
long term. So I would like the new header to contain the type 
definitions and any wrappers that would turn 'generic' operations safe.

I am not entirely sure yet how to formalize the rules in the header. Any 
ideas?

> 
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -1,50 +1,7 @@
>>   /******************************************************************************
>>    * include/xen/mm.h
>>    *
>> - * Definitions for memory pages, frame numbers, addresses, allocations, etc.
>> - *
>>    * Copyright (c) 2002-2006, K A Fraser <keir@xensource.com>
>> - *
>> - *                         +---------------------+
>> - *                          Xen Memory Management
>> - *                         +---------------------+
>> - *
>> - * Xen has to handle many different address spaces.  It is important not to
>> - * get these spaces mixed up.  The following is a consistent terminology which
>> - * should be adhered to.
>> - *
>> - * mfn: Machine Frame Number
>> - *   The values Xen puts into its own pagetables.  This is the host physical
>> - *   memory address space with RAM, MMIO etc.
>> - *
>> - * gfn: Guest Frame Number
>> - *   The values a guest puts in its own pagetables.  For an auto-translated
>> - *   guest (hardware assisted with 2nd stage translation, or shadowed), gfn !=
>> - *   mfn.  For a non-translated guest which is aware of Xen, gfn == mfn.
>> - *
>> - * pfn: Pseudophysical Frame Number
>> - *   A linear idea of a guest physical address space. For an auto-translated
>> - *   guest, pfn == gfn while for a non-translated guest, pfn != gfn.
>> - *
>> - * dfn: Device DMA Frame Number (definitions in include/xen/iommu.h)
>> - *   The linear frame numbers of device DMA address space. All initiators for
>> - *   (i.e. all devices assigned to) a guest share a single DMA address space
>> - *   and, by default, Xen will ensure dfn == pfn.
>> - *
>> - * WARNING: Some of these terms have changed over time while others have been
>> - * used inconsistently, meaning that a lot of existing code does not match the
>> - * definitions above.  New code should use these terms as described here, and
>> - * over time older code should be corrected to be consistent.
>> - *
>> - * An incomplete list of larger work area:
>> - * - Phase out the use of 'pfn' from the x86 pagetable code.  Callers should
>> - *   know explicitly whether they are talking about mfns or gfns.
>> - * - Phase out the use of 'pfn' from the ARM mm code.  A cursory glance
>> - *   suggests that 'mfn' and 'pfn' are currently used interchangeably, where
>> - *   'mfn' is the appropriate term to use.
>> - * - Phase out the use of gpfn/gmfn where pfn/mfn are meant.  This excludes
>> - *   the x86 shadow code, which uses gmfn/smfn pairs with different,
>> - *   documented, meanings.
>>    */
>>   
>>   #ifndef __XEN_MM_H__
>> @@ -54,100 +11,11 @@
>>   #include <xen/types.h>
>>   #include <xen/list.h>
>>   #include <xen/spinlock.h>
>> -#include <xen/typesafe.h>
>>   #include <xen/kernel.h>
>> +#include <xen/mm_types.h>
> 
> Is there anything left in the header here which requires the
> explicit inclusion of xen/kernel.h?

The header was introduced for the sole purpose of the typesafe version 
of the min/max helpers. So it should be possible to drop the include.

I will have a look and remove it if we can.

Cheers,
Jan Beulich March 26, 2020, 9:02 a.m. UTC | #3
On 25.03.2020 19:09, Julien Grall wrote:
> Hi Jan,
> 
> On 25/03/2020 15:00, Jan Beulich wrote:
>> On 22.03.2020 17:14, julien@xen.org wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> It is getting incredibly difficult to use typesafe GFN/MFN/PFN in the
>>> headers because of circular dependency. For instance, asm-x86/page.h
>>> cannot include xen/mm.h.
>>>
>>> In order to convert more code to use typesafe, the types are now moved
>>> in a separate header that requires only a few dependencies.
>>
>> We definitely need to do this, so thanks for investing the
>> time. I think though that we want to settle up front (and
>> perhaps record in a comment in the new header) what is or
>> is not suitable to go into the new header. After all you're
>> moving not just type definitions, but also simple helper
>> functions.
> 
> I am expecting headers to use the typesafe helpers (such mfn_add)
> in the long term. So I would like the new header to contain the
> type definitions and any wrappers that would turn 'generic'
> operations safe.
> 
> I am not entirely sure yet how to formalize the rules in the
> header. Any ideas?

Well, if the header was just for the typesafe types, it could be
renamed (to e.g. mm-typesafe.h) and be left without any respective
comment. The issue I've mentioned arises if, with its currently
suggested name, further types get added. In such a case perhaps it
could be "type definitions and their immediate accessors,
involving no other non-trivial types"?

Jan
Julien Grall March 28, 2020, 10:15 a.m. UTC | #4
Hi Jan,

On 26/03/2020 09:02, Jan Beulich wrote:
> On 25.03.2020 19:09, Julien Grall wrote:
>> Hi Jan,
>>
>> On 25/03/2020 15:00, Jan Beulich wrote:
>>> On 22.03.2020 17:14, julien@xen.org wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> It is getting incredibly difficult to use typesafe GFN/MFN/PFN in the
>>>> headers because of circular dependency. For instance, asm-x86/page.h
>>>> cannot include xen/mm.h.
>>>>
>>>> In order to convert more code to use typesafe, the types are now moved
>>>> in a separate header that requires only a few dependencies.
>>>
>>> We definitely need to do this, so thanks for investing the
>>> time. I think though that we want to settle up front (and
>>> perhaps record in a comment in the new header) what is or
>>> is not suitable to go into the new header. After all you're
>>> moving not just type definitions, but also simple helper
>>> functions.
>>
>> I am expecting headers to use the typesafe helpers (such mfn_add)
>> in the long term. So I would like the new header to contain the
>> type definitions and any wrappers that would turn 'generic'
>> operations safe.
>>
>> I am not entirely sure yet how to formalize the rules in the
>> header. Any ideas?
> 
> Well, if the header was just for the typesafe types, it could be
> renamed (to e.g. mm-typesafe.h) and be left without any respective
> comment. The issue I've mentioned arises if, with its currently
> suggested name, further types get added. In such a case perhaps it
> could be "type definitions and their immediate accessors,
> involving no other non-trivial types"?

I will rename the file to mm-typesafe.h.

Cheers,

Patch
diff mbox series

diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index d0d095d9c7..4337303f99 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -1,50 +1,7 @@ 
 /******************************************************************************
  * include/xen/mm.h
  *
- * Definitions for memory pages, frame numbers, addresses, allocations, etc.
- *
  * Copyright (c) 2002-2006, K A Fraser <keir@xensource.com>
- *
- *                         +---------------------+
- *                          Xen Memory Management
- *                         +---------------------+
- *
- * Xen has to handle many different address spaces.  It is important not to
- * get these spaces mixed up.  The following is a consistent terminology which
- * should be adhered to.
- *
- * mfn: Machine Frame Number
- *   The values Xen puts into its own pagetables.  This is the host physical
- *   memory address space with RAM, MMIO etc.
- *
- * gfn: Guest Frame Number
- *   The values a guest puts in its own pagetables.  For an auto-translated
- *   guest (hardware assisted with 2nd stage translation, or shadowed), gfn !=
- *   mfn.  For a non-translated guest which is aware of Xen, gfn == mfn.
- *
- * pfn: Pseudophysical Frame Number
- *   A linear idea of a guest physical address space. For an auto-translated
- *   guest, pfn == gfn while for a non-translated guest, pfn != gfn.
- *
- * dfn: Device DMA Frame Number (definitions in include/xen/iommu.h)
- *   The linear frame numbers of device DMA address space. All initiators for
- *   (i.e. all devices assigned to) a guest share a single DMA address space
- *   and, by default, Xen will ensure dfn == pfn.
- *
- * WARNING: Some of these terms have changed over time while others have been
- * used inconsistently, meaning that a lot of existing code does not match the
- * definitions above.  New code should use these terms as described here, and
- * over time older code should be corrected to be consistent.
- *
- * An incomplete list of larger work area:
- * - Phase out the use of 'pfn' from the x86 pagetable code.  Callers should
- *   know explicitly whether they are talking about mfns or gfns.
- * - Phase out the use of 'pfn' from the ARM mm code.  A cursory glance
- *   suggests that 'mfn' and 'pfn' are currently used interchangeably, where
- *   'mfn' is the appropriate term to use.
- * - Phase out the use of gpfn/gmfn where pfn/mfn are meant.  This excludes
- *   the x86 shadow code, which uses gmfn/smfn pairs with different,
- *   documented, meanings.
  */
 
 #ifndef __XEN_MM_H__
@@ -54,100 +11,11 @@ 
 #include <xen/types.h>
 #include <xen/list.h>
 #include <xen/spinlock.h>
-#include <xen/typesafe.h>
 #include <xen/kernel.h>
+#include <xen/mm_types.h>
 #include <xen/perfc.h>
 #include <public/memory.h>
 
-TYPE_SAFE(unsigned long, mfn);
-#define PRI_mfn          "05lx"
-#define INVALID_MFN      _mfn(~0UL)
-/*
- * To be used for global variable initialization. This workaround a bug
- * in GCC < 5.0.
- */
-#define INVALID_MFN_INITIALIZER { ~0UL }
-
-#ifndef mfn_t
-#define mfn_t /* Grep fodder: mfn_t, _mfn() and mfn_x() are defined above */
-#define _mfn
-#define mfn_x
-#undef mfn_t
-#undef _mfn
-#undef mfn_x
-#endif
-
-static inline mfn_t mfn_add(mfn_t mfn, unsigned long i)
-{
-    return _mfn(mfn_x(mfn) + i);
-}
-
-static inline mfn_t mfn_max(mfn_t x, mfn_t y)
-{
-    return _mfn(max(mfn_x(x), mfn_x(y)));
-}
-
-static inline mfn_t mfn_min(mfn_t x, mfn_t y)
-{
-    return _mfn(min(mfn_x(x), mfn_x(y)));
-}
-
-static inline bool_t mfn_eq(mfn_t x, mfn_t y)
-{
-    return mfn_x(x) == mfn_x(y);
-}
-
-TYPE_SAFE(unsigned long, gfn);
-#define PRI_gfn          "05lx"
-#define INVALID_GFN      _gfn(~0UL)
-/*
- * To be used for global variable initialization. This workaround a bug
- * in GCC < 5.0 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64856
- */
-#define INVALID_GFN_INITIALIZER { ~0UL }
-
-#ifndef gfn_t
-#define gfn_t /* Grep fodder: gfn_t, _gfn() and gfn_x() are defined above */
-#define _gfn
-#define gfn_x
-#undef gfn_t
-#undef _gfn
-#undef gfn_x
-#endif
-
-static inline gfn_t gfn_add(gfn_t gfn, unsigned long i)
-{
-    return _gfn(gfn_x(gfn) + i);
-}
-
-static inline gfn_t gfn_max(gfn_t x, gfn_t y)
-{
-    return _gfn(max(gfn_x(x), gfn_x(y)));
-}
-
-static inline gfn_t gfn_min(gfn_t x, gfn_t y)
-{
-    return _gfn(min(gfn_x(x), gfn_x(y)));
-}
-
-static inline bool_t gfn_eq(gfn_t x, gfn_t y)
-{
-    return gfn_x(x) == gfn_x(y);
-}
-
-TYPE_SAFE(unsigned long, pfn);
-#define PRI_pfn          "05lx"
-#define INVALID_PFN      (~0UL)
-
-#ifndef pfn_t
-#define pfn_t /* Grep fodder: pfn_t, _pfn() and pfn_x() are defined above */
-#define _pfn
-#define pfn_x
-#undef pfn_t
-#undef _pfn
-#undef pfn_x
-#endif
-
 struct page_info;
 
 void put_page(struct page_info *);
diff --git a/xen/include/xen/mm_types.h b/xen/include/xen/mm_types.h
new file mode 100644
index 0000000000..f14359f571
--- /dev/null
+++ b/xen/include/xen/mm_types.h
@@ -0,0 +1,155 @@ 
+/******************************************************************************
+ * include/xen/mm_types.h
+ *
+ * Definitions for memory pages, frame numbers, addresses, allocations, etc.
+ *
+ * Copyright (c) 2002-2006, K A Fraser <keir@xensource.com>
+ *
+ *                         +---------------------+
+ *                          Xen Memory Management
+ *                         +---------------------+
+ *
+ * Xen has to handle many different address spaces.  It is important not to
+ * get these spaces mixed up.  The following is a consistent terminology which
+ * should be adhered to.
+ *
+ * mfn: Machine Frame Number
+ *   The values Xen puts into its own pagetables.  This is the host physical
+ *   memory address space with RAM, MMIO etc.
+ *
+ * gfn: Guest Frame Number
+ *   The values a guest puts in its own pagetables.  For an auto-translated
+ *   guest (hardware assisted with 2nd stage translation, or shadowed), gfn !=
+ *   mfn.  For a non-translated guest which is aware of Xen, gfn == mfn.
+ *
+ * pfn: Pseudophysical Frame Number
+ *   A linear idea of a guest physical address space. For an auto-translated
+ *   guest, pfn == gfn while for a non-translated guest, pfn != gfn.
+ *
+ * dfn: Device DMA Frame Number (definitions in include/xen/iommu.h)
+ *   The linear frame numbers of device DMA address space. All initiators for
+ *   (i.e. all devices assigned to) a guest share a single DMA address space
+ *   and, by default, Xen will ensure dfn == pfn.
+ *
+ * WARNING: Some of these terms have changed over time while others have been
+ * used inconsistently, meaning that a lot of existing code does not match the
+ * definitions above.  New code should use these terms as described here, and
+ * over time older code should be corrected to be consistent.
+ *
+ * An incomplete list of larger work area:
+ * - Phase out the use of 'pfn' from the x86 pagetable code.  Callers should
+ *   know explicitly whether they are talking about mfns or gfns.
+ * - Phase out the use of 'pfn' from the ARM mm code.  A cursory glance
+ *   suggests that 'mfn' and 'pfn' are currently used interchangeably, where
+ *   'mfn' is the appropriate term to use.
+ * - Phase out the use of gpfn/gmfn where pfn/mfn are meant.  This excludes
+ *   the x86 shadow code, which uses gmfn/smfn pairs with different,
+ *   documented, meanings.
+ */
+
+#ifndef __XEN_MM_TYPES_H__
+#define __XEN_MM_TYPES_H__
+
+#include <xen/typesafe.h>
+#include <xen/kernel.h>
+
+TYPE_SAFE(unsigned long, mfn);
+#define PRI_mfn          "05lx"
+#define INVALID_MFN      _mfn(~0UL)
+/*
+ * To be used for global variable initialization. This workaround a bug
+ * in GCC < 5.0.
+ */
+#define INVALID_MFN_INITIALIZER { ~0UL }
+
+#ifndef mfn_t
+#define mfn_t /* Grep fodder: mfn_t, _mfn() and mfn_x() are defined above */
+#define _mfn
+#define mfn_x
+#undef mfn_t
+#undef _mfn
+#undef mfn_x
+#endif
+
+static inline mfn_t mfn_add(mfn_t mfn, unsigned long i)
+{
+    return _mfn(mfn_x(mfn) + i);
+}
+
+static inline mfn_t mfn_max(mfn_t x, mfn_t y)
+{
+    return _mfn(max(mfn_x(x), mfn_x(y)));
+}
+
+static inline mfn_t mfn_min(mfn_t x, mfn_t y)
+{
+    return _mfn(min(mfn_x(x), mfn_x(y)));
+}
+
+static inline bool_t mfn_eq(mfn_t x, mfn_t y)
+{
+    return mfn_x(x) == mfn_x(y);
+}
+
+TYPE_SAFE(unsigned long, gfn);
+#define PRI_gfn          "05lx"
+#define INVALID_GFN      _gfn(~0UL)
+/*
+ * To be used for global variable initialization. This workaround a bug
+ * in GCC < 5.0 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64856
+ */
+#define INVALID_GFN_INITIALIZER { ~0UL }
+
+#ifndef gfn_t
+#define gfn_t /* Grep fodder: gfn_t, _gfn() and gfn_x() are defined above */
+#define _gfn
+#define gfn_x
+#undef gfn_t
+#undef _gfn
+#undef gfn_x
+#endif
+
+static inline gfn_t gfn_add(gfn_t gfn, unsigned long i)
+{
+    return _gfn(gfn_x(gfn) + i);
+}
+
+static inline gfn_t gfn_max(gfn_t x, gfn_t y)
+{
+    return _gfn(max(gfn_x(x), gfn_x(y)));
+}
+
+static inline gfn_t gfn_min(gfn_t x, gfn_t y)
+{
+    return _gfn(min(gfn_x(x), gfn_x(y)));
+}
+
+static inline bool_t gfn_eq(gfn_t x, gfn_t y)
+{
+    return gfn_x(x) == gfn_x(y);
+}
+
+TYPE_SAFE(unsigned long, pfn);
+#define PRI_pfn          "05lx"
+#define INVALID_PFN      (~0UL)
+
+#ifndef pfn_t
+#define pfn_t /* Grep fodder: pfn_t, _pfn() and pfn_x() are defined above */
+#define _pfn
+#define pfn_x
+#undef pfn_t
+#undef _pfn
+#undef pfn_x
+#endif
+
+#endif /* __XEN_MM_TYPES_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */