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

[PATCH v4] 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 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 1fd1194..a7d6596 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,9 +1600,11 @@ 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);
@@ -1638,18 +1640,19 @@ 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 = _gfn(gfn_x(gfn) + start); nr > start;
+          gfn = _gfn(gfn_x(gfn) + 1) )
     {
-        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_x(gfn), &t, &_a, 0, NULL);
+        rc = p2m->set_entry(p2m, gfn_x(gfn), 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 v4] x86/arm/mm: use gfn instead of pfn in p2m_get_mem_access/p2m_set_mem_access

[Andrew Cooper]

On 29/06/15 16:45, 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: Andrew Cooper <andrew.cooper3@citrix.com>

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

[Razvan Cojocaru]

On 06/29/2015 06:45 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>
> ---
> 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(-)

Reviewed-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

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

[Ian Campbell]

On Mon, 2015-06-29 at 17:45 +0200, 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 ARM:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

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

[George Dunlap]

On 06/29/2015 04:45 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>
> ---
> 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]

But you missed a change...

> @@ -1600,9 +1600,11 @@ 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);
> @@ -1638,18 +1640,19 @@ 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 = _gfn(gfn_x(gfn) + start); nr > start;
> +          gfn = _gfn(gfn_x(gfn) + 1) )

Now in p2m_set_mem_access(), rather than just using an unsigned long in
the loop iterating over gfns, you do this thing where you convert gfn_t
to unsigned long, add one, and then convert it back to gfn_t again.

I can't see any comments in v3 that suggest you doing that, and it seems
a bit clunky.  Is that really necessary?  Wouldn't it be better to
declare a local variable?

I'm not strongly opinionated on this one, it just seems a bit strange.

Everything else looks good, thanks.

 -George

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

[Andrew Cooper]

On 02/07/15 11:48, George Dunlap wrote:
> On 06/29/2015 04:45 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>
>> ---
>> 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]
> But you missed a change...
>
>> @@ -1600,9 +1600,11 @@ 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);
>> @@ -1638,18 +1640,19 @@ 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 = _gfn(gfn_x(gfn) + start); nr > start;
>> +          gfn = _gfn(gfn_x(gfn) + 1) )
> Now in p2m_set_mem_access(), rather than just using an unsigned long in
> the loop iterating over gfns, you do this thing where you convert gfn_t
> to unsigned long, add one, and then convert it back to gfn_t again.
>
> I can't see any comments in v3 that suggest you doing that, and it seems
> a bit clunky.  Is that really necessary?  Wouldn't it be better to
> declare a local variable?
>
> I'm not strongly opinionated on this one, it just seems a bit strange.
>
> Everything else looks good, thanks.

Looping over {g,m,p}fn_t's is indeed awkward, as the compiler tricks for
typesafety don't allow for simply adding 1 to a typesafe variable.

In a cases like this, I think it is acceptable to keep a unsigned long
shadow variable and manipulate it is a plain integer.  The eventual
_gfn() required to pass it further down the callchain will help to
visually re-enforce the appropriate type.

After all, the entire point of these typesafes are to try and avoid
accidentally mixing up the different address spaces, but a function
which takes a typesafe, loops over a subset and passes the same typesafe
further down can probably be trusted to DTRT, catching errors at review
time. 

Off the top of my head, the only functions which would normally expect
to mix and match the typesafes are the pagetable walking ones.

~Andrew

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

[Tim Deegan]

At 12:09 +0100 on 02 Jul (1435838956), Andrew Cooper wrote:
> On 02/07/15 11:48, George Dunlap wrote:
> > Now in p2m_set_mem_access(), rather than just using an unsigned long in
> > the loop iterating over gfns, you do this thing where you convert gfn_t
> > to unsigned long, add one, and then convert it back to gfn_t again.
> >
> > I can't see any comments in v3 that suggest you doing that, and it seems
> > a bit clunky.  Is that really necessary?  Wouldn't it be better to
> > declare a local variable?
> >
> > I'm not strongly opinionated on this one, it just seems a bit strange.
> >
> > Everything else looks good, thanks.
> 
> Looping over {g,m,p}fn_t's is indeed awkward, as the compiler tricks for
> typesafety don't allow for simply adding 1 to a typesafe variable.
> 
> In a cases like this, I think it is acceptable to keep a unsigned long
> shadow variable and manipulate it is a plain integer.  The eventual
> _gfn() required to pass it further down the callchain will help to
> visually re-enforce the appropriate type.
> 
> After all, the entire point of these typesafes are to try and avoid
> accidentally mixing up the different address spaces, but a function
> which takes a typesafe, loops over a subset and passes the same typesafe
> further down can probably be trusted to DTRT, catching errors at review
> time. 
> 
> Off the top of my head, the only functions which would normally expect
> to mix and match the typesafes are the pagetable walking ones.

It should be easy enough to extend the macros to define a
gfn_inc(&gfn_t) operator for this kind of thing.

Tim.

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

[George Dunlap]

On 07/02/2015 12:25 PM, Tim Deegan wrote:
> At 12:09 +0100 on 02 Jul (1435838956), Andrew Cooper wrote:
>> On 02/07/15 11:48, George Dunlap wrote:
>>> Now in p2m_set_mem_access(), rather than just using an unsigned long in
>>> the loop iterating over gfns, you do this thing where you convert gfn_t
>>> to unsigned long, add one, and then convert it back to gfn_t again.
>>>
>>> I can't see any comments in v3 that suggest you doing that, and it seems
>>> a bit clunky.  Is that really necessary?  Wouldn't it be better to
>>> declare a local variable?
>>>
>>> I'm not strongly opinionated on this one, it just seems a bit strange.
>>>
>>> Everything else looks good, thanks.
>>
>> Looping over {g,m,p}fn_t's is indeed awkward, as the compiler tricks for
>> typesafety don't allow for simply adding 1 to a typesafe variable.
>>
>> In a cases like this, I think it is acceptable to keep a unsigned long
>> shadow variable and manipulate it is a plain integer.  The eventual
>> _gfn() required to pass it further down the callchain will help to
>> visually re-enforce the appropriate type.
>>
>> After all, the entire point of these typesafes are to try and avoid
>> accidentally mixing up the different address spaces, but a function
>> which takes a typesafe, loops over a subset and passes the same typesafe
>> further down can probably be trusted to DTRT, catching errors at review
>> time. 
>>
>> Off the top of my head, the only functions which would normally expect
>> to mix and match the typesafes are the pagetable walking ones.
> 
> It should be easy enough to extend the macros to define a
> gfn_inc(&gfn_t) operator for this kind of thing.

I was thinking that -- although in this case you'd still need to un-pack
it to do the loop exit conditional.  To really make things pretty you'd
want a for_gfn_range() macro or something like that that takes a start
gfn and a number.

But that's really starting to be feature creep for this patch, which is
why I didn't want to suggest it on v4. :-)

 -George

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

[Andrew Cooper]

On 02/07/15 12:25, Tim Deegan wrote:
> At 12:09 +0100 on 02 Jul (1435838956), Andrew Cooper wrote:
>> On 02/07/15 11:48, George Dunlap wrote:
>>> Now in p2m_set_mem_access(), rather than just using an unsigned long in
>>> the loop iterating over gfns, you do this thing where you convert gfn_t
>>> to unsigned long, add one, and then convert it back to gfn_t again.
>>>
>>> I can't see any comments in v3 that suggest you doing that, and it seems
>>> a bit clunky.  Is that really necessary?  Wouldn't it be better to
>>> declare a local variable?
>>>
>>> I'm not strongly opinionated on this one, it just seems a bit strange.
>>>
>>> Everything else looks good, thanks.
>> Looping over {g,m,p}fn_t's is indeed awkward, as the compiler tricks for
>> typesafety don't allow for simply adding 1 to a typesafe variable.
>>
>> In a cases like this, I think it is acceptable to keep a unsigned long
>> shadow variable and manipulate it is a plain integer.  The eventual
>> _gfn() required to pass it further down the callchain will help to
>> visually re-enforce the appropriate type.
>>
>> After all, the entire point of these typesafes are to try and avoid
>> accidentally mixing up the different address spaces, but a function
>> which takes a typesafe, loops over a subset and passes the same typesafe
>> further down can probably be trusted to DTRT, catching errors at review
>> time. 
>>
>> Off the top of my head, the only functions which would normally expect
>> to mix and match the typesafes are the pagetable walking ones.
> It should be easy enough to extend the macros to define a
> gfn_inc(&gfn_t) operator for this kind of thing.

Very true.  I suspect inc and dec will cover the majority of the
unbox/modify/box bits we currently have.

OTOH, ideally I would prefer not to introduce even more magic which
cscope/tags can't spot, but introducing gfn_inc() does appear to be the
neater solution.

~Andrew

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

[Vitaly Kuznetsov]

George Dunlap <george.dunlap@eu.citrix.com> writes:

> On 07/02/2015 12:25 PM, Tim Deegan wrote:
>> At 12:09 +0100 on 02 Jul (1435838956), Andrew Cooper wrote:
>>> On 02/07/15 11:48, George Dunlap wrote:
>>>> Now in p2m_set_mem_access(), rather than just using an unsigned long in
>>>> the loop iterating over gfns, you do this thing where you convert gfn_t
>>>> to unsigned long, add one, and then convert it back to gfn_t again.
>>>>
>>>> I can't see any comments in v3 that suggest you doing that, and it seems
>>>> a bit clunky.  Is that really necessary?  Wouldn't it be better to
>>>> declare a local variable?
>>>>
>>>> I'm not strongly opinionated on this one, it just seems a bit strange.
>>>>
>>>> Everything else looks good, thanks.
>>>
>>> Looping over {g,m,p}fn_t's is indeed awkward, as the compiler tricks for
>>> typesafety don't allow for simply adding 1 to a typesafe variable.
>>>
>>> In a cases like this, I think it is acceptable to keep a unsigned long
>>> shadow variable and manipulate it is a plain integer.  The eventual
>>> _gfn() required to pass it further down the callchain will help to
>>> visually re-enforce the appropriate type.
>>>
>>> After all, the entire point of these typesafes are to try and avoid
>>> accidentally mixing up the different address spaces, but a function
>>> which takes a typesafe, loops over a subset and passes the same typesafe
>>> further down can probably be trusted to DTRT, catching errors at review
>>> time. 
>>>
>>> Off the top of my head, the only functions which would normally expect
>>> to mix and match the typesafes are the pagetable walking ones.
>> 
>> It should be easy enough to extend the macros to define a
>> gfn_inc(&gfn_t) operator for this kind of thing.
>
> I was thinking that -- although in this case you'd still need to un-pack
> it to do the loop exit conditional.  To really make things pretty you'd
> want a for_gfn_range() macro or something like that that takes a start
> gfn and a number.
>
> But that's really starting to be feature creep for this patch, which is
> why I didn't want to suggest it on v4. :-)

Well, if you look at what I was fixing in v1 ... :-)

I suggest we add a local unsigned long here and close the deal, when
{g,m,p}fn_{inc,dec} macros are available we'll get rid of it.

-- 
  Vitaly

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

[George Dunlap]

On 07/02/2015 01:26 PM, Vitaly Kuznetsov wrote:
> George Dunlap <george.dunlap@eu.citrix.com> writes:
> 
>> On 07/02/2015 12:25 PM, Tim Deegan wrote:
>>> At 12:09 +0100 on 02 Jul (1435838956), Andrew Cooper wrote:
>>>> On 02/07/15 11:48, George Dunlap wrote:
>>>>> Now in p2m_set_mem_access(), rather than just using an unsigned long in
>>>>> the loop iterating over gfns, you do this thing where you convert gfn_t
>>>>> to unsigned long, add one, and then convert it back to gfn_t again.
>>>>>
>>>>> I can't see any comments in v3 that suggest you doing that, and it seems
>>>>> a bit clunky.  Is that really necessary?  Wouldn't it be better to
>>>>> declare a local variable?
>>>>>
>>>>> I'm not strongly opinionated on this one, it just seems a bit strange.
>>>>>
>>>>> Everything else looks good, thanks.
>>>>
>>>> Looping over {g,m,p}fn_t's is indeed awkward, as the compiler tricks for
>>>> typesafety don't allow for simply adding 1 to a typesafe variable.
>>>>
>>>> In a cases like this, I think it is acceptable to keep a unsigned long
>>>> shadow variable and manipulate it is a plain integer.  The eventual
>>>> _gfn() required to pass it further down the callchain will help to
>>>> visually re-enforce the appropriate type.
>>>>
>>>> After all, the entire point of these typesafes are to try and avoid
>>>> accidentally mixing up the different address spaces, but a function
>>>> which takes a typesafe, loops over a subset and passes the same typesafe
>>>> further down can probably be trusted to DTRT, catching errors at review
>>>> time. 
>>>>
>>>> Off the top of my head, the only functions which would normally expect
>>>> to mix and match the typesafes are the pagetable walking ones.
>>>
>>> It should be easy enough to extend the macros to define a
>>> gfn_inc(&gfn_t) operator for this kind of thing.
>>
>> I was thinking that -- although in this case you'd still need to un-pack
>> it to do the loop exit conditional.  To really make things pretty you'd
>> want a for_gfn_range() macro or something like that that takes a start
>> gfn and a number.
>>
>> But that's really starting to be feature creep for this patch, which is
>> why I didn't want to suggest it on v4. :-)
> 
> Well, if you look at what I was fixing in v1 ... :-)
> 
> I suggest we add a local unsigned long here and close the deal, when
> {g,m,p}fn_{inc,dec} macros are available we'll get rid of it.

+1

 -G

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

[Tim Deegan]

At 12:32 +0100 on 02 Jul (1435840328), George Dunlap wrote:
> On 07/02/2015 12:25 PM, Tim Deegan wrote:
> > At 12:09 +0100 on 02 Jul (1435838956), Andrew Cooper wrote:
> >> On 02/07/15 11:48, George Dunlap wrote:
> >>> Now in p2m_set_mem_access(), rather than just using an unsigned long in
> >>> the loop iterating over gfns, you do this thing where you convert gfn_t
> >>> to unsigned long, add one, and then convert it back to gfn_t again.
> >>>
> >>> I can't see any comments in v3 that suggest you doing that, and it seems
> >>> a bit clunky.  Is that really necessary?  Wouldn't it be better to
> >>> declare a local variable?
> >>>
> >>> I'm not strongly opinionated on this one, it just seems a bit strange.
> >>>
> >>> Everything else looks good, thanks.
> >>
> >> Looping over {g,m,p}fn_t's is indeed awkward, as the compiler tricks for
> >> typesafety don't allow for simply adding 1 to a typesafe variable.
> >>
> >> In a cases like this, I think it is acceptable to keep a unsigned long
> >> shadow variable and manipulate it is a plain integer.  The eventual
> >> _gfn() required to pass it further down the callchain will help to
> >> visually re-enforce the appropriate type.
> >>
> >> After all, the entire point of these typesafes are to try and avoid
> >> accidentally mixing up the different address spaces, but a function
> >> which takes a typesafe, loops over a subset and passes the same typesafe
> >> further down can probably be trusted to DTRT, catching errors at review
> >> time. 
> >>
> >> Off the top of my head, the only functions which would normally expect
> >> to mix and match the typesafes are the pagetable walking ones.
> > 
> > It should be easy enough to extend the macros to define a
> > gfn_inc(&gfn_t) operator for this kind of thing.
> 
> I was thinking that -- although in this case you'd still need to un-pack
> it to do the loop exit conditional.  To really make things pretty you'd
> want a for_gfn_range() macro or something like that that takes a start
> gfn and a number.
> 
> But that's really starting to be feature creep for this patch, which is
> why I didn't want to suggest it on v4. :-)

Oh, quite. :)

Tim.