[PATCH v3 0/8] xen/arm: Use the typesafes gfn and mfn

[PATCH v3 0/8] xen/arm: Use the typesafes gfn and mfn

[Julien Grall]

Hello all,

Some of the ARM functions are mixing gfn vs mfn and even physical vs frame.

To avoid more confusion, this patch series makes use of the terminology
described in xen/include/xen/mm.h and the associated typesafe.

I pushed a branch with this series applied on xenbits:
git://xenbits.xen.org/people/julieng/xen-unstable.git branch typesafe-v3

Yours sincerely,

Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Paul Durrant <paul.durrant@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

Julien Grall (8):
  xen/arm: Rename gmfn_to_mfn to gfn_to_mfn and use gfn/mfn typesafe
  xen/mm: Introduce a bunch of helpers for the typesafes mfn and gfn
  xen: Use typesafe gfn/mfn in guest_physmap_* helpers
  xen: Use typesafe gfn in xenmem_add_to_physmap_one
  xen/arm: Rename grant_table_gfpn into grant_table_gfn and use the
    typesafe gfn
  xen: Use the typesafe mfn and gfn in map_mmio_regions...
  xen/arm: Rework the interface of p2m_lookup and use typesafe gfn and
    mfn
  xen/arm: p2m_cache_flush: Use the correct terminology and typesafe gfn

 xen/arch/arm/domain.c              |  4 +-
 xen/arch/arm/domain_build.c        |  6 +--
 xen/arch/arm/domctl.c              |  2 +-
 xen/arch/arm/gic-v2.c              |  4 +-
 xen/arch/arm/mm.c                  | 18 +++----
 xen/arch/arm/p2m.c                 | 91 +++++++++++++++++++-----------------
 xen/arch/arm/platforms/exynos5.c   |  8 ++--
 xen/arch/arm/platforms/omap5.c     | 16 +++----
 xen/arch/arm/traps.c               | 21 +++++----
 xen/arch/arm/vgic-v2.c             |  4 +-
 xen/arch/x86/domain.c              |  5 +-
 xen/arch/x86/domain_build.c        |  6 +--
 xen/arch/x86/hvm/ioreq.c           |  8 ++--
 xen/arch/x86/mm.c                  | 21 +++++----
 xen/arch/x86/mm/p2m.c              | 96 +++++++++++++++++++++-----------------
 xen/common/domctl.c                |  4 +-
 xen/common/grant_table.c           | 11 +++--
 xen/common/memory.c                | 40 ++++++++--------
 xen/drivers/passthrough/arm/smmu.c |  4 +-
 xen/include/asm-arm/domain.h       |  2 +-
 xen/include/asm-arm/grant_table.h  |  2 +-
 xen/include/asm-arm/p2m.h          | 23 +++++----
 xen/include/asm-x86/p2m.h          | 11 ++---
 xen/include/xen/mm.h               | 45 +++++++++++++++++-
 xen/include/xen/p2m-common.h       |  8 ++--
 25 files changed, 258 insertions(+), 202 deletions(-)

-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 1/8] xen/arm: Rename gmfn_to_mfn to gfn_to_mfn and use gfn/mfn typesafe

[Julien Grall]

The correct acronym for a guest physical frame is gfn. Also use
the recently introduced typesafe gfn/mfn to avoid mixing the two
different kind of frame.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

    Changes in v2:
        - Add Jan's acked-by
        - CCed the relevant maintainers
---
 xen/arch/arm/p2m.c        | 6 +++---
 xen/common/grant_table.c  | 4 ++--
 xen/common/memory.c       | 4 ++--
 xen/include/asm-arm/p2m.h | 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 65d8f1a..ab0cb41 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1481,10 +1481,10 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
                              d->arch.p2m.default_access);
 }
 
-unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
+mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
 {
-    paddr_t p = p2m_lookup(d, pfn_to_paddr(gpfn), NULL);
-    return p >> PAGE_SHIFT;
+    paddr_t p = p2m_lookup(d, pfn_to_paddr(gfn_x(gfn)), NULL);
+    return _mfn(p >> PAGE_SHIFT);
 }
 
 /*
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 8b22299..3c304f4 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -256,7 +256,7 @@ static int __get_paged_frame(unsigned long gfn, unsigned long *frame, struct pag
     }
     *frame = page_to_mfn(*page);
 #else
-    *frame = gmfn_to_mfn(rd, gfn);
+    *frame = mfn_x(gfn_to_mfn(rd, _gfn(gfn)));
     *page = mfn_valid(*frame) ? mfn_to_page(*frame) : NULL;
     if ( (!(*page)) || (!get_page(*page, rd)) )
     {
@@ -1788,7 +1788,7 @@ gnttab_transfer(
                 mfn = INVALID_MFN;
         }
 #else
-        mfn = gmfn_to_mfn(d, gop.mfn);
+        mfn = mfn_x(gfn_to_mfn(d, _gfn(gop.mfn)));
 #endif
 
         /* Check the passed page frame for basic validity. */
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 46b1d9f..b54b076 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -264,7 +264,7 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
         return 1;
     }
 #else
-    mfn = gmfn_to_mfn(d, gmfn);
+    mfn = mfn_x(gfn_to_mfn(d, _gfn(gmfn)));
 #endif
     if ( unlikely(!mfn_valid(mfn)) )
     {
@@ -488,7 +488,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
                     goto fail; 
                 }
 #else /* !CONFIG_X86 */
-                mfn = gmfn_to_mfn(d, gmfn + k);
+                mfn = mfn_x(gfn_to_mfn(d, _gfn(gmfn + k)));
 #endif
                 if ( unlikely(!mfn_valid(mfn)) )
                 {
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index d240d1e..75c65a8 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -178,7 +178,7 @@ void guest_physmap_remove_page(struct domain *d,
                                unsigned long gpfn,
                                unsigned long mfn, unsigned int page_order);
 
-unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn);
+mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn);
 
 /*
  * Populate-on-demand
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 1/8] xen/arm: Rename gmfn_to_mfn to gfn_to_mfn and use gfn/mfn typesafe

[Stefano Stabellini]

On Tue, 21 Jun 2016, Julien Grall wrote:
> The correct acronym for a guest physical frame is gfn. Also use
> the recently introduced typesafe gfn/mfn to avoid mixing the two
> different kind of frame.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> 
>     Changes in v2:
>         - Add Jan's acked-by
>         - CCed the relevant maintainers
> ---
>  xen/arch/arm/p2m.c        | 6 +++---
>  xen/common/grant_table.c  | 4 ++--
>  xen/common/memory.c       | 4 ++--
>  xen/include/asm-arm/p2m.h | 2 +-
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 65d8f1a..ab0cb41 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1481,10 +1481,10 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
>                               d->arch.p2m.default_access);
>  }
>  
> -unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
> +mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
>  {
> -    paddr_t p = p2m_lookup(d, pfn_to_paddr(gpfn), NULL);
> -    return p >> PAGE_SHIFT;
> +    paddr_t p = p2m_lookup(d, pfn_to_paddr(gfn_x(gfn)), NULL);
> +    return _mfn(p >> PAGE_SHIFT);
>  }
>  
>  /*
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 8b22299..3c304f4 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -256,7 +256,7 @@ static int __get_paged_frame(unsigned long gfn, unsigned long *frame, struct pag
>      }
>      *frame = page_to_mfn(*page);
>  #else
> -    *frame = gmfn_to_mfn(rd, gfn);
> +    *frame = mfn_x(gfn_to_mfn(rd, _gfn(gfn)));
>      *page = mfn_valid(*frame) ? mfn_to_page(*frame) : NULL;
>      if ( (!(*page)) || (!get_page(*page, rd)) )
>      {
> @@ -1788,7 +1788,7 @@ gnttab_transfer(
>                  mfn = INVALID_MFN;
>          }
>  #else
> -        mfn = gmfn_to_mfn(d, gop.mfn);
> +        mfn = mfn_x(gfn_to_mfn(d, _gfn(gop.mfn)));
>  #endif
>  
>          /* Check the passed page frame for basic validity. */
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 46b1d9f..b54b076 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -264,7 +264,7 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>          return 1;
>      }
>  #else
> -    mfn = gmfn_to_mfn(d, gmfn);
> +    mfn = mfn_x(gfn_to_mfn(d, _gfn(gmfn)));
>  #endif
>      if ( unlikely(!mfn_valid(mfn)) )
>      {
> @@ -488,7 +488,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>                      goto fail; 
>                  }
>  #else /* !CONFIG_X86 */
> -                mfn = gmfn_to_mfn(d, gmfn + k);
> +                mfn = mfn_x(gfn_to_mfn(d, _gfn(gmfn + k)));
>  #endif
>                  if ( unlikely(!mfn_valid(mfn)) )
>                  {
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index d240d1e..75c65a8 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -178,7 +178,7 @@ void guest_physmap_remove_page(struct domain *d,
>                                 unsigned long gpfn,
>                                 unsigned long mfn, unsigned int page_order);
>  
> -unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn);
> +mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn);
>  
>  /*
>   * Populate-on-demand
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 2/8] xen/mm: Introduce a bunch of helpers for the typesafes mfn and gfn

[Julien Grall]

Those helpers will be useful to do common operations without having to
unbox/box manually the GFNs/MFNs.

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

---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

    Changes in v3:
        - Use inline functions rather than macros

    Changes in v2:
        - Rename min_gfn/max_gfn to gfn_min/gfn_max
        - Add more helpers for gfn and mfn
---
 xen/include/xen/mm.h | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 3cf646a..13f706e 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -50,6 +50,7 @@
 #include <xen/list.h>
 #include <xen/spinlock.h>
 #include <xen/typesafe.h>
+#include <xen/kernel.h>
 #include <public/memory.h>
 
 TYPE_SAFE(unsigned long, mfn);
@@ -61,6 +62,26 @@ TYPE_SAFE(unsigned long, mfn);
 #undef mfn_t
 #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      (~0UL)
@@ -70,6 +91,26 @@ TYPE_SAFE(unsigned long, gfn);
 #undef gfn_t
 #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)
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 2/8] xen/mm: Introduce a bunch of helpers for the typesafes mfn and gfn

[Jan Beulich]

>>> On 21.06.16 at 15:20, <julien.grall@arm.com> wrote:
> Those helpers will be useful to do common operations without having to
> unbox/box manually the GFNs/MFNs.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Acked-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 3/8] xen: Use typesafe gfn/mfn in guest_physmap_* helpers

[Julien Grall]

Also rename some variables to gfn or mfn when it does not require much
rework.

Finally replace %hu with %d when printing the domain id in
guest_physmap_add_entry (arch/x86/mm/p2m.c).

Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Paul Durrant <paul.durrant@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

    Changes in v3:
        - Use %d to print the domain id rather than %hu
        - Add Jan's Acked-by for non-ARM bits

    Changes in v2:
        - Don't use a wrapper for x86. Instead use mfn_* to make
        the change simpler.
---
 xen/arch/arm/domain_build.c        |  2 +-
 xen/arch/arm/mm.c                  | 10 ++---
 xen/arch/arm/p2m.c                 | 20 +++++-----
 xen/arch/x86/domain.c              |  5 ++-
 xen/arch/x86/domain_build.c        |  6 +--
 xen/arch/x86/hvm/ioreq.c           |  8 ++--
 xen/arch/x86/mm.c                  | 12 +++---
 xen/arch/x86/mm/p2m.c              | 78 ++++++++++++++++++++------------------
 xen/common/grant_table.c           |  7 ++--
 xen/common/memory.c                | 32 ++++++++--------
 xen/drivers/passthrough/arm/smmu.c |  4 +-
 xen/include/asm-arm/p2m.h          | 12 +++---
 xen/include/asm-x86/p2m.h          | 11 +++---
 xen/include/xen/mm.h               |  2 +-
 14 files changed, 110 insertions(+), 99 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 410bb4f..9035486 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -117,7 +117,7 @@ static bool_t insert_11_bank(struct domain *d,
         goto fail;
     }
 
-    res = guest_physmap_add_page(d, spfn, spfn, order);
+    res = guest_physmap_add_page(d, _gfn(spfn), _mfn(spfn), order);
     if ( res )
         panic("Failed map pages to DOM0: %d", res);
 
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 2ec211b..5ab9b75 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1153,7 +1153,7 @@ int xenmem_add_to_physmap_one(
     }
 
     /* Map at new location. */
-    rc = guest_physmap_add_entry(d, gpfn, mfn, 0, t);
+    rc = guest_physmap_add_entry(d, _gfn(gpfn), _mfn(mfn), 0, t);
 
     /* If we fail to add the mapping, we need to drop the reference we
      * took earlier on foreign pages */
@@ -1282,8 +1282,8 @@ int create_grant_host_mapping(unsigned long addr, unsigned long frame,
     if ( flags & GNTMAP_readonly )
         t = p2m_grant_map_ro;
 
-    rc = guest_physmap_add_entry(current->domain, addr >> PAGE_SHIFT,
-                                 frame, 0, t);
+    rc = guest_physmap_add_entry(current->domain, _gfn(addr >> PAGE_SHIFT),
+                                 _mfn(frame), 0, t);
 
     if ( rc )
         return GNTST_general_error;
@@ -1294,13 +1294,13 @@ int create_grant_host_mapping(unsigned long addr, unsigned long frame,
 int replace_grant_host_mapping(unsigned long addr, unsigned long mfn,
         unsigned long new_addr, unsigned int flags)
 {
-    unsigned long gfn = (unsigned long)(addr >> PAGE_SHIFT);
+    gfn_t gfn = _gfn(addr >> PAGE_SHIFT);
     struct domain *d = current->domain;
 
     if ( new_addr != 0 || (flags & GNTMAP_contains_pte) )
         return GNTST_general_error;
 
-    guest_physmap_remove_page(d, gfn, mfn, 0);
+    guest_physmap_remove_page(d, gfn, _mfn(mfn), 0);
 
     return GNTST_okay;
 }
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index ab0cb41..aa4e774 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1292,26 +1292,26 @@ int map_dev_mmio_region(struct domain *d,
 }
 
 int guest_physmap_add_entry(struct domain *d,
-                            unsigned long gpfn,
-                            unsigned long mfn,
+                            gfn_t gfn,
+                            mfn_t mfn,
                             unsigned long page_order,
                             p2m_type_t t)
 {
     return apply_p2m_changes(d, INSERT,
-                             pfn_to_paddr(gpfn),
-                             pfn_to_paddr(gpfn + (1 << page_order)),
-                             pfn_to_paddr(mfn), MATTR_MEM, 0, t,
+                             pfn_to_paddr(gfn_x(gfn)),
+                             pfn_to_paddr(gfn_x(gfn) + (1 << page_order)),
+                             pfn_to_paddr(mfn_x(mfn)), MATTR_MEM, 0, t,
                              d->arch.p2m.default_access);
 }
 
 void guest_physmap_remove_page(struct domain *d,
-                               unsigned long gpfn,
-                               unsigned long mfn, unsigned int page_order)
+                               gfn_t gfn,
+                               mfn_t mfn, unsigned int page_order)
 {
     apply_p2m_changes(d, REMOVE,
-                      pfn_to_paddr(gpfn),
-                      pfn_to_paddr(gpfn + (1<<page_order)),
-                      pfn_to_paddr(mfn), MATTR_MEM, 0, p2m_invalid,
+                      pfn_to_paddr(gfn_x(gfn)),
+                      pfn_to_paddr(gfn_x(gfn) + (1<<page_order)),
+                      pfn_to_paddr(mfn_x(mfn)), MATTR_MEM, 0, p2m_invalid,
                       d->arch.p2m.default_access);
 }
 
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 3ba7ed1..bb59247 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -802,9 +802,10 @@ int arch_domain_soft_reset(struct domain *d)
         ret = -ENOMEM;
         goto exit_put_gfn;
     }
-    guest_physmap_remove_page(d, gfn, mfn, PAGE_ORDER_4K);
+    guest_physmap_remove_page(d, _gfn(gfn), _mfn(mfn), PAGE_ORDER_4K);
 
-    ret = guest_physmap_add_page(d, gfn, page_to_mfn(new_page), PAGE_ORDER_4K);
+    ret = guest_physmap_add_page(d, _gfn(gfn), _mfn(page_to_mfn(new_page)),
+                                 PAGE_ORDER_4K);
     if ( ret )
     {
         printk(XENLOG_G_ERR "Failed to add a page to replace"
diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index b29c377..0a02d65 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -427,7 +427,7 @@ static __init void pvh_add_mem_mapping(struct domain *d, unsigned long gfn,
         if ( !iomem_access_permitted(d, mfn + i, mfn + i) )
         {
             omfn = get_gfn_query_unlocked(d, gfn + i, &t);
-            guest_physmap_remove_page(d, gfn + i, mfn_x(omfn), PAGE_ORDER_4K);
+            guest_physmap_remove_page(d, _gfn(gfn + i), omfn, PAGE_ORDER_4K);
             continue;
         }
 
@@ -530,7 +530,7 @@ static __init void pvh_map_all_iomem(struct domain *d, unsigned long nr_pages)
             if ( get_gpfn_from_mfn(mfn) != INVALID_M2P_ENTRY )
                 continue;
 
-            rc = guest_physmap_add_page(d, start_pfn, mfn, 0);
+            rc = guest_physmap_add_page(d, _gfn(start_pfn), _mfn(mfn), 0);
             if ( rc != 0 )
                 panic("Unable to add gpfn %#lx mfn %#lx to Dom0 physmap: %d",
                       start_pfn, mfn, rc);
@@ -605,7 +605,7 @@ static __init void dom0_update_physmap(struct domain *d, unsigned long pfn,
 {
     if ( is_pvh_domain(d) )
     {
-        int rc = guest_physmap_add_page(d, pfn, mfn, 0);
+        int rc = guest_physmap_add_page(d, _gfn(pfn), _mfn(mfn), 0);
         BUG_ON(rc);
         return;
     }
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 333ce14..7148ac4 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -267,8 +267,8 @@ bool_t is_ioreq_server_page(struct domain *d, const struct page_info *page)
 static void hvm_remove_ioreq_gmfn(
     struct domain *d, struct hvm_ioreq_page *iorp)
 {
-    guest_physmap_remove_page(d, iorp->gmfn,
-                              page_to_mfn(iorp->page), 0);
+    guest_physmap_remove_page(d, _gfn(iorp->gmfn),
+                              _mfn(page_to_mfn(iorp->page)), 0);
     clear_page(iorp->va);
 }
 
@@ -279,8 +279,8 @@ static int hvm_add_ioreq_gmfn(
 
     clear_page(iorp->va);
 
-    rc = guest_physmap_add_page(d, iorp->gmfn,
-                                page_to_mfn(iorp->page), 0);
+    rc = guest_physmap_add_page(d, _gfn(iorp->gmfn),
+                                _mfn(page_to_mfn(iorp->page)), 0);
     if ( rc == 0 )
         paging_mark_dirty(d, page_to_mfn(iorp->page));
 
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index ae7c8ab..7fbc94e 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4211,7 +4211,8 @@ static int create_grant_p2m_mapping(uint64_t addr, unsigned long frame,
     else
         p2mt = p2m_grant_map_rw;
     rc = guest_physmap_add_entry(current->domain,
-                                 addr >> PAGE_SHIFT, frame, PAGE_ORDER_4K, p2mt);
+                                 _gfn(addr >> PAGE_SHIFT),
+                                 _mfn(frame), PAGE_ORDER_4K, p2mt);
     if ( rc )
         return GNTST_general_error;
     else
@@ -4268,7 +4269,7 @@ static int replace_grant_p2m_mapping(
                 type, mfn_x(old_mfn), frame);
         return GNTST_general_error;
     }
-    guest_physmap_remove_page(d, gfn, frame, PAGE_ORDER_4K);
+    guest_physmap_remove_page(d, _gfn(gfn), _mfn(frame), PAGE_ORDER_4K);
 
     put_gfn(d, gfn);
     return GNTST_okay;
@@ -4853,7 +4854,8 @@ int xenmem_add_to_physmap_one(
     {
         if ( is_xen_heap_mfn(prev_mfn) )
             /* Xen heap frames are simply unhooked from this phys slot. */
-            guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K);
+            guest_physmap_remove_page(d, _gfn(gpfn), _mfn(prev_mfn),
+                                      PAGE_ORDER_4K);
         else
             /* Normal domain memory is freed, to avoid leaking memory. */
             guest_remove_page(d, gpfn);
@@ -4867,10 +4869,10 @@ int xenmem_add_to_physmap_one(
     if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
         ASSERT( old_gpfn == gfn );
     if ( old_gpfn != INVALID_M2P_ENTRY )
-        guest_physmap_remove_page(d, old_gpfn, mfn, PAGE_ORDER_4K);
+        guest_physmap_remove_page(d, _gfn(old_gpfn), _mfn(mfn), PAGE_ORDER_4K);
 
     /* Map at new location. */
-    rc = guest_physmap_add_page(d, gpfn, mfn, PAGE_ORDER_4K);
+    rc = guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), PAGE_ORDER_4K);
 
     /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */
     if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 89462b2..16733a4 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -675,21 +675,20 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn,
 }
 
 int
-guest_physmap_remove_page(struct domain *d, unsigned long gfn,
-                          unsigned long mfn, unsigned int page_order)
+guest_physmap_remove_page(struct domain *d, gfn_t gfn,
+                          mfn_t mfn, unsigned int page_order)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     int rc;
     gfn_lock(p2m, gfn, page_order);
-    rc = p2m_remove_page(p2m, gfn, mfn, page_order);
+    rc = p2m_remove_page(p2m, gfn_x(gfn), mfn_x(mfn), page_order);
     gfn_unlock(p2m, gfn, page_order);
     return rc;
 }
 
 int
-guest_physmap_add_entry(struct domain *d, unsigned long gfn,
-                        unsigned long mfn, unsigned int page_order, 
-                        p2m_type_t t)
+guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
+                        unsigned int page_order, p2m_type_t t)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     unsigned long i, ogfn;
@@ -705,13 +704,14 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn,
         {
             for ( i = 0; i < (1 << page_order); i++ )
             {
-                rc = iommu_map_page(
-                    d, mfn + i, mfn + i, IOMMUF_readable|IOMMUF_writable);
+                rc = iommu_map_page(d, mfn_x(mfn_add(mfn, i)),
+                                    mfn_x(mfn_add(mfn, i)),
+                                    IOMMUF_readable|IOMMUF_writable);
                 if ( rc != 0 )
                 {
                     while ( i-- > 0 )
                         /* If statement to satisfy __must_check. */
-                        if ( iommu_unmap_page(d, mfn + i) )
+                        if ( iommu_unmap_page(d, mfn_x(mfn_add(mfn, i))) )
                             continue;
 
                     return rc;
@@ -727,18 +727,20 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn,
 
     p2m_lock(p2m);
 
-    P2M_DEBUG("adding gfn=%#lx mfn=%#lx\n", gfn, mfn);
+    P2M_DEBUG("adding gfn=%#lx mfn=%#lx\n", gfn_x(gfn), mfn_x(mfn));
 
     /* First, remove m->p mappings for existing p->m mappings */
     for ( i = 0; i < (1UL << page_order); i++ )
     {
-        omfn = p2m->get_entry(p2m, gfn + i, &ot, &a, 0, NULL, NULL);
+        omfn = p2m->get_entry(p2m, gfn_x(gfn_add(gfn, i)), &ot,
+                              &a, 0, NULL, NULL);
         if ( p2m_is_shared(ot) )
         {
             /* Do an unshare to cleanly take care of all corner 
              * cases. */
             int rc;
-            rc = mem_sharing_unshare_page(p2m->domain, gfn + i, 0);
+            rc = mem_sharing_unshare_page(p2m->domain,
+                                          gfn_x(gfn_add(gfn, i)), 0);
             if ( rc )
             {
                 p2m_unlock(p2m);
@@ -753,10 +755,13 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn,
                  *
                  * Foreign domains are okay to place an event as they 
                  * won't go to sleep. */
-                (void)mem_sharing_notify_enomem(p2m->domain, gfn + i, 0);
+                (void)mem_sharing_notify_enomem(p2m->domain,
+                                                gfn_x(gfn_add(gfn, i)),
+                                                0);
                 return rc;
             }
-            omfn = p2m->get_entry(p2m, gfn + i, &ot, &a, 0, NULL, NULL);
+            omfn = p2m->get_entry(p2m, gfn_x(gfn_add(gfn, i)),
+                                  &ot, &a, 0, NULL, NULL);
             ASSERT(!p2m_is_shared(ot));
         }
         if ( p2m_is_grant(ot) || p2m_is_foreign(ot) )
@@ -787,39 +792,39 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn,
     /* Then, look for m->p mappings for this range and deal with them */
     for ( i = 0; i < (1UL << page_order); i++ )
     {
-        if ( page_get_owner(mfn_to_page(_mfn(mfn + i))) == dom_cow )
+        if ( page_get_owner(mfn_to_page(mfn_add(mfn, i))) == dom_cow )
         {
             /* This is no way to add a shared page to your physmap! */
-            gdprintk(XENLOG_ERR, "Adding shared mfn %lx directly to dom %hu "
-                        "physmap not allowed.\n", mfn+i, d->domain_id);
+            gdprintk(XENLOG_ERR, "Adding shared mfn %lx directly to dom%d physmap not allowed.\n",
+                     mfn_x(mfn_add(mfn, i)), d->domain_id);
             p2m_unlock(p2m);
             return -EINVAL;
         }
-        if ( page_get_owner(mfn_to_page(_mfn(mfn + i))) != d )
+        if ( page_get_owner(mfn_to_page(mfn_add(mfn, i))) != d )
             continue;
-        ogfn = mfn_to_gfn(d, _mfn(mfn+i));
-        if ( (ogfn != INVALID_M2P_ENTRY) && (ogfn != gfn + i) )
+        ogfn = mfn_to_gfn(d, mfn_add(mfn, i));
+        if ( (ogfn != INVALID_M2P_ENTRY) && (ogfn != gfn_x(gfn_add(gfn, i))) )
         {
             /* This machine frame is already mapped at another physical
              * address */
             P2M_DEBUG("aliased! mfn=%#lx, old gfn=%#lx, new gfn=%#lx\n",
-                      mfn + i, ogfn, gfn + i);
+                      mfn_x(mfn_add(mfn, i)), ogfn, gfn_x(gfn_add(gfn, i)));
             omfn = p2m->get_entry(p2m, ogfn, &ot, &a, 0, NULL, NULL);
             if ( p2m_is_ram(ot) && !p2m_is_paged(ot) )
             {
                 ASSERT(mfn_valid(omfn));
                 P2M_DEBUG("old gfn=%#lx -> mfn %#lx\n",
                           ogfn , mfn_x(omfn));
-                if ( mfn_x(omfn) == (mfn + i) )
-                    p2m_remove_page(p2m, ogfn, mfn + i, 0);
+                if ( mfn_eq(omfn, mfn_add(mfn, i)) )
+                    p2m_remove_page(p2m, ogfn, mfn_x(mfn_add(mfn, i)), 0);
             }
         }
     }
 
     /* Now, actually do the two-way mapping */
-    if ( mfn_valid(_mfn(mfn)) ) 
+    if ( mfn_valid(mfn) )
     {
-        rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t,
+        rc = p2m_set_entry(p2m, gfn_x(gfn), mfn, page_order, t,
                            p2m->default_access);
         if ( rc )
             goto out; /* Failed to update p2m, bail without updating m2p. */
@@ -827,14 +832,15 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn,
         if ( !p2m_is_grant(t) )
         {
             for ( i = 0; i < (1UL << page_order); i++ )
-                set_gpfn_from_mfn(mfn+i, gfn+i);
+                set_gpfn_from_mfn(mfn_x(mfn_add(mfn, i)),
+                                  gfn_x(gfn_add(gfn, i)));
         }
     }
     else
     {
         gdprintk(XENLOG_WARNING, "Adding bad mfn to p2m map (%#lx -> %#lx)\n",
-                 gfn, mfn);
-        rc = p2m_set_entry(p2m, gfn, _mfn(INVALID_MFN), page_order,
+                 gfn_x(gfn), mfn_x(mfn));
+        rc = p2m_set_entry(p2m, gfn_x(gfn), _mfn(INVALID_MFN), page_order,
                            p2m_invalid, p2m->default_access);
         if ( rc == 0 )
         {
@@ -2798,7 +2804,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
                     unsigned long gpfn, domid_t foreigndom)
 {
     p2m_type_t p2mt, p2mt_prev;
-    unsigned long prev_mfn, mfn;
+    mfn_t prev_mfn, mfn;
     struct page_info *page;
     int rc;
     struct domain *fdom;
@@ -2841,15 +2847,15 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
         rc = -EINVAL;
         goto out;
     }
-    mfn = mfn_x(page_to_mfn(page));
+    mfn = page_to_mfn(page);
 
     /* Remove previously mapped page if it is present. */
-    prev_mfn = mfn_x(get_gfn(tdom, gpfn, &p2mt_prev));
-    if ( mfn_valid(_mfn(prev_mfn)) )
+    prev_mfn = get_gfn(tdom, gpfn, &p2mt_prev);
+    if ( mfn_valid(prev_mfn) )
     {
-        if ( is_xen_heap_mfn(prev_mfn) )
+        if ( is_xen_heap_mfn(mfn_x(prev_mfn)) )
             /* Xen heap frames are simply unhooked from this phys slot */
-            guest_physmap_remove_page(tdom, gpfn, prev_mfn, 0);
+            guest_physmap_remove_page(tdom, _gfn(gpfn), prev_mfn, 0);
         else
             /* Normal domain memory is freed, to avoid leaking memory. */
             guest_remove_page(tdom, gpfn);
@@ -2859,11 +2865,11 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
      * will update the m2p table which will result in  mfn -> gpfn of dom0
      * and not fgfn of domU.
      */
-    rc = set_foreign_p2m_entry(tdom, gpfn, _mfn(mfn));
+    rc = set_foreign_p2m_entry(tdom, gpfn, mfn);
     if ( rc )
         gdprintk(XENLOG_WARNING, "set_foreign_p2m_entry failed. "
                  "gpfn:%lx mfn:%lx fgfn:%lx td:%d fd:%d\n",
-                 gpfn, mfn, fgfn, tdom->domain_id, fdom->domain_id);
+                 gpfn, mfn_x(mfn), fgfn, tdom->domain_id, fdom->domain_id);
 
     put_page(page);
 
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 3c304f4..3f15543 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1818,7 +1818,7 @@ gnttab_transfer(
             goto copyback;
         }
 
-        guest_physmap_remove_page(d, gop.mfn, mfn, 0);
+        guest_physmap_remove_page(d, _gfn(gop.mfn), _mfn(mfn), 0);
         gnttab_flush_tlb(d);
 
         /* Find the target domain. */
@@ -1946,7 +1946,7 @@ gnttab_transfer(
         {
             grant_entry_v1_t *sha = &shared_entry_v1(e->grant_table, gop.ref);
 
-            guest_physmap_add_page(e, sha->frame, mfn, 0);
+            guest_physmap_add_page(e, _gfn(sha->frame), _mfn(mfn), 0);
             if ( !paging_mode_translate(e) )
                 sha->frame = mfn;
         }
@@ -1954,7 +1954,8 @@ gnttab_transfer(
         {
             grant_entry_v2_t *sha = &shared_entry_v2(e->grant_table, gop.ref);
 
-            guest_physmap_add_page(e, sha->full_page.frame, mfn, 0);
+            guest_physmap_add_page(e, _gfn(sha->full_page.frame),
+                                   _mfn(mfn), 0);
             if ( !paging_mode_translate(e) )
                 sha->full_page.frame = mfn;
         }
diff --git a/xen/common/memory.c b/xen/common/memory.c
index b54b076..a8a75e0 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -213,7 +213,7 @@ static void populate_physmap(struct memop_args *a)
                 mfn = page_to_mfn(page);
             }
 
-            guest_physmap_add_page(d, gpfn, mfn, a->extent_order);
+            guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), a->extent_order);
 
             if ( !paging_mode_translate(d) )
             {
@@ -237,20 +237,20 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
 #ifdef CONFIG_X86
     p2m_type_t p2mt;
 #endif
-    unsigned long mfn;
+    mfn_t mfn;
 
 #ifdef CONFIG_X86
-    mfn = mfn_x(get_gfn_query(d, gmfn, &p2mt)); 
+    mfn = get_gfn_query(d, gmfn, &p2mt);
     if ( unlikely(p2m_is_paging(p2mt)) )
     {
-        guest_physmap_remove_page(d, gmfn, mfn, 0);
+        guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
         put_gfn(d, gmfn);
         /* If the page hasn't yet been paged out, there is an
          * actual page that needs to be released. */
         if ( p2mt == p2m_ram_paging_out )
         {
-            ASSERT(mfn_valid(mfn));
-            page = mfn_to_page(mfn);
+            ASSERT(mfn_valid(mfn_x(mfn)));
+            page = mfn_to_page(mfn_x(mfn));
             if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
                 put_page(page);
         }
@@ -259,14 +259,14 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
     }
     if ( p2mt == p2m_mmio_direct )
     {
-        clear_mmio_p2m_entry(d, gmfn, _mfn(mfn), 0);
+        clear_mmio_p2m_entry(d, gmfn, mfn, 0);
         put_gfn(d, gmfn);
         return 1;
     }
 #else
-    mfn = mfn_x(gfn_to_mfn(d, _gfn(gmfn)));
+    mfn = gfn_to_mfn(d, _gfn(gmfn));
 #endif
-    if ( unlikely(!mfn_valid(mfn)) )
+    if ( unlikely(!mfn_valid(mfn_x(mfn))) )
     {
         put_gfn(d, gmfn);
         gdprintk(XENLOG_INFO, "Domain %u page number %lx invalid\n",
@@ -288,12 +288,12 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
             return 0;
         }
         /* Maybe the mfn changed */
-        mfn = mfn_x(get_gfn_query_unlocked(d, gmfn, &p2mt));
+        mfn = get_gfn_query_unlocked(d, gmfn, &p2mt);
         ASSERT(!p2m_is_shared(p2mt));
     }
 #endif /* CONFIG_X86 */
 
-    page = mfn_to_page(mfn);
+    page = mfn_to_page(mfn_x(mfn));
     if ( unlikely(!get_page(page, d)) )
     {
         put_gfn(d, gmfn);
@@ -316,7 +316,7 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
          test_and_clear_bit(_PGC_allocated, &page->count_info) )
         put_page(page);
 
-    guest_physmap_remove_page(d, gmfn, mfn, 0);
+    guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
 
     put_page(page);
     put_gfn(d, gmfn);
@@ -540,7 +540,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
             gfn = mfn_to_gmfn(d, mfn);
             /* Pages were unshared above */
             BUG_ON(SHARED_M2P(gfn));
-            guest_physmap_remove_page(d, gfn, mfn, 0);
+            guest_physmap_remove_page(d, _gfn(gfn), _mfn(mfn), 0);
             put_page(page);
         }
 
@@ -584,7 +584,8 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
             }
 
             mfn = page_to_mfn(page);
-            guest_physmap_add_page(d, gpfn, mfn, exch.out.extent_order);
+            guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn),
+                                   exch.out.extent_order);
 
             if ( !paging_mode_translate(d) )
             {
@@ -1095,7 +1096,8 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
         if ( page )
         {
-            guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
+            guest_physmap_remove_page(d, _gfn(xrfp.gpfn),
+                                      _mfn(page_to_mfn(page)), 0);
             put_page(page);
         }
         else
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 8a4b123..cf8b8b8 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2774,7 +2774,7 @@ static int __must_check arm_smmu_map_page(struct domain *d, unsigned long gfn,
 	 * The function guest_physmap_add_entry replaces the current mapping
 	 * if there is already one...
 	 */
-	return guest_physmap_add_entry(d, gfn, mfn, 0, t);
+	return guest_physmap_add_entry(d, _gfn(gfn), _mfn(mfn), 0, t);
 }
 
 static int __must_check arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
@@ -2786,7 +2786,7 @@ static int __must_check arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
 	if ( !is_domain_direct_mapped(d) )
 		return -EINVAL;
 
-	guest_physmap_remove_page(d, gfn, gfn, 0);
+	guest_physmap_remove_page(d, _gfn(gfn), _mfn(gfn), 0);
 
 	return 0;
 }
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 75c65a8..0d1e61e 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -160,23 +160,23 @@ int map_dev_mmio_region(struct domain *d,
                         unsigned long mfn);
 
 int guest_physmap_add_entry(struct domain *d,
-                            unsigned long gfn,
-                            unsigned long mfn,
+                            gfn_t gfn,
+                            mfn_t mfn,
                             unsigned long page_order,
                             p2m_type_t t);
 
 /* Untyped version for RAM only, for compatibility */
 static inline int guest_physmap_add_page(struct domain *d,
-                                         unsigned long gfn,
-                                         unsigned long mfn,
+                                         gfn_t gfn,
+                                         mfn_t mfn,
                                          unsigned int page_order)
 {
     return guest_physmap_add_entry(d, gfn, mfn, page_order, p2m_ram_rw);
 }
 
 void guest_physmap_remove_page(struct domain *d,
-                               unsigned long gpfn,
-                               unsigned long mfn, unsigned int page_order);
+                               gfn_t gfn,
+                               mfn_t mfn, unsigned int page_order);
 
 mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn);
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 65675a2..4ab3574 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -545,14 +545,14 @@ void p2m_teardown(struct p2m_domain *p2m);
 void p2m_final_teardown(struct domain *d);
 
 /* Add a page to a domain's p2m table */
-int guest_physmap_add_entry(struct domain *d, unsigned long gfn,
-                            unsigned long mfn, unsigned int page_order, 
+int guest_physmap_add_entry(struct domain *d, gfn_t gfn,
+                            mfn_t mfn, unsigned int page_order,
                             p2m_type_t t);
 
 /* Untyped version for RAM only, for compatibility */
 static inline int guest_physmap_add_page(struct domain *d,
-                                         unsigned long gfn,
-                                         unsigned long mfn,
+                                         gfn_t gfn,
+                                         mfn_t mfn,
                                          unsigned int page_order)
 {
     return guest_physmap_add_entry(d, gfn, mfn, page_order, p2m_ram_rw);
@@ -560,8 +560,7 @@ static inline int guest_physmap_add_page(struct domain *d,
 
 /* Remove a page from a domain's p2m table */
 int guest_physmap_remove_page(struct domain *d,
-                              unsigned long gfn,
-                              unsigned long mfn, unsigned int page_order);
+                              gfn_t gfn, mfn_t mfn, unsigned int page_order);
 
 /* Set a p2m range as populate-on-demand */
 int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn,
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 13f706e..b62f473 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -552,7 +552,7 @@ int xenmem_add_to_physmap_one(struct domain *d, unsigned int space,
 
 /* Returns 1 on success, 0 on error, negative if the ring
  * for event propagation is full in the presence of paging */
-int guest_remove_page(struct domain *d, unsigned long gmfn);
+int guest_remove_page(struct domain *d, unsigned long gfn);
 
 #define RAM_TYPE_CONVENTIONAL 0x00000001
 #define RAM_TYPE_RESERVED     0x00000002
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 3/8] xen: Use typesafe gfn/mfn in guest_physmap_* helpers

[Stefano Stabellini]

On Tue, 21 Jun 2016, Julien Grall wrote:
> Also rename some variables to gfn or mfn when it does not require much
> rework.
> 
> Finally replace %hu with %d when printing the domain id in
> guest_physmap_add_entry (arch/x86/mm/p2m.c).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Paul Durrant <paul.durrant@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> 
>     Changes in v3:
>         - Use %d to print the domain id rather than %hu
>         - Add Jan's Acked-by for non-ARM bits
> 
>     Changes in v2:
>         - Don't use a wrapper for x86. Instead use mfn_* to make
>         the change simpler.
> ---
>  xen/arch/arm/domain_build.c        |  2 +-
>  xen/arch/arm/mm.c                  | 10 ++---
>  xen/arch/arm/p2m.c                 | 20 +++++-----
>  xen/arch/x86/domain.c              |  5 ++-
>  xen/arch/x86/domain_build.c        |  6 +--
>  xen/arch/x86/hvm/ioreq.c           |  8 ++--
>  xen/arch/x86/mm.c                  | 12 +++---
>  xen/arch/x86/mm/p2m.c              | 78 ++++++++++++++++++++------------------
>  xen/common/grant_table.c           |  7 ++--
>  xen/common/memory.c                | 32 ++++++++--------
>  xen/drivers/passthrough/arm/smmu.c |  4 +-
>  xen/include/asm-arm/p2m.h          | 12 +++---
>  xen/include/asm-x86/p2m.h          | 11 +++---
>  xen/include/xen/mm.h               |  2 +-
>  14 files changed, 110 insertions(+), 99 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 410bb4f..9035486 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -117,7 +117,7 @@ static bool_t insert_11_bank(struct domain *d,
>          goto fail;
>      }
>  
> -    res = guest_physmap_add_page(d, spfn, spfn, order);
> +    res = guest_physmap_add_page(d, _gfn(spfn), _mfn(spfn), order);
>      if ( res )
>          panic("Failed map pages to DOM0: %d", res);
>  
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 2ec211b..5ab9b75 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1153,7 +1153,7 @@ int xenmem_add_to_physmap_one(
>      }
>  
>      /* Map at new location. */
> -    rc = guest_physmap_add_entry(d, gpfn, mfn, 0, t);
> +    rc = guest_physmap_add_entry(d, _gfn(gpfn), _mfn(mfn), 0, t);
>  
>      /* If we fail to add the mapping, we need to drop the reference we
>       * took earlier on foreign pages */
> @@ -1282,8 +1282,8 @@ int create_grant_host_mapping(unsigned long addr, unsigned long frame,
>      if ( flags & GNTMAP_readonly )
>          t = p2m_grant_map_ro;
>  
> -    rc = guest_physmap_add_entry(current->domain, addr >> PAGE_SHIFT,
> -                                 frame, 0, t);
> +    rc = guest_physmap_add_entry(current->domain, _gfn(addr >> PAGE_SHIFT),
> +                                 _mfn(frame), 0, t);
>  
>      if ( rc )
>          return GNTST_general_error;
> @@ -1294,13 +1294,13 @@ int create_grant_host_mapping(unsigned long addr, unsigned long frame,
>  int replace_grant_host_mapping(unsigned long addr, unsigned long mfn,
>          unsigned long new_addr, unsigned int flags)
>  {
> -    unsigned long gfn = (unsigned long)(addr >> PAGE_SHIFT);
> +    gfn_t gfn = _gfn(addr >> PAGE_SHIFT);
>      struct domain *d = current->domain;
>  
>      if ( new_addr != 0 || (flags & GNTMAP_contains_pte) )
>          return GNTST_general_error;
>  
> -    guest_physmap_remove_page(d, gfn, mfn, 0);
> +    guest_physmap_remove_page(d, gfn, _mfn(mfn), 0);
>  
>      return GNTST_okay;
>  }
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index ab0cb41..aa4e774 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1292,26 +1292,26 @@ int map_dev_mmio_region(struct domain *d,
>  }
>  
>  int guest_physmap_add_entry(struct domain *d,
> -                            unsigned long gpfn,
> -                            unsigned long mfn,
> +                            gfn_t gfn,
> +                            mfn_t mfn,
>                              unsigned long page_order,
>                              p2m_type_t t)
>  {
>      return apply_p2m_changes(d, INSERT,
> -                             pfn_to_paddr(gpfn),
> -                             pfn_to_paddr(gpfn + (1 << page_order)),
> -                             pfn_to_paddr(mfn), MATTR_MEM, 0, t,
> +                             pfn_to_paddr(gfn_x(gfn)),
> +                             pfn_to_paddr(gfn_x(gfn) + (1 << page_order)),
> +                             pfn_to_paddr(mfn_x(mfn)), MATTR_MEM, 0, t,
>                               d->arch.p2m.default_access);
>  }
>  
>  void guest_physmap_remove_page(struct domain *d,
> -                               unsigned long gpfn,
> -                               unsigned long mfn, unsigned int page_order)
> +                               gfn_t gfn,
> +                               mfn_t mfn, unsigned int page_order)
>  {
>      apply_p2m_changes(d, REMOVE,
> -                      pfn_to_paddr(gpfn),
> -                      pfn_to_paddr(gpfn + (1<<page_order)),
> -                      pfn_to_paddr(mfn), MATTR_MEM, 0, p2m_invalid,
> +                      pfn_to_paddr(gfn_x(gfn)),
> +                      pfn_to_paddr(gfn_x(gfn) + (1<<page_order)),
> +                      pfn_to_paddr(mfn_x(mfn)), MATTR_MEM, 0, p2m_invalid,
>                        d->arch.p2m.default_access);
>  }
>  
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 3ba7ed1..bb59247 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -802,9 +802,10 @@ int arch_domain_soft_reset(struct domain *d)
>          ret = -ENOMEM;
>          goto exit_put_gfn;
>      }
> -    guest_physmap_remove_page(d, gfn, mfn, PAGE_ORDER_4K);
> +    guest_physmap_remove_page(d, _gfn(gfn), _mfn(mfn), PAGE_ORDER_4K);
>  
> -    ret = guest_physmap_add_page(d, gfn, page_to_mfn(new_page), PAGE_ORDER_4K);
> +    ret = guest_physmap_add_page(d, _gfn(gfn), _mfn(page_to_mfn(new_page)),
> +                                 PAGE_ORDER_4K);
>      if ( ret )
>      {
>          printk(XENLOG_G_ERR "Failed to add a page to replace"
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index b29c377..0a02d65 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -427,7 +427,7 @@ static __init void pvh_add_mem_mapping(struct domain *d, unsigned long gfn,
>          if ( !iomem_access_permitted(d, mfn + i, mfn + i) )
>          {
>              omfn = get_gfn_query_unlocked(d, gfn + i, &t);
> -            guest_physmap_remove_page(d, gfn + i, mfn_x(omfn), PAGE_ORDER_4K);
> +            guest_physmap_remove_page(d, _gfn(gfn + i), omfn, PAGE_ORDER_4K);
>              continue;
>          }
>  
> @@ -530,7 +530,7 @@ static __init void pvh_map_all_iomem(struct domain *d, unsigned long nr_pages)
>              if ( get_gpfn_from_mfn(mfn) != INVALID_M2P_ENTRY )
>                  continue;
>  
> -            rc = guest_physmap_add_page(d, start_pfn, mfn, 0);
> +            rc = guest_physmap_add_page(d, _gfn(start_pfn), _mfn(mfn), 0);
>              if ( rc != 0 )
>                  panic("Unable to add gpfn %#lx mfn %#lx to Dom0 physmap: %d",
>                        start_pfn, mfn, rc);
> @@ -605,7 +605,7 @@ static __init void dom0_update_physmap(struct domain *d, unsigned long pfn,
>  {
>      if ( is_pvh_domain(d) )
>      {
> -        int rc = guest_physmap_add_page(d, pfn, mfn, 0);
> +        int rc = guest_physmap_add_page(d, _gfn(pfn), _mfn(mfn), 0);
>          BUG_ON(rc);
>          return;
>      }
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index 333ce14..7148ac4 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -267,8 +267,8 @@ bool_t is_ioreq_server_page(struct domain *d, const struct page_info *page)
>  static void hvm_remove_ioreq_gmfn(
>      struct domain *d, struct hvm_ioreq_page *iorp)
>  {
> -    guest_physmap_remove_page(d, iorp->gmfn,
> -                              page_to_mfn(iorp->page), 0);
> +    guest_physmap_remove_page(d, _gfn(iorp->gmfn),
> +                              _mfn(page_to_mfn(iorp->page)), 0);
>      clear_page(iorp->va);
>  }
>  
> @@ -279,8 +279,8 @@ static int hvm_add_ioreq_gmfn(
>  
>      clear_page(iorp->va);
>  
> -    rc = guest_physmap_add_page(d, iorp->gmfn,
> -                                page_to_mfn(iorp->page), 0);
> +    rc = guest_physmap_add_page(d, _gfn(iorp->gmfn),
> +                                _mfn(page_to_mfn(iorp->page)), 0);
>      if ( rc == 0 )
>          paging_mark_dirty(d, page_to_mfn(iorp->page));
>  
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index ae7c8ab..7fbc94e 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4211,7 +4211,8 @@ static int create_grant_p2m_mapping(uint64_t addr, unsigned long frame,
>      else
>          p2mt = p2m_grant_map_rw;
>      rc = guest_physmap_add_entry(current->domain,
> -                                 addr >> PAGE_SHIFT, frame, PAGE_ORDER_4K, p2mt);
> +                                 _gfn(addr >> PAGE_SHIFT),
> +                                 _mfn(frame), PAGE_ORDER_4K, p2mt);
>      if ( rc )
>          return GNTST_general_error;
>      else
> @@ -4268,7 +4269,7 @@ static int replace_grant_p2m_mapping(
>                  type, mfn_x(old_mfn), frame);
>          return GNTST_general_error;
>      }
> -    guest_physmap_remove_page(d, gfn, frame, PAGE_ORDER_4K);
> +    guest_physmap_remove_page(d, _gfn(gfn), _mfn(frame), PAGE_ORDER_4K);
>  
>      put_gfn(d, gfn);
>      return GNTST_okay;
> @@ -4853,7 +4854,8 @@ int xenmem_add_to_physmap_one(
>      {
>          if ( is_xen_heap_mfn(prev_mfn) )
>              /* Xen heap frames are simply unhooked from this phys slot. */
> -            guest_physmap_remove_page(d, gpfn, prev_mfn, PAGE_ORDER_4K);
> +            guest_physmap_remove_page(d, _gfn(gpfn), _mfn(prev_mfn),
> +                                      PAGE_ORDER_4K);
>          else
>              /* Normal domain memory is freed, to avoid leaking memory. */
>              guest_remove_page(d, gpfn);
> @@ -4867,10 +4869,10 @@ int xenmem_add_to_physmap_one(
>      if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
>          ASSERT( old_gpfn == gfn );
>      if ( old_gpfn != INVALID_M2P_ENTRY )
> -        guest_physmap_remove_page(d, old_gpfn, mfn, PAGE_ORDER_4K);
> +        guest_physmap_remove_page(d, _gfn(old_gpfn), _mfn(mfn), PAGE_ORDER_4K);
>  
>      /* Map at new location. */
> -    rc = guest_physmap_add_page(d, gpfn, mfn, PAGE_ORDER_4K);
> +    rc = guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), PAGE_ORDER_4K);
>  
>      /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */
>      if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 89462b2..16733a4 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -675,21 +675,20 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn, unsigned long mfn,
>  }
>  
>  int
> -guest_physmap_remove_page(struct domain *d, unsigned long gfn,
> -                          unsigned long mfn, unsigned int page_order)
> +guest_physmap_remove_page(struct domain *d, gfn_t gfn,
> +                          mfn_t mfn, unsigned int page_order)
>  {
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>      int rc;
>      gfn_lock(p2m, gfn, page_order);
> -    rc = p2m_remove_page(p2m, gfn, mfn, page_order);
> +    rc = p2m_remove_page(p2m, gfn_x(gfn), mfn_x(mfn), page_order);
>      gfn_unlock(p2m, gfn, page_order);
>      return rc;
>  }
>  
>  int
> -guest_physmap_add_entry(struct domain *d, unsigned long gfn,
> -                        unsigned long mfn, unsigned int page_order, 
> -                        p2m_type_t t)
> +guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
> +                        unsigned int page_order, p2m_type_t t)
>  {
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>      unsigned long i, ogfn;
> @@ -705,13 +704,14 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn,
>          {
>              for ( i = 0; i < (1 << page_order); i++ )
>              {
> -                rc = iommu_map_page(
> -                    d, mfn + i, mfn + i, IOMMUF_readable|IOMMUF_writable);
> +                rc = iommu_map_page(d, mfn_x(mfn_add(mfn, i)),
> +                                    mfn_x(mfn_add(mfn, i)),
> +                                    IOMMUF_readable|IOMMUF_writable);
>                  if ( rc != 0 )
>                  {
>                      while ( i-- > 0 )
>                          /* If statement to satisfy __must_check. */
> -                        if ( iommu_unmap_page(d, mfn + i) )
> +                        if ( iommu_unmap_page(d, mfn_x(mfn_add(mfn, i))) )
>                              continue;
>  
>                      return rc;
> @@ -727,18 +727,20 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn,
>  
>      p2m_lock(p2m);
>  
> -    P2M_DEBUG("adding gfn=%#lx mfn=%#lx\n", gfn, mfn);
> +    P2M_DEBUG("adding gfn=%#lx mfn=%#lx\n", gfn_x(gfn), mfn_x(mfn));
>  
>      /* First, remove m->p mappings for existing p->m mappings */
>      for ( i = 0; i < (1UL << page_order); i++ )
>      {
> -        omfn = p2m->get_entry(p2m, gfn + i, &ot, &a, 0, NULL, NULL);
> +        omfn = p2m->get_entry(p2m, gfn_x(gfn_add(gfn, i)), &ot,
> +                              &a, 0, NULL, NULL);
>          if ( p2m_is_shared(ot) )
>          {
>              /* Do an unshare to cleanly take care of all corner 
>               * cases. */
>              int rc;
> -            rc = mem_sharing_unshare_page(p2m->domain, gfn + i, 0);
> +            rc = mem_sharing_unshare_page(p2m->domain,
> +                                          gfn_x(gfn_add(gfn, i)), 0);
>              if ( rc )
>              {
>                  p2m_unlock(p2m);
> @@ -753,10 +755,13 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn,
>                   *
>                   * Foreign domains are okay to place an event as they 
>                   * won't go to sleep. */
> -                (void)mem_sharing_notify_enomem(p2m->domain, gfn + i, 0);
> +                (void)mem_sharing_notify_enomem(p2m->domain,
> +                                                gfn_x(gfn_add(gfn, i)),
> +                                                0);
>                  return rc;
>              }
> -            omfn = p2m->get_entry(p2m, gfn + i, &ot, &a, 0, NULL, NULL);
> +            omfn = p2m->get_entry(p2m, gfn_x(gfn_add(gfn, i)),
> +                                  &ot, &a, 0, NULL, NULL);
>              ASSERT(!p2m_is_shared(ot));
>          }
>          if ( p2m_is_grant(ot) || p2m_is_foreign(ot) )
> @@ -787,39 +792,39 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn,
>      /* Then, look for m->p mappings for this range and deal with them */
>      for ( i = 0; i < (1UL << page_order); i++ )
>      {
> -        if ( page_get_owner(mfn_to_page(_mfn(mfn + i))) == dom_cow )
> +        if ( page_get_owner(mfn_to_page(mfn_add(mfn, i))) == dom_cow )
>          {
>              /* This is no way to add a shared page to your physmap! */
> -            gdprintk(XENLOG_ERR, "Adding shared mfn %lx directly to dom %hu "
> -                        "physmap not allowed.\n", mfn+i, d->domain_id);
> +            gdprintk(XENLOG_ERR, "Adding shared mfn %lx directly to dom%d physmap not allowed.\n",
> +                     mfn_x(mfn_add(mfn, i)), d->domain_id);
>              p2m_unlock(p2m);
>              return -EINVAL;
>          }
> -        if ( page_get_owner(mfn_to_page(_mfn(mfn + i))) != d )
> +        if ( page_get_owner(mfn_to_page(mfn_add(mfn, i))) != d )
>              continue;
> -        ogfn = mfn_to_gfn(d, _mfn(mfn+i));
> -        if ( (ogfn != INVALID_M2P_ENTRY) && (ogfn != gfn + i) )
> +        ogfn = mfn_to_gfn(d, mfn_add(mfn, i));
> +        if ( (ogfn != INVALID_M2P_ENTRY) && (ogfn != gfn_x(gfn_add(gfn, i))) )
>          {
>              /* This machine frame is already mapped at another physical
>               * address */
>              P2M_DEBUG("aliased! mfn=%#lx, old gfn=%#lx, new gfn=%#lx\n",
> -                      mfn + i, ogfn, gfn + i);
> +                      mfn_x(mfn_add(mfn, i)), ogfn, gfn_x(gfn_add(gfn, i)));
>              omfn = p2m->get_entry(p2m, ogfn, &ot, &a, 0, NULL, NULL);
>              if ( p2m_is_ram(ot) && !p2m_is_paged(ot) )
>              {
>                  ASSERT(mfn_valid(omfn));
>                  P2M_DEBUG("old gfn=%#lx -> mfn %#lx\n",
>                            ogfn , mfn_x(omfn));
> -                if ( mfn_x(omfn) == (mfn + i) )
> -                    p2m_remove_page(p2m, ogfn, mfn + i, 0);
> +                if ( mfn_eq(omfn, mfn_add(mfn, i)) )
> +                    p2m_remove_page(p2m, ogfn, mfn_x(mfn_add(mfn, i)), 0);
>              }
>          }
>      }
>  
>      /* Now, actually do the two-way mapping */
> -    if ( mfn_valid(_mfn(mfn)) ) 
> +    if ( mfn_valid(mfn) )
>      {
> -        rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t,
> +        rc = p2m_set_entry(p2m, gfn_x(gfn), mfn, page_order, t,
>                             p2m->default_access);
>          if ( rc )
>              goto out; /* Failed to update p2m, bail without updating m2p. */
> @@ -827,14 +832,15 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn,
>          if ( !p2m_is_grant(t) )
>          {
>              for ( i = 0; i < (1UL << page_order); i++ )
> -                set_gpfn_from_mfn(mfn+i, gfn+i);
> +                set_gpfn_from_mfn(mfn_x(mfn_add(mfn, i)),
> +                                  gfn_x(gfn_add(gfn, i)));
>          }
>      }
>      else
>      {
>          gdprintk(XENLOG_WARNING, "Adding bad mfn to p2m map (%#lx -> %#lx)\n",
> -                 gfn, mfn);
> -        rc = p2m_set_entry(p2m, gfn, _mfn(INVALID_MFN), page_order,
> +                 gfn_x(gfn), mfn_x(mfn));
> +        rc = p2m_set_entry(p2m, gfn_x(gfn), _mfn(INVALID_MFN), page_order,
>                             p2m_invalid, p2m->default_access);
>          if ( rc == 0 )
>          {
> @@ -2798,7 +2804,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
>                      unsigned long gpfn, domid_t foreigndom)
>  {
>      p2m_type_t p2mt, p2mt_prev;
> -    unsigned long prev_mfn, mfn;
> +    mfn_t prev_mfn, mfn;
>      struct page_info *page;
>      int rc;
>      struct domain *fdom;
> @@ -2841,15 +2847,15 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
>          rc = -EINVAL;
>          goto out;
>      }
> -    mfn = mfn_x(page_to_mfn(page));
> +    mfn = page_to_mfn(page);
>  
>      /* Remove previously mapped page if it is present. */
> -    prev_mfn = mfn_x(get_gfn(tdom, gpfn, &p2mt_prev));
> -    if ( mfn_valid(_mfn(prev_mfn)) )
> +    prev_mfn = get_gfn(tdom, gpfn, &p2mt_prev);
> +    if ( mfn_valid(prev_mfn) )
>      {
> -        if ( is_xen_heap_mfn(prev_mfn) )
> +        if ( is_xen_heap_mfn(mfn_x(prev_mfn)) )
>              /* Xen heap frames are simply unhooked from this phys slot */
> -            guest_physmap_remove_page(tdom, gpfn, prev_mfn, 0);
> +            guest_physmap_remove_page(tdom, _gfn(gpfn), prev_mfn, 0);
>          else
>              /* Normal domain memory is freed, to avoid leaking memory. */
>              guest_remove_page(tdom, gpfn);
> @@ -2859,11 +2865,11 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
>       * will update the m2p table which will result in  mfn -> gpfn of dom0
>       * and not fgfn of domU.
>       */
> -    rc = set_foreign_p2m_entry(tdom, gpfn, _mfn(mfn));
> +    rc = set_foreign_p2m_entry(tdom, gpfn, mfn);
>      if ( rc )
>          gdprintk(XENLOG_WARNING, "set_foreign_p2m_entry failed. "
>                   "gpfn:%lx mfn:%lx fgfn:%lx td:%d fd:%d\n",
> -                 gpfn, mfn, fgfn, tdom->domain_id, fdom->domain_id);
> +                 gpfn, mfn_x(mfn), fgfn, tdom->domain_id, fdom->domain_id);
>  
>      put_page(page);
>  
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 3c304f4..3f15543 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1818,7 +1818,7 @@ gnttab_transfer(
>              goto copyback;
>          }
>  
> -        guest_physmap_remove_page(d, gop.mfn, mfn, 0);
> +        guest_physmap_remove_page(d, _gfn(gop.mfn), _mfn(mfn), 0);
>          gnttab_flush_tlb(d);
>  
>          /* Find the target domain. */
> @@ -1946,7 +1946,7 @@ gnttab_transfer(
>          {
>              grant_entry_v1_t *sha = &shared_entry_v1(e->grant_table, gop.ref);
>  
> -            guest_physmap_add_page(e, sha->frame, mfn, 0);
> +            guest_physmap_add_page(e, _gfn(sha->frame), _mfn(mfn), 0);
>              if ( !paging_mode_translate(e) )
>                  sha->frame = mfn;
>          }
> @@ -1954,7 +1954,8 @@ gnttab_transfer(
>          {
>              grant_entry_v2_t *sha = &shared_entry_v2(e->grant_table, gop.ref);
>  
> -            guest_physmap_add_page(e, sha->full_page.frame, mfn, 0);
> +            guest_physmap_add_page(e, _gfn(sha->full_page.frame),
> +                                   _mfn(mfn), 0);
>              if ( !paging_mode_translate(e) )
>                  sha->full_page.frame = mfn;
>          }
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index b54b076..a8a75e0 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -213,7 +213,7 @@ static void populate_physmap(struct memop_args *a)
>                  mfn = page_to_mfn(page);
>              }
>  
> -            guest_physmap_add_page(d, gpfn, mfn, a->extent_order);
> +            guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), a->extent_order);
>  
>              if ( !paging_mode_translate(d) )
>              {
> @@ -237,20 +237,20 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>  #ifdef CONFIG_X86
>      p2m_type_t p2mt;
>  #endif
> -    unsigned long mfn;
> +    mfn_t mfn;
>  
>  #ifdef CONFIG_X86
> -    mfn = mfn_x(get_gfn_query(d, gmfn, &p2mt)); 
> +    mfn = get_gfn_query(d, gmfn, &p2mt);
>      if ( unlikely(p2m_is_paging(p2mt)) )
>      {
> -        guest_physmap_remove_page(d, gmfn, mfn, 0);
> +        guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
>          put_gfn(d, gmfn);
>          /* If the page hasn't yet been paged out, there is an
>           * actual page that needs to be released. */
>          if ( p2mt == p2m_ram_paging_out )
>          {
> -            ASSERT(mfn_valid(mfn));
> -            page = mfn_to_page(mfn);
> +            ASSERT(mfn_valid(mfn_x(mfn)));
> +            page = mfn_to_page(mfn_x(mfn));
>              if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
>                  put_page(page);
>          }
> @@ -259,14 +259,14 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>      }
>      if ( p2mt == p2m_mmio_direct )
>      {
> -        clear_mmio_p2m_entry(d, gmfn, _mfn(mfn), 0);
> +        clear_mmio_p2m_entry(d, gmfn, mfn, 0);
>          put_gfn(d, gmfn);
>          return 1;
>      }
>  #else
> -    mfn = mfn_x(gfn_to_mfn(d, _gfn(gmfn)));
> +    mfn = gfn_to_mfn(d, _gfn(gmfn));
>  #endif
> -    if ( unlikely(!mfn_valid(mfn)) )
> +    if ( unlikely(!mfn_valid(mfn_x(mfn))) )
>      {
>          put_gfn(d, gmfn);
>          gdprintk(XENLOG_INFO, "Domain %u page number %lx invalid\n",
> @@ -288,12 +288,12 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>              return 0;
>          }
>          /* Maybe the mfn changed */
> -        mfn = mfn_x(get_gfn_query_unlocked(d, gmfn, &p2mt));
> +        mfn = get_gfn_query_unlocked(d, gmfn, &p2mt);
>          ASSERT(!p2m_is_shared(p2mt));
>      }
>  #endif /* CONFIG_X86 */
>  
> -    page = mfn_to_page(mfn);
> +    page = mfn_to_page(mfn_x(mfn));
>      if ( unlikely(!get_page(page, d)) )
>      {
>          put_gfn(d, gmfn);
> @@ -316,7 +316,7 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>           test_and_clear_bit(_PGC_allocated, &page->count_info) )
>          put_page(page);
>  
> -    guest_physmap_remove_page(d, gmfn, mfn, 0);
> +    guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0);
>  
>      put_page(page);
>      put_gfn(d, gmfn);
> @@ -540,7 +540,7 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>              gfn = mfn_to_gmfn(d, mfn);
>              /* Pages were unshared above */
>              BUG_ON(SHARED_M2P(gfn));
> -            guest_physmap_remove_page(d, gfn, mfn, 0);
> +            guest_physmap_remove_page(d, _gfn(gfn), _mfn(mfn), 0);
>              put_page(page);
>          }
>  
> @@ -584,7 +584,8 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>              }
>  
>              mfn = page_to_mfn(page);
> -            guest_physmap_add_page(d, gpfn, mfn, exch.out.extent_order);
> +            guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn),
> +                                   exch.out.extent_order);
>  
>              if ( !paging_mode_translate(d) )
>              {
> @@ -1095,7 +1096,8 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
>          if ( page )
>          {
> -            guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
> +            guest_physmap_remove_page(d, _gfn(xrfp.gpfn),
> +                                      _mfn(page_to_mfn(page)), 0);
>              put_page(page);
>          }
>          else
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 8a4b123..cf8b8b8 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2774,7 +2774,7 @@ static int __must_check arm_smmu_map_page(struct domain *d, unsigned long gfn,
>  	 * The function guest_physmap_add_entry replaces the current mapping
>  	 * if there is already one...
>  	 */
> -	return guest_physmap_add_entry(d, gfn, mfn, 0, t);
> +	return guest_physmap_add_entry(d, _gfn(gfn), _mfn(mfn), 0, t);
>  }
>  
>  static int __must_check arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
> @@ -2786,7 +2786,7 @@ static int __must_check arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
>  	if ( !is_domain_direct_mapped(d) )
>  		return -EINVAL;
>  
> -	guest_physmap_remove_page(d, gfn, gfn, 0);
> +	guest_physmap_remove_page(d, _gfn(gfn), _mfn(gfn), 0);
>  
>  	return 0;
>  }
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 75c65a8..0d1e61e 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -160,23 +160,23 @@ int map_dev_mmio_region(struct domain *d,
>                          unsigned long mfn);
>  
>  int guest_physmap_add_entry(struct domain *d,
> -                            unsigned long gfn,
> -                            unsigned long mfn,
> +                            gfn_t gfn,
> +                            mfn_t mfn,
>                              unsigned long page_order,
>                              p2m_type_t t);
>  
>  /* Untyped version for RAM only, for compatibility */
>  static inline int guest_physmap_add_page(struct domain *d,
> -                                         unsigned long gfn,
> -                                         unsigned long mfn,
> +                                         gfn_t gfn,
> +                                         mfn_t mfn,
>                                           unsigned int page_order)
>  {
>      return guest_physmap_add_entry(d, gfn, mfn, page_order, p2m_ram_rw);
>  }
>  
>  void guest_physmap_remove_page(struct domain *d,
> -                               unsigned long gpfn,
> -                               unsigned long mfn, unsigned int page_order);
> +                               gfn_t gfn,
> +                               mfn_t mfn, unsigned int page_order);
>  
>  mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn);
>  
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index 65675a2..4ab3574 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -545,14 +545,14 @@ void p2m_teardown(struct p2m_domain *p2m);
>  void p2m_final_teardown(struct domain *d);
>  
>  /* Add a page to a domain's p2m table */
> -int guest_physmap_add_entry(struct domain *d, unsigned long gfn,
> -                            unsigned long mfn, unsigned int page_order, 
> +int guest_physmap_add_entry(struct domain *d, gfn_t gfn,
> +                            mfn_t mfn, unsigned int page_order,
>                              p2m_type_t t);
>  
>  /* Untyped version for RAM only, for compatibility */
>  static inline int guest_physmap_add_page(struct domain *d,
> -                                         unsigned long gfn,
> -                                         unsigned long mfn,
> +                                         gfn_t gfn,
> +                                         mfn_t mfn,
>                                           unsigned int page_order)
>  {
>      return guest_physmap_add_entry(d, gfn, mfn, page_order, p2m_ram_rw);
> @@ -560,8 +560,7 @@ static inline int guest_physmap_add_page(struct domain *d,
>  
>  /* Remove a page from a domain's p2m table */
>  int guest_physmap_remove_page(struct domain *d,
> -                              unsigned long gfn,
> -                              unsigned long mfn, unsigned int page_order);
> +                              gfn_t gfn, mfn_t mfn, unsigned int page_order);
>  
>  /* Set a p2m range as populate-on-demand */
>  int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn,
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 13f706e..b62f473 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -552,7 +552,7 @@ int xenmem_add_to_physmap_one(struct domain *d, unsigned int space,
>  
>  /* Returns 1 on success, 0 on error, negative if the ring
>   * for event propagation is full in the presence of paging */
> -int guest_remove_page(struct domain *d, unsigned long gmfn);
> +int guest_remove_page(struct domain *d, unsigned long gfn);
>  
>  #define RAM_TYPE_CONVENTIONAL 0x00000001
>  #define RAM_TYPE_RESERVED     0x00000002
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 4/8] xen: Use typesafe gfn in xenmem_add_to_physmap_one

[Julien Grall]

The x86 version of the function xenmem_add_to_physmap_one contains
variable name gpfn and gfn which make the code very confusing.
I have left unchanged for now.

Also, rename gpfn to gfn in the ARM version as the latter is the correct
acronym for a guest physical frame.

Finally, remove the trailing whitespace around the changes.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

    Changes in v3:
        - Add Jan's acked-by for non-ARM bits
---
 xen/arch/arm/mm.c    | 10 +++++-----
 xen/arch/x86/mm.c    | 15 +++++++--------
 xen/common/memory.c  |  6 +++---
 xen/include/xen/mm.h |  2 +-
 4 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 5ab9b75..6882d54 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1046,7 +1046,7 @@ int xenmem_add_to_physmap_one(
     unsigned int space,
     union xen_add_to_physmap_batch_extra extra,
     unsigned long idx,
-    xen_pfn_t gpfn)
+    gfn_t gfn)
 {
     unsigned long mfn = 0;
     int rc;
@@ -1081,8 +1081,8 @@ int xenmem_add_to_physmap_one(
             else
                 return -EINVAL;
         }
-        
-        d->arch.grant_table_gpfn[idx] = gpfn;
+
+        d->arch.grant_table_gpfn[idx] = gfn_x(gfn);
 
         t = p2m_ram_rw;
 
@@ -1145,7 +1145,7 @@ int xenmem_add_to_physmap_one(
         if ( extra.res0 )
             return -EOPNOTSUPP;
 
-        rc = map_dev_mmio_region(d, gpfn, 1, idx);
+        rc = map_dev_mmio_region(d, gfn_x(gfn), 1, idx);
         return rc;
 
     default:
@@ -1153,7 +1153,7 @@ int xenmem_add_to_physmap_one(
     }
 
     /* Map at new location. */
-    rc = guest_physmap_add_entry(d, _gfn(gpfn), _mfn(mfn), 0, t);
+    rc = guest_physmap_add_entry(d, gfn, _mfn(mfn), 0, t);
 
     /* If we fail to add the mapping, we need to drop the reference we
      * took earlier on foreign pages */
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 7fbc94e..dbcf6cb 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4775,7 +4775,7 @@ int xenmem_add_to_physmap_one(
     unsigned int space,
     union xen_add_to_physmap_batch_extra extra,
     unsigned long idx,
-    xen_pfn_t gpfn)
+    gfn_t gpfn)
 {
     struct page_info *page = NULL;
     unsigned long gfn = 0; /* gcc ... */
@@ -4834,7 +4834,7 @@ int xenmem_add_to_physmap_one(
             break;
         }
         case XENMAPSPACE_gmfn_foreign:
-            return p2m_add_foreign(d, idx, gpfn, extra.foreign_domid);
+            return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid);
         default:
             break;
     }
@@ -4849,19 +4849,18 @@ int xenmem_add_to_physmap_one(
     }
 
     /* Remove previously mapped page if it was present. */
-    prev_mfn = mfn_x(get_gfn(d, gpfn, &p2mt));
+    prev_mfn = mfn_x(get_gfn(d, gfn_x(gpfn), &p2mt));
     if ( mfn_valid(prev_mfn) )
     {
         if ( is_xen_heap_mfn(prev_mfn) )
             /* Xen heap frames are simply unhooked from this phys slot. */
-            guest_physmap_remove_page(d, _gfn(gpfn), _mfn(prev_mfn),
-                                      PAGE_ORDER_4K);
+            guest_physmap_remove_page(d, gpfn, _mfn(prev_mfn), PAGE_ORDER_4K);
         else
             /* Normal domain memory is freed, to avoid leaking memory. */
-            guest_remove_page(d, gpfn);
+            guest_remove_page(d, gfn_x(gpfn));
     }
     /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */
-    put_gfn(d, gpfn);
+    put_gfn(d, gfn_x(gpfn));
 
     /* Unmap from old location, if any. */
     old_gpfn = get_gpfn_from_mfn(mfn);
@@ -4872,7 +4871,7 @@ int xenmem_add_to_physmap_one(
         guest_physmap_remove_page(d, _gfn(old_gpfn), _mfn(mfn), PAGE_ORDER_4K);
 
     /* Map at new location. */
-    rc = guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), PAGE_ORDER_4K);
+    rc = guest_physmap_add_page(d, gpfn, _mfn(mfn), PAGE_ORDER_4K);
 
     /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */
     if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
diff --git a/xen/common/memory.c b/xen/common/memory.c
index a8a75e0..812334b 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -649,7 +649,7 @@ static int xenmem_add_to_physmap(struct domain *d,
 
     if ( xatp->space != XENMAPSPACE_gmfn_range )
         return xenmem_add_to_physmap_one(d, xatp->space, extra,
-                                         xatp->idx, xatp->gpfn);
+                                         xatp->idx, _gfn(xatp->gpfn));
 
     if ( xatp->size < start )
         return -EILSEQ;
@@ -666,7 +666,7 @@ static int xenmem_add_to_physmap(struct domain *d,
     while ( xatp->size > done )
     {
         rc = xenmem_add_to_physmap_one(d, xatp->space, extra,
-                                       xatp->idx, xatp->gpfn);
+                                       xatp->idx, _gfn(xatp->gpfn));
         if ( rc < 0 )
             break;
 
@@ -735,7 +735,7 @@ static int xenmem_add_to_physmap_batch(struct domain *d,
 
         rc = xenmem_add_to_physmap_one(d, xatpb->space,
                                        xatpb->u,
-                                       idx, gpfn);
+                                       idx, _gfn(gpfn));
 
         if ( unlikely(__copy_to_guest_offset(xatpb->errs, 0, &rc, 1)) )
         {
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index b62f473..afbb1a1 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -548,7 +548,7 @@ void scrub_one_page(struct page_info *);
 
 int xenmem_add_to_physmap_one(struct domain *d, unsigned int space,
                               union xen_add_to_physmap_batch_extra extra,
-                              unsigned long idx, xen_pfn_t gpfn);
+                              unsigned long idx, gfn_t gfn);
 
 /* Returns 1 on success, 0 on error, negative if the ring
  * for event propagation is full in the presence of paging */
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 4/8] xen: Use typesafe gfn in xenmem_add_to_physmap_one

[Stefano Stabellini]

On Tue, 21 Jun 2016, Julien Grall wrote:
> The x86 version of the function xenmem_add_to_physmap_one contains
> variable name gpfn and gfn which make the code very confusing.
> I have left unchanged for now.
> 
> Also, rename gpfn to gfn in the ARM version as the latter is the correct
> acronym for a guest physical frame.
> 
> Finally, remove the trailing whitespace around the changes.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> 
>     Changes in v3:
>         - Add Jan's acked-by for non-ARM bits
> ---
>  xen/arch/arm/mm.c    | 10 +++++-----
>  xen/arch/x86/mm.c    | 15 +++++++--------
>  xen/common/memory.c  |  6 +++---
>  xen/include/xen/mm.h |  2 +-
>  4 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 5ab9b75..6882d54 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1046,7 +1046,7 @@ int xenmem_add_to_physmap_one(
>      unsigned int space,
>      union xen_add_to_physmap_batch_extra extra,
>      unsigned long idx,
> -    xen_pfn_t gpfn)
> +    gfn_t gfn)

I think there is a possible loss of information here: xen_pfn_t is
always uint64_t on both ARM and ARM64, while gfn_t is unsigned long with
its current definition. Or am I missing something?

In fact, given that ARM supports LPAE, shouldn't gfn by defined as
xen_pfn_t in terms of storage size (TYPE_SAFE(xen_pfn_t, gfn)) ?


>  {
>      unsigned long mfn = 0;
>      int rc;
> @@ -1081,8 +1081,8 @@ int xenmem_add_to_physmap_one(
>              else
>                  return -EINVAL;
>          }
> -        
> -        d->arch.grant_table_gpfn[idx] = gpfn;
> +
> +        d->arch.grant_table_gpfn[idx] = gfn_x(gfn);

Similarly grant_table_gpfn is an array of xen_pfn_t (uint64_t), while
gfn_x unboxes to the storage size of gfn (unsigned long).


>          t = p2m_ram_rw;
>  
> @@ -1145,7 +1145,7 @@ int xenmem_add_to_physmap_one(
>          if ( extra.res0 )
>              return -EOPNOTSUPP;
>  
> -        rc = map_dev_mmio_region(d, gpfn, 1, idx);
> +        rc = map_dev_mmio_region(d, gfn_x(gfn), 1, idx);
>          return rc;
>  
>      default:
> @@ -1153,7 +1153,7 @@ int xenmem_add_to_physmap_one(
>      }
>  
>      /* Map at new location. */
> -    rc = guest_physmap_add_entry(d, _gfn(gpfn), _mfn(mfn), 0, t);
> +    rc = guest_physmap_add_entry(d, gfn, _mfn(mfn), 0, t);
>  
>      /* If we fail to add the mapping, we need to drop the reference we
>       * took earlier on foreign pages */
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 7fbc94e..dbcf6cb 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4775,7 +4775,7 @@ int xenmem_add_to_physmap_one(
>      unsigned int space,
>      union xen_add_to_physmap_batch_extra extra,
>      unsigned long idx,
> -    xen_pfn_t gpfn)
> +    gfn_t gpfn)
>  {
>      struct page_info *page = NULL;
>      unsigned long gfn = 0; /* gcc ... */
> @@ -4834,7 +4834,7 @@ int xenmem_add_to_physmap_one(
>              break;
>          }
>          case XENMAPSPACE_gmfn_foreign:
> -            return p2m_add_foreign(d, idx, gpfn, extra.foreign_domid);
> +            return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid);
>          default:
>              break;
>      }
> @@ -4849,19 +4849,18 @@ int xenmem_add_to_physmap_one(
>      }
>  
>      /* Remove previously mapped page if it was present. */
> -    prev_mfn = mfn_x(get_gfn(d, gpfn, &p2mt));
> +    prev_mfn = mfn_x(get_gfn(d, gfn_x(gpfn), &p2mt));
>      if ( mfn_valid(prev_mfn) )
>      {
>          if ( is_xen_heap_mfn(prev_mfn) )
>              /* Xen heap frames are simply unhooked from this phys slot. */
> -            guest_physmap_remove_page(d, _gfn(gpfn), _mfn(prev_mfn),
> -                                      PAGE_ORDER_4K);
> +            guest_physmap_remove_page(d, gpfn, _mfn(prev_mfn), PAGE_ORDER_4K);
>          else
>              /* Normal domain memory is freed, to avoid leaking memory. */
> -            guest_remove_page(d, gpfn);
> +            guest_remove_page(d, gfn_x(gpfn));
>      }
>      /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */
> -    put_gfn(d, gpfn);
> +    put_gfn(d, gfn_x(gpfn));
>  
>      /* Unmap from old location, if any. */
>      old_gpfn = get_gpfn_from_mfn(mfn);
> @@ -4872,7 +4871,7 @@ int xenmem_add_to_physmap_one(
>          guest_physmap_remove_page(d, _gfn(old_gpfn), _mfn(mfn), PAGE_ORDER_4K);
>  
>      /* Map at new location. */
> -    rc = guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), PAGE_ORDER_4K);
> +    rc = guest_physmap_add_page(d, gpfn, _mfn(mfn), PAGE_ORDER_4K);
>  
>      /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */
>      if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index a8a75e0..812334b 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -649,7 +649,7 @@ static int xenmem_add_to_physmap(struct domain *d,
>  
>      if ( xatp->space != XENMAPSPACE_gmfn_range )
>          return xenmem_add_to_physmap_one(d, xatp->space, extra,
> -                                         xatp->idx, xatp->gpfn);
> +                                         xatp->idx, _gfn(xatp->gpfn));
>  
>      if ( xatp->size < start )
>          return -EILSEQ;
> @@ -666,7 +666,7 @@ static int xenmem_add_to_physmap(struct domain *d,
>      while ( xatp->size > done )
>      {
>          rc = xenmem_add_to_physmap_one(d, xatp->space, extra,
> -                                       xatp->idx, xatp->gpfn);
> +                                       xatp->idx, _gfn(xatp->gpfn));
>          if ( rc < 0 )
>              break;
>  
> @@ -735,7 +735,7 @@ static int xenmem_add_to_physmap_batch(struct domain *d,
>  
>          rc = xenmem_add_to_physmap_one(d, xatpb->space,
>                                         xatpb->u,
> -                                       idx, gpfn);
> +                                       idx, _gfn(gpfn));
>  
>          if ( unlikely(__copy_to_guest_offset(xatpb->errs, 0, &rc, 1)) )
>          {
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index b62f473..afbb1a1 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -548,7 +548,7 @@ void scrub_one_page(struct page_info *);
>  
>  int xenmem_add_to_physmap_one(struct domain *d, unsigned int space,
>                                union xen_add_to_physmap_batch_extra extra,
> -                              unsigned long idx, xen_pfn_t gpfn);
> +                              unsigned long idx, gfn_t gfn);
>  
>  /* Returns 1 on success, 0 on error, negative if the ring
>   * for event propagation is full in the presence of paging */
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 4/8] xen: Use typesafe gfn in xenmem_add_to_physmap_one

[Julien Grall]

Hi Stefano,

On 23/06/16 11:20, Stefano Stabellini wrote:
> On Tue, 21 Jun 2016, Julien Grall wrote:
>> The x86 version of the function xenmem_add_to_physmap_one contains
>> variable name gpfn and gfn which make the code very confusing.
>> I have left unchanged for now.
>>
>> Also, rename gpfn to gfn in the ARM version as the latter is the correct
>> acronym for a guest physical frame.
>>
>> Finally, remove the trailing whitespace around the changes.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Tim Deegan <tim@xen.org>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>>
>>      Changes in v3:
>>          - Add Jan's acked-by for non-ARM bits
>> ---
>>   xen/arch/arm/mm.c    | 10 +++++-----
>>   xen/arch/x86/mm.c    | 15 +++++++--------
>>   xen/common/memory.c  |  6 +++---
>>   xen/include/xen/mm.h |  2 +-
>>   4 files changed, 16 insertions(+), 17 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 5ab9b75..6882d54 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -1046,7 +1046,7 @@ int xenmem_add_to_physmap_one(
>>       unsigned int space,
>>       union xen_add_to_physmap_batch_extra extra,
>>       unsigned long idx,
>> -    xen_pfn_t gpfn)
>> +    gfn_t gfn)
>
> I think there is a possible loss of information here: xen_pfn_t is
> always uint64_t on both ARM and ARM64, while gfn_t is unsigned long with
> its current definition. Or am I missing something?
>
> In fact, given that ARM supports LPAE, shouldn't gfn by defined as
> xen_pfn_t in terms of storage size (TYPE_SAFE(xen_pfn_t, gfn)) ?

With LPAE, ARM32 supports up to 40-bit PA so the frame will be encoded 
on 28-bit. So unsigned long is perfectly fine here.

Note that the truncation already occurs in subsequent caller because we 
are using unsigned long for the P2M parameters.

>
>
>>   {
>>       unsigned long mfn = 0;
>>       int rc;
>> @@ -1081,8 +1081,8 @@ int xenmem_add_to_physmap_one(
>>               else
>>                   return -EINVAL;
>>           }
>> -
>> -        d->arch.grant_table_gpfn[idx] = gpfn;
>> +
>> +        d->arch.grant_table_gpfn[idx] = gfn_x(gfn);
>
> Similarly grant_table_gpfn is an array of xen_pfn_t (uint64_t), while
> gfn_x unboxes to the storage size of gfn (unsigned long).

Patch #5 will switch grant_table_gpfn to an array of gfn_t.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 4/8] xen: Use typesafe gfn in xenmem_add_to_physmap_one

[Stefano Stabellini]

On Thu, 23 Jun 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 23/06/16 11:20, Stefano Stabellini wrote:
> > On Tue, 21 Jun 2016, Julien Grall wrote:
> > > The x86 version of the function xenmem_add_to_physmap_one contains
> > > variable name gpfn and gfn which make the code very confusing.
> > > I have left unchanged for now.
> > > 
> > > Also, rename gpfn to gfn in the ARM version as the latter is the correct
> > > acronym for a guest physical frame.
> > > 
> > > Finally, remove the trailing whitespace around the changes.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > Acked-by: Jan Beulich <jbeulich@suse.com>
> > > 
> > > ---
> > > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > > Cc: Jan Beulich <jbeulich@suse.com>
> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > Cc: George Dunlap <george.dunlap@eu.citrix.com>
> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > Cc: Tim Deegan <tim@xen.org>
> > > Cc: Wei Liu <wei.liu2@citrix.com>
> > > 
> > >      Changes in v3:
> > >          - Add Jan's acked-by for non-ARM bits
> > > ---
> > >   xen/arch/arm/mm.c    | 10 +++++-----
> > >   xen/arch/x86/mm.c    | 15 +++++++--------
> > >   xen/common/memory.c  |  6 +++---
> > >   xen/include/xen/mm.h |  2 +-
> > >   4 files changed, 16 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index 5ab9b75..6882d54 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c
> > > @@ -1046,7 +1046,7 @@ int xenmem_add_to_physmap_one(
> > >       unsigned int space,
> > >       union xen_add_to_physmap_batch_extra extra,
> > >       unsigned long idx,
> > > -    xen_pfn_t gpfn)
> > > +    gfn_t gfn)
> > 
> > I think there is a possible loss of information here: xen_pfn_t is
> > always uint64_t on both ARM and ARM64, while gfn_t is unsigned long with
> > its current definition. Or am I missing something?
> > 
> > In fact, given that ARM supports LPAE, shouldn't gfn by defined as
> > xen_pfn_t in terms of storage size (TYPE_SAFE(xen_pfn_t, gfn)) ?
> 
> With LPAE, ARM32 supports up to 40-bit PA so the frame will be encoded on
> 28-bit. So unsigned long is perfectly fine here.

Somehow I have always assumed that the 40-bit limitation was just
temporary. That ARM in the future will just increase that number up to
64-bit eventually.

If you don't think there is any risk of that happening, then I am fine
with this. We just have to keep in mind that many of the guest
interfaces use xen_pfn_t, which has a different size on ARM.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 4/8] xen: Use typesafe gfn in xenmem_add_to_physmap_one

[Julien Grall]

On 23/06/16 14:06, Stefano Stabellini wrote:
> On Thu, 23 Jun 2016, Julien Grall wrote:
>> On 23/06/16 11:20, Stefano Stabellini wrote:
>>> On Tue, 21 Jun 2016, Julien Grall wrote:
>>> I think there is a possible loss of information here: xen_pfn_t is
>>> always uint64_t on both ARM and ARM64, while gfn_t is unsigned long with
>>> its current definition. Or am I missing something?
>>>
>>> In fact, given that ARM supports LPAE, shouldn't gfn by defined as
>>> xen_pfn_t in terms of storage size (TYPE_SAFE(xen_pfn_t, gfn)) ?
>>
>> With LPAE, ARM32 supports up to 40-bit PA so the frame will be encoded on
>> 28-bit. So unsigned long is perfectly fine here.
>
> Somehow I have always assumed that the 40-bit limitation was just
> temporary. That ARM in the future will just increase that number up to
> 64-bit eventually.
>
> If you don't think there is any risk of that happening, then I am fine
> with this. We just have to keep in mind that many of the guest
> interfaces use xen_pfn_t, which has a different size on ARM.

Currently, Aarch32 supports up to 40-bit whilst Aarch64 supports up to 
48-bit (even 52-bit with ARMv8.2). So this should be ok for now.

However, pretty much everywhere in Xen we assume that the frame number 
is unsigned long (see mm.c, p2m.c,...). We would have much more work to 
do than this small patch.

I would rather start to switch to gfn/mfn internally and keep the 
underlying type as "unsigned long" until we effectively need 64-bit frame.

The main reason is 64-bit frame will result into a bigger binary for 
ARM32 with no apparent reason (40-bit is hardcoded in 
setup_virt_paging). Switching to gfn/mfn will allow us to uint64_t where 
it will be required.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 4/8] xen: Use typesafe gfn in xenmem_add_to_physmap_one

[Stefano Stabellini]

On Thu, 23 Jun 2016, Julien Grall wrote:
> On 23/06/16 14:06, Stefano Stabellini wrote:
> > On Thu, 23 Jun 2016, Julien Grall wrote:
> > > On 23/06/16 11:20, Stefano Stabellini wrote:
> > > > On Tue, 21 Jun 2016, Julien Grall wrote:
> > > > I think there is a possible loss of information here: xen_pfn_t is
> > > > always uint64_t on both ARM and ARM64, while gfn_t is unsigned long with
> > > > its current definition. Or am I missing something?
> > > > 
> > > > In fact, given that ARM supports LPAE, shouldn't gfn by defined as
> > > > xen_pfn_t in terms of storage size (TYPE_SAFE(xen_pfn_t, gfn)) ?
> > > 
> > > With LPAE, ARM32 supports up to 40-bit PA so the frame will be encoded on
> > > 28-bit. So unsigned long is perfectly fine here.
> > 
> > Somehow I have always assumed that the 40-bit limitation was just
> > temporary. That ARM in the future will just increase that number up to
> > 64-bit eventually.
> > 
> > If you don't think there is any risk of that happening, then I am fine
> > with this. We just have to keep in mind that many of the guest
> > interfaces use xen_pfn_t, which has a different size on ARM.
> 
> Currently, Aarch32 supports up to 40-bit whilst Aarch64 supports up to 48-bit
> (even 52-bit with ARMv8.2). So this should be ok for now.
> 
> However, pretty much everywhere in Xen we assume that the frame number is
> unsigned long (see mm.c, p2m.c,...). We would have much more work to do than
> this small patch.
> 
> I would rather start to switch to gfn/mfn internally and keep the underlying
> type as "unsigned long" until we effectively need 64-bit frame.
> 
> The main reason is 64-bit frame will result into a bigger binary for ARM32
> with no apparent reason (40-bit is hardcoded in setup_virt_paging). Switching
> to gfn/mfn will allow us to uint64_t where it will be required.

OK.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 4/8] xen: Use typesafe gfn in xenmem_add_to_physmap_one

[Stefano Stabellini]

On Tue, 21 Jun 2016, Julien Grall wrote:
> The x86 version of the function xenmem_add_to_physmap_one contains
> variable name gpfn and gfn which make the code very confusing.
> I have left unchanged for now.
> 
> Also, rename gpfn to gfn in the ARM version as the latter is the correct
> acronym for a guest physical frame.
> 
> Finally, remove the trailing whitespace around the changes.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> 
>     Changes in v3:
>         - Add Jan's acked-by for non-ARM bits
> ---
>  xen/arch/arm/mm.c    | 10 +++++-----
>  xen/arch/x86/mm.c    | 15 +++++++--------
>  xen/common/memory.c  |  6 +++---
>  xen/include/xen/mm.h |  2 +-
>  4 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 5ab9b75..6882d54 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1046,7 +1046,7 @@ int xenmem_add_to_physmap_one(
>      unsigned int space,
>      union xen_add_to_physmap_batch_extra extra,
>      unsigned long idx,
> -    xen_pfn_t gpfn)
> +    gfn_t gfn)
>  {
>      unsigned long mfn = 0;
>      int rc;
> @@ -1081,8 +1081,8 @@ int xenmem_add_to_physmap_one(
>              else
>                  return -EINVAL;
>          }
> -        
> -        d->arch.grant_table_gpfn[idx] = gpfn;
> +
> +        d->arch.grant_table_gpfn[idx] = gfn_x(gfn);
>  
>          t = p2m_ram_rw;
>  
> @@ -1145,7 +1145,7 @@ int xenmem_add_to_physmap_one(
>          if ( extra.res0 )
>              return -EOPNOTSUPP;
>  
> -        rc = map_dev_mmio_region(d, gpfn, 1, idx);
> +        rc = map_dev_mmio_region(d, gfn_x(gfn), 1, idx);
>          return rc;
>  
>      default:
> @@ -1153,7 +1153,7 @@ int xenmem_add_to_physmap_one(
>      }
>  
>      /* Map at new location. */
> -    rc = guest_physmap_add_entry(d, _gfn(gpfn), _mfn(mfn), 0, t);
> +    rc = guest_physmap_add_entry(d, gfn, _mfn(mfn), 0, t);
>  
>      /* If we fail to add the mapping, we need to drop the reference we
>       * took earlier on foreign pages */
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 7fbc94e..dbcf6cb 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4775,7 +4775,7 @@ int xenmem_add_to_physmap_one(
>      unsigned int space,
>      union xen_add_to_physmap_batch_extra extra,
>      unsigned long idx,
> -    xen_pfn_t gpfn)
> +    gfn_t gpfn)
>  {
>      struct page_info *page = NULL;
>      unsigned long gfn = 0; /* gcc ... */
> @@ -4834,7 +4834,7 @@ int xenmem_add_to_physmap_one(
>              break;
>          }
>          case XENMAPSPACE_gmfn_foreign:
> -            return p2m_add_foreign(d, idx, gpfn, extra.foreign_domid);
> +            return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid);
>          default:
>              break;
>      }
> @@ -4849,19 +4849,18 @@ int xenmem_add_to_physmap_one(
>      }
>  
>      /* Remove previously mapped page if it was present. */
> -    prev_mfn = mfn_x(get_gfn(d, gpfn, &p2mt));
> +    prev_mfn = mfn_x(get_gfn(d, gfn_x(gpfn), &p2mt));
>      if ( mfn_valid(prev_mfn) )
>      {
>          if ( is_xen_heap_mfn(prev_mfn) )
>              /* Xen heap frames are simply unhooked from this phys slot. */
> -            guest_physmap_remove_page(d, _gfn(gpfn), _mfn(prev_mfn),
> -                                      PAGE_ORDER_4K);
> +            guest_physmap_remove_page(d, gpfn, _mfn(prev_mfn), PAGE_ORDER_4K);
>          else
>              /* Normal domain memory is freed, to avoid leaking memory. */
> -            guest_remove_page(d, gpfn);
> +            guest_remove_page(d, gfn_x(gpfn));
>      }
>      /* In the XENMAPSPACE_gmfn case we still hold a ref on the old page. */
> -    put_gfn(d, gpfn);
> +    put_gfn(d, gfn_x(gpfn));
>  
>      /* Unmap from old location, if any. */
>      old_gpfn = get_gpfn_from_mfn(mfn);
> @@ -4872,7 +4871,7 @@ int xenmem_add_to_physmap_one(
>          guest_physmap_remove_page(d, _gfn(old_gpfn), _mfn(mfn), PAGE_ORDER_4K);
>  
>      /* Map at new location. */
> -    rc = guest_physmap_add_page(d, _gfn(gpfn), _mfn(mfn), PAGE_ORDER_4K);
> +    rc = guest_physmap_add_page(d, gpfn, _mfn(mfn), PAGE_ORDER_4K);
>  
>      /* In the XENMAPSPACE_gmfn, we took a ref of the gfn at the top */
>      if ( space == XENMAPSPACE_gmfn || space == XENMAPSPACE_gmfn_range )
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index a8a75e0..812334b 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -649,7 +649,7 @@ static int xenmem_add_to_physmap(struct domain *d,
>  
>      if ( xatp->space != XENMAPSPACE_gmfn_range )
>          return xenmem_add_to_physmap_one(d, xatp->space, extra,
> -                                         xatp->idx, xatp->gpfn);
> +                                         xatp->idx, _gfn(xatp->gpfn));
>  
>      if ( xatp->size < start )
>          return -EILSEQ;
> @@ -666,7 +666,7 @@ static int xenmem_add_to_physmap(struct domain *d,
>      while ( xatp->size > done )
>      {
>          rc = xenmem_add_to_physmap_one(d, xatp->space, extra,
> -                                       xatp->idx, xatp->gpfn);
> +                                       xatp->idx, _gfn(xatp->gpfn));
>          if ( rc < 0 )
>              break;
>  
> @@ -735,7 +735,7 @@ static int xenmem_add_to_physmap_batch(struct domain *d,
>  
>          rc = xenmem_add_to_physmap_one(d, xatpb->space,
>                                         xatpb->u,
> -                                       idx, gpfn);
> +                                       idx, _gfn(gpfn));
>  
>          if ( unlikely(__copy_to_guest_offset(xatpb->errs, 0, &rc, 1)) )
>          {
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index b62f473..afbb1a1 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -548,7 +548,7 @@ void scrub_one_page(struct page_info *);
>  
>  int xenmem_add_to_physmap_one(struct domain *d, unsigned int space,
>                                union xen_add_to_physmap_batch_extra extra,
> -                              unsigned long idx, xen_pfn_t gpfn);
> +                              unsigned long idx, gfn_t gfn);
>  
>  /* Returns 1 on success, 0 on error, negative if the ring
>   * for event propagation is full in the presence of paging */
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 5/8] xen/arm: Rename grant_table_gfpn into grant_table_gfn and use the typesafe gfn

[Julien Grall]

The correct acronym for a guest physical frame is gfn. Also use
the typesafe gfn to ensure that a guest frame is effectively used.

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

---
    Changes in v2:
        - Remove extra pair of brackets.
---
 xen/arch/arm/domain.c             | 4 ++--
 xen/arch/arm/mm.c                 | 2 +-
 xen/include/asm-arm/domain.h      | 2 +-
 xen/include/asm-arm/grant_table.h | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index d8a804c..6ce4645 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -464,13 +464,13 @@ struct domain *alloc_domain_struct(void)
         return NULL;
 
     clear_page(d);
-    d->arch.grant_table_gpfn = xzalloc_array(xen_pfn_t, max_grant_frames);
+    d->arch.grant_table_gfn = xzalloc_array(gfn_t, max_grant_frames);
     return d;
 }
 
 void free_domain_struct(struct domain *d)
 {
-    xfree(d->arch.grant_table_gpfn);
+    xfree(d->arch.grant_table_gfn);
     free_xenheap_page(d);
 }
 
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 6882d54..0e408f8 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1082,7 +1082,7 @@ int xenmem_add_to_physmap_one(
                 return -EINVAL;
         }
 
-        d->arch.grant_table_gpfn[idx] = gfn_x(gfn);
+        d->arch.grant_table_gfn[idx] = gfn;
 
         t = p2m_ram_rw;
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 370cdeb..979f7de 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -51,7 +51,7 @@ struct arch_domain
     uint64_t vttbr;
 
     struct hvm_domain hvm_domain;
-    xen_pfn_t *grant_table_gpfn;
+    gfn_t *grant_table_gfn;
 
     struct vmmio vmmio;
 
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 5e076cc..eb02423 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -30,7 +30,7 @@ static inline int replace_grant_supported(void)
 
 #define gnttab_shared_gmfn(d, t, i)                                      \
     ( ((i >= nr_grant_frames(d->grant_table)) &&                         \
-     (i < max_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i]))
+     (i < max_grant_frames)) ? 0 : gfn_x(d->arch.grant_table_gfn[i]))
 
 #define gnttab_need_iommu_mapping(d)                    \
     (is_domain_direct_mapped(d) && need_iommu(d))
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 5/8] xen/arm: Rename grant_table_gfpn into grant_table_gfn and use the typesafe gfn

[Stefano Stabellini]

On Tue, 21 Jun 2016, Julien Grall wrote:
> The correct acronym for a guest physical frame is gfn. Also use
> the typesafe gfn to ensure that a guest frame is effectively used.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Remove extra pair of brackets.
> ---
>  xen/arch/arm/domain.c             | 4 ++--
>  xen/arch/arm/mm.c                 | 2 +-
>  xen/include/asm-arm/domain.h      | 2 +-
>  xen/include/asm-arm/grant_table.h | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index d8a804c..6ce4645 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -464,13 +464,13 @@ struct domain *alloc_domain_struct(void)
>          return NULL;
>  
>      clear_page(d);
> -    d->arch.grant_table_gpfn = xzalloc_array(xen_pfn_t, max_grant_frames);
> +    d->arch.grant_table_gfn = xzalloc_array(gfn_t, max_grant_frames);
>      return d;
>  }
>  
>  void free_domain_struct(struct domain *d)
>  {
> -    xfree(d->arch.grant_table_gpfn);
> +    xfree(d->arch.grant_table_gfn);
>      free_xenheap_page(d);
>  }
>  
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 6882d54..0e408f8 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1082,7 +1082,7 @@ int xenmem_add_to_physmap_one(
>                  return -EINVAL;
>          }
>  
> -        d->arch.grant_table_gpfn[idx] = gfn_x(gfn);
> +        d->arch.grant_table_gfn[idx] = gfn;
>  
>          t = p2m_ram_rw;
>  
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 370cdeb..979f7de 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -51,7 +51,7 @@ struct arch_domain
>      uint64_t vttbr;
>  
>      struct hvm_domain hvm_domain;
> -    xen_pfn_t *grant_table_gpfn;
> +    gfn_t *grant_table_gfn;
>  
>      struct vmmio vmmio;
>  
> diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
> index 5e076cc..eb02423 100644
> --- a/xen/include/asm-arm/grant_table.h
> +++ b/xen/include/asm-arm/grant_table.h
> @@ -30,7 +30,7 @@ static inline int replace_grant_supported(void)
>  
>  #define gnttab_shared_gmfn(d, t, i)                                      \
>      ( ((i >= nr_grant_frames(d->grant_table)) &&                         \
> -     (i < max_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i]))
> +     (i < max_grant_frames)) ? 0 : gfn_x(d->arch.grant_table_gfn[i]))
>  
>  #define gnttab_need_iommu_mapping(d)                    \
>      (is_domain_direct_mapped(d) && need_iommu(d))
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 6/8] xen: Use the typesafe mfn and gfn in map_mmio_regions...

[Julien Grall]

to avoid mixing machine frame with guest frame.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

    Changes in v3:
        - Use mfn_add when it is possible
        - Add Jan's acked-by
---
 xen/arch/arm/domain_build.c      |  4 ++--
 xen/arch/arm/gic-v2.c            |  4 ++--
 xen/arch/arm/p2m.c               | 22 +++++++++++-----------
 xen/arch/arm/platforms/exynos5.c |  8 ++++----
 xen/arch/arm/platforms/omap5.c   | 16 ++++++++--------
 xen/arch/arm/vgic-v2.c           |  4 ++--
 xen/arch/x86/mm/p2m.c            | 18 ++++++++++--------
 xen/common/domctl.c              |  4 ++--
 xen/include/xen/p2m-common.h     |  8 ++++----
 9 files changed, 45 insertions(+), 43 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 9035486..49185f0 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1036,9 +1036,9 @@ static int map_range_to_domain(const struct dt_device_node *dev,
     if ( need_mapping )
     {
         res = map_mmio_regions(d,
-                               paddr_to_pfn(addr),
+                               _gfn(paddr_to_pfn(addr)),
                                DIV_ROUND_UP(len, PAGE_SIZE),
-                               paddr_to_pfn(addr));
+                               _mfn(paddr_to_pfn(addr)));
         if ( res < 0 )
         {
             printk(XENLOG_ERR "Unable to map 0x%"PRIx64
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 4e2f4c7..3893ece 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -601,9 +601,9 @@ static int gicv2_map_hwdown_extra_mappings(struct domain *d)
                d->domain_id, v2m_data->addr, v2m_data->size,
                v2m_data->spi_start, v2m_data->nr_spis);
 
-        ret = map_mmio_regions(d, paddr_to_pfn(v2m_data->addr),
+        ret = map_mmio_regions(d, _gfn(paddr_to_pfn(v2m_data->addr)),
                             DIV_ROUND_UP(v2m_data->size, PAGE_SIZE),
-                            paddr_to_pfn(v2m_data->addr));
+                            _mfn(paddr_to_pfn(v2m_data->addr)));
         if ( ret )
         {
             printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n",
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index aa4e774..47cb383 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1245,27 +1245,27 @@ int unmap_regions_rw_cache(struct domain *d,
 }
 
 int map_mmio_regions(struct domain *d,
-                     unsigned long start_gfn,
+                     gfn_t start_gfn,
                      unsigned long nr,
-                     unsigned long mfn)
+                     mfn_t mfn)
 {
     return apply_p2m_changes(d, INSERT,
-                             pfn_to_paddr(start_gfn),
-                             pfn_to_paddr(start_gfn + nr),
-                             pfn_to_paddr(mfn),
+                             pfn_to_paddr(gfn_x(start_gfn)),
+                             pfn_to_paddr(gfn_x(start_gfn) + nr),
+                             pfn_to_paddr(mfn_x(mfn)),
                              MATTR_DEV, 0, p2m_mmio_direct,
                              d->arch.p2m.default_access);
 }
 
 int unmap_mmio_regions(struct domain *d,
-                       unsigned long start_gfn,
+                       gfn_t start_gfn,
                        unsigned long nr,
-                       unsigned long mfn)
+                       mfn_t mfn)
 {
     return apply_p2m_changes(d, REMOVE,
-                             pfn_to_paddr(start_gfn),
-                             pfn_to_paddr(start_gfn + nr),
-                             pfn_to_paddr(mfn),
+                             pfn_to_paddr(gfn_x(start_gfn)),
+                             pfn_to_paddr(gfn_x(start_gfn) + nr),
+                             pfn_to_paddr(mfn_x(mfn)),
                              MATTR_DEV, 0, p2m_invalid,
                              d->arch.p2m.default_access);
 }
@@ -1280,7 +1280,7 @@ int map_dev_mmio_region(struct domain *d,
     if ( !(nr && iomem_access_permitted(d, start_gfn, start_gfn + nr - 1)) )
         return 0;
 
-    res = map_mmio_regions(d, start_gfn, nr, mfn);
+    res = map_mmio_regions(d, _gfn(start_gfn), nr, _mfn(mfn));
     if ( res < 0 )
     {
         printk(XENLOG_G_ERR "Unable to map [%#lx - %#lx] in Dom%d\n",
diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index bf4964d..c43934f 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -83,12 +83,12 @@ static int exynos5_init_time(void)
 static int exynos5250_specific_mapping(struct domain *d)
 {
     /* Map the chip ID */
-    map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_CHIPID), 1,
-                     paddr_to_pfn(EXYNOS5_PA_CHIPID));
+    map_mmio_regions(d, _gfn(paddr_to_pfn(EXYNOS5_PA_CHIPID)), 1,
+                     _mfn(paddr_to_pfn(EXYNOS5_PA_CHIPID)));
 
     /* Map the PWM region */
-    map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_TIMER), 2,
-                     paddr_to_pfn(EXYNOS5_PA_TIMER));
+    map_mmio_regions(d, _gfn(paddr_to_pfn(EXYNOS5_PA_TIMER)), 2,
+                     _mfn(paddr_to_pfn(EXYNOS5_PA_TIMER)));
 
     return 0;
 }
diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
index a49ba62..539588e 100644
--- a/xen/arch/arm/platforms/omap5.c
+++ b/xen/arch/arm/platforms/omap5.c
@@ -102,20 +102,20 @@ static int omap5_init_time(void)
 static int omap5_specific_mapping(struct domain *d)
 {
     /* Map the PRM module */
-    map_mmio_regions(d, paddr_to_pfn(OMAP5_PRM_BASE), 2,
-                     paddr_to_pfn(OMAP5_PRM_BASE));
+    map_mmio_regions(d, _gfn(paddr_to_pfn(OMAP5_PRM_BASE)), 2,
+                     _mfn(paddr_to_pfn(OMAP5_PRM_BASE)));
 
     /* Map the PRM_MPU */
-    map_mmio_regions(d, paddr_to_pfn(OMAP5_PRCM_MPU_BASE), 1,
-                     paddr_to_pfn(OMAP5_PRCM_MPU_BASE));
+    map_mmio_regions(d, _gfn(paddr_to_pfn(OMAP5_PRCM_MPU_BASE)), 1,
+                     _mfn(paddr_to_pfn(OMAP5_PRCM_MPU_BASE)));
 
     /* Map the Wakeup Gen */
-    map_mmio_regions(d, paddr_to_pfn(OMAP5_WKUPGEN_BASE), 1,
-                     paddr_to_pfn(OMAP5_WKUPGEN_BASE));
+    map_mmio_regions(d, _gfn(paddr_to_pfn(OMAP5_WKUPGEN_BASE)), 1,
+                     _mfn(paddr_to_pfn(OMAP5_WKUPGEN_BASE)));
 
     /* Map the on-chip SRAM */
-    map_mmio_regions(d, paddr_to_pfn(OMAP5_SRAM_PA), 32,
-                     paddr_to_pfn(OMAP5_SRAM_PA));
+    map_mmio_regions(d, _gfn(paddr_to_pfn(OMAP5_SRAM_PA)), 32,
+                     _mfn(paddr_to_pfn(OMAP5_SRAM_PA)));
 
     return 0;
 }
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 9adb4a9..cbe61cf 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -688,8 +688,8 @@ static int vgic_v2_domain_init(struct domain *d)
      * Map the gic virtual cpu interface in the gic cpu interface
      * region of the guest.
      */
-    ret = map_mmio_regions(d, paddr_to_pfn(cbase), csize / PAGE_SIZE,
-                           paddr_to_pfn(vbase));
+    ret = map_mmio_regions(d, _gfn(paddr_to_pfn(cbase)), csize / PAGE_SIZE,
+                           _mfn(paddr_to_pfn(vbase)));
     if ( ret )
         return ret;
 
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 16733a4..6258a5b 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2214,9 +2214,9 @@ static unsigned int mmio_order(const struct domain *d,
 #define MAP_MMIO_MAX_ITER 64 /* pretty arbitrary */
 
 int map_mmio_regions(struct domain *d,
-                     unsigned long start_gfn,
+                     gfn_t start_gfn,
                      unsigned long nr,
-                     unsigned long mfn)
+                     mfn_t mfn)
 {
     int ret = 0;
     unsigned long i;
@@ -2229,10 +2229,11 @@ int map_mmio_regions(struct domain *d,
           i += 1UL << order, ++iter )
     {
         /* OR'ing gfn and mfn values will return an order suitable to both. */
-        for ( order = mmio_order(d, (start_gfn + i) | (mfn + i), nr - i); ;
+        for ( order = mmio_order(d, (gfn_x(start_gfn) + i) | (mfn_x(mfn) + i), nr - i); ;
               order = ret - 1 )
         {
-            ret = set_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i), order,
+            ret = set_mmio_p2m_entry(d, gfn_x(start_gfn) + i,
+                                     mfn_add(mfn, i), order,
                                      p2m_get_hostp2m(d)->default_access);
             if ( ret <= 0 )
                 break;
@@ -2246,9 +2247,9 @@ int map_mmio_regions(struct domain *d,
 }
 
 int unmap_mmio_regions(struct domain *d,
-                       unsigned long start_gfn,
+                       gfn_t start_gfn,
                        unsigned long nr,
-                       unsigned long mfn)
+                       mfn_t mfn)
 {
     int ret = 0;
     unsigned long i;
@@ -2261,10 +2262,11 @@ int unmap_mmio_regions(struct domain *d,
           i += 1UL << order, ++iter )
     {
         /* OR'ing gfn and mfn values will return an order suitable to both. */
-        for ( order = mmio_order(d, (start_gfn + i) | (mfn + i), nr - i); ;
+        for ( order = mmio_order(d, (gfn_x(start_gfn) + i) | (mfn_x(mfn) + i), nr - i); ;
               order = ret - 1 )
         {
-            ret = clear_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i), order);
+            ret = clear_mmio_p2m_entry(d, gfn_x(start_gfn) + i,
+                                       mfn_add(mfn, i), order);
             if ( ret <= 0 )
                 break;
             ASSERT(ret <= order);
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index e43904e..b784e6c 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -1074,7 +1074,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
                    "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
                    d->domain_id, gfn, mfn, nr_mfns);
 
-            ret = map_mmio_regions(d, gfn, nr_mfns, mfn);
+            ret = map_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn));
             if ( ret < 0 )
                 printk(XENLOG_G_WARNING
                        "memory_map:fail: dom%d gfn=%lx mfn=%lx nr=%lx ret:%ld\n",
@@ -1086,7 +1086,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
                    "memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
                    d->domain_id, gfn, mfn, nr_mfns);
 
-            ret = unmap_mmio_regions(d, gfn, nr_mfns, mfn);
+            ret = unmap_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn));
             if ( ret < 0 && is_hardware_domain(current->domain) )
                 printk(XENLOG_ERR
                        "memory_map: error %ld removing dom%d access to [%lx,%lx]\n",
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
index 6374a5b..b4f9077 100644
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -37,13 +37,13 @@ typedef enum {
  *  * the guest physical address space to map, starting from the machine
  *   * frame number mfn. */
 int map_mmio_regions(struct domain *d,
-                     unsigned long start_gfn,
+                     gfn_t start_gfn,
                      unsigned long nr,
-                     unsigned long mfn);
+                     mfn_t mfn);
 int unmap_mmio_regions(struct domain *d,
-                       unsigned long start_gfn,
+                       gfn_t start_gfn,
                        unsigned long nr,
-                       unsigned long mfn);
+                       mfn_t mfn);
 
 /*
  * Set access type for a region of gfns.
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 6/8] xen: Use the typesafe mfn and gfn in map_mmio_regions...

[Stefano Stabellini]

On Tue, 21 Jun 2016, Julien Grall wrote:
> to avoid mixing machine frame with guest frame.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> 
>     Changes in v3:
>         - Use mfn_add when it is possible
>         - Add Jan's acked-by
> ---
>  xen/arch/arm/domain_build.c      |  4 ++--
>  xen/arch/arm/gic-v2.c            |  4 ++--
>  xen/arch/arm/p2m.c               | 22 +++++++++++-----------
>  xen/arch/arm/platforms/exynos5.c |  8 ++++----
>  xen/arch/arm/platforms/omap5.c   | 16 ++++++++--------
>  xen/arch/arm/vgic-v2.c           |  4 ++--
>  xen/arch/x86/mm/p2m.c            | 18 ++++++++++--------
>  xen/common/domctl.c              |  4 ++--
>  xen/include/xen/p2m-common.h     |  8 ++++----
>  9 files changed, 45 insertions(+), 43 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 9035486..49185f0 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1036,9 +1036,9 @@ static int map_range_to_domain(const struct dt_device_node *dev,
>      if ( need_mapping )
>      {
>          res = map_mmio_regions(d,
> -                               paddr_to_pfn(addr),
> +                               _gfn(paddr_to_pfn(addr)),
>                                 DIV_ROUND_UP(len, PAGE_SIZE),
> -                               paddr_to_pfn(addr));
> +                               _mfn(paddr_to_pfn(addr)));
>          if ( res < 0 )
>          {
>              printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 4e2f4c7..3893ece 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -601,9 +601,9 @@ static int gicv2_map_hwdown_extra_mappings(struct domain *d)
>                 d->domain_id, v2m_data->addr, v2m_data->size,
>                 v2m_data->spi_start, v2m_data->nr_spis);
>  
> -        ret = map_mmio_regions(d, paddr_to_pfn(v2m_data->addr),
> +        ret = map_mmio_regions(d, _gfn(paddr_to_pfn(v2m_data->addr)),
>                              DIV_ROUND_UP(v2m_data->size, PAGE_SIZE),
> -                            paddr_to_pfn(v2m_data->addr));
> +                            _mfn(paddr_to_pfn(v2m_data->addr)));
>          if ( ret )
>          {
>              printk(XENLOG_ERR "GICv2: Map v2m frame to d%d failed.\n",
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index aa4e774..47cb383 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1245,27 +1245,27 @@ int unmap_regions_rw_cache(struct domain *d,
>  }
>  
>  int map_mmio_regions(struct domain *d,
> -                     unsigned long start_gfn,
> +                     gfn_t start_gfn,
>                       unsigned long nr,
> -                     unsigned long mfn)
> +                     mfn_t mfn)
>  {
>      return apply_p2m_changes(d, INSERT,
> -                             pfn_to_paddr(start_gfn),
> -                             pfn_to_paddr(start_gfn + nr),
> -                             pfn_to_paddr(mfn),
> +                             pfn_to_paddr(gfn_x(start_gfn)),
> +                             pfn_to_paddr(gfn_x(start_gfn) + nr),
> +                             pfn_to_paddr(mfn_x(mfn)),
>                               MATTR_DEV, 0, p2m_mmio_direct,
>                               d->arch.p2m.default_access);

Any reason why you didn't push these changes down to apply_p2m_changes too?


>  }
>  
>  int unmap_mmio_regions(struct domain *d,
> -                       unsigned long start_gfn,
> +                       gfn_t start_gfn,
>                         unsigned long nr,
> -                       unsigned long mfn)
> +                       mfn_t mfn)
>  {
>      return apply_p2m_changes(d, REMOVE,
> -                             pfn_to_paddr(start_gfn),
> -                             pfn_to_paddr(start_gfn + nr),
> -                             pfn_to_paddr(mfn),
> +                             pfn_to_paddr(gfn_x(start_gfn)),
> +                             pfn_to_paddr(gfn_x(start_gfn) + nr),
> +                             pfn_to_paddr(mfn_x(mfn)),
>                               MATTR_DEV, 0, p2m_invalid,
>                               d->arch.p2m.default_access);
>  }
> @@ -1280,7 +1280,7 @@ int map_dev_mmio_region(struct domain *d,
>      if ( !(nr && iomem_access_permitted(d, start_gfn, start_gfn + nr - 1)) )
>          return 0;
>  
> -    res = map_mmio_regions(d, start_gfn, nr, mfn);
> +    res = map_mmio_regions(d, _gfn(start_gfn), nr, _mfn(mfn));
>      if ( res < 0 )
>      {
>          printk(XENLOG_G_ERR "Unable to map [%#lx - %#lx] in Dom%d\n",
> diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
> index bf4964d..c43934f 100644
> --- a/xen/arch/arm/platforms/exynos5.c
> +++ b/xen/arch/arm/platforms/exynos5.c
> @@ -83,12 +83,12 @@ static int exynos5_init_time(void)
>  static int exynos5250_specific_mapping(struct domain *d)
>  {
>      /* Map the chip ID */
> -    map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_CHIPID), 1,
> -                     paddr_to_pfn(EXYNOS5_PA_CHIPID));
> +    map_mmio_regions(d, _gfn(paddr_to_pfn(EXYNOS5_PA_CHIPID)), 1,
> +                     _mfn(paddr_to_pfn(EXYNOS5_PA_CHIPID)));
>  
>      /* Map the PWM region */
> -    map_mmio_regions(d, paddr_to_pfn(EXYNOS5_PA_TIMER), 2,
> -                     paddr_to_pfn(EXYNOS5_PA_TIMER));
> +    map_mmio_regions(d, _gfn(paddr_to_pfn(EXYNOS5_PA_TIMER)), 2,
> +                     _mfn(paddr_to_pfn(EXYNOS5_PA_TIMER)));
>  
>      return 0;
>  }
> diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
> index a49ba62..539588e 100644
> --- a/xen/arch/arm/platforms/omap5.c
> +++ b/xen/arch/arm/platforms/omap5.c
> @@ -102,20 +102,20 @@ static int omap5_init_time(void)
>  static int omap5_specific_mapping(struct domain *d)
>  {
>      /* Map the PRM module */
> -    map_mmio_regions(d, paddr_to_pfn(OMAP5_PRM_BASE), 2,
> -                     paddr_to_pfn(OMAP5_PRM_BASE));
> +    map_mmio_regions(d, _gfn(paddr_to_pfn(OMAP5_PRM_BASE)), 2,
> +                     _mfn(paddr_to_pfn(OMAP5_PRM_BASE)));
>  
>      /* Map the PRM_MPU */
> -    map_mmio_regions(d, paddr_to_pfn(OMAP5_PRCM_MPU_BASE), 1,
> -                     paddr_to_pfn(OMAP5_PRCM_MPU_BASE));
> +    map_mmio_regions(d, _gfn(paddr_to_pfn(OMAP5_PRCM_MPU_BASE)), 1,
> +                     _mfn(paddr_to_pfn(OMAP5_PRCM_MPU_BASE)));
>  
>      /* Map the Wakeup Gen */
> -    map_mmio_regions(d, paddr_to_pfn(OMAP5_WKUPGEN_BASE), 1,
> -                     paddr_to_pfn(OMAP5_WKUPGEN_BASE));
> +    map_mmio_regions(d, _gfn(paddr_to_pfn(OMAP5_WKUPGEN_BASE)), 1,
> +                     _mfn(paddr_to_pfn(OMAP5_WKUPGEN_BASE)));
>  
>      /* Map the on-chip SRAM */
> -    map_mmio_regions(d, paddr_to_pfn(OMAP5_SRAM_PA), 32,
> -                     paddr_to_pfn(OMAP5_SRAM_PA));
> +    map_mmio_regions(d, _gfn(paddr_to_pfn(OMAP5_SRAM_PA)), 32,
> +                     _mfn(paddr_to_pfn(OMAP5_SRAM_PA)));
>  
>      return 0;
>  }
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 9adb4a9..cbe61cf 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -688,8 +688,8 @@ static int vgic_v2_domain_init(struct domain *d)
>       * Map the gic virtual cpu interface in the gic cpu interface
>       * region of the guest.
>       */
> -    ret = map_mmio_regions(d, paddr_to_pfn(cbase), csize / PAGE_SIZE,
> -                           paddr_to_pfn(vbase));
> +    ret = map_mmio_regions(d, _gfn(paddr_to_pfn(cbase)), csize / PAGE_SIZE,
> +                           _mfn(paddr_to_pfn(vbase)));
>      if ( ret )
>          return ret;
>  
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 16733a4..6258a5b 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2214,9 +2214,9 @@ static unsigned int mmio_order(const struct domain *d,
>  #define MAP_MMIO_MAX_ITER 64 /* pretty arbitrary */
>  
>  int map_mmio_regions(struct domain *d,
> -                     unsigned long start_gfn,
> +                     gfn_t start_gfn,
>                       unsigned long nr,
> -                     unsigned long mfn)
> +                     mfn_t mfn)
>  {
>      int ret = 0;
>      unsigned long i;
> @@ -2229,10 +2229,11 @@ int map_mmio_regions(struct domain *d,
>            i += 1UL << order, ++iter )
>      {
>          /* OR'ing gfn and mfn values will return an order suitable to both. */
> -        for ( order = mmio_order(d, (start_gfn + i) | (mfn + i), nr - i); ;
> +        for ( order = mmio_order(d, (gfn_x(start_gfn) + i) | (mfn_x(mfn) + i), nr - i); ;
>                order = ret - 1 )
>          {
> -            ret = set_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i), order,
> +            ret = set_mmio_p2m_entry(d, gfn_x(start_gfn) + i,
> +                                     mfn_add(mfn, i), order,
>                                       p2m_get_hostp2m(d)->default_access);
>              if ( ret <= 0 )
>                  break;
> @@ -2246,9 +2247,9 @@ int map_mmio_regions(struct domain *d,
>  }
>  
>  int unmap_mmio_regions(struct domain *d,
> -                       unsigned long start_gfn,
> +                       gfn_t start_gfn,
>                         unsigned long nr,
> -                       unsigned long mfn)
> +                       mfn_t mfn)
>  {
>      int ret = 0;
>      unsigned long i;
> @@ -2261,10 +2262,11 @@ int unmap_mmio_regions(struct domain *d,
>            i += 1UL << order, ++iter )
>      {
>          /* OR'ing gfn and mfn values will return an order suitable to both. */
> -        for ( order = mmio_order(d, (start_gfn + i) | (mfn + i), nr - i); ;
> +        for ( order = mmio_order(d, (gfn_x(start_gfn) + i) | (mfn_x(mfn) + i), nr - i); ;
>                order = ret - 1 )
>          {
> -            ret = clear_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i), order);
> +            ret = clear_mmio_p2m_entry(d, gfn_x(start_gfn) + i,
> +                                       mfn_add(mfn, i), order);
>              if ( ret <= 0 )
>                  break;
>              ASSERT(ret <= order);
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index e43904e..b784e6c 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -1074,7 +1074,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>                     "memory_map:add: dom%d gfn=%lx mfn=%lx nr=%lx\n",
>                     d->domain_id, gfn, mfn, nr_mfns);
>  
> -            ret = map_mmio_regions(d, gfn, nr_mfns, mfn);
> +            ret = map_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn));
>              if ( ret < 0 )
>                  printk(XENLOG_G_WARNING
>                         "memory_map:fail: dom%d gfn=%lx mfn=%lx nr=%lx ret:%ld\n",
> @@ -1086,7 +1086,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>                     "memory_map:remove: dom%d gfn=%lx mfn=%lx nr=%lx\n",
>                     d->domain_id, gfn, mfn, nr_mfns);
>  
> -            ret = unmap_mmio_regions(d, gfn, nr_mfns, mfn);
> +            ret = unmap_mmio_regions(d, _gfn(gfn), nr_mfns, _mfn(mfn));
>              if ( ret < 0 && is_hardware_domain(current->domain) )
>                  printk(XENLOG_ERR
>                         "memory_map: error %ld removing dom%d access to [%lx,%lx]\n",
> diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
> index 6374a5b..b4f9077 100644
> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -37,13 +37,13 @@ typedef enum {
>   *  * the guest physical address space to map, starting from the machine
>   *   * frame number mfn. */
>  int map_mmio_regions(struct domain *d,
> -                     unsigned long start_gfn,
> +                     gfn_t start_gfn,
>                       unsigned long nr,
> -                     unsigned long mfn);
> +                     mfn_t mfn);
>  int unmap_mmio_regions(struct domain *d,
> -                       unsigned long start_gfn,
> +                       gfn_t start_gfn,
>                         unsigned long nr,
> -                       unsigned long mfn);
> +                       mfn_t mfn);
>  
>  /*
>   * Set access type for a region of gfns.
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 6/8] xen: Use the typesafe mfn and gfn in map_mmio_regions...

[Julien Grall]

On 23/06/16 15:05, Stefano Stabellini wrote:
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index aa4e774..47cb383 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1245,27 +1245,27 @@ int unmap_regions_rw_cache(struct domain *d,
>>   }
>>
>>   int map_mmio_regions(struct domain *d,
>> -                     unsigned long start_gfn,
>> +                     gfn_t start_gfn,
>>                        unsigned long nr,
>> -                     unsigned long mfn)
>> +                     mfn_t mfn)
>>   {
>>       return apply_p2m_changes(d, INSERT,
>> -                             pfn_to_paddr(start_gfn),
>> -                             pfn_to_paddr(start_gfn + nr),
>> -                             pfn_to_paddr(mfn),
>> +                             pfn_to_paddr(gfn_x(start_gfn)),
>> +                             pfn_to_paddr(gfn_x(start_gfn) + nr),
>> +                             pfn_to_paddr(mfn_x(mfn)),
>>                                MATTR_DEV, 0, p2m_mmio_direct,
>>                                d->arch.p2m.default_access);
>
> Any reason why you didn't push these changes down to apply_p2m_changes too?

To keep this series simple. I have another series coming up to push the 
change down to apply_p2m_changes and clean up the P2M code.

I can move the patch to push down the change in this series if you prefer.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 6/8] xen: Use the typesafe mfn and gfn in map_mmio_regions...

[Stefano Stabellini]

On Thu, 23 Jun 2016, Julien Grall wrote:
> On 23/06/16 15:05, Stefano Stabellini wrote:
> > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> > > index aa4e774..47cb383 100644
> > > --- a/xen/arch/arm/p2m.c
> > > +++ b/xen/arch/arm/p2m.c
> > > @@ -1245,27 +1245,27 @@ int unmap_regions_rw_cache(struct domain *d,
> > >   }
> > > 
> > >   int map_mmio_regions(struct domain *d,
> > > -                     unsigned long start_gfn,
> > > +                     gfn_t start_gfn,
> > >                        unsigned long nr,
> > > -                     unsigned long mfn)
> > > +                     mfn_t mfn)
> > >   {
> > >       return apply_p2m_changes(d, INSERT,
> > > -                             pfn_to_paddr(start_gfn),
> > > -                             pfn_to_paddr(start_gfn + nr),
> > > -                             pfn_to_paddr(mfn),
> > > +                             pfn_to_paddr(gfn_x(start_gfn)),
> > > +                             pfn_to_paddr(gfn_x(start_gfn) + nr),
> > > +                             pfn_to_paddr(mfn_x(mfn)),
> > >                                MATTR_DEV, 0, p2m_mmio_direct,
> > >                                d->arch.p2m.default_access);
> > 
> > Any reason why you didn't push these changes down to apply_p2m_changes too?
> 
> To keep this series simple. I have another series coming up to push the change
> down to apply_p2m_changes and clean up the P2M code.
> 
> I can move the patch to push down the change in this series if you prefer.

Yeah, it makes sense to keep them together.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 6/8] xen: Use the typesafe mfn and gfn in map_mmio_regions...

[Julien Grall]

On 23/06/16 15:15, Stefano Stabellini wrote:
> On Thu, 23 Jun 2016, Julien Grall wrote:
>> On 23/06/16 15:05, Stefano Stabellini wrote:
>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>> index aa4e774..47cb383 100644
>>>> --- a/xen/arch/arm/p2m.c
>>>> +++ b/xen/arch/arm/p2m.c
>>>> @@ -1245,27 +1245,27 @@ int unmap_regions_rw_cache(struct domain *d,
>>>>    }
>>>>
>>>>    int map_mmio_regions(struct domain *d,
>>>> -                     unsigned long start_gfn,
>>>> +                     gfn_t start_gfn,
>>>>                         unsigned long nr,
>>>> -                     unsigned long mfn)
>>>> +                     mfn_t mfn)
>>>>    {
>>>>        return apply_p2m_changes(d, INSERT,
>>>> -                             pfn_to_paddr(start_gfn),
>>>> -                             pfn_to_paddr(start_gfn + nr),
>>>> -                             pfn_to_paddr(mfn),
>>>> +                             pfn_to_paddr(gfn_x(start_gfn)),
>>>> +                             pfn_to_paddr(gfn_x(start_gfn) + nr),
>>>> +                             pfn_to_paddr(mfn_x(mfn)),
>>>>                                 MATTR_DEV, 0, p2m_mmio_direct,
>>>>                                 d->arch.p2m.default_access);
>>>
>>> Any reason why you didn't push these changes down to apply_p2m_changes too?
>>
>> To keep this series simple. I have another series coming up to push the change
>> down to apply_p2m_changes and clean up the P2M code.
>>
>> I can move the patch to push down the change in this series if you prefer.
>
> Yeah, it makes sense to keep them together.

Well, I still plan to have a different patch to push down the change. 
Switching from unsigned long to gfn/mfn is a long work which need to be 
split to ease the review.

I will see what I can do.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 7/8] xen/arm: Rework the interface of p2m_lookup and use typesafe gfn and mfn

[Julien Grall]

The prototype and the declaration of p2m_lookup disagree on how the
function should be used. One expect a frame number whilst the other
an address.

Thankfully, everyone is using with an address today. However, most of
the callers have to convert a guest frame to an address. Modify
the interface to take a guest physical frame in parameter and return
a machine frame.

Whilst modifying the interface, use typesafe gfn and mfn for clarity
and catching possible misusage.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/p2m.c        | 37 ++++++++++++++++++++-----------------
 xen/arch/arm/traps.c      | 21 +++++++++++----------
 xen/include/asm-arm/p2m.h |  7 +++----
 3 files changed, 34 insertions(+), 31 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 47cb383..f3330dd 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -140,14 +140,15 @@ void flush_tlb_domain(struct domain *d)
 }
 
 /*
- * Lookup the MFN corresponding to a domain's PFN.
+ * Lookup the MFN corresponding to a domain's GFN.
  *
  * There are no processor functions to do a stage 2 only lookup therefore we
  * do a a software walk.
  */
-static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
+static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
+    const paddr_t paddr = pfn_to_paddr(gfn_x(gfn));
     const unsigned int offsets[4] = {
         zeroeth_table_offset(paddr),
         first_table_offset(paddr),
@@ -158,7 +159,7 @@ static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
         ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK
     };
     lpae_t pte, *map;
-    paddr_t maddr = INVALID_PADDR;
+    mfn_t mfn = _mfn(INVALID_MFN);
     paddr_t mask = 0;
     p2m_type_t _t;
     unsigned int level, root_table;
@@ -216,21 +217,22 @@ static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
     {
         ASSERT(mask);
         ASSERT(pte.p2m.type != p2m_invalid);
-        maddr = (pte.bits & PADDR_MASK & mask) | (paddr & ~mask);
+        mfn = _mfn(paddr_to_pfn((pte.bits & PADDR_MASK & mask) |
+                                (paddr & ~mask)));
         *t = pte.p2m.type;
     }
 
 err:
-    return maddr;
+    return mfn;
 }
 
-paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
+mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
 {
-    paddr_t ret;
+    mfn_t ret;
     struct p2m_domain *p2m = &d->arch.p2m;
 
     spin_lock(&p2m->lock);
-    ret = __p2m_lookup(d, paddr, t);
+    ret = __p2m_lookup(d, gfn, t);
     spin_unlock(&p2m->lock);
 
     return ret;
@@ -493,8 +495,9 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
          * No setting was found in the Radix tree. Check if the
          * entry exists in the page-tables.
          */
-        paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
-        if ( INVALID_PADDR == maddr )
+        mfn_t mfn = __p2m_lookup(d, gfn, NULL);
+
+        if ( mfn_x(mfn) == INVALID_MFN )
             return -ESRCH;
 
         /* If entry exists then its rwx. */
@@ -1483,8 +1486,7 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
 
 mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
 {
-    paddr_t p = p2m_lookup(d, pfn_to_paddr(gfn_x(gfn)), NULL);
-    return _mfn(p >> PAGE_SHIFT);
+    return p2m_lookup(d, gfn, NULL);
 }
 
 /*
@@ -1498,7 +1500,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
 {
     long rc;
     paddr_t ipa;
-    unsigned long maddr;
+    gfn_t gfn;
     unsigned long mfn;
     xenmem_access_t xma;
     p2m_type_t t;
@@ -1508,11 +1510,13 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
     if ( rc < 0 )
         goto err;
 
+    gfn = _gfn(paddr_to_pfn(ipa));
+
     /*
      * We do this first as this is faster in the default case when no
      * permission is set on the page.
      */
-    rc = __p2m_get_mem_access(current->domain, _gfn(paddr_to_pfn(ipa)), &xma);
+    rc = __p2m_get_mem_access(current->domain, gfn, &xma);
     if ( rc < 0 )
         goto err;
 
@@ -1561,11 +1565,10 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
      * We had a mem_access permission limiting the access, but the page type
      * could also be limiting, so we need to check that as well.
      */
-    maddr = __p2m_lookup(current->domain, ipa, &t);
-    if ( maddr == INVALID_PADDR )
+    mfn = mfn_x(__p2m_lookup(current->domain, gfn, &t));
+    if ( mfn == INVALID_MFN )
         goto err;
 
-    mfn = maddr >> PAGE_SHIFT;
     if ( !mfn_valid(mfn) )
         goto err;
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 44926ca..02fe117 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2319,14 +2319,16 @@ void dump_guest_s1_walk(struct domain *d, vaddr_t addr)
 {
     register_t ttbcr = READ_SYSREG(TCR_EL1);
     uint64_t ttbr0 = READ_SYSREG64(TTBR0_EL1);
-    paddr_t paddr;
     uint32_t offset;
     uint32_t *first = NULL, *second = NULL;
+    mfn_t mfn;
+
+    mfn = p2m_lookup(d, _gfn(paddr_to_pfn(ttbr0)), NULL);
 
     printk("dom%d VA 0x%08"PRIvaddr"\n", d->domain_id, addr);
     printk("    TTBCR: 0x%08"PRIregister"\n", ttbcr);
     printk("    TTBR0: 0x%016"PRIx64" = 0x%"PRIpaddr"\n",
-           ttbr0, p2m_lookup(d, ttbr0 & PAGE_MASK, NULL));
+           ttbr0, pfn_to_paddr(mfn_x(mfn)));
 
     if ( ttbcr & TTBCR_EAE )
     {
@@ -2339,32 +2341,31 @@ void dump_guest_s1_walk(struct domain *d, vaddr_t addr)
         return;
     }
 
-    paddr = p2m_lookup(d, ttbr0 & PAGE_MASK, NULL);
-    if ( paddr == INVALID_PADDR )
+    if ( mfn_x(mfn) == INVALID_MFN )
     {
         printk("Failed TTBR0 maddr lookup\n");
         goto done;
     }
-    first = map_domain_page(_mfn(paddr_to_pfn(paddr)));
+    first = map_domain_page(mfn);
 
     offset = addr >> (12+10);
     printk("1ST[0x%"PRIx32"] (0x%"PRIpaddr") = 0x%08"PRIx32"\n",
-           offset, paddr, first[offset]);
+           offset, pfn_to_paddr(mfn_x(mfn)), first[offset]);
     if ( !(first[offset] & 0x1) ||
          !(first[offset] & 0x2) )
         goto done;
 
-    paddr = p2m_lookup(d, first[offset] & PAGE_MASK, NULL);
+    mfn = p2m_lookup(d, _gfn(paddr_to_pfn(first[offset])), NULL);
 
-    if ( paddr == INVALID_PADDR )
+    if ( mfn_x(mfn) == INVALID_MFN )
     {
         printk("Failed L1 entry maddr lookup\n");
         goto done;
     }
-    second = map_domain_page(_mfn(paddr_to_pfn(paddr)));
+    second = map_domain_page(mfn);
     offset = (addr >> 12) & 0x3FF;
     printk("2ND[0x%"PRIx32"] (0x%"PRIpaddr") = 0x%08"PRIx32"\n",
-           offset, paddr, second[offset]);
+           offset, pfn_to_paddr(mfn_x(mfn)), second[offset]);
 
 done:
     if (second) unmap_domain_page(second);
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 0d1e61e..f204482 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -135,8 +135,8 @@ void p2m_restore_state(struct vcpu *n);
 /* Print debugging/statistial info about a domain's p2m */
 void p2m_dump_info(struct domain *d);
 
-/* Look up the MFN corresponding to a domain's PFN. */
-paddr_t p2m_lookup(struct domain *d, paddr_t gpfn, p2m_type_t *t);
+/* Look up the MFN corresponding to a domain's GFN. */
+mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
 
 /* Clean & invalidate caches corresponding to a region of guest address space */
 int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn);
@@ -201,8 +201,7 @@ static inline struct page_info *get_page_from_gfn(
 {
     struct page_info *page;
     p2m_type_t p2mt;
-    paddr_t maddr = p2m_lookup(d, pfn_to_paddr(gfn), &p2mt);
-    unsigned long mfn = maddr >> PAGE_SHIFT;
+    unsigned long mfn = mfn_x(p2m_lookup(d, _gfn(gfn), &p2mt));
 
     if (t)
         *t = p2mt;
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 7/8] xen/arm: Rework the interface of p2m_lookup and use typesafe gfn and mfn

[Stefano Stabellini]

On Tue, 21 Jun 2016, Julien Grall wrote:
> The prototype and the declaration of p2m_lookup disagree on how the
> function should be used. One expect a frame number whilst the other
> an address.
> 
> Thankfully, everyone is using with an address today. However, most of
> the callers have to convert a guest frame to an address. Modify
> the interface to take a guest physical frame in parameter and return
> a machine frame.
> 
> Whilst modifying the interface, use typesafe gfn and mfn for clarity
> and catching possible misusage.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/p2m.c        | 37 ++++++++++++++++++++-----------------
>  xen/arch/arm/traps.c      | 21 +++++++++++----------
>  xen/include/asm-arm/p2m.h |  7 +++----
>  3 files changed, 34 insertions(+), 31 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 47cb383..f3330dd 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -140,14 +140,15 @@ void flush_tlb_domain(struct domain *d)
>  }
>  
>  /*
> - * Lookup the MFN corresponding to a domain's PFN.
> + * Lookup the MFN corresponding to a domain's GFN.
>   *
>   * There are no processor functions to do a stage 2 only lookup therefore we
>   * do a a software walk.
>   */
> -static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
> +static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>  {
>      struct p2m_domain *p2m = &d->arch.p2m;
> +    const paddr_t paddr = pfn_to_paddr(gfn_x(gfn));
>      const unsigned int offsets[4] = {
>          zeroeth_table_offset(paddr),
>          first_table_offset(paddr),
> @@ -158,7 +159,7 @@ static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
>          ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK
>      };
>      lpae_t pte, *map;
> -    paddr_t maddr = INVALID_PADDR;
> +    mfn_t mfn = _mfn(INVALID_MFN);

It might be worth defining INVALID_MFN_T and just assign that to mfn.


>      paddr_t mask = 0;
>      p2m_type_t _t;
>      unsigned int level, root_table;
> @@ -216,21 +217,22 @@ static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
>      {
>          ASSERT(mask);
>          ASSERT(pte.p2m.type != p2m_invalid);
> -        maddr = (pte.bits & PADDR_MASK & mask) | (paddr & ~mask);
> +        mfn = _mfn(paddr_to_pfn((pte.bits & PADDR_MASK & mask) |
> +                                (paddr & ~mask)));
>          *t = pte.p2m.type;
>      }
>  
>  err:
> -    return maddr;
> +    return mfn;
>  }
>  
> -paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
> +mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>  {
> -    paddr_t ret;
> +    mfn_t ret;
>      struct p2m_domain *p2m = &d->arch.p2m;
>  
>      spin_lock(&p2m->lock);
> -    ret = __p2m_lookup(d, paddr, t);
> +    ret = __p2m_lookup(d, gfn, t);
>      spin_unlock(&p2m->lock);
>  
>      return ret;
> @@ -493,8 +495,9 @@ static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
>           * No setting was found in the Radix tree. Check if the
>           * entry exists in the page-tables.
>           */
> -        paddr_t maddr = __p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
> -        if ( INVALID_PADDR == maddr )
> +        mfn_t mfn = __p2m_lookup(d, gfn, NULL);
> +
> +        if ( mfn_x(mfn) == INVALID_MFN )
>              return -ESRCH;
>  
>          /* If entry exists then its rwx. */
> @@ -1483,8 +1486,7 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
>  
>  mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn)
>  {
> -    paddr_t p = p2m_lookup(d, pfn_to_paddr(gfn_x(gfn)), NULL);
> -    return _mfn(p >> PAGE_SHIFT);
> +    return p2m_lookup(d, gfn, NULL);
>  }
>  
>  /*
> @@ -1498,7 +1500,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
>  {
>      long rc;
>      paddr_t ipa;
> -    unsigned long maddr;
> +    gfn_t gfn;
>      unsigned long mfn;
>      xenmem_access_t xma;
>      p2m_type_t t;
> @@ -1508,11 +1510,13 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
>      if ( rc < 0 )
>          goto err;
>  
> +    gfn = _gfn(paddr_to_pfn(ipa));
> +
>      /*
>       * We do this first as this is faster in the default case when no
>       * permission is set on the page.
>       */
> -    rc = __p2m_get_mem_access(current->domain, _gfn(paddr_to_pfn(ipa)), &xma);
> +    rc = __p2m_get_mem_access(current->domain, gfn, &xma);
>      if ( rc < 0 )
>          goto err;
>  
> @@ -1561,11 +1565,10 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
>       * We had a mem_access permission limiting the access, but the page type
>       * could also be limiting, so we need to check that as well.
>       */
> -    maddr = __p2m_lookup(current->domain, ipa, &t);
> -    if ( maddr == INVALID_PADDR )
> +    mfn = mfn_x(__p2m_lookup(current->domain, gfn, &t));
> +    if ( mfn == INVALID_MFN )

The conversion would go away if we had an INVALID_MFN which is mfn_t


>          goto err;
>  
> -    mfn = maddr >> PAGE_SHIFT;
>      if ( !mfn_valid(mfn) )
>          goto err;
>  
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 44926ca..02fe117 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2319,14 +2319,16 @@ void dump_guest_s1_walk(struct domain *d, vaddr_t addr)
>  {
>      register_t ttbcr = READ_SYSREG(TCR_EL1);
>      uint64_t ttbr0 = READ_SYSREG64(TTBR0_EL1);
> -    paddr_t paddr;
>      uint32_t offset;
>      uint32_t *first = NULL, *second = NULL;
> +    mfn_t mfn;
> +
> +    mfn = p2m_lookup(d, _gfn(paddr_to_pfn(ttbr0)), NULL);
>  
>      printk("dom%d VA 0x%08"PRIvaddr"\n", d->domain_id, addr);
>      printk("    TTBCR: 0x%08"PRIregister"\n", ttbcr);
>      printk("    TTBR0: 0x%016"PRIx64" = 0x%"PRIpaddr"\n",
> -           ttbr0, p2m_lookup(d, ttbr0 & PAGE_MASK, NULL));
> +           ttbr0, pfn_to_paddr(mfn_x(mfn)));
>  
>      if ( ttbcr & TTBCR_EAE )
>      {
> @@ -2339,32 +2341,31 @@ void dump_guest_s1_walk(struct domain *d, vaddr_t addr)
>          return;
>      }
>  
> -    paddr = p2m_lookup(d, ttbr0 & PAGE_MASK, NULL);
> -    if ( paddr == INVALID_PADDR )
> +    if ( mfn_x(mfn) == INVALID_MFN )
>      {
>          printk("Failed TTBR0 maddr lookup\n");
>          goto done;
>      }
> -    first = map_domain_page(_mfn(paddr_to_pfn(paddr)));
> +    first = map_domain_page(mfn);
>  
>      offset = addr >> (12+10);
>      printk("1ST[0x%"PRIx32"] (0x%"PRIpaddr") = 0x%08"PRIx32"\n",
> -           offset, paddr, first[offset]);
> +           offset, pfn_to_paddr(mfn_x(mfn)), first[offset]);
>      if ( !(first[offset] & 0x1) ||
>           !(first[offset] & 0x2) )
>          goto done;
>  
> -    paddr = p2m_lookup(d, first[offset] & PAGE_MASK, NULL);
> +    mfn = p2m_lookup(d, _gfn(paddr_to_pfn(first[offset])), NULL);
>  
> -    if ( paddr == INVALID_PADDR )
> +    if ( mfn_x(mfn) == INVALID_MFN )
>      {
>          printk("Failed L1 entry maddr lookup\n");
>          goto done;
>      }
> -    second = map_domain_page(_mfn(paddr_to_pfn(paddr)));
> +    second = map_domain_page(mfn);
>      offset = (addr >> 12) & 0x3FF;
>      printk("2ND[0x%"PRIx32"] (0x%"PRIpaddr") = 0x%08"PRIx32"\n",
> -           offset, paddr, second[offset]);
> +           offset, pfn_to_paddr(mfn_x(mfn)), second[offset]);
>  
>  done:
>      if (second) unmap_domain_page(second);
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index 0d1e61e..f204482 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -135,8 +135,8 @@ void p2m_restore_state(struct vcpu *n);
>  /* Print debugging/statistial info about a domain's p2m */
>  void p2m_dump_info(struct domain *d);
>  
> -/* Look up the MFN corresponding to a domain's PFN. */
> -paddr_t p2m_lookup(struct domain *d, paddr_t gpfn, p2m_type_t *t);
> +/* Look up the MFN corresponding to a domain's GFN. */
> +mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
>  
>  /* Clean & invalidate caches corresponding to a region of guest address space */
>  int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn);
> @@ -201,8 +201,7 @@ static inline struct page_info *get_page_from_gfn(
>  {
>      struct page_info *page;
>      p2m_type_t p2mt;
> -    paddr_t maddr = p2m_lookup(d, pfn_to_paddr(gfn), &p2mt);
> -    unsigned long mfn = maddr >> PAGE_SHIFT;
> +    unsigned long mfn = mfn_x(p2m_lookup(d, _gfn(gfn), &p2mt));
>  
>      if (t)
>          *t = p2mt;
> -- 
> 1.9.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 7/8] xen/arm: Rework the interface of p2m_lookup and use typesafe gfn and mfn

[Julien Grall]

Hi Stefano,

On 23/06/16 15:14, Stefano Stabellini wrote:
> On Tue, 21 Jun 2016, Julien Grall wrote:
>> The prototype and the declaration of p2m_lookup disagree on how the
>> function should be used. One expect a frame number whilst the other
>> an address.
>>
>> Thankfully, everyone is using with an address today. However, most of
>> the callers have to convert a guest frame to an address. Modify
>> the interface to take a guest physical frame in parameter and return
>> a machine frame.
>>
>> Whilst modifying the interface, use typesafe gfn and mfn for clarity
>> and catching possible misusage.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/p2m.c        | 37 ++++++++++++++++++++-----------------
>>   xen/arch/arm/traps.c      | 21 +++++++++++----------
>>   xen/include/asm-arm/p2m.h |  7 +++----
>>   3 files changed, 34 insertions(+), 31 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 47cb383..f3330dd 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -140,14 +140,15 @@ void flush_tlb_domain(struct domain *d)
>>   }
>>
>>   /*
>> - * Lookup the MFN corresponding to a domain's PFN.
>> + * Lookup the MFN corresponding to a domain's GFN.
>>    *
>>    * There are no processor functions to do a stage 2 only lookup therefore we
>>    * do a a software walk.
>>    */
>> -static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
>> +static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>>   {
>>       struct p2m_domain *p2m = &d->arch.p2m;
>> +    const paddr_t paddr = pfn_to_paddr(gfn_x(gfn));
>>       const unsigned int offsets[4] = {
>>           zeroeth_table_offset(paddr),
>>           first_table_offset(paddr),
>> @@ -158,7 +159,7 @@ static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
>>           ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK
>>       };
>>       lpae_t pte, *map;
>> -    paddr_t maddr = INVALID_PADDR;
>> +    mfn_t mfn = _mfn(INVALID_MFN);
>
> It might be worth defining INVALID_MFN_T and just assign that to mfn.

Good idea. It will be useful in other places too.

>
>>       paddr_t mask = 0;
>>       p2m_type_t _t;
>>       unsigned int level, root_table;


[...]

>> @@ -1561,11 +1565,10 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
>>        * We had a mem_access permission limiting the access, but the page type
>>        * could also be limiting, so we need to check that as well.
>>        */
>> -    maddr = __p2m_lookup(current->domain, ipa, &t);
>> -    if ( maddr == INVALID_PADDR )
>> +    mfn = mfn_x(__p2m_lookup(current->domain, gfn, &t));
>> +    if ( mfn == INVALID_MFN )
>
> The conversion would go away if we had an INVALID_MFN which is mfn_t

Will do.

Regards,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 7/8] xen/arm: Rework the interface of p2m_lookup and use typesafe gfn and mfn

[Andrew Cooper]

On 24/06/16 14:58, Julien Grall wrote:
> Hi Stefano,
>
> On 23/06/16 15:14, Stefano Stabellini wrote:
>> On Tue, 21 Jun 2016, Julien Grall wrote:
>>> The prototype and the declaration of p2m_lookup disagree on how the
>>> function should be used. One expect a frame number whilst the other
>>> an address.
>>>
>>> Thankfully, everyone is using with an address today. However, most of
>>> the callers have to convert a guest frame to an address. Modify
>>> the interface to take a guest physical frame in parameter and return
>>> a machine frame.
>>>
>>> Whilst modifying the interface, use typesafe gfn and mfn for clarity
>>> and catching possible misusage.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>> ---
>>>   xen/arch/arm/p2m.c        | 37 ++++++++++++++++++++-----------------
>>>   xen/arch/arm/traps.c      | 21 +++++++++++----------
>>>   xen/include/asm-arm/p2m.h |  7 +++----
>>>   3 files changed, 34 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>> index 47cb383..f3330dd 100644
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -140,14 +140,15 @@ void flush_tlb_domain(struct domain *d)
>>>   }
>>>
>>>   /*
>>> - * Lookup the MFN corresponding to a domain's PFN.
>>> + * Lookup the MFN corresponding to a domain's GFN.
>>>    *
>>>    * There are no processor functions to do a stage 2 only lookup
>>> therefore we
>>>    * do a a software walk.
>>>    */
>>> -static paddr_t __p2m_lookup(struct domain *d, paddr_t paddr,
>>> p2m_type_t *t)
>>> +static mfn_t __p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
>>>   {
>>>       struct p2m_domain *p2m = &d->arch.p2m;
>>> +    const paddr_t paddr = pfn_to_paddr(gfn_x(gfn));
>>>       const unsigned int offsets[4] = {
>>>           zeroeth_table_offset(paddr),
>>>           first_table_offset(paddr),
>>> @@ -158,7 +159,7 @@ static paddr_t __p2m_lookup(struct domain *d,
>>> paddr_t paddr, p2m_type_t *t)
>>>           ZEROETH_MASK, FIRST_MASK, SECOND_MASK, THIRD_MASK
>>>       };
>>>       lpae_t pte, *map;
>>> -    paddr_t maddr = INVALID_PADDR;
>>> +    mfn_t mfn = _mfn(INVALID_MFN);
>>
>> It might be worth defining INVALID_MFN_T and just assign that to mfn.
>
> Good idea. It will be useful in other places too.
>
>>
>>>       paddr_t mask = 0;
>>>       p2m_type_t _t;
>>>       unsigned int level, root_table;
>
>
> [...]
>
>>> @@ -1561,11 +1565,10 @@ p2m_mem_access_check_and_get_page(vaddr_t
>>> gva, unsigned long flag)
>>>        * We had a mem_access permission limiting the access, but the
>>> page type
>>>        * could also be limiting, so we need to check that as well.
>>>        */
>>> -    maddr = __p2m_lookup(current->domain, ipa, &t);
>>> -    if ( maddr == INVALID_PADDR )
>>> +    mfn = mfn_x(__p2m_lookup(current->domain, gfn, &t));
>>> +    if ( mfn == INVALID_MFN )
>>
>> The conversion would go away if we had an INVALID_MFN which is mfn_t

Be careful.

INVALID_MFN_T is fine for assignment, but you can't do plain equality
tests of opaque structures.

mfn_t m = _mfn(0);
mfn_t inval = INVALID_MFN_T; // Ok
mfn_equal(m, INVALID_MFN_T); // Ok
mfn_equal(m, inval); // Ok
m == inval; // Compilation error

This was the reason that I didn't previously introduce INVALID_MFN_T,
although with mfn_equal(), perhaps the time has come.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 8/8] xen/arm: p2m_cache_flush: Use the correct terminology and typesafe gfn

[Julien Grall]

p2m_cache_flush is expecting GFNs in parameter and not MFNs. Rename
the variable to *gfn* and use typesafe to avoid possible misusage.

Note that the type of the parameters 'start' and 'end' is changed
from xen_pfn_t (aka uint64_t) to gfn_t (aka unsigned long). This
means that a truncation will occur for ARM32. It is fine because it
will always be encoded on 28 bits maximum (40 bits address).

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

---
    Changes in v3:
        - Add a word in the commit message about the truncation.

    Changes in v2:
        - Drop _gfn suffix
---
 xen/arch/arm/domctl.c     |  2 +-
 xen/arch/arm/p2m.c        | 10 +++++-----
 xen/include/asm-arm/p2m.h |  2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index 30453d8..b94e97c 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -30,7 +30,7 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
         if ( e < s )
             return -EINVAL;
 
-        return p2m_cache_flush(d, s, e);
+        return p2m_cache_flush(d, _gfn(s), _gfn(e));
     }
     case XEN_DOMCTL_bind_pt_irq:
     {
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index f3330dd..9149981 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1469,16 +1469,16 @@ int relinquish_p2m_mapping(struct domain *d)
                               d->arch.p2m.default_access);
 }
 
-int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
+int p2m_cache_flush(struct domain *d, gfn_t start, gfn_t end)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
 
-    start_mfn = MAX(start_mfn, p2m->lowest_mapped_gfn);
-    end_mfn = MIN(end_mfn, p2m->max_mapped_gfn);
+    start = gfn_max(start, _gfn(p2m->lowest_mapped_gfn));
+    end = gfn_min(end, _gfn(p2m->max_mapped_gfn));
 
     return apply_p2m_changes(d, CACHEFLUSH,
-                             pfn_to_paddr(start_mfn),
-                             pfn_to_paddr(end_mfn),
+                             pfn_to_paddr(gfn_x(start)),
+                             pfn_to_paddr(gfn_x(end)),
                              pfn_to_paddr(INVALID_MFN),
                              MATTR_MEM, 0, p2m_invalid,
                              d->arch.p2m.default_access);
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index f204482..976e51e 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -139,7 +139,7 @@ void p2m_dump_info(struct domain *d);
 mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t);
 
 /* Clean & invalidate caches corresponding to a region of guest address space */
-int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn);
+int p2m_cache_flush(struct domain *d, gfn_t start, gfn_t end);
 
 /* Setup p2m RAM mapping for domain d from start-end. */
 int p2m_populate_ram(struct domain *d, paddr_t start, paddr_t end);
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel