[RFC PATCH 1/2] mem_access: Fix npfec.kind propagation

[RFC PATCH 1/2] mem_access: Fix npfec.kind propagation

[George Dunlap]

The name of the "with_gla" flag is confusing; it has nothing to do
with the existence or lack thereof of a faulting GLA, but rather where
the fault originated.  The npfec.kind value is always valid, and
should thus be propagated, regardless of whether gla_valid is set or
not.

In particular, gla_valid will never be set on AMD systems; but
npfec.kind will still be valid and should still be propagated.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 xen/arch/x86/mm/mem_access.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index d9e64fcbb9..699a315076 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -232,12 +232,12 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
         {
             req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
             req->u.mem_access.gla = gla;
-
-            if ( npfec.kind == npfec_kind_with_gla )
-                req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
-            else if ( npfec.kind == npfec_kind_in_gpt )
-                req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
         }
+
+        if ( npfec.kind == npfec_kind_with_gla )
+            req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
+        else if ( npfec.kind == npfec_kind_in_gpt )
+            req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
         req->u.mem_access.flags |= npfec.read_access    ? MEM_ACCESS_R : 0;
         req->u.mem_access.flags |= npfec.write_access   ? MEM_ACCESS_W : 0;
         req->u.mem_access.flags |= npfec.insn_fetch     ? MEM_ACCESS_X : 0;
-- 
2.19.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[RFC PATCH 2/2] x86/mm: Add mem access rights to NPT

[George Dunlap]

From: Isaila Alexandru <aisaila@bitdefender.com>

This patch adds access control for NPT mode.

There aren’t enough extra bits to store the access rights in the NPT p2m
table, so we add a radix tree to store extra information.

For efficiency:
 - Only allocate this radix tree when we first store "non-default"
   extra information

 - Remove entires which match the default extra information rather
   than continuing to store them

 - For superpages, only store an entry for the first gfn in the
   superpage.  Use the order of the p2m entry being read to determine
   the proper place to look in the radix table.

Modify p2m_type_to_flags() to accept and interpret an access value,
parallel to the ept code.

Add a set_default_access() method to the p2m-pt and p2m-ept versions
of the p2m rather than setting it directly, to deal with different
default permitted access values.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
NB, this is compile-tested only.

cc'ing Paul because this is functionality he may want at some point in
the future.

I'm not sure why we only allow 'int' to be stored in the radix tree,
but that throws away 30-some bits we could otherwise use.  We might
consider revising this if we run out of bits here.

CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
 xen/arch/x86/mm/mem_access.c     |   5 +-
 xen/arch/x86/mm/p2m-ept.c        |   9 ++
 xen/arch/x86/mm/p2m-pt.c         | 173 ++++++++++++++++++++++++++++---
 xen/arch/x86/mm/p2m.c            |   6 ++
 xen/arch/x86/monitor.c           |   1 +
 xen/include/asm-x86/mem_access.h |   2 +-
 xen/include/asm-x86/p2m.h        |  18 ++++
 7 files changed, 192 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 699a315076..0f96c43f88 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -389,10 +389,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
 
     /* If request to set default access. */
     if ( gfn_eq(gfn, INVALID_GFN) )
-    {
-        p2m->default_access = a;
-        return 0;
-    }
+        return p2m->set_default_access(p2m, a);
 
     p2m_lock(p2m);
     if ( ap2m )
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 1ff4f14ae4..37bdbcf186 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -667,6 +667,14 @@ bool_t ept_handle_misconfig(uint64_t gpa)
     return spurious ? (rc >= 0) : (rc > 0);
 }
 
+int ept_set_default_access(struct p2m_domain *p2m, p2m_access_t p2ma)
+{
+    /* All access is permitted */
+    p2m->default_access = p2ma;
+    
+    return 0;
+}
+
 /*
  * ept_set_entry() computes 'need_modify_vtd_table' for itself,
  * by observing whether any gfn->mfn translations are modified.
@@ -1255,6 +1263,7 @@ int ept_p2m_init(struct p2m_domain *p2m)
     p2m->change_entry_type_global = ept_change_entry_type_global;
     p2m->change_entry_type_range = ept_change_entry_type_range;
     p2m->memory_type_changed = ept_memory_type_changed;
+    p2m->set_default_access = ept_set_default_access;
     p2m->audit_p2m = NULL;
     p2m->tlb_flush = ept_tlb_flush;
 
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 40bfc76a6f..cd8b8c9187 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -68,7 +68,8 @@
 static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m,
                                        p2m_type_t t,
                                        mfn_t mfn,
-                                       unsigned int level)
+                                       unsigned int level,
+                                       p2m_access_t access)
 {
     unsigned long flags;
     /*
@@ -87,23 +88,28 @@ static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m,
     case p2m_ram_paged:
     case p2m_ram_paging_in:
     default:
-        return flags | _PAGE_NX_BIT;
+        flags |= _PAGE_NX_BIT;
+        break;
     case p2m_grant_map_ro:
-        return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
+        flags |= P2M_BASE_FLAGS | _PAGE_NX_BIT;
+        break;
     case p2m_ioreq_server:
         flags |= P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
         if ( p2m->ioreq.flags & XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
-            return flags & ~_PAGE_RW;
-        return flags;
+            flags &= ~_PAGE_RW;
+        break;
     case p2m_ram_ro:
     case p2m_ram_logdirty:
     case p2m_ram_shared:
-        return flags | P2M_BASE_FLAGS;
+        flags |= P2M_BASE_FLAGS;
+        break;
     case p2m_ram_rw:
-        return flags | P2M_BASE_FLAGS | _PAGE_RW;
+        flags |= P2M_BASE_FLAGS | _PAGE_RW;
+        break;
     case p2m_grant_map_rw:
     case p2m_map_foreign:
-        return flags | P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
+        flags |= P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT;
+        break;
     case p2m_mmio_direct:
         if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)) )
             flags |= _PAGE_RW;
@@ -112,8 +118,32 @@ static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m,
             flags |= _PAGE_PWT;
             ASSERT(!level);
         }
-        return flags | P2M_BASE_FLAGS | _PAGE_PCD;
+        flags |= P2M_BASE_FLAGS | _PAGE_PCD;
+        break;
     }
+
+    switch ( access )
+    {
+    case p2m_access_r:
+        flags |= _PAGE_NX_BIT;
+        flags &= ~_PAGE_RW;
+        break;
+    case p2m_access_rw:
+        flags |= _PAGE_NX_BIT;
+        break;
+    case p2m_access_rx:
+    case p2m_access_rx2rw:
+        flags &= ~(_PAGE_NX_BIT | _PAGE_RW);
+        break;
+    case p2m_access_x:
+        flags &= ~_PAGE_RW;
+        break;
+    case p2m_access_rwx:
+    default:
+        break;
+    }
+
+    return flags;
 }
 
 
@@ -174,6 +204,48 @@ static void p2m_add_iommu_flags(l1_pgentry_t *p2m_entry,
         l1e_add_flags(*p2m_entry, iommu_nlevel_to_flags(nlevel, flags));
 }
 
+const p2m_extra_t pt_extra_default = { .access = p2m_access_rwx };
+
+static p2m_extra_t p2m_pt_get_extra(struct p2m_domain *p2m, unsigned long gfn,
+                                      unsigned level)
+{
+    void *ptr;
+    unsigned long mask = ~0UL << (level * PAGETABLE_ORDER);
+
+    if ( !p2m->extra )
+        return pt_extra_default;
+
+    ptr = radix_tree_lookup(p2m->extra, gfn & mask);
+    if ( !ptr )
+        return pt_extra_default;
+    else
+        return (p2m_extra_t)radix_tree_ptr_to_int(ptr);
+}
+
+static void p2m_pt_set_extra(struct p2m_domain *p2m, unsigned long gfn,
+                                      p2m_extra_t extra)
+{
+    int rc;
+
+    if ( extra.extra == pt_extra_default.extra )
+    {
+        if ( p2m->extra )
+            radix_tree_delete(p2m->extra, gfn);
+        return;
+    }
+
+    ASSERT(p2m->extra);
+
+    rc = radix_tree_insert(p2m->extra, gfn,
+                           radix_tree_int_to_ptr(extra.extra));
+    if ( rc == -EEXIST )
+        /* If a setting already exists, change it to the new one. */
+        radix_tree_replace_slot(
+            radix_tree_lookup_slot(
+                p2m->extra, gfn),
+            radix_tree_int_to_ptr(extra.extra));
+}
+
 /* Returns: 0 for success, -errno for failure */
 static int
 p2m_next_level(struct p2m_domain *p2m, void **table,
@@ -210,6 +282,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
         mfn_t mfn;
         l1_pgentry_t *l1_entry;
         unsigned int i;
+        p2m_extra_t extra;
 
         switch ( level )
         {
@@ -247,8 +320,14 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
 
         for ( i = 0; i < (1u << PAGETABLE_ORDER); i++ )
         {
-            new_entry = l1e_from_pfn(pfn | (i << ((level - 1) * PAGETABLE_ORDER)),
-                                     flags);
+            unsigned long frame_offset = (i << ((level - 1) * PAGETABLE_ORDER));
+            new_entry = l1e_from_pfn(pfn | frame_offset, flags);
+
+            /* Shattering a p2m mapping requires duplicating extra info as well */
+            if ( i == 0 )
+                extra = p2m_pt_get_extra(p2m, gfn, level);
+            else
+                p2m_pt_set_extra(p2m, gfn + frame_offset, extra);
             p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, level);
         }
 
@@ -420,8 +499,10 @@ static int do_recalc(struct p2m_domain *p2m, unsigned long gfn)
         if ( nt != ot )
         {
             unsigned long mfn = l1e_get_pfn(e);
+            p2m_extra_t extra = p2m_pt_get_extra(p2m, gfn, level);
             unsigned long flags = p2m_type_to_flags(p2m, nt,
-                                                    _mfn(mfn), level);
+                                                    _mfn(mfn), level,
+                                                    extra.access);
 
             if ( level )
             {
@@ -471,6 +552,32 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
     return rc;
 }
 
+/* Returns: 0 for success, -errno for failure */
+static int p2m_pt_check_access(p2m_access_t p2ma)
+{
+    switch ( p2ma )
+    {
+    case p2m_access_n:
+    case p2m_access_w:
+    case p2m_access_wx:
+    case p2m_access_n2rwx:
+        return -EINVAL;
+    default:
+        break;
+    }
+    return 0;
+}
+
+static int p2m_pt_set_default_access(struct p2m_domain *p2m,
+                                     p2m_access_t p2ma) {
+    int rc = p2m_pt_check_access(p2ma);
+
+    if ( !rc )
+        p2m->default_access = p2ma;
+
+    return rc;
+}
+
 /* Returns: 0 for success, -errno for failure */
 static int
 p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
@@ -500,9 +607,13 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
      */
     unsigned int flags, iommu_old_flags = 0;
     unsigned long old_mfn = mfn_x(INVALID_MFN);
+    p2m_extra_t extra = pt_extra_default;
 
     ASSERT(sve != 0);
 
+    if ( (rc = p2m_pt_check_access(p2ma)) )
+        return rc;
+
     if ( tb_init_done )
     {
         struct {
@@ -532,6 +643,24 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
     if ( rc < 0 )
         return rc;
 
+    if ( p2ma != extra.access )
+        extra.access = p2ma;
+
+    /* 
+     * If we need 'extra' bits and we haven't allocated space for it
+     * yet, do so
+     */
+    if ( (extra.extra != pt_extra_default.extra) && !p2m->extra )
+    {
+        p2m->extra = xmalloc(struct radix_tree_root);
+        
+        if( !p2m->extra )
+        {
+            return -ENOMEM;
+        }
+        radix_tree_init(p2m->extra);
+    }
+
     table = map_domain_page(pagetable_get_mfn(p2m_get_pagetable(p2m)));
     rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn,
                         L4_PAGETABLE_SHIFT - PAGE_SHIFT,
@@ -569,7 +698,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
         ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
         l3e_content = mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt)
             ? p2m_l3e_from_pfn(mfn_x(mfn),
-                               p2m_type_to_flags(p2m, p2mt, mfn, 2))
+                               p2m_type_to_flags(p2m, p2mt, mfn, 2, p2ma))
             : l3e_empty();
         entry_content.l1 = l3e_content.l3;
 
@@ -608,7 +737,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
 
         if ( mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt) )
             entry_content = p2m_l1e_from_pfn(mfn_x(mfn),
-                                         p2m_type_to_flags(p2m, p2mt, mfn, 0));
+                                         p2m_type_to_flags(p2m, p2mt, mfn, 0, p2ma));
         else
             entry_content = l1e_empty();
 
@@ -661,7 +790,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
         ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
         l2e_content = mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt)
             ? p2m_l2e_from_pfn(mfn_x(mfn),
-                               p2m_type_to_flags(p2m, p2mt, mfn, 1))
+                               p2m_type_to_flags(p2m, p2mt, mfn, 1, p2ma))
             : l2e_empty();
         entry_content.l1 = l2e_content.l2;
 
@@ -672,6 +801,13 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn,
         /* NB: paging_write_p2m_entry() handles tlb flushes properly */
     }
 
+    /* 
+     * For storing of 'extra' information, only set the first gfn in
+     * any range; use the p2m table itself to record the order covered
+     * by that entry.
+     */
+    p2m_pt_set_extra(p2m, gfn, extra);
+
     /* Track the highest gfn for which we have ever had a valid mapping */
     if ( p2mt != p2m_invalid
          && (gfn + (1UL << page_order) - 1 > p2m->max_mapped_pfn) )
@@ -749,8 +885,7 @@ p2m_pt_get_entry(struct p2m_domain *p2m, gfn_t gfn_,
      * XXX Once we start explicitly registering MMIO regions in the p2m 
      * XXX we will return p2m_invalid for unmapped gfns */
     *t = p2m_mmio_dm;
-    /* Not implemented except with EPT */
-    *a = p2m_access_rwx; 
+    *a = p2m_access_n;
 
     if ( gfn > p2m->max_mapped_pfn )
     {
@@ -813,6 +948,7 @@ pod_retry_l3:
                        l1_table_offset(addr));
             *t = p2m_recalc_type(recalc || _needs_recalc(flags),
                                  p2m_flags_to_type(flags), p2m, gfn);
+            *a = p2m_pt_get_extra(p2m, gfn, 2).access;
             unmap_domain_page(l3e);
 
             ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t));
@@ -852,6 +988,7 @@ pod_retry_l2:
         mfn = _mfn(l2e_get_pfn(*l2e) + l1_table_offset(addr));
         *t = p2m_recalc_type(recalc || _needs_recalc(flags),
                              p2m_flags_to_type(flags), p2m, gfn);
+        *a = p2m_pt_get_extra(p2m, gfn, 1).access;
         unmap_domain_page(l2e);
         
         ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t));
@@ -888,6 +1025,7 @@ pod_retry_l1:
     }
     mfn = l1e_get_mfn(*l1e);
     *t = p2m_recalc_type(recalc || _needs_recalc(flags), l1t, p2m, gfn);
+    *a = p2m_pt_get_extra(p2m, gfn, 0).access;
     unmap_domain_page(l1e);
 
     ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t) || p2m_is_paging(*t));
@@ -1129,6 +1267,7 @@ void p2m_pt_init(struct p2m_domain *p2m)
     p2m->change_entry_type_global = p2m_pt_change_entry_type_global;
     p2m->change_entry_type_range = p2m_pt_change_entry_type_range;
     p2m->write_p2m_entry = paging_write_p2m_entry;
+    p2m->set_default_access = p2m_pt_set_default_access;
 #if P2M_AUDIT
     p2m->audit_p2m = p2m_pt_audit_p2m;
 #else
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index ed2e8daf58..706a8ec8bb 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -682,6 +682,12 @@ void p2m_teardown(struct p2m_domain *p2m)
 
     d = p2m->domain;
 
+    if ( p2m->extra )
+    {
+        radix_tree_destroy(p2m->extra, NULL);
+        xfree(p2m->extra);
+    }
+
     p2m_lock(p2m);
     ASSERT(atomic_read(&d->shr_pages) == 0);
     p2m->phys_table = pagetable_null();
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 3c42e21906..2e6b1e75e4 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -20,6 +20,7 @@
  */
 
 #include <asm/monitor.h>
+#include <asm/p2m.h>
 #include <public/vm_event.h>
 
 int arch_monitor_init_domain(struct domain *d)
diff --git a/xen/include/asm-x86/mem_access.h b/xen/include/asm-x86/mem_access.h
index 4043c9fb4d..34f2c07088 100644
--- a/xen/include/asm-x86/mem_access.h
+++ b/xen/include/asm-x86/mem_access.h
@@ -46,7 +46,7 @@ bool p2m_mem_access_emulate_check(struct vcpu *v,
 /* Sanity check for mem_access hardware support */
 static inline bool p2m_mem_access_sanity_check(struct domain *d)
 {
-    return is_hvm_domain(d) && cpu_has_vmx && hap_enabled(d);
+    return is_hvm_domain(d) && hap_enabled(d);
 }
 
 #endif /*__ASM_X86_MEM_ACCESS_H__ */
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index be3b6fcaf0..a5e1193dcc 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -185,6 +185,16 @@ typedef enum {
     p2m_alternate,
 } p2m_class_t;
 
+typedef union {
+    struct {
+        u32  access      :   4,  /* p2m_access_t */
+            _available   :  25,
+            _reserved    :   3;  /* Upper two bits reserved */
+    };
+    int extra;
+} p2m_extra_t;
+
+
 /* Per-p2m-table state */
 struct p2m_domain {
     /* Lock that protects updates to the p2m */
@@ -270,6 +280,8 @@ struct p2m_domain {
                                           unsigned long gfn, l1_pgentry_t *p,
                                           l1_pgentry_t new, unsigned int level);
     long               (*audit_p2m)(struct p2m_domain *p2m);
+    int                (*set_default_access)(struct p2m_domain *p2m,
+                                             p2m_access_t p2ma);
 
     /*
      * P2M updates may require TLBs to be flushed (invalidated).
@@ -292,6 +304,12 @@ struct p2m_domain {
      * retyped get this access type.  See definition of p2m_access_t. */
     p2m_access_t default_access;
 
+    /*
+     * Radix tree to store extra information which doesn't fit in the
+     * p2m's PTE
+     */
+    struct radix_tree_root *extra;
+
     /* If true, and an access fault comes in and there is no vm_event listener, 
      * pause domain.  Otherwise, remove access restrictions. */
     bool_t       access_required;
-- 
2.19.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT

[Andrew Cooper]

On 26/09/18 17:47, George Dunlap wrote:
> From: Isaila Alexandru <aisaila@bitdefender.com>
>
> This patch adds access control for NPT mode.
>
> There aren’t enough extra bits to store the access rights in the NPT p2m
> table, so we add a radix tree to store extra information.

I'm sorry to re-open this argument, but why?

ISTR there being some argument based on pagetable sharing with the
IOMMU, but that doesn't work at the moment and can't reasonably be made
to work.  For one, attempting to use pt sharing will break as soon as
you try and DMA to a mapped grant.

I'm disinclined to let a broken vestigial feature get in the way of real
improvements.

Beyond that, an NPT PTE has basically the same number of software
available bits as an EPT PTE.

Am I missing anything?

>
> For efficiency:
>  - Only allocate this radix tree when we first store "non-default"
>    extra information
>
>  - Remove entires which match the default extra information rather
>    than continuing to store them
>
>  - For superpages, only store an entry for the first gfn in the
>    superpage.  Use the order of the p2m entry being read to determine
>    the proper place to look in the radix table.
>
> Modify p2m_type_to_flags() to accept and interpret an access value,
> parallel to the ept code.
>
> Add a set_default_access() method to the p2m-pt and p2m-ept versions
> of the p2m rather than setting it directly, to deal with different
> default permitted access values.
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> NB, this is compile-tested only.
>
> cc'ing Paul because this is functionality he may want at some point in
> the future.
>
> I'm not sure why we only allow 'int' to be stored in the radix tree,
> but that throws away 30-some bits we could otherwise use.  We might
> consider revising this if we run out of bits here.

Probably because we have a old fork of the Linux radix tree
functionality, rather than any specific reason to waste 30-some bits.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT

[George Dunlap]

On 09/26/2018 06:22 PM, Andrew Cooper wrote:
> On 26/09/18 17:47, George Dunlap wrote:
>> From: Isaila Alexandru <aisaila@bitdefender.com>
>>
>> This patch adds access control for NPT mode.
>>
>> There aren’t enough extra bits to store the access rights in the NPT p2m
>> table, so we add a radix tree to store extra information.
> 
> I'm sorry to re-open this argument, but why?
> 
> ISTR there being some argument based on pagetable sharing with the
> IOMMU, but that doesn't work at the moment and can't reasonably be made
> to work.  For one, attempting to use pt sharing will break as soon as
> you try and DMA to a mapped grant.
> 
> I'm disinclined to let a broken vestigial feature get in the way of real
> improvements.
> 
> Beyond that, an NPT PTE has basically the same number of software
> available bits as an EPT PTE.
> 
> Am I missing anything?

Wow -- looks like IOMMU/p2m sharing has been disabled unconditionally
since 2014.  If nobody has complained since then, that seems like a good
enough reason to me to rip it out.

Suravee / Brian / Boris -- any opinions?

The main reason to go with the 'extra bits' solution rather than the
'rip out iommu/p2m sharing' solution is because people have been
prognosticating for years that we would be running out of bits and need
more at some point in the future.  I thought Paul, for instance, might
have a use for the extra bits.  But I'm happy to wait until such time as
we need it and then fish this patch out of the mail archives.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT

[Paul Durrant]

> -----Original Message-----
> From: George Dunlap [mailto:george.dunlap@citrix.com]
> Sent: 27 September 2018 11:38
> To: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Cc: Isaila Alexandru <aisaila@bitdefender.com>; Jan Beulich
> <jbeulich@suse.com>; Tim (Xen.org) <tim@xen.org>; Tamas K Lengyel
> <tamas.lengyel@zentific.com>; Paul Durrant <Paul.Durrant@citrix.com>;
> Razvan Cojocaru <rcojocaru@bitdefender.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; Brian Woods <brian.woods@amd.com>; Boris
> Ostrovsky <boris.ostrovsky@oracle.com>
> Subject: Re: [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT
> 
> On 09/26/2018 06:22 PM, Andrew Cooper wrote:
> > On 26/09/18 17:47, George Dunlap wrote:
> >> From: Isaila Alexandru <aisaila@bitdefender.com>
> >>
> >> This patch adds access control for NPT mode.
> >>
> >> There aren’t enough extra bits to store the access rights in the NPT
> p2m
> >> table, so we add a radix tree to store extra information.
> >
> > I'm sorry to re-open this argument, but why?
> >
> > ISTR there being some argument based on pagetable sharing with the
> > IOMMU, but that doesn't work at the moment and can't reasonably be made
> > to work.  For one, attempting to use pt sharing will break as soon as
> > you try and DMA to a mapped grant.
> >
> > I'm disinclined to let a broken vestigial feature get in the way of real
> > improvements.
> >
> > Beyond that, an NPT PTE has basically the same number of software
> > available bits as an EPT PTE.
> >
> > Am I missing anything?
> 
> Wow -- looks like IOMMU/p2m sharing has been disabled unconditionally
> since 2014.  If nobody has complained since then, that seems like a good
> enough reason to me to rip it out.
> 
> Suravee / Brian / Boris -- any opinions?
> 
> The main reason to go with the 'extra bits' solution rather than the
> 'rip out iommu/p2m sharing' solution is because people have been
> prognosticating for years that we would be running out of bits and need
> more at some point in the future.  I thought Paul, for instance, might
> have a use for the extra bits.  But I'm happy to wait until such time as
> we need it and then fish this patch out of the mail archives.
> 

The main angle I had was to have a more generic page-to-type mapping such that it would be suitable to allow steering of accesses to certain pages to distinct IOREQ servers.

  Paul

>  -George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT

[Alexandru Stefan ISAILA]

Ping

Suravee / Brian / Boris any ideas on this topic are appreciated.

Regards,
Alex

On 27.09.2018 13:37, George Dunlap wrote:
> On 09/26/2018 06:22 PM, Andrew Cooper wrote:
>> On 26/09/18 17:47, George Dunlap wrote:
>>> From: Isaila Alexandru <aisaila@bitdefender.com>
>>>
>>> This patch adds access control for NPT mode.
>>>
>>> There aren’t enough extra bits to store the access rights in the NPT p2m
>>> table, so we add a radix tree to store extra information.
>>
>> I'm sorry to re-open this argument, but why?
>>
>> ISTR there being some argument based on pagetable sharing with the
>> IOMMU, but that doesn't work at the moment and can't reasonably be made
>> to work.  For one, attempting to use pt sharing will break as soon as
>> you try and DMA to a mapped grant.
>>
>> I'm disinclined to let a broken vestigial feature get in the way of real
>> improvements.
>>
>> Beyond that, an NPT PTE has basically the same number of software
>> available bits as an EPT PTE.
>>
>> Am I missing anything?
> 
> Wow -- looks like IOMMU/p2m sharing has been disabled unconditionally
> since 2014.  If nobody has complained since then, that seems like a good
> enough reason to me to rip it out.
> 
> Suravee / Brian / Boris -- any opinions?
> 
> The main reason to go with the 'extra bits' solution rather than the
> 'rip out iommu/p2m sharing' solution is because people have been
> prognosticating for years that we would be running out of bits and need
> more at some point in the future.  I thought Paul, for instance, might
> have a use for the extra bits.  But I'm happy to wait until such time as
> we need it and then fish this patch out of the mail archives.
> 
>   -George
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT

[Isaila Alexandru]

On Wed, 2018-09-26 at 17:47 +0100, George Dunlap wrote:
> From: Isaila Alexandru <aisaila@bitdefender.com>
> 
> This patch adds access control for NPT mode.
> 
> There aren’t enough extra bits to store the access rights in the NPT
> p2m
> table, so we add a radix tree to store extra information.
> 
> For efficiency:
>  - Only allocate this radix tree when we first store "non-default"
>    extra information
> 
>  - Remove entires which match the default extra information rather
>    than continuing to store them
> 
>  - For superpages, only store an entry for the first gfn in the
>    superpage.  Use the order of the p2m entry being read to determine
>    the proper place to look in the radix table.
> 
> Modify p2m_type_to_flags() to accept and interpret an access value,
> parallel to the ept code.
> 
> Add a set_default_access() method to the p2m-pt and p2m-ept versions
> of the p2m rather than setting it directly, to deal with different
> default permitted access values.
> 
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> NB, this is compile-tested only.

I've tested this with xen-access and it works as expected
> 
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 3c42e21906..2e6b1e75e4 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -20,6 +20,7 @@
>   */
>  
>  #include <asm/monitor.h>
> +#include <asm/p2m.h>

Is this intended?

Regards,
Alex



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT

[Alexandru Stefan ISAILA]

On 26.09.2018 19:47, George Dunlap wrote:
> From: Isaila Alexandru <aisaila@bitdefender.com>
> 
> This patch adds access control for NPT mode.
> 
> There aren’t enough extra bits to store the access rights in the NPT p2m
> table, so we add a radix tree to store extra information.
> 
> For efficiency:
>   - Only allocate this radix tree when we first store "non-default"
>     extra information
> 
>   - Remove entires which match the default extra information rather
>     than continuing to store them
> 
>   - For superpages, only store an entry for the first gfn in the
>     superpage.  Use the order of the p2m entry being read to determine
>     the proper place to look in the radix table.
> 
> Modify p2m_type_to_flags() to accept and interpret an access value,
> parallel to the ept code.
> 
> Add a set_default_access() method to the p2m-pt and p2m-ept versions
> of the p2m rather than setting it directly, to deal with different
> default permitted access values.
> 
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> NB, this is compile-tested only.
> 
> cc'ing Paul because this is functionality he may want at some point in
> the future.
> 
> I'm not sure why we only allow 'int' to be stored in the radix tree,
> but that throws away 30-some bits we could otherwise use.  We might
> consider revising this if we run out of bits here.
> 
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Razvan Cojocaru <rcojocaru@bitdefender.com>

Hi all,

I know it's been some time from the start of this patch but can this 
move forward? Any thoughts or acks are appreciated.

Thanks,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT

[George Dunlap]

On 6/13/19 11:56 AM, Alexandru Stefan ISAILA wrote:
> 
> 
> On 26.09.2018 19:47, George Dunlap wrote:
>> From: Isaila Alexandru <aisaila@bitdefender.com>
>>
>> This patch adds access control for NPT mode.
>>
>> There aren’t enough extra bits to store the access rights in the NPT p2m
>> table, so we add a radix tree to store extra information.
>>
>> For efficiency:
>>   - Only allocate this radix tree when we first store "non-default"
>>     extra information
>>
>>   - Remove entires which match the default extra information rather
>>     than continuing to store them
>>
>>   - For superpages, only store an entry for the first gfn in the
>>     superpage.  Use the order of the p2m entry being read to determine
>>     the proper place to look in the radix table.
>>
>> Modify p2m_type_to_flags() to accept and interpret an access value,
>> parallel to the ept code.
>>
>> Add a set_default_access() method to the p2m-pt and p2m-ept versions
>> of the p2m rather than setting it directly, to deal with different
>> default permitted access values.
>>
>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>> ---
>> NB, this is compile-tested only.
>>
>> cc'ing Paul because this is functionality he may want at some point in
>> the future.
>>
>> I'm not sure why we only allow 'int' to be stored in the radix tree,
>> but that throws away 30-some bits we could otherwise use.  We might
>> consider revising this if we run out of bits here.
>>
>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Tim Deegan <tim@xen.org>
>> CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
>> CC: Paul Durrant <paul.durrant@citrix.com>
>> CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
> 
> Hi all,
> 
> I know it's been some time from the start of this patch but can this 
> move forward? Any thoughts or acks are appreciated.

Right, well where we left this, the situation was that on AMD hardware
with IOMMU / p2m sharing, there aren't enough bits.

The two general fixes we have so far are:
1. Add a parallel tree for extra bits (this patch)
2. Rip out IOMMU / p2m sharing for AMD.

#2 has the advantage that we don't need an entirely separate tree, as
well as getting rid of code that has (apparently) been completely dead
for 5 years. #1 has the advantage that we're set up for having a much
larger number of IOREQ servers in the future.

Nobody objected to #2.  Without looking deeply into it, it seems like it
might be a good idea, but I can't be sure without seeing what it would
actually look like.

The easiest way to press the point then would be to post a patch series
ripping it out.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT

[Andrew Cooper]

On 17/06/2019 11:48, George Dunlap wrote:
> On 6/13/19 11:56 AM, Alexandru Stefan ISAILA wrote:
>>
>> On 26.09.2018 19:47, George Dunlap wrote:
>>> From: Isaila Alexandru <aisaila@bitdefender.com>
>>>
>>> This patch adds access control for NPT mode.
>>>
>>> There aren’t enough extra bits to store the access rights in the NPT p2m
>>> table, so we add a radix tree to store extra information.
>>>
>>> For efficiency:
>>>   - Only allocate this radix tree when we first store "non-default"
>>>     extra information
>>>
>>>   - Remove entires which match the default extra information rather
>>>     than continuing to store them
>>>
>>>   - For superpages, only store an entry for the first gfn in the
>>>     superpage.  Use the order of the p2m entry being read to determine
>>>     the proper place to look in the radix table.
>>>
>>> Modify p2m_type_to_flags() to accept and interpret an access value,
>>> parallel to the ept code.
>>>
>>> Add a set_default_access() method to the p2m-pt and p2m-ept versions
>>> of the p2m rather than setting it directly, to deal with different
>>> default permitted access values.
>>>
>>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
>>> ---
>>> NB, this is compile-tested only.
>>>
>>> cc'ing Paul because this is functionality he may want at some point in
>>> the future.
>>>
>>> I'm not sure why we only allow 'int' to be stored in the radix tree,
>>> but that throws away 30-some bits we could otherwise use.  We might
>>> consider revising this if we run out of bits here.
>>>
>>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>>> CC: Jan Beulich <jbeulich@suse.com>
>>> CC: Tim Deegan <tim@xen.org>
>>> CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
>>> CC: Paul Durrant <paul.durrant@citrix.com>
>>> CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Hi all,
>>
>> I know it's been some time from the start of this patch but can this 
>> move forward? Any thoughts or acks are appreciated.
> Right, well where we left this, the situation was that on AMD hardware
> with IOMMU / p2m sharing, there aren't enough bits.
>
> The two general fixes we have so far are:
> 1. Add a parallel tree for extra bits (this patch)
> 2. Rip out IOMMU / p2m sharing for AMD.
>
> #2 has the advantage that we don't need an entirely separate tree, as
> well as getting rid of code that has (apparently) been completely dead
> for 5 years. #1 has the advantage that we're set up for having a much
> larger number of IOREQ servers in the future.
>
> Nobody objected to #2.  Without looking deeply into it, it seems like it
> might be a good idea, but I can't be sure without seeing what it would
> actually look like.
>
> The easiest way to press the point then would be to post a patch series
> ripping it out.

IOMMU / p2m sharing on AMD doesn't work, and isn't available any more
(despite the code looking suspiciously like it is usable).

There was a bugfix to allow DMA mapping of granted pages, which is a
prerequisite for PVH support, which requires using a nonzero p2m type,
and is therefore incompatible with IOMMU/p2m sharing.

I don't see any feasible way to bring p2m-sharing back into a working
state on AMD.

As a result, we'd be much better ripping out the dead code and more
formally acknowledging that it is a dead feature, after which the
existing p2m type/access infrastructure should work compatibly with the
Intel implementation.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT

[Tamas Lengyel]

On Wed, Sep 26, 2018 at 10:49 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> From: Isaila Alexandru <aisaila@bitdefender.com>
>
> This patch adds access control for NPT mode.
>
> There aren’t enough extra bits to store the access rights in the NPT p2m
> table, so we add a radix tree to store extra information.
>
> For efficiency:
>  - Only allocate this radix tree when we first store "non-default"
>    extra information
>
>  - Remove entires which match the default extra information rather
>    than continuing to store them
>
>  - For superpages, only store an entry for the first gfn in the
>    superpage.  Use the order of the p2m entry being read to determine
>    the proper place to look in the radix table.
>
> Modify p2m_type_to_flags() to accept and interpret an access value,
> parallel to the ept code.
>
> Add a set_default_access() method to the p2m-pt and p2m-ept versions
> of the p2m rather than setting it directly, to deal with different
> default permitted access values.
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

The mem_access/monitor bits are fairly trivial:

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

> ---
> NB, this is compile-tested only.

Are you planning to do some actual testing? I would highly recommend
that we see real test results before this is merged to verify
functionality.

Thanks,
Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT

[Razvan Cojocaru]

On 6/13/19 6:19 PM, Tamas Lengyel wrote:
> On Wed, Sep 26, 2018 at 10:49 AM George Dunlap <george.dunlap@citrix.com> wrote:
>>
>> From: Isaila Alexandru <aisaila@bitdefender.com>
>>
>> This patch adds access control for NPT mode.
>>
>> There aren’t enough extra bits to store the access rights in the NPT p2m
>> table, so we add a radix tree to store extra information.
>>
>> For efficiency:
>>   - Only allocate this radix tree when we first store "non-default"
>>     extra information
>>
>>   - Remove entires which match the default extra information rather
>>     than continuing to store them
>>
>>   - For superpages, only store an entry for the first gfn in the
>>     superpage.  Use the order of the p2m entry being read to determine
>>     the proper place to look in the radix table.
>>
>> Modify p2m_type_to_flags() to accept and interpret an access value,
>> parallel to the ept code.
>>
>> Add a set_default_access() method to the p2m-pt and p2m-ept versions
>> of the p2m rather than setting it directly, to deal with different
>> default permitted access values.
>>
>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
>> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> 
> The mem_access/monitor bits are fairly trivial:
> 
> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
> 
>> ---
>> NB, this is compile-tested only.
> 
> Are you planning to do some actual testing? I would highly recommend
> that we see real test results before this is merged to verify
> functionality.

We did do some testing with xen-access at the time, but limited testing 
with the actual full-blown introspection agent (because not all the 
needed pieces align yet).

Things did appear to work as intended.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation

[Tamas Lengyel]

On Wed, Sep 26, 2018 at 10:49 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> The name of the "with_gla" flag is confusing; it has nothing to do
> with the existence or lack thereof of a faulting GLA, but rather where
> the fault originated.  The npfec.kind value is always valid, and
> should thus be propagated, regardless of whether gla_valid is set or
> not.
>
> In particular, gla_valid will never be set on AMD systems; but
> npfec.kind will still be valid and should still be propagated.
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation

[Andrew Cooper]

On 26/09/18 17:47, George Dunlap wrote:
> The name of the "with_gla" flag is confusing; it has nothing to do
> with the existence or lack thereof of a faulting GLA, but rather where
> the fault originated.  The npfec.kind value is always valid, and
> should thus be propagated, regardless of whether gla_valid is set or
> not.
>
> In particular, gla_valid will never be set on AMD systems; but
> npfec.kind will still be valid and should still be propagated.
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Tamas K Lengyel <tamas.lengyel@zentific.com>
> CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
>  xen/arch/x86/mm/mem_access.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index d9e64fcbb9..699a315076 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -232,12 +232,12 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>          {
>              req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
>              req->u.mem_access.gla = gla;
> -
> -            if ( npfec.kind == npfec_kind_with_gla )
> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
> -            else if ( npfec.kind == npfec_kind_in_gpt )
> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
>          }
> +
> +        if ( npfec.kind == npfec_kind_with_gla )
> +            req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
> +        else if ( npfec.kind == npfec_kind_in_gpt )
> +            req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;

Nit.  Newline here please, as it is not logically related with the block
below.

~Andrew

>          req->u.mem_access.flags |= npfec.read_access    ? MEM_ACCESS_R : 0;
>          req->u.mem_access.flags |= npfec.write_access   ? MEM_ACCESS_W : 0;
>          req->u.mem_access.flags |= npfec.insn_fetch     ? MEM_ACCESS_X : 0;


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation

[Jan Beulich]

>>> On 26.09.18 at 19:00, <andrew.cooper3@citrix.com> wrote:
> On 26/09/18 17:47, George Dunlap wrote:
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -232,12 +232,12 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>          {
>>              req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
>>              req->u.mem_access.gla = gla;
>> -
>> -            if ( npfec.kind == npfec_kind_with_gla )
>> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
>> -            else if ( npfec.kind == npfec_kind_in_gpt )
>> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
>>          }
>> +
>> +        if ( npfec.kind == npfec_kind_with_gla )
>> +            req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
>> +        else if ( npfec.kind == npfec_kind_in_gpt )
>> +            req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
> 
> Nit.  Newline here please, as it is not logically related with the block
> below.

And, despite it being just two comparisons, perhaps better to
make it a switch() at the same time?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation

[George Dunlap]

On 09/27/2018 08:04 AM, Jan Beulich wrote:
>>>> On 26.09.18 at 19:00, <andrew.cooper3@citrix.com> wrote:
>> On 26/09/18 17:47, George Dunlap wrote:
>>> --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -232,12 +232,12 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>>>          {
>>>              req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
>>>              req->u.mem_access.gla = gla;
>>> -
>>> -            if ( npfec.kind == npfec_kind_with_gla )
>>> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
>>> -            else if ( npfec.kind == npfec_kind_in_gpt )
>>> -                req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
>>>          }
>>> +
>>> +        if ( npfec.kind == npfec_kind_with_gla )
>>> +            req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA;
>>> +        else if ( npfec.kind == npfec_kind_in_gpt )
>>> +            req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT;
>>
>> Nit.  Newline here please, as it is not logically related with the block
>> below.
> 
> And, despite it being just two comparisons, perhaps better to
> make it a switch() at the same time?

Sure -- this is logically separate from the follow-up patch, so I'll
re-send it as a singleton with the comments.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel