[PATCH 0/2] xen/mem Common cleanup

[PATCH 0/2] xen/mem Common cleanup

[Andrew Cooper]

Following c/s e758ed14 which describes the current terms used in Xens memory
management, introduce some infrastructure so that new code can be written
correctly.

This is the start of a long potential cleanup.

Andrew Cooper (2):
  xen/mem: Expose typesafe mfns/gfns/pfns to common code
  xen/vmap: Convert vmap() to using mfn_t

 xen/arch/arm/mm.c              |    8 ++++---
 xen/arch/x86/domain_page.c     |    3 ++-
 xen/arch/x86/mm.c              |    6 +++---
 xen/arch/x86/mm/shadow/multi.c |    6 +++---
 xen/common/vmap.c              |   14 ++++++------
 xen/drivers/acpi/osl.c         |    5 +++--
 xen/include/asm-arm/mm.h       |    2 --
 xen/include/asm-x86/guest_pt.h |    9 --------
 xen/include/asm-x86/mm.h       |   37 --------------------------------
 xen/include/asm-x86/paging.h   |    1 -
 xen/include/xen/mm.h           |   28 ++++++++++++++++++++++++
 xen/include/xen/typesafe.h     |   46 ++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/vmap.h         |    6 +++---
 13 files changed, 100 insertions(+), 71 deletions(-)
 create mode 100644 xen/include/xen/typesafe.h

-- 
1.7.10.4

[PATCH 1/2] xen/mem: Expose typesafe mfns/gfns/pfns to common code

[Andrew Cooper]

As the first step of memory management cleanup, introduce the common code to
mfn_t, gfn_t and pfn_t.

The typesafe construction moves to its own header file, while the declarations
and sentinal values are moved to being common.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>

---
Compile tested on all architectures.  arm32 is currently using unsigned long
for mfns, which is why I made it all common.  If this needs to change in the
future, the #include <asm/mm.h> can move up and each arch can provide their
own TYPE_SAFE()s.
---
 xen/include/asm-arm/mm.h       |    2 --
 xen/include/asm-x86/guest_pt.h |    9 --------
 xen/include/asm-x86/mm.h       |   37 --------------------------------
 xen/include/asm-x86/paging.h   |    1 -
 xen/include/xen/mm.h           |   28 ++++++++++++++++++++++++
 xen/include/xen/typesafe.h     |   46 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 74 insertions(+), 49 deletions(-)
 create mode 100644 xen/include/xen/typesafe.h

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index c0afcec..3601140 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -310,8 +310,6 @@ static inline int relinquish_shared_pages(struct domain *d)
     return 0;
 }
 
-#define INVALID_MFN             (~0UL)
-
 /* Xen always owns P2M on ARM */
 #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0)
 #define mfn_to_gmfn(_d, mfn)  (mfn)
diff --git a/xen/include/asm-x86/guest_pt.h b/xen/include/asm-x86/guest_pt.h
index d2a8250..cf21617 100644
--- a/xen/include/asm-x86/guest_pt.h
+++ b/xen/include/asm-x86/guest_pt.h
@@ -33,15 +33,6 @@
 #error GUEST_PAGING_LEVELS not defined
 #endif
 
-/* Type of the guest's frame numbers */
-TYPE_SAFE(unsigned long,gfn)
-#define PRI_gfn "05lx"
-
-#ifndef gfn_t
-#define gfn_t /* Grep fodder: gfn_t, _gfn() and gfn_x() are defined above */
-#undef gfn_t
-#endif
-
 #define VALID_GFN(m) (m != INVALID_GFN)
 
 static inline int
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index d1f95c8..8595c38 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -433,41 +433,6 @@ static inline int get_page_and_type(struct page_info *page,
  * guest L2 page), etc...
  */
 
-/* With this defined, we do some ugly things to force the compiler to
- * give us type safety between mfns and gfns and other integers.
- * TYPE_SAFE(int foo) defines a foo_t, and _foo() and foo_x() functions 
- * that translate beween int and foo_t.
- * 
- * It does have some performance cost because the types now have 
- * a different storage attribute, so may not want it on all the time. */
-
-#ifndef NDEBUG
-#define TYPE_SAFETY 1
-#endif
-
-#ifdef TYPE_SAFETY
-#define TYPE_SAFE(_type,_name)                                  \
-typedef struct { _type _name; } _name##_t;                      \
-static inline _name##_t _##_name(_type n) { return (_name##_t) { n }; } \
-static inline _type _name##_x(_name##_t n) { return n._name; }
-#else
-#define TYPE_SAFE(_type,_name)                                          \
-typedef _type _name##_t;                                                \
-static inline _name##_t _##_name(_type n) { return n; }                 \
-static inline _type _name##_x(_name##_t n) { return n; }
-#endif
-
-TYPE_SAFE(unsigned long,mfn);
-
-#ifndef mfn_t
-#define mfn_t /* Grep fodder: mfn_t, _mfn() and mfn_x() are defined above */
-#undef mfn_t
-#endif
-
-/* Macro for printk formats: use as printk("%"PRI_mfn"\n", mfn_x(foo)); */
-#define PRI_mfn "05lx"
-
-
 /*
  * The MPT (machine->physical mapping table) is an array of word-sized
  * values, indexed on machine frame number. It is expected that guest OSes
@@ -510,8 +475,6 @@ static inline int get_page_and_type(struct page_info *page,
       ? get_gpfn_from_mfn(mfn)                          \
       : (mfn) )
 
-#define INVALID_MFN             (~0UL)
-
 #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) | ((unsigned)(pfn) >> 20))
 #define compat_cr3_to_pfn(cr3) (((unsigned)(cr3) >> 12) | ((unsigned)(cr3) << 20))
 
diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
index c99324c..9c32665 100644
--- a/xen/include/asm-x86/paging.h
+++ b/xen/include/asm-x86/paging.h
@@ -255,7 +255,6 @@ static inline int paging_invlpg(struct vcpu *v, unsigned long va)
  * pfec[0] is used to determine which kind of access this is when
  * walking the tables.  The caller should set the PFEC_page_present bit
  * in pfec[0]; in the failure case, that bit will be cleared if appropriate. */
-#define INVALID_GFN (-1UL)
 unsigned long paging_gva_to_gfn(struct vcpu *v,
                                 unsigned long va,
                                 uint32_t *pfec);
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index a7563cd..876d370 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -48,6 +48,34 @@
 #include <xen/types.h>
 #include <xen/list.h>
 #include <xen/spinlock.h>
+#include <xen/typesafe.h>
+
+TYPE_SAFE(unsigned long, mfn);
+#define PRI_mfn          "05lx"
+#define INVALID_MFN      (~0UL)
+
+#ifndef mfn_t
+#define mfn_t /* Grep fodder: mfn_t, _mfn() and mfn_x() are defined above */
+#undef mfn_t
+#endif
+
+TYPE_SAFE(unsigned long, gfn);
+#define PRI_gfn          "05lx"
+#define INVALID_GFN      (~0UL)
+
+#ifndef gfn_t
+#define gfn_t /* Grep fodder: gfn_t, _gfn() and gfn_x() are defined above */
+#undef gfn_t
+#endif
+
+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 */
+#undef pfn_t
+#endif
 
 struct domain;
 struct page_info;
diff --git a/xen/include/xen/typesafe.h b/xen/include/xen/typesafe.h
new file mode 100644
index 0000000..7ecd3b4
--- /dev/null
+++ b/xen/include/xen/typesafe.h
@@ -0,0 +1,46 @@
+#ifndef __XEN_TYPESAFE_H__
+#define __XEN_TYPESAFE_H__
+
+/*
+ * Compiler games to gain type safety between different logical integers.
+ *
+ * TYPE_SAFE($TYPE, $FOO) declares:
+ *  * $FOO_t   which encapsulates $TYPE
+ *  * _$FOO()  which boxes a $TYPE as a $FOO_t
+ *  * $FOO_x() which unboxes a $FOO_t to $TYPE
+ *
+ * This makes a $FOO_t and a $BAR_t incompatible even when the box the same
+ * $TYPE.
+ *
+ * It does have some performance cost because the types now have a different
+ * storage attribute, so type safety is only enforced in a debug build.
+ * Non-debug builds degrade to a simple typedef and noops for the functions.
+ */
+
+#ifndef NDEBUG
+
+#define TYPE_SAFE(_type, _name)                                         \
+    typedef struct { _type _name; } _name##_t;                          \
+    static inline _name##_t _##_name(_type n) { return (_name##_t) { n }; } \
+    static inline _type _name##_x(_name##_t n) { return n._name; }
+
+#else
+
+#define TYPE_SAFE(_type, _name)                                         \
+    typedef _type _name##_t;                                            \
+    static inline _name##_t _##_name(_type n) { return n; }             \
+    static inline _type _name##_x(_name##_t n) { return n; }
+
+#endif
+
+#endif /* __XEN_TYPESAFE_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.7.10.4

[PATCH 2/2] xen/vmap: Convert vmap() to using mfn_t

[Andrew Cooper]

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>

---
Compile tested on all architecutres, functionally tested on x86.
---
 xen/arch/arm/mm.c              |    8 +++++---
 xen/arch/x86/domain_page.c     |    3 ++-
 xen/arch/x86/mm.c              |    6 +++---
 xen/arch/x86/mm/shadow/multi.c |    6 +++---
 xen/common/vmap.c              |   14 +++++++-------
 xen/drivers/acpi/osl.c         |    5 +++--
 xen/include/xen/vmap.h         |    6 +++---
 7 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index a91ea77..feabe34 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -273,7 +273,9 @@ void clear_fixmap(unsigned map)
 #ifdef CONFIG_DOMAIN_PAGE
 void *map_domain_page_global(unsigned long mfn)
 {
-    return vmap(&mfn, 1);
+    mfn_t m = _mfn(mfn);
+
+    return vmap(&m, 1);
 }
 
 void unmap_domain_page_global(const void *va)
@@ -794,10 +796,10 @@ void *__init arch_vmap_virt_end(void)
  */
 void *ioremap_attr(paddr_t pa, size_t len, unsigned int attributes)
 {
-    unsigned long pfn = PFN_DOWN(pa);
+    mfn_t mfn = _mfn(PFN_DOWN(pa));
     unsigned int offs = pa & (PAGE_SIZE - 1);
     unsigned int nr = PFN_UP(offs + len);
-    void *ptr = __vmap(&pfn, nr, 1, 1, attributes);
+    void *ptr = __vmap(&mfn, nr, 1, 1, attributes);
 
     if ( ptr == NULL )
         return NULL;
diff --git a/xen/arch/x86/domain_page.c b/xen/arch/x86/domain_page.c
index 7954998..8f3217b 100644
--- a/xen/arch/x86/domain_page.c
+++ b/xen/arch/x86/domain_page.c
@@ -322,6 +322,7 @@ int mapcache_vcpu_init(struct vcpu *v)
 
 void *map_domain_page_global(unsigned long mfn)
 {
+    mfn_t m = _mfn(mfn);
     ASSERT(!in_irq() && local_irq_is_enabled());
 
 #ifdef NDEBUG
@@ -329,7 +330,7 @@ void *map_domain_page_global(unsigned long mfn)
         return mfn_to_virt(mfn);
 #endif
 
-    return vmap(&mfn, 1);
+    return vmap(&m, 1);
 }
 
 void unmap_domain_page_global(const void *ptr)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 7a7a854..1c0783f 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5883,10 +5883,10 @@ void *__init arch_vmap_virt_end(void)
 
 void __iomem *ioremap(paddr_t pa, size_t len)
 {
-    unsigned long pfn = PFN_DOWN(pa);
+    mfn_t mfn = _mfn(PFN_DOWN(pa));
     void *va;
 
-    WARN_ON(page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL));
+    WARN_ON(page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL));
 
     /* The low first Mb is always mapped. */
     if ( !((pa + len - 1) >> 20) )
@@ -5896,7 +5896,7 @@ void __iomem *ioremap(paddr_t pa, size_t len)
         unsigned int offs = pa & (PAGE_SIZE - 1);
         unsigned int nr = PFN_UP(offs + len);
 
-        va = __vmap(&pfn, nr, 1, 1, PAGE_HYPERVISOR_NOCACHE) + offs;
+        va = __vmap(&mfn, nr, 1, 1, PAGE_HYPERVISOR_NOCACHE) + offs;
     }
 
     return (void __force __iomem *)va;
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 43e70d8..6edac71 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -4670,7 +4670,7 @@ static void *emulate_map_dest(struct vcpu *v,
     }
     else
     {
-        unsigned long mfns[2];
+        mfn_t mfns[2];
 
         /* Cross-page emulated writes are only supported for HVM guests;
          * PV guests ought to know better */
@@ -4689,8 +4689,8 @@ static void *emulate_map_dest(struct vcpu *v,
         /* Cross-page writes mean probably not a pagetable */
         sh_remove_shadows(d, sh_ctxt->mfn2, 0, 0 /* Slow, can fail */ );
 
-        mfns[0] = mfn_x(sh_ctxt->mfn1);
-        mfns[1] = mfn_x(sh_ctxt->mfn2);
+        mfns[0] = sh_ctxt->mfn1;
+        mfns[1] = sh_ctxt->mfn2;
         map = vmap(mfns, 2);
         if ( !map )
             return MAPPING_UNHANDLEABLE;
diff --git a/xen/common/vmap.c b/xen/common/vmap.c
index ac66a8c..c57239f 100644
--- a/xen/common/vmap.c
+++ b/xen/common/vmap.c
@@ -181,7 +181,7 @@ void vm_free(const void *va)
     spin_unlock(&vm_lock);
 }
 
-void *__vmap(const unsigned long *mfn, unsigned int granularity,
+void *__vmap(const mfn_t *mfn, unsigned int granularity,
              unsigned int nr, unsigned int align, unsigned int flags)
 {
     void *va = vm_alloc(nr * granularity, align);
@@ -189,7 +189,7 @@ void *__vmap(const unsigned long *mfn, unsigned int granularity,
 
     for ( ; va && nr--; ++mfn, cur += PAGE_SIZE * granularity )
     {
-        if ( map_pages_to_xen(cur, *mfn, granularity, flags) )
+        if ( map_pages_to_xen(cur, mfn_x(*mfn), granularity, flags) )
         {
             vunmap(va);
             va = NULL;
@@ -199,7 +199,7 @@ void *__vmap(const unsigned long *mfn, unsigned int granularity,
     return va;
 }
 
-void *vmap(const unsigned long *mfn, unsigned int nr)
+void *vmap(const mfn_t *mfn, unsigned int nr)
 {
     return __vmap(mfn, 1, nr, 1, PAGE_HYPERVISOR);
 }
@@ -218,7 +218,7 @@ void vunmap(const void *va)
 
 void *vmalloc(size_t size)
 {
-    unsigned long *mfn;
+    mfn_t *mfn;
     size_t pages, i;
     struct page_info *pg;
     void *va;
@@ -226,7 +226,7 @@ void *vmalloc(size_t size)
     ASSERT(size);
 
     pages = PFN_UP(size);
-    mfn = xmalloc_array(unsigned long, pages);
+    mfn = xmalloc_array(mfn_t, pages);
     if ( mfn == NULL )
         return NULL;
 
@@ -235,7 +235,7 @@ void *vmalloc(size_t size)
         pg = alloc_domheap_page(NULL, 0);
         if ( pg == NULL )
             goto error;
-        mfn[i] = page_to_mfn(pg);
+        mfn[i] = _mfn(page_to_mfn(pg));
     }
 
     va = vmap(mfn, pages);
@@ -247,7 +247,7 @@ void *vmalloc(size_t size)
 
  error:
     while ( i-- )
-         free_domheap_page(mfn_to_page(mfn[i]));
+        free_domheap_page(mfn_to_page(mfn_x(mfn[i])));
     xfree(mfn);
     return NULL;
 }
diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
index 93c983c..4c09859 100644
--- a/xen/drivers/acpi/osl.c
+++ b/xen/drivers/acpi/osl.c
@@ -88,13 +88,14 @@ acpi_physical_address __init acpi_os_get_root_pointer(void)
 acpi_os_map_memory(acpi_physical_address phys, acpi_size size)
 {
 	if (system_state >= SYS_STATE_active) {
-		unsigned long pfn = PFN_DOWN(phys);
+		mfn_t mfn = _mfn(PFN_DOWN(phys));
 		unsigned int offs = phys & (PAGE_SIZE - 1);
 
 		/* The low first Mb is always mapped. */
 		if ( !((phys + size - 1) >> 20) )
 			return __va(phys);
-		return __vmap(&pfn, PFN_UP(offs + size), 1, 1, PAGE_HYPERVISOR_NOCACHE) + offs;
+		return __vmap(&mfn, PFN_UP(offs + size), 1, 1,
+			      PAGE_HYPERVISOR_NOCACHE) + offs;
 	}
 	return __acpi_map_table(phys, size);
 }
diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index a13591d..5671ac8 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -1,15 +1,15 @@
 #if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START)
 #define __XEN_VMAP_H__
 
-#include <xen/types.h>
+#include <xen/mm.h>
 #include <asm/page.h>
 
 void *vm_alloc(unsigned int nr, unsigned int align);
 void vm_free(const void *);
 
-void *__vmap(const unsigned long *mfn, unsigned int granularity,
+void *__vmap(const mfn_t *mfn, unsigned int granularity,
              unsigned int nr, unsigned int align, unsigned int flags);
-void *vmap(const unsigned long *mfn, unsigned int nr);
+void *vmap(const mfn_t *mfn, unsigned int nr);
 void vunmap(const void *);
 void *vmalloc(size_t size);
 void *vzalloc(size_t size);
-- 
1.7.10.4

Re: [PATCH 0/2] xen/mem Common cleanup

[Tim Deegan]

At 12:43 +0100 on 03 Jun (1433335401), Andrew Cooper wrote:
> Following c/s e758ed14 which describes the current terms used in Xens memory
> management, introduce some infrastructure so that new code can be written
> correctly.
> 
> This is the start of a long potential cleanup.

Acked-by: Tim Deegan <tim@xen.org>