[PATCH v3 1/3] xen-access: minor fixes

[PATCH v3 1/3] xen-access: minor fixes

[Tamas K Lengyel]

Only copy the VCPU_PAUSED flag to the response. Copy the entire mem_access
struct which is useful and easily forgotten when also testing the emulate
response flags. Turn off singlestepping on the vCPUs once we are done
processing all events, as we might have turned on singlestep there and leave
the VM in an undesirable state.

Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/tests/xen-access/xen-access.c | 46 ++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index 7993947..ef89246 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -537,16 +537,10 @@ int main(int argc, char *argv[])
 
             if ( altp2m )
             {
-                uint32_t vcpu_id;
-
                 rc = xc_altp2m_switch_to_view( xch, domain_id, 0 );
                 rc = xc_altp2m_destroy_view(xch, domain_id, altp2m_view_id);
                 rc = xc_altp2m_set_domain_state(xch, domain_id, 0);
                 rc = xc_monitor_singlestep(xch, domain_id, 0);
-
-                for ( vcpu_id = 0; vcpu_id<XEN_LEGACY_MAX_VCPUS; vcpu_id++)
-                    rc = control_singlestep(xch, domain_id, vcpu_id, 0);
-
             } else {
                 rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, ~0ull, 0);
                 rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, START_PFN,
@@ -570,8 +564,6 @@ int main(int argc, char *argv[])
 
         while ( RING_HAS_UNCONSUMED_REQUESTS(&xenaccess->vm_event.back_ring) )
         {
-            xenmem_access_t access;
-
             get_request(&xenaccess->vm_event, &req);
 
             if ( req.version != VM_EVENT_INTERFACE_VERSION )
@@ -584,16 +576,25 @@ int main(int argc, char *argv[])
             memset( &rsp, 0, sizeof (rsp) );
             rsp.version = VM_EVENT_INTERFACE_VERSION;
             rsp.vcpu_id = req.vcpu_id;
-            rsp.flags = req.flags;
+            rsp.flags = (req.flags & VM_EVENT_FLAG_VCPU_PAUSED);
+            rsp.reason = req.reason;
 
             switch (req.reason) {
             case VM_EVENT_REASON_MEM_ACCESS:
-                rc = xc_get_mem_access(xch, domain_id, req.u.mem_access.gfn, &access);
-                if (rc < 0)
+                if ( !shutting_down )
                 {
-                    ERROR("Error %d getting mem_access event\n", rc);
-                    interrupted = -1;
-                    continue;
+                    /*
+                     * This serves no other purpose here then demonstrating the use of the API.
+                     * At shutdown we have already reset all the permissions so really no use getting it again.
+                     */
+                    xenmem_access_t access;
+                    rc = xc_get_mem_access(xch, domain_id, req.u.mem_access.gfn, &access);
+                    if (rc < 0)
+                    {
+                        ERROR("Error %d getting mem_access event\n", rc);
+                        interrupted = -1;
+                        continue;
+                    }
                 }
 
                 printf("PAGE ACCESS: %c%c%c for GFN %"PRIx64" (offset %06"
@@ -614,11 +615,8 @@ int main(int argc, char *argv[])
                 {
                     DPRINTF("\tSwitching back to default view!\n");
 
-                    rsp.reason = req.reason;
-                    rsp.flags = req.flags;
+                    rsp.flags |= VM_EVENT_FLAG_TOGGLE_SINGLESTEP;
                     rsp.altp2m_idx = 0;
-
-                    control_singlestep(xch, domain_id, rsp.vcpu_id, 1);
                 }
                 else if ( default_access != after_first_access )
                 {
@@ -633,7 +631,7 @@ int main(int argc, char *argv[])
                     }
                 }
 
-                rsp.u.mem_access.gfn = req.u.mem_access.gfn;
+                rsp.u.mem_access = req.u.mem_access;
                 break;
             case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
                 printf("Breakpoint: rip=%016"PRIx64", gfn=%"PRIx64" (vcpu %d)\n",
@@ -662,12 +660,11 @@ int main(int argc, char *argv[])
                 {
                     printf("\tSwitching altp2m to view %u!\n", altp2m_view_id);
 
-                    rsp.reason = req.reason;
                     rsp.flags |= VM_EVENT_FLAG_ALTERNATE_P2M;
                     rsp.altp2m_idx = altp2m_view_id;
                 }
 
-                control_singlestep(xch, domain_id, req.vcpu_id, 0);
+                rsp.flags |= VM_EVENT_FLAG_TOGGLE_SINGLESTEP;
 
                 break;
             default:
@@ -694,6 +691,13 @@ int main(int argc, char *argv[])
     DPRINTF("xenaccess shut down on signal %d\n", interrupted);
 
 exit:
+    if ( altp2m )
+    {
+        uint32_t vcpu_id;
+        for ( vcpu_id = 0; vcpu_id<XEN_LEGACY_MAX_VCPUS; vcpu_id++)
+            rc = control_singlestep(xch, domain_id, vcpu_id, 0);
+    }
+
     /* Tear down domain xenaccess */
     rc1 = xenaccess_teardown(xch, xenaccess);
     if ( rc1 != 0 )
-- 
2.1.4

[PATCH v3 2/3] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Tamas K Lengyel]

The altp2m subsystem in its current form duplicates much of the existing
code present in p2m for setting mem_access permissions. In this patch we
consolidate the two versions but keep the separate MEMOP and HVMOP interfaces.

Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
v3: Keep the external-facing interfaces and tools as they are
    Pass gfn_t and make p2m_set_altp2m_mem_access inline
    Remove goto and just return rc directly
v2: Don't deprecate the HVMOP hypercall for setting mem_access
    Use unsigned int instead of unsigned long
---
 xen/arch/arm/p2m.c           |   9 +--
 xen/arch/x86/hvm/hvm.c       |   6 +-
 xen/arch/x86/mm/p2m.c        | 168 +++++++++++++++++++------------------------
 xen/common/mem_access.c      |   2 +-
 xen/include/asm-x86/p2m.h    |   4 --
 xen/include/xen/p2m-common.h |   3 +-
 6 files changed, 86 insertions(+), 106 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 2190908..8568087 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1709,13 +1709,13 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
     if ( npfec.write_access && xma == XENMEM_access_rx2rw )
     {
         rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1,
-                                0, ~0, XENMEM_access_rw);
+                                0, ~0, XENMEM_access_rw, 0);
         return false;
     }
     else if ( xma == XENMEM_access_n2rwx )
     {
         rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1,
-                                0, ~0, XENMEM_access_rwx);
+                                0, ~0, XENMEM_access_rwx, 0);
     }
 
     /* Otherwise, check if there is a vm_event monitor subscriber */
@@ -1737,7 +1737,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, _gfn(paddr_to_pfn(gpa)), 1,
-                                        0, ~0, XENMEM_access_rwx);
+                                        0, ~0, XENMEM_access_rwx, 0);
             }
         }
 
@@ -1788,7 +1788,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
  * 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)
+                        uint32_t start, uint32_t mask, xenmem_access_t access,
+                        unsigned int altp2m_idx)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     p2m_access_t a;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 674feea..37305fb 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -6398,9 +6398,9 @@ static int do_altp2m_op(
         if ( a.u.set_mem_access.pad )
             rc = -EINVAL;
         else
-            rc = p2m_set_altp2m_mem_access(d, a.u.set_mem_access.view,
-                    _gfn(a.u.set_mem_access.gfn),
-                    a.u.set_mem_access.hvmmem_access);
+            rc = p2m_set_mem_access(d, _gfn(a.u.set_mem_access.gfn), 1, 0, 0,
+                                    a.u.set_mem_access.hvmmem_access,
+                                    a.u.set_mem_access.view);
         break;
 
     case HVMOP_altp2m_change_gfn:
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index a45ee35..226490a 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1777,14 +1777,56 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
     return (p2ma == p2m_access_n2rwx);
 }
 
+static inline
+int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
+                              struct p2m_domain *ap2m, p2m_access_t a,
+                              gfn_t gfn)
+{
+    mfn_t mfn;
+    p2m_type_t t;
+    p2m_access_t old_a;
+    unsigned int page_order;
+    unsigned long gfn_l = gfn_x(gfn);
+    int rc;
+
+    mfn = ap2m->get_entry(ap2m, gfn_l, &t, &old_a, 0, NULL, NULL);
+
+    /* Check host p2m if no valid entry in alternate */
+    if ( !mfn_valid(mfn) )
+    {
+        mfn = hp2m->get_entry(hp2m, gfn_l, &t, &old_a,
+                              P2M_ALLOC | P2M_UNSHARE, &page_order, NULL);
+
+        rc = -ESRCH;
+        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
+            return rc;
+
+        /* If this is a superpage, copy that first */
+        if ( page_order != PAGE_ORDER_4K )
+        {
+            unsigned long mask = ~((1UL << page_order) - 1);
+            unsigned long gfn2_l = gfn_l & mask;
+            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
+
+            rc = ap2m->set_entry(ap2m, gfn2_l, mfn2, page_order, t, old_a, 1);
+            if ( rc )
+                return rc;
+        }
+    }
+
+    return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
+                         (current->domain != d));
+}
+
 /*
  * 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)
+                        uint32_t start, uint32_t mask, xenmem_access_t access,
+                        unsigned int altp2m_idx)
 {
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
     p2m_access_t a, _a;
     p2m_type_t t;
     mfn_t mfn;
@@ -1806,6 +1848,16 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
 #undef ACCESS
     };
 
+    /* altp2m view 0 is treated as the hostp2m */
+    if ( altp2m_idx )
+    {
+        if ( altp2m_idx >= MAX_ALTP2M ||
+             d->arch.altp2m_eptp[altp2m_idx] == INVALID_MFN )
+            return -EINVAL;
+
+        ap2m = d->arch.altp2m_p2m[altp2m_idx];
+    }
+
     switch ( access )
     {
     case 0 ... ARRAY_SIZE(memaccess) - 1:
@@ -1826,12 +1878,25 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
     }
 
     p2m_lock(p2m);
+    if ( ap2m )
+        p2m_lock(ap2m);
+
     for ( gfn_l = gfn_x(gfn) + start; nr > start; ++gfn_l )
     {
-        mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL);
-        rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
-        if ( rc )
-            break;
+        if ( ap2m )
+        {
+            rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, _gfn(gfn_l));
+            /* If the corresponding mfn is invalid we will just skip it */
+            if ( rc && rc != -ESRCH )
+                break;
+        }
+        else
+        {
+            mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL);
+            rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
+            if ( rc )
+                break;
+        }
 
         /* Check for continuation if it's not the last iteration. */
         if ( nr > ++start && !(start & mask) && hypercall_preempt_check() )
@@ -1840,7 +1905,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
             break;
         }
     }
+
+    if ( ap2m )
+        p2m_unlock(ap2m);
     p2m_unlock(p2m);
+
     return rc;
 }
 
@@ -2395,93 +2464,6 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx)
     return rc;
 }
 
-int p2m_set_altp2m_mem_access(struct domain *d, unsigned int idx,
-                              gfn_t gfn, xenmem_access_t access)
-{
-    struct p2m_domain *hp2m, *ap2m;
-    p2m_access_t req_a, old_a;
-    p2m_type_t t;
-    mfn_t mfn;
-    unsigned int page_order;
-    int rc = -EINVAL;
-
-    static const p2m_access_t memaccess[] = {
-#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
-        ACCESS(n),
-        ACCESS(r),
-        ACCESS(w),
-        ACCESS(rw),
-        ACCESS(x),
-        ACCESS(rx),
-        ACCESS(wx),
-        ACCESS(rwx),
-#undef ACCESS
-    };
-
-    if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == INVALID_MFN )
-        return rc;
-
-    ap2m = d->arch.altp2m_p2m[idx];
-
-    switch ( access )
-    {
-    case 0 ... ARRAY_SIZE(memaccess) - 1:
-        req_a = memaccess[access];
-        break;
-    case XENMEM_access_default:
-        req_a = ap2m->default_access;
-        break;
-    default:
-        return rc;
-    }
-
-    /* If request to set default access */
-    if ( gfn_x(gfn) == INVALID_GFN )
-    {
-        ap2m->default_access = req_a;
-        return 0;
-    }
-
-    hp2m = p2m_get_hostp2m(d);
-
-    p2m_lock(ap2m);
-
-    mfn = ap2m->get_entry(ap2m, gfn_x(gfn), &t, &old_a, 0, NULL, NULL);
-
-    /* Check host p2m if no valid entry in alternate */
-    if ( !mfn_valid(mfn) )
-    {
-        mfn = hp2m->get_entry(hp2m, gfn_x(gfn), &t, &old_a,
-                              P2M_ALLOC | P2M_UNSHARE, &page_order, NULL);
-
-        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
-            goto out;
-
-        /* If this is a superpage, copy that first */
-        if ( page_order != PAGE_ORDER_4K )
-        {
-            gfn_t gfn2;
-            unsigned long mask;
-            mfn_t mfn2;
-
-            mask = ~((1UL << page_order) - 1);
-            gfn2 = _gfn(gfn_x(gfn) & mask);
-            mfn2 = _mfn(mfn_x(mfn) & mask);
-
-            if ( ap2m->set_entry(ap2m, gfn_x(gfn2), mfn2, page_order, t, old_a, 1) )
-                goto out;
-        }
-    }
-
-    if ( !ap2m->set_entry(ap2m, gfn_x(gfn), mfn, PAGE_ORDER_4K, t, req_a,
-                          (current->domain != d)) )
-        rc = 0;
-
- out:
-    p2m_unlock(ap2m);
-    return rc;
-}
-
 int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
                           gfn_t old_gfn, gfn_t new_gfn)
 {
diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c
index 159c036..92ebead 100644
--- a/xen/common/mem_access.c
+++ b/xen/common/mem_access.c
@@ -67,7 +67,7 @@ int mem_access_memop(unsigned long cmd,
             break;
 
         rc = p2m_set_mem_access(d, _gfn(mao.pfn), mao.nr, start_iter,
-                                MEMOP_CMD_MASK, mao.access);
+                                MEMOP_CMD_MASK, mao.access, 0);
         if ( rc > 0 )
         {
             ASSERT(!(rc & MEMOP_CMD_MASK));
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index fa46dd9..c0df1ea 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -808,10 +808,6 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx);
 /* Switch alternate p2m for entire domain */
 int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx);
 
-/* Set access type for a gfn */
-int p2m_set_altp2m_mem_access(struct domain *d, unsigned int idx,
-                              gfn_t gfn, xenmem_access_t access);
-
 /* Change a gfn->mfn mapping */
 int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
                           gfn_t old_gfn, gfn_t new_gfn);
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
index 47c40c7..8b70459 100644
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -49,7 +49,8 @@ int unmap_mmio_regions(struct domain *d,
  * 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);
+                        uint32_t start, uint32_t mask, xenmem_access_t access,
+                        unsigned int altp2m_idx);
 
 /*
  * Get access type for a gfn.
-- 
2.1.4

Re: [PATCH v3 2/3] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Razvan Cojocaru]

On 02/05/2016 11:22 PM, Tamas K Lengyel wrote:
> The altp2m subsystem in its current form duplicates much of the existing
> code present in p2m for setting mem_access permissions. In this patch we
> consolidate the two versions but keep the separate MEMOP and HVMOP interfaces.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> ---
> v3: Keep the external-facing interfaces and tools as they are
>     Pass gfn_t and make p2m_set_altp2m_mem_access inline
>     Remove goto and just return rc directly
> v2: Don't deprecate the HVMOP hypercall for setting mem_access
>     Use unsigned int instead of unsigned long
> ---
>  xen/arch/arm/p2m.c           |   9 +--
>  xen/arch/x86/hvm/hvm.c       |   6 +-
>  xen/arch/x86/mm/p2m.c        | 168 +++++++++++++++++++------------------------
>  xen/common/mem_access.c      |   2 +-
>  xen/include/asm-x86/p2m.h    |   4 --
>  xen/include/xen/p2m-common.h |   3 +-
>  6 files changed, 86 insertions(+), 106 deletions(-)

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


Thanks,
Razvan

Re: [PATCH v3 2/3] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[Ian Campbell]

On Fri, 2016-02-05 at 14:22 -0700, Tamas K Lengyel wrote:
> The altp2m subsystem in its current form duplicates much of the existing
> code present in p2m for setting mem_access permissions. In this patch we
> consolidate the two versions but keep the separate MEMOP and HVMOP
> interfaces.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>

For the ARM side:

    Acked-by: Ian Campbell <    ian.campbell@citrix.com    >

Ian.

Re: [PATCH v3 2/3] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

[George Dunlap]

On 05/02/16 21:22, Tamas K Lengyel wrote:
> The altp2m subsystem in its current form duplicates much of the existing
> code present in p2m for setting mem_access permissions. In this patch we
> consolidate the two versions but keep the separate MEMOP and HVMOP interfaces.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>

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

Sorry for the delay!

> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> ---
> v3: Keep the external-facing interfaces and tools as they are
>     Pass gfn_t and make p2m_set_altp2m_mem_access inline
>     Remove goto and just return rc directly
> v2: Don't deprecate the HVMOP hypercall for setting mem_access
>     Use unsigned int instead of unsigned long
> ---
>  xen/arch/arm/p2m.c           |   9 +--
>  xen/arch/x86/hvm/hvm.c       |   6 +-
>  xen/arch/x86/mm/p2m.c        | 168 +++++++++++++++++++------------------------
>  xen/common/mem_access.c      |   2 +-
>  xen/include/asm-x86/p2m.h    |   4 --
>  xen/include/xen/p2m-common.h |   3 +-
>  6 files changed, 86 insertions(+), 106 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 2190908..8568087 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1709,13 +1709,13 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
>      if ( npfec.write_access && xma == XENMEM_access_rx2rw )
>      {
>          rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1,
> -                                0, ~0, XENMEM_access_rw);
> +                                0, ~0, XENMEM_access_rw, 0);
>          return false;
>      }
>      else if ( xma == XENMEM_access_n2rwx )
>      {
>          rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1,
> -                                0, ~0, XENMEM_access_rwx);
> +                                0, ~0, XENMEM_access_rwx, 0);
>      }
>  
>      /* Otherwise, check if there is a vm_event monitor subscriber */
> @@ -1737,7 +1737,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, _gfn(paddr_to_pfn(gpa)), 1,
> -                                        0, ~0, XENMEM_access_rwx);
> +                                        0, ~0, XENMEM_access_rwx, 0);
>              }
>          }
>  
> @@ -1788,7 +1788,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
>   * 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)
> +                        uint32_t start, uint32_t mask, xenmem_access_t access,
> +                        unsigned int altp2m_idx)
>  {
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>      p2m_access_t a;
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 674feea..37305fb 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6398,9 +6398,9 @@ static int do_altp2m_op(
>          if ( a.u.set_mem_access.pad )
>              rc = -EINVAL;
>          else
> -            rc = p2m_set_altp2m_mem_access(d, a.u.set_mem_access.view,
> -                    _gfn(a.u.set_mem_access.gfn),
> -                    a.u.set_mem_access.hvmmem_access);
> +            rc = p2m_set_mem_access(d, _gfn(a.u.set_mem_access.gfn), 1, 0, 0,
> +                                    a.u.set_mem_access.hvmmem_access,
> +                                    a.u.set_mem_access.view);
>          break;
>  
>      case HVMOP_altp2m_change_gfn:
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index a45ee35..226490a 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1777,14 +1777,56 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>      return (p2ma == p2m_access_n2rwx);
>  }
>  
> +static inline
> +int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
> +                              struct p2m_domain *ap2m, p2m_access_t a,
> +                              gfn_t gfn)
> +{
> +    mfn_t mfn;
> +    p2m_type_t t;
> +    p2m_access_t old_a;
> +    unsigned int page_order;
> +    unsigned long gfn_l = gfn_x(gfn);
> +    int rc;
> +
> +    mfn = ap2m->get_entry(ap2m, gfn_l, &t, &old_a, 0, NULL, NULL);
> +
> +    /* Check host p2m if no valid entry in alternate */
> +    if ( !mfn_valid(mfn) )
> +    {
> +        mfn = hp2m->get_entry(hp2m, gfn_l, &t, &old_a,
> +                              P2M_ALLOC | P2M_UNSHARE, &page_order, NULL);
> +
> +        rc = -ESRCH;
> +        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> +            return rc;
> +
> +        /* If this is a superpage, copy that first */
> +        if ( page_order != PAGE_ORDER_4K )
> +        {
> +            unsigned long mask = ~((1UL << page_order) - 1);
> +            unsigned long gfn2_l = gfn_l & mask;
> +            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
> +
> +            rc = ap2m->set_entry(ap2m, gfn2_l, mfn2, page_order, t, old_a, 1);
> +            if ( rc )
> +                return rc;
> +        }
> +    }
> +
> +    return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
> +                         (current->domain != d));
> +}
> +
>  /*
>   * 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)
> +                        uint32_t start, uint32_t mask, xenmem_access_t access,
> +                        unsigned int altp2m_idx)
>  {
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
>      p2m_access_t a, _a;
>      p2m_type_t t;
>      mfn_t mfn;
> @@ -1806,6 +1848,16 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>  #undef ACCESS
>      };
>  
> +    /* altp2m view 0 is treated as the hostp2m */
> +    if ( altp2m_idx )
> +    {
> +        if ( altp2m_idx >= MAX_ALTP2M ||
> +             d->arch.altp2m_eptp[altp2m_idx] == INVALID_MFN )
> +            return -EINVAL;
> +
> +        ap2m = d->arch.altp2m_p2m[altp2m_idx];
> +    }
> +
>      switch ( access )
>      {
>      case 0 ... ARRAY_SIZE(memaccess) - 1:
> @@ -1826,12 +1878,25 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>      }
>  
>      p2m_lock(p2m);
> +    if ( ap2m )
> +        p2m_lock(ap2m);
> +
>      for ( gfn_l = gfn_x(gfn) + start; nr > start; ++gfn_l )
>      {
> -        mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL);
> -        rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
> -        if ( rc )
> -            break;
> +        if ( ap2m )
> +        {
> +            rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, _gfn(gfn_l));
> +            /* If the corresponding mfn is invalid we will just skip it */
> +            if ( rc && rc != -ESRCH )
> +                break;
> +        }
> +        else
> +        {
> +            mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL);
> +            rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
> +            if ( rc )
> +                break;
> +        }
>  
>          /* Check for continuation if it's not the last iteration. */
>          if ( nr > ++start && !(start & mask) && hypercall_preempt_check() )
> @@ -1840,7 +1905,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>              break;
>          }
>      }
> +
> +    if ( ap2m )
> +        p2m_unlock(ap2m);
>      p2m_unlock(p2m);
> +
>      return rc;
>  }
>  
> @@ -2395,93 +2464,6 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx)
>      return rc;
>  }
>  
> -int p2m_set_altp2m_mem_access(struct domain *d, unsigned int idx,
> -                              gfn_t gfn, xenmem_access_t access)
> -{
> -    struct p2m_domain *hp2m, *ap2m;
> -    p2m_access_t req_a, old_a;
> -    p2m_type_t t;
> -    mfn_t mfn;
> -    unsigned int page_order;
> -    int rc = -EINVAL;
> -
> -    static const p2m_access_t memaccess[] = {
> -#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
> -        ACCESS(n),
> -        ACCESS(r),
> -        ACCESS(w),
> -        ACCESS(rw),
> -        ACCESS(x),
> -        ACCESS(rx),
> -        ACCESS(wx),
> -        ACCESS(rwx),
> -#undef ACCESS
> -    };
> -
> -    if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == INVALID_MFN )
> -        return rc;
> -
> -    ap2m = d->arch.altp2m_p2m[idx];
> -
> -    switch ( access )
> -    {
> -    case 0 ... ARRAY_SIZE(memaccess) - 1:
> -        req_a = memaccess[access];
> -        break;
> -    case XENMEM_access_default:
> -        req_a = ap2m->default_access;
> -        break;
> -    default:
> -        return rc;
> -    }
> -
> -    /* If request to set default access */
> -    if ( gfn_x(gfn) == INVALID_GFN )
> -    {
> -        ap2m->default_access = req_a;
> -        return 0;
> -    }
> -
> -    hp2m = p2m_get_hostp2m(d);
> -
> -    p2m_lock(ap2m);
> -
> -    mfn = ap2m->get_entry(ap2m, gfn_x(gfn), &t, &old_a, 0, NULL, NULL);
> -
> -    /* Check host p2m if no valid entry in alternate */
> -    if ( !mfn_valid(mfn) )
> -    {
> -        mfn = hp2m->get_entry(hp2m, gfn_x(gfn), &t, &old_a,
> -                              P2M_ALLOC | P2M_UNSHARE, &page_order, NULL);
> -
> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> -            goto out;
> -
> -        /* If this is a superpage, copy that first */
> -        if ( page_order != PAGE_ORDER_4K )
> -        {
> -            gfn_t gfn2;
> -            unsigned long mask;
> -            mfn_t mfn2;
> -
> -            mask = ~((1UL << page_order) - 1);
> -            gfn2 = _gfn(gfn_x(gfn) & mask);
> -            mfn2 = _mfn(mfn_x(mfn) & mask);
> -
> -            if ( ap2m->set_entry(ap2m, gfn_x(gfn2), mfn2, page_order, t, old_a, 1) )
> -                goto out;
> -        }
> -    }
> -
> -    if ( !ap2m->set_entry(ap2m, gfn_x(gfn), mfn, PAGE_ORDER_4K, t, req_a,
> -                          (current->domain != d)) )
> -        rc = 0;
> -
> - out:
> -    p2m_unlock(ap2m);
> -    return rc;
> -}
> -
>  int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>                            gfn_t old_gfn, gfn_t new_gfn)
>  {
> diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c
> index 159c036..92ebead 100644
> --- a/xen/common/mem_access.c
> +++ b/xen/common/mem_access.c
> @@ -67,7 +67,7 @@ int mem_access_memop(unsigned long cmd,
>              break;
>  
>          rc = p2m_set_mem_access(d, _gfn(mao.pfn), mao.nr, start_iter,
> -                                MEMOP_CMD_MASK, mao.access);
> +                                MEMOP_CMD_MASK, mao.access, 0);
>          if ( rc > 0 )
>          {
>              ASSERT(!(rc & MEMOP_CMD_MASK));
> diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
> index fa46dd9..c0df1ea 100644
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -808,10 +808,6 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx);
>  /* Switch alternate p2m for entire domain */
>  int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx);
>  
> -/* Set access type for a gfn */
> -int p2m_set_altp2m_mem_access(struct domain *d, unsigned int idx,
> -                              gfn_t gfn, xenmem_access_t access);
> -
>  /* Change a gfn->mfn mapping */
>  int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>                            gfn_t old_gfn, gfn_t new_gfn);
> diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
> index 47c40c7..8b70459 100644
> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -49,7 +49,8 @@ int unmap_mmio_regions(struct domain *d,
>   * 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);
> +                        uint32_t start, uint32_t mask, xenmem_access_t access,
> +                        unsigned int altp2m_idx);
>  
>  /*
>   * Get access type for a gfn.
> 

[PATCH v3 3/3] altp2m: Implement p2m_get_mem_access for altp2m views

[Tamas K Lengyel]

Extend the existing get_mem_access memop to allow querying permissions in
altp2m views as well.

Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
v3: Define a union over a memop field for the separate use during get and set
v2: Use unsigned int instead of unsigned long
    Use a single p2m pointer
---
 tools/libxc/include/xenctrl.h       |  3 ++-
 tools/libxc/xc_mem_access.c         | 10 ++++++----
 tools/tests/xen-access/xen-access.c |  5 ++++-
 xen/arch/arm/p2m.c                  |  4 ++--
 xen/arch/x86/mm/p2m.c               | 17 +++++++++++++++--
 xen/common/mem_access.c             | 10 +++++-----
 xen/include/public/memory.h         | 19 ++++++++++++++-----
 xen/include/xen/p2m-common.h        |  3 ++-
 8 files changed, 50 insertions(+), 21 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 1d656ac..a7cb3d5 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2068,7 +2068,8 @@ int xc_set_mem_access(xc_interface *xch, domid_t domain_id,
  * Gets the mem access for the given page (returned in access on success)
  */
 int xc_get_mem_access(xc_interface *xch, domid_t domain_id,
-                      uint64_t pfn, xenmem_access_t *access);
+                      uint64_t pfn, uint16_t altp2m_idx,
+                      xenmem_access_t *access);
 
 /*
  * Instructions causing a mem_access violation can be emulated by Xen
diff --git a/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c
index 3634c39..884418f 100644
--- a/tools/libxc/xc_mem_access.c
+++ b/tools/libxc/xc_mem_access.c
@@ -35,7 +35,7 @@ int xc_set_mem_access(xc_interface *xch,
         .domid  = domain_id,
         .access = access,
         .pfn    = first_pfn,
-        .nr     = nr
+        .u.nr   = nr
     };
 
     return do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
@@ -44,14 +44,16 @@ int xc_set_mem_access(xc_interface *xch,
 int xc_get_mem_access(xc_interface *xch,
                       domid_t domain_id,
                       uint64_t pfn,
+                      uint16_t altp2m_idx,
                       xenmem_access_t *access)
 {
     int rc;
     xen_mem_access_op_t mao =
     {
-        .op    = XENMEM_access_op_get_access,
-        .domid = domain_id,
-        .pfn   = pfn
+        .op           = XENMEM_access_op_get_access,
+        .domid        = domain_id,
+        .pfn          = pfn,
+        .u.altp2m.idx = altp2m_idx
     };
 
     rc = do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao));
diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index ef89246..d70955e 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -586,9 +586,12 @@ int main(int argc, char *argv[])
                     /*
                      * This serves no other purpose here then demonstrating the use of the API.
                      * At shutdown we have already reset all the permissions so really no use getting it again.
+                     * At shutdown the altp2m view is already destroyed so this query would fail.
                      */
                     xenmem_access_t access;
-                    rc = xc_get_mem_access(xch, domain_id, req.u.mem_access.gfn, &access);
+                    rc = xc_get_mem_access(xch, domain_id, req.u.mem_access.gfn,
+                                           ((req.flags & VM_EVENT_FLAG_ALTERNATE_P2M) ? req.altp2m_idx : 0),
+                                           &access);
                     if (rc < 0)
                     {
                         ERROR("Error %d getting mem_access event\n", rc);
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 8568087..957fa57 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1666,7 +1666,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, _gfn(paddr_to_pfn(gpa)), &xma);
+    rc = p2m_get_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 0, &xma);
     if ( rc )
         return true;
 
@@ -1847,7 +1847,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
     return 0;
 }
 
-int p2m_get_mem_access(struct domain *d, gfn_t gfn,
+int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned int altp2m_idx,
                        xenmem_access_t *access)
 {
     int ret;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 226490a..d2efab7 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1572,7 +1572,9 @@ 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, _gfn(data->gfn), &access) == 0 )
+        if ( p2m_get_mem_access(v->domain, _gfn(data->gfn),
+                                altp2m_active(v->domain) ? vcpu_altp2m(v).p2midx : 0,
+                                &access) == 0 )
         {
             switch ( access )
             {
@@ -1917,7 +1919,8 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
  * 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)
+int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned int altp2m_idx,
+                       xenmem_access_t *access)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     p2m_type_t t;
@@ -1946,6 +1949,16 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
         return 0;
     }
 
+    /* altp2m view 0 is treated as the hostp2m */
+    if ( altp2m_idx )
+    {
+        if ( altp2m_idx >= MAX_ALTP2M ||
+             d->arch.altp2m_eptp[altp2m_idx] == INVALID_MFN )
+            return -EINVAL;
+
+        p2m = d->arch.altp2m_p2m[altp2m_idx];
+    }
+
     gfn_lock(p2m, gfn, 0);
     mfn = p2m->get_entry(p2m, gfn_x(gfn), &t, &a, 0, NULL, NULL);
     gfn_unlock(p2m, gfn, 0);
diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c
index 92ebead..f4e77c9 100644
--- a/xen/common/mem_access.c
+++ b/xen/common/mem_access.c
@@ -61,12 +61,12 @@ int mem_access_memop(unsigned long cmd,
     case XENMEM_access_op_set_access:
         rc = -EINVAL;
         if ( (mao.pfn != ~0ull) &&
-             (mao.nr < start_iter ||
-              ((mao.pfn + mao.nr - 1) < mao.pfn) ||
-              ((mao.pfn + mao.nr - 1) > domain_get_maximum_gpfn(d))) )
+             (mao.u.nr < start_iter ||
+              ((mao.pfn + mao.u.nr - 1) < mao.pfn) ||
+              ((mao.pfn + mao.u.nr - 1) > domain_get_maximum_gpfn(d))) )
             break;
 
-        rc = p2m_set_mem_access(d, _gfn(mao.pfn), mao.nr, start_iter,
+        rc = p2m_set_mem_access(d, _gfn(mao.pfn), mao.u.nr, start_iter,
                                 MEMOP_CMD_MASK, mao.access, 0);
         if ( rc > 0 )
         {
@@ -88,7 +88,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, _gfn(mao.pfn), &access);
+        rc = p2m_get_mem_access(d, _gfn(mao.pfn), mao.u.altp2m.idx, &access);
         if ( rc != 0 )
             break;
 
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 4df38d6..c23c12b 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -423,11 +423,20 @@ struct xen_mem_access_op {
     /* xenmem_access_t */
     uint8_t access;
     domid_t domid;
-    /*
-     * Number of pages for set op
-     * Ignored on setting default access and other ops
-     */
-    uint32_t nr;
+    union {
+        /*
+         * Number of pages for set op
+         * Ignored on setting default access and other ops
+         */
+        uint32_t nr;
+        /*
+         * altp2m id used for get op, ignored for other ops
+         */
+        struct altp2m {
+            uint16_t idx;
+            uint16_t pad;
+        } altp2m;
+    } u;
     /*
      * First pfn for set op
      * pfn for get op
diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
index 8b70459..342bc4b 100644
--- a/xen/include/xen/p2m-common.h
+++ b/xen/include/xen/p2m-common.h
@@ -56,6 +56,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
  * 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);
+int p2m_get_mem_access(struct domain *d, gfn_t gfn, unsigned int altp2m_idx,
+                       xenmem_access_t *access);
 
 #endif /* _XEN_P2M_COMMON_H */
-- 
2.1.4

Re: [PATCH v3 3/3] altp2m: Implement p2m_get_mem_access for altp2m views

[Razvan Cojocaru]

On 02/05/2016 11:22 PM, Tamas K Lengyel wrote:
> Extend the existing get_mem_access memop to allow querying permissions in
> altp2m views as well.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> v3: Define a union over a memop field for the separate use during get and set
> v2: Use unsigned int instead of unsigned long
>     Use a single p2m pointer
> ---
>  tools/libxc/include/xenctrl.h       |  3 ++-
>  tools/libxc/xc_mem_access.c         | 10 ++++++----
>  tools/tests/xen-access/xen-access.c |  5 ++++-
>  xen/arch/arm/p2m.c                  |  4 ++--
>  xen/arch/x86/mm/p2m.c               | 17 +++++++++++++++--
>  xen/common/mem_access.c             | 10 +++++-----
>  xen/include/public/memory.h         | 19 ++++++++++++++-----
>  xen/include/xen/p2m-common.h        |  3 ++-
>  8 files changed, 50 insertions(+), 21 deletions(-)

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

Took a bit longer on this one because I wanted to do a test run with our
application after applying these last two patches and make sure things
work. They seem to work.

Re: [PATCH v3 3/3] altp2m: Implement p2m_get_mem_access for altp2m views

[Ian Campbell]

On Fri, 2016-02-05 at 14:22 -0700, Tamas K Lengyel wrote:
> Extend the existing get_mem_access memop to allow querying permissions in
> altp2m views as well.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>

For Xen and tools side:

    Acked-by: Ian Campbell <    ian.campbell@citrix.com    >

Re: [PATCH v3 3/3] altp2m: Implement p2m_get_mem_access for altp2m views

[George Dunlap]

On 05/02/16 21:22, Tamas K Lengyel wrote:
> Extend the existing get_mem_access memop to allow querying permissions in
> altp2m views as well.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>

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

Sorry for the delay.

Re: [PATCH v3 3/3] altp2m: Implement p2m_get_mem_access for altp2m views

[Jan Beulich]

>>> On 05.02.16 at 22:22, <tlengyel@novetta.com> wrote:
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -423,11 +423,20 @@ struct xen_mem_access_op {
>      /* xenmem_access_t */
>      uint8_t access;
>      domid_t domid;
> -    /*
> -     * Number of pages for set op
> -     * Ignored on setting default access and other ops
> -     */
> -    uint32_t nr;
> +    union {
> +        /*
> +         * Number of pages for set op
> +         * Ignored on setting default access and other ops
> +         */
> +        uint32_t nr;
> +        /*
> +         * altp2m id used for get op, ignored for other ops
> +         */
> +        struct altp2m {
> +            uint16_t idx;
> +            uint16_t pad;
> +        } altp2m;
> +    } u;

Am I overlooking something, or is this new padding field not being
checked to be zero on input (allowing seamless use for an actual
purpose later on)? It's a tools-only interface (i.e. not considered
stable), but anyway...

Jan

Re: [PATCH v3 3/3] altp2m: Implement p2m_get_mem_access for altp2m views

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 1484 bytes --]

On Tue, Feb 9, 2016 at 10:17 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 05.02.16 at 22:22, <tlengyel@novetta.com> wrote:
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -423,11 +423,20 @@ struct xen_mem_access_op {
> >      /* xenmem_access_t */
> >      uint8_t access;
> >      domid_t domid;
> > -    /*
> > -     * Number of pages for set op
> > -     * Ignored on setting default access and other ops
> > -     */
> > -    uint32_t nr;
> > +    union {
> > +        /*
> > +         * Number of pages for set op
> > +         * Ignored on setting default access and other ops
> > +         */
> > +        uint32_t nr;
> > +        /*
> > +         * altp2m id used for get op, ignored for other ops
> > +         */
> > +        struct altp2m {
> > +            uint16_t idx;
> > +            uint16_t pad;
> > +        } altp2m;
> > +    } u;
>
> Am I overlooking something, or is this new padding field not being
> checked to be zero on input (allowing seamless use for an actual
> purpose later on)? It's a tools-only interface (i.e. not considered
> stable), but anyway...
>

There is no plan to use it for anything in this context in the future so we
could enforce it being zero. However, even if someone erroneously used the
nr field and set some arbitrary large number in there, the altp2m idx is
checked to be sure it's not larger then the max supported, so IMHO there is
enough error checking on it in place already.

Tamas

[-- Attachment #1.2: Type: text/html, Size: 2246 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH v3 3/3] altp2m: Implement p2m_get_mem_access for altp2m views

[Jan Beulich]

>>> On 09.02.16 at 18:32, <tlengyel@novetta.com> wrote:
> On Tue, Feb 9, 2016 at 10:17 AM, Jan Beulich <JBeulich@suse.com> wrote:
> 
>> >>> On 05.02.16 at 22:22, <tlengyel@novetta.com> wrote:
>> > --- a/xen/include/public/memory.h
>> > +++ b/xen/include/public/memory.h
>> > @@ -423,11 +423,20 @@ struct xen_mem_access_op {
>> >      /* xenmem_access_t */
>> >      uint8_t access;
>> >      domid_t domid;
>> > -    /*
>> > -     * Number of pages for set op
>> > -     * Ignored on setting default access and other ops
>> > -     */
>> > -    uint32_t nr;
>> > +    union {
>> > +        /*
>> > +         * Number of pages for set op
>> > +         * Ignored on setting default access and other ops
>> > +         */
>> > +        uint32_t nr;
>> > +        /*
>> > +         * altp2m id used for get op, ignored for other ops
>> > +         */
>> > +        struct altp2m {
>> > +            uint16_t idx;
>> > +            uint16_t pad;
>> > +        } altp2m;
>> > +    } u;
>>
>> Am I overlooking something, or is this new padding field not being
>> checked to be zero on input (allowing seamless use for an actual
>> purpose later on)? It's a tools-only interface (i.e. not considered
>> stable), but anyway...
> 
> There is no plan to use it for anything in this context in the future so we
> could enforce it being zero. However, even if someone erroneously used the
> nr field and set some arbitrary large number in there, the altp2m idx is
> checked to be sure it's not larger then the max supported, so IMHO there is
> enough error checking on it in place already.

Looks like you didn't understand: Whatever padding fields we have
in the public interface structures, when we don't enforce them to
be set to zero by callers, we can't _later_ assign meaning to these
fields without possibly breaking existing code. This is less of a
problem for domctls and sysctls, since they're versioned, and not
as bad an issue as it would be for general consumption interfaces,
but should imo nevertheless be avoided.

Jan

Re: [PATCH v3 1/3] xen-access: minor fixes

[Razvan Cojocaru]

On 02/05/2016 11:22 PM, Tamas K Lengyel wrote:
> Only copy the VCPU_PAUSED flag to the response. Copy the entire mem_access
> struct which is useful and easily forgotten when also testing the emulate
> response flags. Turn off singlestepping on the vCPUs once we are done
> processing all events, as we might have turned on singlestep there and leave
> the VM in an undesirable state.
> 
> Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/tests/xen-access/xen-access.c | 46 ++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 21 deletions(-)

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

Re: [PATCH v3 1/3] xen-access: minor fixes

[Ian Campbell]

On Sat, 2016-02-06 at 13:13 +0200, Razvan Cojocaru wrote:
> On 02/05/2016 11:22 PM, Tamas K Lengyel wrote:
> > Only copy the VCPU_PAUSED flag to the response. Copy the entire
> > mem_access
> > struct which is useful and easily forgotten when also testing the
> > emulate
> > response flags. Turn off singlestepping on the vCPUs once we are done
> > processing all events, as we might have turned on singlestep there and
> > leave
> > the VM in an undesirable state.
> > 
> > Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com>
> > Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  tools/tests/xen-access/xen-access.c | 46 ++++++++++++++++++++---------
> > --------
> >  1 file changed, 25 insertions(+), 21 deletions(-)
> 
> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

Applied, thanks.

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