Xen devel - Nov 2011 - [PATCH 0 of 2] New mem access type: None -> RWX

This patch adds a new p2m access type, n2rwx. It allows for implement a
"log
access" mode in the hypervisor, aking to log dirty but for all types of
accesses. Faults caused by this access mode automatically promote the
access rights of the ofending p2m entry, place the event in the ring, and
let the vcpu keep on executing.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Signed-off-by: Adin Scannell <adin@scannell.ca>

 xen/arch/x86/hvm/hvm.c          |  45 ++++++++++++++++++++++++++++++++++------
 xen/arch/x86/mm/p2m.c           |   8 ++++--
 xen/include/asm-x86/p2m.h       |   9 ++++---
 xen/arch/x86/hvm/hvm.c          |   1 +
 xen/arch/x86/mm/p2m-ept.c       |   1 +
 xen/arch/x86/mm/p2m.c           |  30 +++++++++++++++++++--------
 xen/include/asm-x86/p2m.h       |   3 ++
 xen/include/public/hvm/hvm_op.h |   3 ++
 8 files changed, 77 insertions(+), 23 deletions(-)

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 <andres@lagarcavilla.org>

diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -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;
+            }
         }
     }
 
@@ -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.
      */
-    if ( (p2mt == p2m_mmio_dm) || (p2mt == p2m_ram_ro) )
+    if ( (p2mt == p2m_mmio_dm) || 
+         (access_w && (p2mt == p2m_ram_ro)) )
     {
         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);
@@ -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.
          */
-        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);
         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;
     }
 
+    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;
+    }
+
+    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;
+    }
+
+    if ( p2mt == p2m_mmio_direct )
+    {
+        if ( !(access_w && 
+                rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn))) ) {
+            rc = 1;
+            goto out_put_gfn;
+        }
+    }
+
     rc = 0;
 out_put_gfn:
     put_gfn(p2m->domain, gfn);
diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -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
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -491,8 +491,9 @@ static inline void p2m_mem_paging_popula
 
 #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. */
+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);
 /* Resumes the running of the VCPU, restarting the last instruction */
 void p2m_mem_access_resume(struct domain *d);
@@ -508,10 +509,10 @@ int p2m_get_mem_access(struct domain *d,
                        hvmmem_access_t *access);
 
 #else
-static inline void p2m_mem_access_check(unsigned long gpa, bool_t gla_valid, 
+static inline 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)
-{ }
+{ return 1; }
 static inline int p2m_set_mem_access(struct domain *d, 
                                      unsigned long start_pfn, 
                                      uint32_t nr, hvmmem_access_t access)

xen/arch/x86/hvm/hvm.c          |   1 +
 xen/arch/x86/mm/p2m-ept.c       |   1 +
 xen/arch/x86/mm/p2m.c           |  30 +++++++++++++++++++++---------
 xen/include/asm-x86/p2m.h       |   3 +++
 xen/include/public/hvm/hvm_op.h |   3 +++
 5 files changed, 29 insertions(+), 9 deletions(-)


This patch adds a new p2m access type, n2rwx. It allows for implement a
"log
access" mode in the hypervisor, aking to log dirty but for all types of
accesses. Faults caused by this access mode automatically promote the
access rights of the ofending p2m entry, place the event in the ring, and
let the vcpu keep on executing.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Signed-off-by: Adin Scannell <adin@scannell.ca>

diff -r d6354df726a0 -r 52d6aede6206 xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1250,6 +1250,7 @@ int hvm_hap_nested_page_fault(unsigned l
         switch (p2ma) 
         {
         case p2m_access_n:
+        case p2m_access_n2rwx:
         default:
             violation = access_r || access_w || access_x;
             break;
diff -r d6354df726a0 -r 52d6aede6206 xen/arch/x86/mm/p2m-ept.c
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -111,6 +111,7 @@ static void ept_p2m_type_to_flags(ept_en
     switch (access) 
     {
         case p2m_access_n:
+        case p2m_access_n2rwx:
             entry->r = entry->w = entry->x = 0;
             break;
         case p2m_access_r:
diff -r d6354df726a0 -r 52d6aede6206 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1148,6 +1148,11 @@ int p2m_mem_access_check(unsigned long g
         p2m_unlock(p2m);
         return 1;
     }
+    else if ( p2ma == p2m_access_n2rwx )
+    {
+        ASSERT(access_w || access_r || access_x);
+        p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt, p2m_access_rwx);
+    }
     p2m_unlock(p2m);
 
     /* Otherwise, check if there is a memory event listener, and send the
message along */
@@ -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);
-            p2m_unlock(p2m);
+            if ( p2ma != p2m_access_n2rwx )
+            {
+                /* 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);
+                p2m_unlock(p2m);
+            }
             return 1;
         }
 
@@ -1176,9 +1184,12 @@ int p2m_mem_access_check(unsigned long g
     req.type = MEM_EVENT_TYPE_ACCESS;
     req.reason = MEM_EVENT_REASON_VIOLATION;
 
-    /* Pause the current VCPU unconditionally */
-    vcpu_pause_nosync(v);
-    req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED;    
+    /* Pause the current VCPU */
+    if ( p2ma != p2m_access_n2rwx )
+    {
+        vcpu_pause_nosync(v);
+        req.flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
+    } 
 
     /* Send request to mem event */
     req.gfn = gfn;
@@ -1192,8 +1203,8 @@ int p2m_mem_access_check(unsigned long g
     req.vcpu_id = v->vcpu_id;
 
     (void)mem_event_put_request(d, &d->mem_access, &req);
-    /* VCPU paused */
-    return 0;
+    /* VCPU may be paused, return whether we promoted automatically */
+    return (p2ma == p2m_access_n2rwx);
 }
 
 void p2m_mem_access_resume(struct domain *d)
@@ -1237,6 +1248,7 @@ int p2m_set_mem_access(struct domain *d,
         p2m_access_wx,
         p2m_access_rwx,
         p2m_access_rx2rw,
+        p2m_access_n2rwx,
         p2m->default_access,
     };
 
diff -r d6354df726a0 -r 52d6aede6206 xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -108,6 +108,9 @@ typedef enum {
     p2m_access_wx    = 6, 
     p2m_access_rwx   = 7,
     p2m_access_rx2rw = 8, /* Special: page goes from RX to RW on write */
+    p2m_access_n2rwx = 9, /* Special: page goes from N to RWX on access, *
+                           * generates an event but does not pause the
+                           * vcpu */
 
     /* NOTE: Assumed to be only 4 bits right now */
 } p2m_access_t;
diff -r d6354df726a0 -r 52d6aede6206 xen/include/public/hvm/hvm_op.h
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -174,6 +174,9 @@ typedef enum {
     HVMMEM_access_rwx,
     HVMMEM_access_rx2rw,       /* Page starts off as r-x, but automatically
                                 * change to r-w on a write */
+    HVMMEM_access_n2rwx,       /* Log access: starts off as n, automatically 
+                                * goes to rwx, generating an event without
+                                * pausing the vcpu */
     HVMMEM_access_default      /* Take the domain default */
 } hvmmem_access_t;
 /* Notify that a region of memory is to have specific access types */

Hi, 

At 16:58 -0500 on 29 Nov (1322585904), Andres Lagar-Cavilla
wrote:
>  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 <andres@lagarcavilla.org>
> 
> diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/hvm/hvm.c
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -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;
> +            }
>          }
>      }
>  
> @@ -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.
>       */
> -    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. 
>      {
>          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);
> @@ -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.
>           */
> -        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. 
>          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;
>      }
>  
> +    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.
> +    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;
> +    }
> +    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. 
> +
>      rc = 0;
>  out_put_gfn:
>      put_gfn(p2m->domain, gfn);
> diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/mm/p2m.c
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -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
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -491,8 +491,9 @@ static inline void p2m_mem_paging_popula
>  
>  #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.

At 16:58 -0500 on 29 Nov (1322585905), Andres Lagar-Cavilla
wrote:
> @@ -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);
> -            p2m_unlock(p2m);
> +            if ( p2ma != p2m_access_n2rwx )
> +            {
> +                /* 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);
> +                p2m_unlock(p2m);
> +            }
>              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.

> At 16:58 -0500 on 29 Nov (1322585905), Andres Lagar-Cavilla wrote:
>> @@ -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);
>> -            p2m_unlock(p2m);
>> +            if ( p2ma != p2m_access_n2rwx )
>> +            {
>> +                /* 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);
>> +                p2m_unlock(p2m);
>> +            }
>>              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.
The entry is set to p2m_access_rwx, so no additional faults will be
generated. In the n2rwx case, the entry was promoted to rwx previously, in
 a p2m_lock protected section. We''re avoiding a repeat here.

Andres
>
> Cheers,
>
> Tim.
>

> Hi,
>
> At 16:58 -0500 on 29 Nov (1322585904), Andres Lagar-Cavilla wrote:
>>  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 <andres@lagarcavilla.org>
>>
>> diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/hvm/hvm.c
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -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;
>> +            }
>>          }
>>      }
>>
>> @@ -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.
>>       */
>> -    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.
>
The new checks here and below are necessary because the fault handler
assumes that the fault could not have happened due to a constrain on the
access rights. So, new cases arise, such as the mmio_direct below. I can
add all the additional checks in patch 1, and allow the fall through in
patch 2.

(more below ...)
>>      {
>>          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);
>> @@ -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.
>>           */
>> -        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.
>
Yup, thanks.
>>          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;
>>      }
>>
>> +    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.
>
>> +    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;
>> +    }
>> +    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.
>
That''s the behavior without this patch, isn''t it?
>> +
>>      rc = 0;
>>  out_put_gfn:
>>      put_gfn(p2m->domain, gfn);
>> diff -r 29701f5bdd84 -r d6354df726a0 xen/arch/x86/mm/p2m.c
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -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
>> --- a/xen/include/asm-x86/p2m.h
>> +++ b/xen/include/asm-x86/p2m.h
>> @@ -491,8 +491,9 @@ static inline void p2m_mem_paging_popula
>>
>>  #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.)
>
Ok, bool_t.
> Cheers,
>
> Tim.
>

At 08:39 -0800 on 01 Dec (1322728771), Andres Lagar-Cavilla
wrote:
> > 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?
Yes, but that''s not the _interesting_ change in this patch. :) 
The interesting thing is that you can upgrade the access rights and also
pass the fault to the mmio emulator without returning and retrying in
between. 

That is, I thin the change from 

    if (access fixup) return 1
    handle MMIO
    ...
    return 0 /* unhandled -- crash */

to 

   if (access fixup) remember we''ve done something
   handle MMIO
   ...
   if (we did a fixup above)
     return 1  /* try again */
   else
     return 0  /* crash */

does what you want without making the handler much bigger and more
brittle. 

Tim.