New mem access type: None -> RWX

 xen/arch/x86/hvm/hvm.c    |  45 ++++++++++++++++++++++++++++++++++++++-------
 xen/arch/x86/mm/p2m.c     |   8 +++++---
 xen/include/asm-x86/p2m.h |   9 +++++----
 3 files changed, 48 insertions(+), 14 deletions(-)

The mem event fault handler in the p2m can automatically promote the access rights of a p2m entry. In those scenarios, vcpu's are not paused and they will immediately retry the faulting instructions. This will generate a second fault if the underlying entry type requires so (paging, unsharing, pod, etc). Collapse the two faults into a single one.

Signed-off-by Andres Lagar-Cavilla <[email protected]>

diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/hvm/hvm.c

xen/arch/x86/hvm/hvm.c +7 -3

@@ -1278,9 +1278,13 @@ int hvm_hap_nested_page_fault(unsigned l
 
         if ( violation )
         {
-            p2m_mem_access_check(gpa, gla_valid, gla, access_r, access_w, access_x);
-            rc = 1;
-            goto out_put_gfn;
+            if ( !p2m_mem_access_check(gpa, gla_valid, gla, access_r, 
+                                        access_w, access_x) )
+            {
+                /* Rights not promoted, vcpu paused, work here is done */
+                rc = 1;
+                goto out_put_gfn;
+            }
         }
     }
 

xen/arch/x86/hvm/hvm.c +1 -1

     {
         if ( !handle_mmio() )
             hvm_inject_exception(TRAP_gp_fault, 0, 0);

@@ -1302,7 +1307,7 @@ int hvm_hap_nested_page_fault(unsigned l
         p2m_mem_paging_populate(v->domain, gfn);
 
     /* Mem sharing: unshare the page and try again */
-    if ( p2mt == p2m_ram_shared )
+    if ( access_w && (p2mt == p2m_ram_shared) )
     {
         ASSERT(!p2m_is_nestedp2m(p2m));
         mem_sharing_unshare_page(p2m->domain, gfn, 0);

xen/arch/x86/hvm/hvm.c +1 -1

         rc = 1;
         goto out_put_gfn;
     }
 
     /* Shouldn't happen: Maybe the guest was writing to a r/o grant mapping? */
-    if ( p2mt == p2m_grant_map_ro )
+    if ( access_w && (p2mt == p2m_grant_map_ro) )
     {
         gdprintk(XENLOG_WARNING,
                  "trying to write to read-only grant mapping\n");

@@ -1335,6 +1341,31 @@ int hvm_hap_nested_page_fault(unsigned l
         goto out_put_gfn;

xen/arch/x86/hvm/hvm.c +4 -0

+    if ( p2m_is_grant(p2mt) )
+    {
+        /* If we haven't caught this by now, then it's a valid access */
+        rc = 1;

xen/arch/x86/hvm/hvm.c +1 -0

+
     rc = 0;
 out_put_gfn:
     put_gfn(p2m->domain, gfn);

diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c +5 -3

@@ -1126,7 +1126,7 @@ void p2m_mem_paging_resume(struct domain
     mem_event_unpause_vcpus(d, &d->mem_paging);
 }
 
-void p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned long gla, 
+int p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned long gla, 
                           bool_t access_r, bool_t access_w, bool_t access_x)
 {
     struct vcpu *v = current;

@@ -1146,7 +1146,7 @@ void p2m_mem_access_check(unsigned long 
     {
         p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rw);
         p2m_unlock(p2m);
-        return;
+        return 1;
     }
     p2m_unlock(p2m);
 

@@ -1166,9 +1166,10 @@ void p2m_mem_access_check(unsigned long 
             p2m_lock(p2m);
             p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rwx);
             p2m_unlock(p2m);
+            return 1;
         }
 
-        return;
+        return 0;
     }
 
     memset(&req, 0, sizeof(req));

@@ -1192,6 +1193,7 @@ void p2m_mem_access_check(unsigned long 
 
     (void)mem_event_put_request(d, &d->mem_access, &req);
     /* VCPU paused */
+    return 0;
 }
 
 void p2m_mem_access_resume(struct domain *d)

diff -r 29701f5bdd84 -r d6354df726a0 xen/include/asm-x86/p2m.h

Hi,

Quoted 40 lines · Andres Lagar-Cavilla

 xen/arch/x86/hvm/hvm.c    |  45
++++++++++++++++++++++++++++++++++++++-------
 xen/arch/x86/mm/p2m.c     |   8 +++++---
 xen/include/asm-x86/p2m.h |   9 +++++----
 3 files changed, 48 insertions(+), 14 deletions(-)

The mem event fault handler in the p2m can automatically promote the access
rights of a p2m entry. In those scenarios, vcpu's are not paused and they will
immediately retry the faulting instructions. This will generate a second fault
if the underlying entry type requires so (paging, unsharing, pod, etc). Collapse the two faults into a single one.

Signed-off-by Andres Lagar-Cavilla <[email protected]>

diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/hvm/hvm.c

xen/arch/x86/hvm/hvm.c +7 -3

@@ -1278,9 +1278,13 @@ int hvm_hap_nested_page_fault(unsigned l
 
         if ( violation )
         {
-            p2m_mem_access_check(gpa, gla_valid, gla, access_r,
 access_w, access_x);
-            rc = 1;
-            goto out_put_gfn;
+            if ( !p2m_mem_access_check(gpa, gla_valid, gla, access_r,
+                                        access_w, access_x) )
+            {
+                /* Rights not promoted, vcpu paused, work here is done
 */
+                rc = 1;
+                goto out_put_gfn;
+            }
         }

     }

xen/arch/x86/hvm/hvm.c int hvm_hap_nested_page_fault(unsigned l · line 1296

Quoted 8 lines

xen/arch/x86/hvm/hvm.c +0 -0

@@ -1288,7 +1292,8 @@ int hvm_hap_nested_page_fault(unsigned l
      * If this GFN is emulated MMIO or marked as read-only, pass the

fault
* to the mmio handler.
*/

xen/arch/x86/hvm/hvm.c +2 -1

-    if ( (p2mt == p2m_mmio_dm) || (p2mt == p2m_ram_ro) )
+    if ( (p2mt == p2m_mmio_dm) ||
+         (access_w && (p2mt == p2m_ram_ro)) )

I think this is a separate change from the main intent of the patch; it would be better to have two patches, once that inserts all these 'access_w' checks and a second that does what the cset comment decribes.

Quoted 16 lines

     {
         if ( !handle_mmio() )
             hvm_inject_exception(TRAP_gp_fault, 0, 0);

xen/arch/x86/hvm/hvm.c +1 -1

@@ -1302,7 +1307,7 @@ int hvm_hap_nested_page_fault(unsigned l
         p2m_mem_paging_populate(v->domain, gfn);
 
     /* Mem sharing: unshare the page and try again */
-    if ( p2mt == p2m_ram_shared )
+    if ( access_w && (p2mt == p2m_ram_shared) )
     {
         ASSERT(!p2m_is_nestedp2m(p2m));
         mem_sharing_unshare_page(p2m->domain, gfn, 0);

@@ -1319,14 +1324,15 @@ int hvm_hap_nested_page_fault(unsigned l
          * a large page, we do not change other pages type within that

large
* page.

xen/arch/x86/hvm/hvm.c int hvm_hap_nested_page_fault(unsigned l · line 1329

Quoted 5 lines

          */

xen/arch/x86/hvm/hvm.c +2 -1

-        paging_mark_dirty(v->domain, mfn_x(mfn));
+        if ( access_w )
+            paging_mark_dirty(v->domain, mfn_x(mfn));
         p2m_change_type(v->domain, gfn, p2m_ram_logdirty, p2m_ram_rw);

No! If we call p2m_change_type(-->ram_rw) we _must_ call mark_dirty() too. It would be OK to put both lines under the test, though.

Quoted 13 lines

         rc = 1;
         goto out_put_gfn;
     }

     /* Shouldn't happen: Maybe the guest was writing to a r/o grant

mapping? */

xen/arch/x86/hvm/hvm.c +1 -1

-    if ( p2mt == p2m_grant_map_ro )
+    if ( access_w && (p2mt == p2m_grant_map_ro) )
     {
         gdprintk(XENLOG_WARNING,
                  "trying to write to read-only grant mapping\n");

@@ -1335,6 +1341,31 @@ int hvm_hap_nested_page_fault(unsigned l
         goto out_put_gfn;

xen/arch/x86/hvm/hvm.c int hvm_hap_nested_page_fault(unsigned l · line 1351

Quoted 10 lines

xen/arch/x86/hvm/hvm.c +8 -0

     }
 
+    if ( access_x && (p2m_is_grant(p2mt)) )
+    {
+        gdprintk(XENLOG_WARNING,
+                 "trying to execut a grant mapping\n");
+        hvm_inject_exception(TRAP_gp_fault, 0, 0);
+        rc = 1;
+        goto out_put_gfn;
+    }

Again, this is a separate bugfix and should go in its own patch.

Quoted 6 lines

xen/arch/x86/hvm/hvm.c +5 -0

+    if ( p2m_is_grant(p2mt) )
+    {
+        /* If we haven't caught this by now, then it's a valid access
 */
+        rc = 1;
+        goto out_put_gfn;

xen/arch/x86/hvm/hvm.c int hvm_hap_nested_page_fault(unsigned l · line 1367

Quoted 10 lines

xen/arch/x86/hvm/hvm.c +9 -0

+    }
+    if ( p2mt == p2m_mmio_direct )
+    {
+        if ( !(access_w &&
+                rangeset_contains_singleton(mmio_ro_ranges,
 mfn_x(mfn))) ) {
+            rc = 1;
+            goto out_put_gfn;
+        }
+    }

I wonder whether, rather than trying to enumerate all the acceptable cases here, you could just remember that p2m_mem_access_check() changed something and always return 1 in that case.

Quoted 55 lines

xen/arch/x86/hvm/hvm.c +1 -0

+
     rc = 0;
 out_put_gfn:
     put_gfn(p2m->domain, gfn);

diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/mm/p2m.c

xen/arch/x86/mm/p2m.c +1 -1

@@ -1126,7 +1126,7 @@ void p2m_mem_paging_resume(struct domain
     mem_event_unpause_vcpus(d, &d->mem_paging);
 }
 
-void p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned
 long gla,
+int p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned
 long gla,
                           bool_t access_r, bool_t access_w, bool_t

access_x)

 {
     struct vcpu *v = current;

xen/arch/x86/mm/p2m.c +3 -2

@@ -1146,7 +1146,7 @@ void p2m_mem_access_check(unsigned long
     {
         p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt,
 p2m_access_rw);
         p2m_unlock(p2m);
-        return;
+        return 1;
     }
     p2m_unlock(p2m);

@@ -1166,9 +1166,10 @@ void p2m_mem_access_check(unsigned long
             p2m_lock(p2m);
             p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt,
 p2m_access_rwx);
             p2m_unlock(p2m);
+            return 1;
         }
 
-        return;
+        return 0;
     }
 

     memset(&req, 0, sizeof(req));

xen/arch/x86/mm/p2m.c +1 -0

@@ -1192,6 +1193,7 @@ void p2m_mem_access_check(unsigned long
 
     (void)mem_event_put_request(d, &d->mem_access, &req);
     /* VCPU paused */
+    return 0;
 }
 
 void p2m_mem_access_resume(struct domain *d)

diff -r 29701f5bdd84 -r d6354df726a0 xen/include/asm-x86/p2m.h

xen/include/asm-x86/p2m.h +0 -0

@@ -491,8 +491,9 @@ static inline void p2m_mem_paging_popula
 

Quoted 9 lines

xen/include/asm-x86/p2m.h +0 -0

 #ifdef __x86_64__
 /* Send mem event based on the access (gla is -1ull if not available).

Handles
- * the rw2rx conversion */
-void p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, unsigned long gla,
+ * the rw2rx conversion. Return value indicate if access rights have been
+ * promoted with no underlying vcpu pause. */

How does it indicate that -- i.e., what values can it return and what do they mean? (And if it only returns 0 or 1, maybe use bool_t.)

Cheers,

Tim.

Quoted 4 lines

I wonder whether, rather than trying to enumerate all the acceptable cases here, you could just remember that p2m_mem_access_check() changed something and always return 1 in that case.

That's the behavior without this patch, isn't it?

xen/arch/x86/mm/p2m.c +0 -3

@@ -1162,10 +1167,13 @@ int p2m_mem_access_check(unsigned long g
         }
         else
         {
-            /* A listener is not required, so clear the access restrictions */
-            p2m_lock(p2m);
-            p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rwx);

Quoted 18 lines · Andres Lagar-Cavilla

(quoted diff) +0 -1

@@ -1162,10 +1167,13 @@ int p2m_mem_access_check(unsigned long g
         }
         else
         {
-            /* A listener is not required, so clear the access

restrictions */

(quoted diff) +0 -2

-            p2m_lock(p2m);
-            p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt,

p2m_access_rwx);

xen/arch/x86/mm/p2m.c +3 -1

-            p2m_unlock(p2m);
+            if ( p2ma != p2m_access_n2rwx )
+            {
+                /* A listener is not required, so clear the access

restrictions */

(quoted diff) +2 -0

+                p2m_lock(p2m);
+                p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt,

p2m_access_rwx);
+ p2m_unlock(p2m);

Quoted 3 lines

+ }

             return 1;
         }

This logic is getting a bit convoluted, and I'm not sure it's correct. If a page is marked n2rwx and there's no listener, it looks like this will cause it to spin forever re-taking the fault rather than pausing it waiting for the listener to attach.

Cheers,

Tim.