diff mbox series

[Xen-devel,v4,13/16] xen/grant: Switch {create, replace}_grant_p2m_mapping to typesafe MFN

Message ID 20180221140259.29360-14-julien.grall@arm.com
State Superseded
Headers show
Series xen: Convert page_to_mfn and mfn_to_page to use typesafe MFN | expand

Commit Message

Julien Grall Feb. 21, 2018, 2:02 p.m. UTC 
The current prototype is slightly confusing because it takes a guest
physical address and a machine physical frame (not address!). Switching to
MFN will improve safety and reduce the chance to mistakenly invert the
2 parameters.

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

---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.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: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

    Changes in v4:
        - Patch added
---
 xen/arch/arm/mm.c                     | 10 +++++-----
 xen/arch/x86/hvm/grant_table.c        | 14 +++++++-------
 xen/arch/x86/pv/grant_table.c         | 10 +++++-----
 xen/common/grant_table.c              |  8 ++++----
 xen/include/asm-arm/grant_table.h     |  9 ++++-----
 xen/include/asm-x86/grant_table.h     |  4 ++--
 xen/include/asm-x86/hvm/grant_table.h |  8 ++++----
 xen/include/asm-x86/pv/grant_table.h  |  8 ++++----
 8 files changed, 35 insertions(+), 36 deletions(-)

Comments

Wei Liu Feb. 23, 2018, 5:29 p.m. UTC | #1 
On Wed, Feb 21, 2018 at 02:02:56PM +0000, Julien Grall wrote:
> The current prototype is slightly confusing because it takes a guest
> physical address and a machine physical frame (not address!). Switching to
> MFN will improve safety and reduce the chance to mistakenly invert the
> 2 parameters.
> 
> Signed-off-by: Julien grall <julien.grall@arm.com>
> 

Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Jan Beulich March 2, 2018, 3:38 p.m. UTC | #2 
>>> On 21.02.18 at 15:02, <julien.grall@arm.com> wrote:
> The current prototype is slightly confusing because it takes a guest
> physical address and a machine physical frame (not address!). Switching to
> MFN will improve safety and reduce the chance to mistakenly invert the
> 2 parameters.
> 
> Signed-off-by: Julien grall <julien.grall@arm.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
diff mbox series

Patch

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index f17907ace8..4268dd5c2d 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1423,7 +1423,7 @@  void gnttab_mark_dirty(struct domain *d, unsigned long l)
     }
 }
 
-int create_grant_host_mapping(unsigned long addr, unsigned long frame,
+int create_grant_host_mapping(unsigned long addr, mfn_t frame,
                               unsigned int flags, unsigned int cache_flags)
 {
     int rc;
@@ -1436,7 +1436,7 @@  int create_grant_host_mapping(unsigned long addr, unsigned long frame,
         t = p2m_grant_map_ro;
 
     rc = guest_physmap_add_entry(current->domain, gaddr_to_gfn(addr),
-                                 _mfn(frame), 0, t);
+                                 frame, 0, t);
 
     if ( rc )
         return GNTST_general_error;
@@ -1444,8 +1444,8 @@  int create_grant_host_mapping(unsigned long addr, unsigned long frame,
         return GNTST_okay;
 }
 
-int replace_grant_host_mapping(unsigned long addr, unsigned long mfn,
-        unsigned long new_addr, unsigned int flags)
+int replace_grant_host_mapping(unsigned long addr, mfn_t mfn,
+                               unsigned long new_addr, unsigned int flags)
 {
     gfn_t gfn = gaddr_to_gfn(addr);
     struct domain *d = current->domain;
@@ -1454,7 +1454,7 @@  int replace_grant_host_mapping(unsigned long addr, unsigned long mfn,
     if ( new_addr != 0 || (flags & GNTMAP_contains_pte) )
         return GNTST_general_error;
 
-    rc = guest_physmap_remove_page(d, gfn, _mfn(mfn), 0);
+    rc = guest_physmap_remove_page(d, gfn, mfn, 0);
 
     return rc ? GNTST_general_error : GNTST_okay;
 }
diff --git a/xen/arch/x86/hvm/grant_table.c b/xen/arch/x86/hvm/grant_table.c
index 9ca9fe0425..ecd7d078ab 100644
--- a/xen/arch/x86/hvm/grant_table.c
+++ b/xen/arch/x86/hvm/grant_table.c
@@ -25,7 +25,7 @@ 
 
 #include <asm/p2m.h>
 
-int create_grant_p2m_mapping(uint64_t addr, unsigned long frame,
+int create_grant_p2m_mapping(uint64_t addr, mfn_t frame,
                              unsigned int flags,
                              unsigned int cache_flags)
 {
@@ -41,14 +41,14 @@  int create_grant_p2m_mapping(uint64_t addr, unsigned long frame,
         p2mt = p2m_grant_map_rw;
     rc = guest_physmap_add_entry(current->domain,
                                  _gfn(addr >> PAGE_SHIFT),
-                                 _mfn(frame), PAGE_ORDER_4K, p2mt);
+                                 frame, PAGE_ORDER_4K, p2mt);
     if ( rc )
         return GNTST_general_error;
     else
         return GNTST_okay;
 }
 
-int replace_grant_p2m_mapping(uint64_t addr, unsigned long frame,
+int replace_grant_p2m_mapping(uint64_t addr, mfn_t frame,
                               uint64_t new_addr, unsigned int flags)
 {
     unsigned long gfn = (unsigned long)(addr >> PAGE_SHIFT);
@@ -60,15 +60,15 @@  int replace_grant_p2m_mapping(uint64_t addr, unsigned long frame,
         return GNTST_general_error;
 
     old_mfn = get_gfn(d, gfn, &type);
-    if ( !p2m_is_grant(type) || mfn_x(old_mfn) != frame )
+    if ( !p2m_is_grant(type) || !mfn_eq(old_mfn, frame) )
     {
         put_gfn(d, gfn);
         gdprintk(XENLOG_WARNING,
-                 "old mapping invalid (type %d, mfn %" PRI_mfn ", frame %lx)\n",
-                 type, mfn_x(old_mfn), frame);
+                 "old mapping invalid (type %d, mfn %" PRI_mfn ", frame %"PRI_mfn")\n",
+                 type, mfn_x(old_mfn), mfn_x(frame));
         return GNTST_general_error;
     }
-    if ( guest_physmap_remove_page(d, _gfn(gfn), _mfn(frame), PAGE_ORDER_4K) )
+    if ( guest_physmap_remove_page(d, _gfn(gfn), frame, PAGE_ORDER_4K) )
     {
         put_gfn(d, gfn);
         return GNTST_general_error;
diff --git a/xen/arch/x86/pv/grant_table.c b/xen/arch/x86/pv/grant_table.c
index 4dbc550366..458085e1b6 100644
--- a/xen/arch/x86/pv/grant_table.c
+++ b/xen/arch/x86/pv/grant_table.c
@@ -50,7 +50,7 @@  static unsigned int grant_to_pte_flags(unsigned int grant_flags,
     return pte_flags;
 }
 
-int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
+int create_grant_pv_mapping(uint64_t addr, mfn_t frame,
                             unsigned int flags, unsigned int cache_flags)
 {
     struct vcpu *curr = current;
@@ -60,7 +60,7 @@  int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
     mfn_t gl1mfn;
     int rc = GNTST_general_error;
 
-    nl1e = l1e_from_pfn(frame, grant_to_pte_flags(flags, cache_flags));
+    nl1e = l1e_from_mfn(frame, grant_to_pte_flags(flags, cache_flags));
     nl1e = adjust_guest_l1e(nl1e, currd);
 
     /*
@@ -192,7 +192,7 @@  static bool steal_linear_address(unsigned long linear, l1_pgentry_t *out)
  * new_addr has only ever been available via GNTABOP_unmap_and_replace, and
  * only when !(flags & GNTMAP_contains_pte).
  */
-int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
+int replace_grant_pv_mapping(uint64_t addr, mfn_t frame,
                              uint64_t new_addr, unsigned int flags)
 {
     struct vcpu *curr = current;
@@ -282,14 +282,14 @@  int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
      * Check that the address supplied is actually mapped to frame (with
      * appropriate permissions).
      */
-    if ( unlikely(l1e_get_pfn(ol1e) != frame) ||
+    if ( unlikely(!mfn_eq(l1e_get_mfn(ol1e), frame)) ||
          unlikely((l1e_get_flags(ol1e) ^ grant_pte_flags) &
                   (_PAGE_PRESENT | _PAGE_RW)) )
     {
         gdprintk(XENLOG_ERR,
                  "PTE %"PRIpte" for %"PRIx64" doesn't match grant (%"PRIpte")\n",
                  l1e_get_intpte(ol1e), addr,
-                 l1e_get_intpte(l1e_from_pfn(frame, grant_pte_flags)));
+                 l1e_get_intpte(l1e_from_mfn(frame, grant_pte_flags)));
         goto out_unlock;
     }
 
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 48c547930c..c555c1d451 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1036,7 +1036,7 @@  map_grant_ref(
 
         if ( op->flags & GNTMAP_host_map )
         {
-            rc = create_grant_host_mapping(op->host_addr, frame, op->flags,
+            rc = create_grant_host_mapping(op->host_addr, _mfn(frame), op->flags,
                                            cache_flags);
             if ( rc != GNTST_okay )
                 goto undo_out;
@@ -1076,7 +1076,7 @@  map_grant_ref(
                 typecnt++;
             }
 
-            rc = create_grant_host_mapping(op->host_addr, frame, op->flags, 0);
+            rc = create_grant_host_mapping(op->host_addr, _mfn(frame), op->flags, 0);
             if ( rc != GNTST_okay )
                 goto undo_out;
 
@@ -1153,7 +1153,7 @@  map_grant_ref(
  undo_out:
     if ( host_map_created )
     {
-        replace_grant_host_mapping(op->host_addr, frame, 0, op->flags);
+        replace_grant_host_mapping(op->host_addr, _mfn(frame), 0, op->flags);
         gnttab_flush_tlb(ld);
     }
 
@@ -1339,7 +1339,7 @@  unmap_common(
     if ( op->host_addr && (flags & GNTMAP_host_map) )
     {
         if ( (rc = replace_grant_host_mapping(op->host_addr,
-                                              op->frame, op->new_addr,
+                                              _mfn(op->frame), op->new_addr,
                                               flags)) < 0 )
             goto act_release_out;
 
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 0dfdc5577f..ffff18d456 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -13,12 +13,11 @@  struct grant_table_arch {
 };
 
 void gnttab_clear_flag(unsigned long nr, uint16_t *addr);
-int create_grant_host_mapping(unsigned long gpaddr,
-        unsigned long mfn, unsigned int flags, unsigned int
-        cache_flags);
+int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
+                              unsigned int flags, unsigned int cache_flags);
 #define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
-int replace_grant_host_mapping(unsigned long gpaddr, unsigned long mfn,
-        unsigned long new_gpaddr, unsigned int flags);
+int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
+                               unsigned long new_gpaddr, unsigned int flags);
 void gnttab_mark_dirty(struct domain *d, unsigned long l);
 #define gnttab_create_status_page(d, t, i) do {} while (0)
 #define gnttab_status_gmfn(d, t, i) (0)
diff --git a/xen/include/asm-x86/grant_table.h b/xen/include/asm-x86/grant_table.h
index d9157e4417..e40ac8e3e7 100644
--- a/xen/include/asm-x86/grant_table.h
+++ b/xen/include/asm-x86/grant_table.h
@@ -21,7 +21,7 @@  struct grant_table_arch {
  * Caller must own caller's BIGLOCK, is responsible for flushing the TLB, and
  * must hold a reference to the page.
  */
-static inline int create_grant_host_mapping(uint64_t addr, unsigned long frame,
+static inline int create_grant_host_mapping(uint64_t addr, mfn_t frame,
                                             unsigned int flags,
                                             unsigned int cache_flags)
 {
@@ -30,7 +30,7 @@  static inline int create_grant_host_mapping(uint64_t addr, unsigned long frame,
     return create_grant_pv_mapping(addr, frame, flags, cache_flags);
 }
 
-static inline int replace_grant_host_mapping(uint64_t addr, unsigned long frame,
+static inline int replace_grant_host_mapping(uint64_t addr, mfn_t frame,
                                              uint64_t new_addr,
                                              unsigned int flags)
 {
diff --git a/xen/include/asm-x86/hvm/grant_table.h b/xen/include/asm-x86/hvm/grant_table.h
index 711ce9b560..a5612585b3 100644
--- a/xen/include/asm-x86/hvm/grant_table.h
+++ b/xen/include/asm-x86/hvm/grant_table.h
@@ -23,24 +23,24 @@ 
 
 #ifdef CONFIG_HVM
 
-int create_grant_p2m_mapping(uint64_t addr, unsigned long frame,
+int create_grant_p2m_mapping(uint64_t addr, mfn_t frame,
                              unsigned int flags,
                              unsigned int cache_flags);
-int replace_grant_p2m_mapping(uint64_t addr, unsigned long frame,
+int replace_grant_p2m_mapping(uint64_t addr, mfn_t frame,
                               uint64_t new_addr, unsigned int flags);
 
 #else
 
 #include <public/grant_table.h>
 
-static inline int create_grant_p2m_mapping(uint64_t addr, unsigned long frame,
+static inline int create_grant_p2m_mapping(uint64_t addr, mfn_t frame,
                                            unsigned int flags,
                                            unsigned int cache_flags)
 {
     return GNTST_general_error;
 }
 
-static inline int replace_grant_p2m_mapping(uint64_t addr, unsigned long frame,
+static inline int replace_grant_p2m_mapping(uint64_t addr, mfn_t frame,
                                             uint64_t new_addr, unsigned int flags)
 {
     return GNTST_general_error;
diff --git a/xen/include/asm-x86/pv/grant_table.h b/xen/include/asm-x86/pv/grant_table.h
index 556e68f0eb..85442b6074 100644
--- a/xen/include/asm-x86/pv/grant_table.h
+++ b/xen/include/asm-x86/pv/grant_table.h
@@ -23,23 +23,23 @@ 
 
 #ifdef CONFIG_PV
 
-int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
+int create_grant_pv_mapping(uint64_t addr, mfn_t frame,
                             unsigned int flags, unsigned int cache_flags);
-int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
+int replace_grant_pv_mapping(uint64_t addr, mfn_t frame,
                              uint64_t new_addr, unsigned int flags);
 
 #else
 
 #include <public/grant_table.h>
 
-static inline int create_grant_pv_mapping(uint64_t addr, unsigned long frame,
+static inline int create_grant_pv_mapping(uint64_t addr, mfn_t frame,
                                           unsigned int flags,
                                           unsigned int cache_flags)
 {
     return GNTST_general_error;
 }
 
-static inline int replace_grant_pv_mapping(uint64_t addr, unsigned long frame,
+static inline int replace_grant_pv_mapping(uint64_t addr, mfn_t frame,
                                            uint64_t new_addr, unsigned int flags)
 {
     return GNTST_general_error;