[PATCH v5] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access

[PATCH v5] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access

[Vitaly Kuznetsov]

'pfn' and 'start_pfn' are ambiguous, both these functions expect GFNs as input.

On x86 the interface of p2m_set_mem_access() in p2m.c doesn't match the
declaration in p2m-common.h as 'pfn' is being used instead of 'start_pfn'.

On ARM both p2m_set_mem_access and p2m_get_mem_access interfaces don't match
declarations from p2m-common.h: p2m_set_mem_access uses 'pfn' instead of
'start_pfn' and p2m_get_mem_access uses 'gpfn' instead of 'pfn'.

Convert p2m_get_mem_access/p2m_set_mem_access (and __p2m_get_mem_access on ARM)
interfaces to using gft_t instead of unsigned long and update all users of
these functions.

There is also an issue in p2m_get_mem_access on x86: 'gfn' parameter passed to
gfn_lock/gfn_unlock is not defined. This code compiles only because of a
coincidence: gfn_lock/gfn_unlock are currently macros which don't use their
second argument.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
Changes since v4:
- In p2m_set_mem_access use local unsigned long gfn_l for looping instead of
  boxing/unboxing [George Dunlap]

Changes since v3:
- Comment codying style fix [Razvan Cojocaru]
- Use INVALID_GFN instead of ~0 and -1 [Andrew Cooper]
- Convert p2m_get_mem_access/p2m_set_mem_access interfaces to using gfn_t
  [Andrew Cooper]

Changes since v2:
- Instead of adding start_ prefix on ARM remove it on x86 [Jan Beulich,
  Ian Campbell, Razvan Cojocaru]

Changes since v1:
- This patch is a successor of '[PATCH] x86/mm: use existing 'pfn' in
  p2m_get_mem_access', instead of fixing gfn_lock/gfn_unlock arguments we do
  s/pfn/gfn/g for both p2m_get_mem_access/p2m_set_mem_access [Andrew Cooper,
  Jan Beulich]

P.S.
- The patch was compile-tested on x86 and ARM64.
---
 xen/arch/arm/p2m.c           | 33 +++++++++++++++++----------------
 xen/arch/x86/mm/p2m.c        | 36 ++++++++++++++++++++----------------
 xen/common/mem_access.c      |  4 ++--
 xen/include/xen/p2m-common.h | 13 ++++++-------
 4 files changed, 45 insertions(+), 41 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 903fa3f..6b9ef33 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -436,7 +436,7 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
     return 0;
 }
 
-static int __p2m_get_mem_access(struct domain *d, unsigned long gpfn,
+static int __p2m_get_mem_access(struct domain *d, gfn_t gfn,
                                 xenmem_access_t *access)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
@@ -465,14 +465,14 @@ static int __p2m_get_mem_access(struct domain *d, unsigned long gpfn,
         return 0;
     }
 
-    /* If request to get default access */
-    if ( gpfn == ~0ul )
+    /* If request to get default access. */
+    if ( gfn_x(gfn) == INVALID_GFN )
     {
         *access = memaccess[p2m->default_access];
         return 0;
     }
 
-    i = radix_tree_lookup(&p2m->mem_access_settings, gpfn);
+    i = radix_tree_lookup(&p2m->mem_access_settings, gfn_x(gfn));
 
     if ( !i )
     {
@@ -480,7 +480,7 @@ static int __p2m_get_mem_access(struct domain *d, unsigned long gpfn,
          * No setting was found in the Radix tree. Check if the
          * entry exists in the page-tables.
          */
-        paddr_t maddr = p2m_lookup(d, gpfn << PAGE_SHIFT, NULL);
+        paddr_t maddr = p2m_lookup(d, gfn_x(gfn) << PAGE_SHIFT, NULL);
         if ( INVALID_PADDR == maddr )
             return -ESRCH;
 
@@ -1386,7 +1386,7 @@ p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
      * 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, paddr_to_pfn(ipa), &xma);
+    rc = __p2m_get_mem_access(current->domain, _gfn(paddr_to_pfn(ipa)), &xma);
     if ( rc < 0 )
         goto err;
 
@@ -1590,7 +1590,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
     if ( !p2m->mem_access_enabled )
         return true;
 
-    rc = p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xma);
+    rc = p2m_get_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), &xma);
     if ( rc )
         return true;
 
@@ -1632,13 +1632,13 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
     /* First, handle rx2rw and n2rwx conversion automatically. */
     if ( npfec.write_access && xma == XENMEM_access_rx2rw )
     {
-        rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
+        rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1,
                                 0, ~0, XENMEM_access_rw);
         return false;
     }
     else if ( xma == XENMEM_access_n2rwx )
     {
-        rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
+        rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1,
                                 0, ~0, XENMEM_access_rwx);
     }
 
@@ -1660,7 +1660,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
             {
                 /* A listener is not required, so clear the access
                  * restrictions. */
-                rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
+                rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1,
                                         0, ~0, XENMEM_access_rwx);
             }
         }
@@ -1709,9 +1709,9 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
 
 /*
  * Set access type for a region of pfns.
- * If start_pfn == -1ul, sets the default access type.
+ * If gfn == INVALID_GFN, sets the default access type.
  */
-long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
+long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
                         uint32_t start, uint32_t mask, xenmem_access_t access)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
@@ -1752,14 +1752,15 @@ long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
     p2m->mem_access_enabled = true;
 
     /* If request to set default access. */
-    if ( pfn == ~0ul )
+    if ( gfn_x(gfn) == INVALID_GFN )
     {
         p2m->default_access = a;
         return 0;
     }
 
     rc = apply_p2m_changes(d, MEMACCESS,
-                           pfn_to_paddr(pfn+start), pfn_to_paddr(pfn+nr),
+                           pfn_to_paddr(gfn_x(gfn) + start),
+                           pfn_to_paddr(gfn_x(gfn) + nr),
                            0, MATTR_MEM, mask, 0, a);
     if ( rc < 0 )
         return rc;
@@ -1769,14 +1770,14 @@ long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
     return 0;
 }
 
-int p2m_get_mem_access(struct domain *d, unsigned long gpfn,
+int p2m_get_mem_access(struct domain *d, gfn_t gfn,
                        xenmem_access_t *access)
 {
     int ret;
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
 
     spin_lock(&p2m->lock);
-    ret = __p2m_get_mem_access(d, gpfn, access);
+    ret = __p2m_get_mem_access(d, gfn, access);
     spin_unlock(&p2m->lock);
 
     return ret;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 6b39733..fd33acc 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1424,7 +1424,7 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
         bool_t violation = 1;
         const struct vm_event_mem_access *data = &rsp->u.mem_access;
 
-        if ( p2m_get_mem_access(v->domain, data->gfn, &access) == 0 )
+        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn), &access) == 0 )
         {
             switch ( access )
             {
@@ -1600,15 +1600,18 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
     return (p2ma == p2m_access_n2rwx);
 }
 
-/* Set access type for a region of pfns.
- * If start_pfn == -1ul, sets the default access type */
-long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
+/*
+ * Set access type for a region of gfns.
+ * If gfn == INVALID_GFN, sets the default access type.
+ */
+long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
                         uint32_t start, uint32_t mask, xenmem_access_t access)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     p2m_access_t a, _a;
     p2m_type_t t;
     mfn_t mfn;
+    unsigned long gfn_l;
     long rc = 0;
 
     static const p2m_access_t memaccess[] = {
@@ -1638,18 +1641,18 @@ long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
         return -EINVAL;
     }
 
-    /* If request to set default access */
-    if ( pfn == ~0ul )
+    /* If request to set default access. */
+    if ( gfn_x(gfn) == INVALID_GFN )
     {
         p2m->default_access = a;
         return 0;
     }
 
     p2m_lock(p2m);
-    for ( pfn += start; nr > start; ++pfn )
+    for ( gfn_l = gfn_x(gfn) + start; nr > start; ++gfn_l )
     {
-        mfn = p2m->get_entry(p2m, pfn, &t, &_a, 0, NULL);
-        rc = p2m->set_entry(p2m, pfn, mfn, PAGE_ORDER_4K, t, a);
+        mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL);
+        rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a);
         if ( rc )
             break;
 
@@ -1664,10 +1667,11 @@ long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
     return rc;
 }
 
-/* Get access type for a pfn
- * If pfn == -1ul, gets the default access type */
-int p2m_get_mem_access(struct domain *d, unsigned long pfn, 
-                       xenmem_access_t *access)
+/*
+ * Get access type for a gfn.
+ * If gfn == INVALID_GFN, gets the default access type.
+ */
+int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     p2m_type_t t;
@@ -1689,15 +1693,15 @@ int p2m_get_mem_access(struct domain *d, unsigned long pfn,
 #undef ACCESS
     };
 
-    /* If request to get default access */
-    if ( pfn == ~0ull ) 
+    /* If request to get default access. */
+    if ( gfn_x(gfn) == INVALID_GFN )
     {
         *access = memaccess[p2m->default_access];
         return 0;
     }
 
     gfn_lock(p2m, gfn, 0);
-    mfn = p2m->get_entry(p2m, pfn, &t, &a, 0, NULL);
+    mfn = p2m->get_entry(p2m, gfn_x(gfn), &t, &a, 0, NULL);
     gfn_unlock(p2m, gfn, 0);
 
     if ( mfn_x(mfn) == INVALID_MFN )
diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c
index 4ed55a6..ad4beaf 100644
--- a/xen/common/mem_access.c
+++ b/xen/common/mem_access.c
@@ -67,7 +67,7 @@ int mem_access_memop(unsigned long cmd,
               ((mao.pfn + mao.nr - 1) > domain_get_maximum_gpfn(d))) )
             break;
 
-        rc = p2m_set_mem_access(d, mao.pfn, mao.nr, start_iter,
+        rc = p2m_set_mem_access(d, _gfn(mao.pfn), mao.nr, start_iter,
                                 MEMOP_CMD_MASK, mao.access);
         if ( rc > 0 )
         {
@@ -89,7 +89,7 @@ int mem_access_memop(unsigned long cmd,
         if ( (mao.pfn > domain_get_maximum_gpfn(d)) && mao.pfn != ~0ull )
             break;
 
-        rc = p2m_get_mem_access(d, mao.pfn, &access);
+        rc = p2m_get_mem_access(d, _gfn(mao.pfn), &access);
         if ( rc != 0 )
             break;
 
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
index bd36826..47c40c7 100644
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -45,17 +45,16 @@ int unmap_mmio_regions(struct domain *d,
                        unsigned long mfn);
 
 /*
- * Set access type for a region of pfns.
- * If start_pfn == -1ul, sets the default access type.
+ * Set access type for a region of gfns.
+ * If gfn == INVALID_GFN, sets the default access type.
  */
-long p2m_set_mem_access(struct domain *d, unsigned long start_pfn, uint32_t nr,
+long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
                         uint32_t start, uint32_t mask, xenmem_access_t access);
 
 /*
- * Get access type for a pfn.
- * If pfn == -1ul, gets the default access type.
+ * Get access type for a gfn.
+ * If gfn == INVALID_GFN, gets the default access type.
  */
-int p2m_get_mem_access(struct domain *d, unsigned long pfn,
-                       xenmem_access_t *access);
+int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access);
 
 #endif /* _XEN_P2M_COMMON_H */
-- 
2.4.3

Re: [PATCH v5] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access

[George Dunlap]

On 07/02/2015 02:18 PM, Vitaly Kuznetsov wrote:
> 'pfn' and 'start_pfn' are ambiguous, both these functions expect GFNs as input.
> 
> On x86 the interface of p2m_set_mem_access() in p2m.c doesn't match the
> declaration in p2m-common.h as 'pfn' is being used instead of 'start_pfn'.
> 
> On ARM both p2m_set_mem_access and p2m_get_mem_access interfaces don't match
> declarations from p2m-common.h: p2m_set_mem_access uses 'pfn' instead of
> 'start_pfn' and p2m_get_mem_access uses 'gpfn' instead of 'pfn'.
> 
> Convert p2m_get_mem_access/p2m_set_mem_access (and __p2m_get_mem_access on ARM)
> interfaces to using gft_t instead of unsigned long and update all users of
> these functions.
> 
> There is also an issue in p2m_get_mem_access on x86: 'gfn' parameter passed to
> gfn_lock/gfn_unlock is not defined. This code compiles only because of a
> coincidence: gfn_lock/gfn_unlock are currently macros which don't use their
> second argument.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>

Re: [PATCH v5] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access

[Julien Grall]

Hi,

On 02/07/15 14:18, Vitaly Kuznetsov wrote:
> 'pfn' and 'start_pfn' are ambiguous, both these functions expect GFNs as input.
> 
> On x86 the interface of p2m_set_mem_access() in p2m.c doesn't match the
> declaration in p2m-common.h as 'pfn' is being used instead of 'start_pfn'.
> 
> On ARM both p2m_set_mem_access and p2m_get_mem_access interfaces don't match
> declarations from p2m-common.h: p2m_set_mem_access uses 'pfn' instead of
> 'start_pfn' and p2m_get_mem_access uses 'gpfn' instead of 'pfn'.
> 
> Convert p2m_get_mem_access/p2m_set_mem_access (and __p2m_get_mem_access on ARM)
> interfaces to using gft_t instead of unsigned long and update all users of
> these functions.
> 
> There is also an issue in p2m_get_mem_access on x86: 'gfn' parameter passed to
> gfn_lock/gfn_unlock is not defined. This code compiles only because of a
> coincidence: gfn_lock/gfn_unlock are currently macros which don't use their
> second argument.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

For the ARM bits:

Reviewed-by: Julien Grall <julien.grall@citrix.com>

Regards,

-- 
Julien Grall

Re: [PATCH v5] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access

[Jan Beulich]

>>> On 02.07.15 at 18:14, <julien.grall@citrix.com> wrote:
> On 02/07/15 14:18, Vitaly Kuznetsov wrote:
>> 'pfn' and 'start_pfn' are ambiguous, both these functions expect GFNs as input.
>> 
>> On x86 the interface of p2m_set_mem_access() in p2m.c doesn't match the
>> declaration in p2m-common.h as 'pfn' is being used instead of 'start_pfn'.
>> 
>> On ARM both p2m_set_mem_access and p2m_get_mem_access interfaces don't match
>> declarations from p2m-common.h: p2m_set_mem_access uses 'pfn' instead of
>> 'start_pfn' and p2m_get_mem_access uses 'gpfn' instead of 'pfn'.
>> 
>> Convert p2m_get_mem_access/p2m_set_mem_access (and __p2m_get_mem_access on ARM)
>> interfaces to using gft_t instead of unsigned long and update all users of
>> these functions.
>> 
>> There is also an issue in p2m_get_mem_access on x86: 'gfn' parameter passed to
>> gfn_lock/gfn_unlock is not defined. This code compiles only because of a
>> coincidence: gfn_lock/gfn_unlock are currently macros which don't use their
>> second argument.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> 
> For the ARM bits:
> 
> Reviewed-by: Julien Grall <julien.grall@citrix.com>

Any chance to get an ARM maintainer ack on this one?

Thanks, Jan

Re: [PATCH v5] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access

[Ian Campbell]

On Fri, 2015-07-10 at 11:43 +0100, Jan Beulich wrote:
> >>> On 02.07.15 at 18:14, <julien.grall@citrix.com> wrote:
> > On 02/07/15 14:18, Vitaly Kuznetsov wrote:
> >> 'pfn' and 'start_pfn' are ambiguous, both these functions expect GFNs as input.
> >> 
> >> On x86 the interface of p2m_set_mem_access() in p2m.c doesn't match the
> >> declaration in p2m-common.h as 'pfn' is being used instead of 'start_pfn'.
> >> 
> >> On ARM both p2m_set_mem_access and p2m_get_mem_access interfaces don't match
> >> declarations from p2m-common.h: p2m_set_mem_access uses 'pfn' instead of
> >> 'start_pfn' and p2m_get_mem_access uses 'gpfn' instead of 'pfn'.
> >> 
> >> Convert p2m_get_mem_access/p2m_set_mem_access (and __p2m_get_mem_access on ARM)
> >> interfaces to using gft_t instead of unsigned long and update all users of
> >> these functions.
> >> 
> >> There is also an issue in p2m_get_mem_access on x86: 'gfn' parameter passed to
> >> gfn_lock/gfn_unlock is not defined. This code compiles only because of a
> >> coincidence: gfn_lock/gfn_unlock are currently macros which don't use their
> >> second argument.
> >> 
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > 
> > For the ARM bits:
> > 
> > Reviewed-by: Julien Grall <julien.grall@citrix.com>
> 
> Any chance to get an ARM maintainer ack on this one?

On the basis of Julien's R-by:
Acked-by: Ian Campbell <ian.campbell@citrix.com>