[PATCH v5 0/2] add new p2m type class and new p2m type

[PATCH v5 0/2] add new p2m type class and new p2m type

[Yu Zhang]

XenGT (Intel Graphics Virtualization technology, please refer to
https://01.org/xen/blogs/srclarkx/2013/graphics-virtualization-
xengt) driver runs inside Dom0 as a virtual graphics device model,
and needs to trap and emulate the guest's write operations to some
specific memory pages, like memory pages used by guest graphics
driver as PPGTT(per-process graphics translation table). We added
a new p2m type, p2m_mmio_write_dm, to trap and emulate the write
operations on these graphic page tables. 

Handling of this new p2m type are similar with existing p2m_ram_ro
in most condition checks, with only difference on final policy of
emulation vs. drop. For p2m_ram_ro types, write operations will not
trigger the device model, and will be discarded later in __hvm_copy();
while for the p2m_mmio_write_dm type pages, writes will go to the
device model via ioreq-server.

Previously, the conclusion in our v3 patch review is to provide a
more generalized HVMOP_map_io_range_to_ioreq_server hypercall, by
seperating rangesets inside a ioreq server to read-protected/write-
protected/both-prtected. Yet, after offline discussion with Paul,
we believe a more simplified solution may suffice. We can keep the 
existing HVMOP_map_io_range_to_ioreq_server hypercall, and let the 
user decide whether or not a p2m type change is necessary, because
in most cases the emulator will already use the p2m_mmio_dm type.

Changes from v4:
 - A new p2m type class, P2M_DISCARD_WRITE_TYPES, is added;
 - A new predicate, p2m_is_discard_write, is used in __hvm_copy()/
   __hvm_clear()/emulate_gva_to_mfn()/hvm_hap_nested_page_fault(),
   to discard the write operations;
 - The new p2m type, p2m_mmio_write_dm, is added to P2M_RO_TYPES;
 - Coding style changes;

Changes from v3:
 - Use the existing HVMOP_map_io_range_to_ioreq_server hypercall
   to add write protected range;
 - Modify the HVMOP_set_mem_type hypercall to support the new p2m
   type for this range.

Changes from v2:
 - Remove excute attribute of the new p2m type p2m_mmio_write_dm;
 - Use existing rangeset for keeping the write protection page range
   instead of introducing hash table;
 - Some code style fix.

Changes from v1:
 - Changes the new p2m type name from p2m_ram_wp to p2m_mmio_write_dm.
   This means that we treat the pages as a special mmio range instead
   of ram;
 - Move macros to c file since only this file is using them.
 - Address various comments from Jan.

Yu Zhang (2):
  Add a new p2m type class - P2M_DISCARD_WRITE_TYPES
  add a new p2m type - p2m_mmio_write_dm

 xen/arch/x86/hvm/hvm.c          | 25 ++++++++++---------------
 xen/arch/x86/mm/p2m-ept.c       |  1 +
 xen/arch/x86/mm/p2m-pt.c        |  1 +
 xen/arch/x86/mm/shadow/multi.c  |  2 +-
 xen/include/asm-x86/p2m.h       |  9 ++++++++-
 xen/include/public/hvm/hvm_op.h |  1 +
 6 files changed, 22 insertions(+), 17 deletions(-)

-- 
1.9.1

[PATCH v5 1/2] add a new p2m type class - P2M_DISCARD_WRITE_TYPES

[Yu Zhang]

From: Yu Zhang <yu.c.zhang@intel.com>

Currently, the P2M_RO_TYPES bears 2 meanings: one is
"_PAGE_RW bit is clear in their PTEs", and another is
to discard the write operations on these pages. This
patch adds a p2m type class, P2M_DISCARD_WRITE_TYPES,
to bear the second meaning, so we can use this type
class instead of the P2M_RO_TYPES, to decide if a write
operation is to be ignored.

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
---
 xen/arch/x86/hvm/hvm.c         | 16 +++-------------
 xen/arch/x86/mm/shadow/multi.c |  2 +-
 xen/include/asm-x86/p2m.h      |  5 +++++
 3 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 51ffc90..967f822 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2837,7 +2837,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
      * to the mmio handler.
      */
     if ( (p2mt == p2m_mmio_dm) || 
-         (npfec.write_access && (p2mt == p2m_ram_ro)) )
+         (npfec.write_access && (p2m_is_discard_write(p2mt))) )
     {
         put_gfn(p2m->domain, gfn);
 
@@ -2882,16 +2882,6 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
         goto out_put_gfn;
     }
 
-    /* Shouldn't happen: Maybe the guest was writing to a r/o grant mapping? */
-    if ( npfec.write_access && (p2mt == p2m_grant_map_ro) )
-    {
-        gdprintk(XENLOG_WARNING,
-                 "trying to write to read-only grant mapping\n");
-        hvm_inject_hw_exception(TRAP_gp_fault, 0);
-        rc = 1;
-        goto out_put_gfn;
-    }
-
     /* If we fell through, the vcpu will retry now that access restrictions have
      * been removed. It may fault again if the p2m entry type still requires so.
      * Otherwise, this is an error condition. */
@@ -3941,7 +3931,7 @@ static enum hvm_copy_result __hvm_copy(
 
         if ( flags & HVMCOPY_to_guest )
         {
-            if ( p2mt == p2m_ram_ro )
+            if ( p2m_is_discard_write(p2mt) )
             {
                 static unsigned long lastpage;
                 if ( xchg(&lastpage, gfn) != gfn )
@@ -4035,7 +4025,7 @@ static enum hvm_copy_result __hvm_clear(paddr_t addr, int size)
 
         p = (char *)__map_domain_page(page) + (addr & ~PAGE_MASK);
 
-        if ( p2mt == p2m_ram_ro )
+        if ( p2m_is_discard_write(p2mt) )
         {
             static unsigned long lastpage;
             if ( xchg(&lastpage, gfn) != gfn )
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 225290e..94cf06d 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -4575,7 +4575,7 @@ static mfn_t emulate_gva_to_mfn(struct vcpu *v,
     {
         return _mfn(BAD_GFN_TO_MFN);
     }
-    if ( p2m_is_readonly(p2mt) )
+    if ( p2m_is_discard_write(p2mt) )
     {
         put_page(page);
         return _mfn(READONLY_GFN);
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 5f7fe71..42de75d 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -113,6 +113,10 @@ typedef unsigned int p2m_query_t;
                       | p2m_to_mask(p2m_grant_map_ro)   \
                       | p2m_to_mask(p2m_ram_shared) )
 
+/* Write-discard types, which should discard the write operations */
+#define P2M_DISCARD_WRITE_TYPES (p2m_to_mask(p2m_ram_ro)     \
+                      | p2m_to_mask(p2m_grant_map_ro))
+
 /* Types that can be subject to bulk transitions. */
 #define P2M_CHANGEABLE_TYPES (p2m_to_mask(p2m_ram_rw) \
                               | p2m_to_mask(p2m_ram_logdirty) )
@@ -145,6 +149,7 @@ typedef unsigned int p2m_query_t;
 #define p2m_is_hole(_t) (p2m_to_mask(_t) & P2M_HOLE_TYPES)
 #define p2m_is_mmio(_t) (p2m_to_mask(_t) & P2M_MMIO_TYPES)
 #define p2m_is_readonly(_t) (p2m_to_mask(_t) & P2M_RO_TYPES)
+#define p2m_is_discard_write(_t) (p2m_to_mask(_t) & P2M_DISCARD_WRITE_TYPES)
 #define p2m_is_changeable(_t) (p2m_to_mask(_t) & P2M_CHANGEABLE_TYPES)
 #define p2m_is_pod(_t) (p2m_to_mask(_t) & P2M_POD_TYPES)
 #define p2m_is_grant(_t) (p2m_to_mask(_t) & P2M_GRANT_TYPES)
-- 
1.9.1

[PATCH v5 2/2] add a new p2m type - p2m_mmio_write_dm

[Yu Zhang]

From: Yu Zhang <yu.c.zhang@intel.com>

A new p2m type, p2m_mmio_write_dm, is added to trap and emulate
the write operations on GPU's page tables. Handling of this new
p2m type are similar with existing p2m_ram_ro in most condition
checks, with only difference on final policy of emulation vs. drop.
For p2m_ram_ro types, write operations will not trigger the device
model, and will be discarded later in __hvm_copy(); while for the
p2m_mmio_write_dm type pages, writes will go to the device model
via ioreq-server.

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
Signed-off-by: Wei Ye <wei.ye@intel.com>
---
 xen/arch/x86/hvm/hvm.c          | 11 ++++++++---
 xen/arch/x86/mm/p2m-ept.c       |  1 +
 xen/arch/x86/mm/p2m-pt.c        |  1 +
 xen/include/asm-x86/p2m.h       |  4 +++-
 xen/include/public/hvm/hvm_op.h |  1 +
 5 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 967f822..b4bdfab 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2837,7 +2837,8 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
      * to the mmio handler.
      */
     if ( (p2mt == p2m_mmio_dm) || 
-         (npfec.write_access && (p2m_is_discard_write(p2mt))) )
+         (npfec.write_access &&
+          (p2m_is_discard_write(p2mt) || (p2mt == p2m_mmio_write_dm))) )
     {
         put_gfn(p2m->domain, gfn);
 
@@ -5904,6 +5905,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
             get_gfn_query_unlocked(d, a.pfn, &t);
             if ( p2m_is_mmio(t) )
                 a.mem_type =  HVMMEM_mmio_dm;
+            else if ( t == p2m_mmio_write_dm )
+                a.mem_type = HVMMEM_mmio_write_dm;
             else if ( p2m_is_readonly(t) )
                 a.mem_type =  HVMMEM_ram_ro;
             else if ( p2m_is_ram(t) )
@@ -5931,7 +5934,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         static const p2m_type_t memtype[] = {
             [HVMMEM_ram_rw]  = p2m_ram_rw,
             [HVMMEM_ram_ro]  = p2m_ram_ro,
-            [HVMMEM_mmio_dm] = p2m_mmio_dm
+            [HVMMEM_mmio_dm] = p2m_mmio_dm,
+            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
         };
 
         if ( copy_from_guest(&a, arg, 1) )
@@ -5978,7 +5982,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
                 goto param_fail4;
             }
             if ( !p2m_is_ram(t) &&
-                 (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) )
+                 (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) &&
+                 t != p2m_mmio_write_dm )
             {
                 put_gfn(d, pfn);
                 goto param_fail4;
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 15c6e83..e21a92d 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -136,6 +136,7 @@ static void ept_p2m_type_to_flags(ept_entry_t *entry, p2m_type_t type, p2m_acces
             entry->x = 0;
             break;
         case p2m_grant_map_ro:
+        case p2m_mmio_write_dm:
             entry->r = 1;
             entry->w = entry->x = 0;
             break;
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index e48b63a..26fb18d 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -94,6 +94,7 @@ static unsigned long p2m_type_to_flags(p2m_type_t t, mfn_t mfn)
     default:
         return flags | _PAGE_NX_BIT;
     case p2m_grant_map_ro:
+    case p2m_mmio_write_dm:
         return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
     case p2m_ram_ro:
     case p2m_ram_logdirty:
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 42de75d..866fb0d 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -72,6 +72,7 @@ typedef enum {
     p2m_ram_shared = 12,          /* Shared or sharable memory */
     p2m_ram_broken = 13,          /* Broken page, access cause domain crash */
     p2m_map_foreign  = 14,        /* ram pages from foreign domain */
+    p2m_mmio_write_dm = 15,       /* Read-only; writes go to the device model */
 } p2m_type_t;
 
 /* Modifiers to the query */
@@ -111,7 +112,8 @@ typedef unsigned int p2m_query_t;
 #define P2M_RO_TYPES (p2m_to_mask(p2m_ram_logdirty)     \
                       | p2m_to_mask(p2m_ram_ro)         \
                       | p2m_to_mask(p2m_grant_map_ro)   \
-                      | p2m_to_mask(p2m_ram_shared) )
+                      | p2m_to_mask(p2m_ram_shared)   \
+                      | p2m_to_mask(p2m_mmio_write_dm))
 
 /* Write-discard types, which should discard the write operations */
 #define P2M_DISCARD_WRITE_TYPES (p2m_to_mask(p2m_ram_ro)     \
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index eeb0a60..a4e5345 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -81,6 +81,7 @@ typedef enum {
     HVMMEM_ram_rw,             /* Normal read/write guest RAM */
     HVMMEM_ram_ro,             /* Read-only; writes are discarded */
     HVMMEM_mmio_dm,            /* Reads and write go to the device model */
+    HVMMEM_mmio_write_dm       /* Read-only; writes go to the device model */
 } hvmmem_type_t;
 
 /* Following tools-only interfaces may change in future. */
-- 
1.9.1

Re: [PATCH v5 1/2] add a new p2m type class - P2M_DISCARD_WRITE_TYPES

[Tim Deegan]

At 21:13 +0800 on 04 Dec (1417724005), Yu Zhang wrote:
> From: Yu Zhang <yu.c.zhang@intel.com>
> 
> Currently, the P2M_RO_TYPES bears 2 meanings: one is
> "_PAGE_RW bit is clear in their PTEs", and another is
> to discard the write operations on these pages. This
> patch adds a p2m type class, P2M_DISCARD_WRITE_TYPES,
> to bear the second meaning, so we can use this type
> class instead of the P2M_RO_TYPES, to decide if a write
> operation is to be ignored.
> 
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>

Reviewed-by: Tim Deegan <tim@xen.org>

Re: [PATCH v5 2/2] add a new p2m type - p2m_mmio_write_dm

[Tim Deegan]

Hi, 

At 21:13 +0800 on 04 Dec (1417724006), Yu Zhang wrote:
> A new p2m type, p2m_mmio_write_dm, is added to trap and emulate
> the write operations on GPU's page tables. Handling of this new
> p2m type are similar with existing p2m_ram_ro in most condition
> checks, with only difference on final policy of emulation vs. drop.
> For p2m_ram_ro types, write operations will not trigger the device
> model, and will be discarded later in __hvm_copy(); while for the
> p2m_mmio_write_dm type pages, writes will go to the device model
> via ioreq-server.
> 
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Signed-off-by: Wei Ye <wei.ye@intel.com>

Thanks for this -- only two comments:

> @@ -5978,7 +5982,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>                  goto param_fail4;
>              }
>              if ( !p2m_is_ram(t) &&
> -                 (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) )
> +                 (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) &&
> +                 t != p2m_mmio_write_dm )

I think that Jan already brough this up, and maybe I missed your
answer: this realaxation looks wrong to me. I would have thought that
transition between p2m_mmio_write_dm and p2m_ram_rw/p2m_ram_logdirty
would be the only ones you would want to allow.

> @@ -111,7 +112,8 @@ typedef unsigned int p2m_query_t;
>  #define P2M_RO_TYPES (p2m_to_mask(p2m_ram_logdirty)     \
>                        | p2m_to_mask(p2m_ram_ro)         \
>                        | p2m_to_mask(p2m_grant_map_ro)   \
> -                      | p2m_to_mask(p2m_ram_shared) )
> +                      | p2m_to_mask(p2m_ram_shared)   \
> +                      | p2m_to_mask(p2m_mmio_write_dm))

Nit: please align the '\' with the others above it. 

Cheers,

Tim. 

Re: [PATCH v5 2/2] add a new p2m type - p2m_mmio_write_dm

[Yu, Zhang]

On 12/5/2014 12:04 AM, Tim Deegan wrote:
> Hi,
>
> At 21:13 +0800 on 04 Dec (1417724006), Yu Zhang wrote:
>> A new p2m type, p2m_mmio_write_dm, is added to trap and emulate
>> the write operations on GPU's page tables. Handling of this new
>> p2m type are similar with existing p2m_ram_ro in most condition
>> checks, with only difference on final policy of emulation vs. drop.
>> For p2m_ram_ro types, write operations will not trigger the device
>> model, and will be discarded later in __hvm_copy(); while for the
>> p2m_mmio_write_dm type pages, writes will go to the device model
>> via ioreq-server.
>>
>> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>> Signed-off-by: Wei Ye <wei.ye@intel.com>
>
> Thanks for this -- only two comments:
>
>> @@ -5978,7 +5982,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>                   goto param_fail4;
>>               }
>>               if ( !p2m_is_ram(t) &&
>> -                 (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) )
>> +                 (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) &&
>> +                 t != p2m_mmio_write_dm )
>
> I think that Jan already brough this up, and maybe I missed your
> answer: this realaxation looks wrong to me. I would have thought that
> transition between p2m_mmio_write_dm and p2m_ram_rw/p2m_ram_logdirty
> would be the only ones you would want to allow.

Ha. Sorry, my negligence, and thanks for pointing out. :)
The transition we use now is only between p2m_mmio_write_dm and 
p2m_ram_rw. So how about this:

     if ( !p2m_is_ram(t) &&
          (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) &&
          (t != p2m_mmio_write_dm || a.hvmmem_type != HVMMEM_ram_rw) )

>
>> @@ -111,7 +112,8 @@ typedef unsigned int p2m_query_t;
>>   #define P2M_RO_TYPES (p2m_to_mask(p2m_ram_logdirty)     \
>>                         | p2m_to_mask(p2m_ram_ro)         \
>>                         | p2m_to_mask(p2m_grant_map_ro)   \
>> -                      | p2m_to_mask(p2m_ram_shared) )
>> +                      | p2m_to_mask(p2m_ram_shared)   \
>> +                      | p2m_to_mask(p2m_mmio_write_dm))
>
> Nit: please align the '\' with the others above it.
Got it, and thanks.

B.R.
Yu
>
> Cheers,
>
> Tim.
>
>

Re: [PATCH v5 2/2] add a new p2m type - p2m_mmio_write_dm

[Tim Deegan]

Hi,

At 10:00 +0800 on 05 Dec (1417770044), Yu, Zhang wrote:
> >> @@ -5978,7 +5982,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> >>                   goto param_fail4;
> >>               }
> >>               if ( !p2m_is_ram(t) &&
> >> -                 (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) )
> >> +                 (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) &&
> >> +                 t != p2m_mmio_write_dm )
> >
> > I think that Jan already brough this up, and maybe I missed your
> > answer: this realaxation looks wrong to me. I would have thought that
> > transition between p2m_mmio_write_dm and p2m_ram_rw/p2m_ram_logdirty
> > would be the only ones you would want to allow.
> 
> Ha. Sorry, my negligence, and thanks for pointing out. :)
> The transition we use now is only between p2m_mmio_write_dm and 
> p2m_ram_rw. So how about this:
> 
>      if ( !p2m_is_ram(t) &&
>           (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) &&
>           (t != p2m_mmio_write_dm || a.hvmmem_type != HVMMEM_ram_rw) )

Yes, I think that's right. 

Cheers,

Tim.