[PATCH v4] x86: add p2m_mmio_write_dm

[PATCH v4] x86: add p2m_mmio_write_dm

[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 agreed
to add a new p2m type "p2m_mmio_write_dm" to trap the write operation
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_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 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.

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          | 13 ++++++++++---
 xen/arch/x86/mm/p2m-ept.c       |  1 +
 xen/arch/x86/mm/p2m-pt.c        |  1 +
 xen/include/asm-x86/p2m.h       |  1 +
 xen/include/public/hvm/hvm_op.h |  1 +
 5 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8f49b44..5f806e8 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2838,7 +2838,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 && (p2mt == p2m_ram_ro)) )
+         (npfec.write_access &&
+		((p2mt == p2m_ram_ro) || (p2mt == p2m_mmio_write_dm))) )
     {
         put_gfn(p2m->domain, gfn);
 
@@ -5922,6 +5923,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
                 a.mem_type =  HVMMEM_ram_rw;
             else if ( p2m_is_grant(t) )
                 a.mem_type =  HVMMEM_ram_rw;
+            else if ( t == p2m_mmio_write_dm )
+                a.mem_type = HVMMEM_mmio_write_dm;
             else
                 a.mem_type =  HVMMEM_mmio_dm;
             rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
@@ -5941,7 +5944,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) )
@@ -5987,14 +5991,17 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
                 rc = -EAGAIN;
                 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;
             }
 
             rc = p2m_change_type_one(d, pfn, t, memtype[a.hvmmem_type]);
+
             put_gfn(d, pfn);
             if ( rc )
                 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 5f7fe71..a7a45af 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 */
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 v4] x86: add p2m_mmio_write_dm

[Jan Beulich]

>>> On 28.11.14 at 08:59, <yu.c.zhang@linux.intel.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2838,7 +2838,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 && (p2mt == p2m_ram_ro)) )
> +         (npfec.write_access &&
> +		((p2mt == p2m_ram_ro) || (p2mt == p2m_mmio_write_dm))) )

Why are you corrupting indentation here?

Furthermore the code you modify here suggests that p2m_ram_ro
already has the needed semantics - writes get passed to the DM.
None of the other changes you make, and none of the other uses
of p2m_ram_ro appear to be in conflict with your intentions, so
you'd really need to explain better why you need the new type.

> @@ -5922,6 +5923,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>                  a.mem_type =  HVMMEM_ram_rw;
>              else if ( p2m_is_grant(t) )
>                  a.mem_type =  HVMMEM_ram_rw;
> +            else if ( t == p2m_mmio_write_dm )
> +                a.mem_type = HVMMEM_mmio_write_dm;
>              else
>                  a.mem_type =  HVMMEM_mmio_dm;
>              rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> @@ -5941,7 +5944,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) )
> @@ -5987,14 +5991,17 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>                  rc = -EAGAIN;
>                  goto param_fail4;
>              }
> +

Stray addition of a blank line?

>              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 )

Do you really want to permit e.g. transitions between mmio_dm and
mmio_write_dm? We should be as restrictive as possible here to not
open up paths to security problems.

>              {
>                  put_gfn(d, pfn);
>                  goto param_fail4;
>              }
>  
>              rc = p2m_change_type_one(d, pfn, t, memtype[a.hvmmem_type]);
> +
>              put_gfn(d, pfn);
>              if ( rc )
>                  goto param_fail4;

Another stray newline addition.

> --- 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 */
> 

If the new type is really needed, shouldn't this get added to
P2M_RO_TYPES?

Jan

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Yu, Zhang]

On 11/28/2014 5:57 PM, Jan Beulich wrote:
>>>> On 28.11.14 at 08:59, <yu.c.zhang@linux.intel.com> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2838,7 +2838,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 && (p2mt == p2m_ram_ro)) )
>> +         (npfec.write_access &&
>> +		((p2mt == p2m_ram_ro) || (p2mt == p2m_mmio_write_dm))) )
>
> Why are you corrupting indentation here?
Thanks for your comments, Jan.
The indentation here is to make sure the
((p2mt == p2m_ram_ro) || (p2mt == p2m_mmio_write_dm)) are grouped 
together. But I am not sure if this is correct according to xen coding 
style. I may have misunderstood your previous comments on Sep 3, which 
said "the indentation would need adjustment" in reply to "[Xen-devel] 
[PATCH v3 1/2] x86: add p2m_mmio_write_dm"

>
> Furthermore the code you modify here suggests that p2m_ram_ro
> already has the needed semantics - writes get passed to the DM.
> None of the other changes you make, and none of the other uses
> of p2m_ram_ro appear to be in conflict with your intentions, so
> you'd really need to explain better why you need the new type.
>
Thanks Jan.
To my understanding, pages with p2m_ram_ro are not supposed to be 
modified by guest. So in __hvm_copy(), when p2m type of a page is 
p2m_ram_rom, no copy will occur.
However, to our usage, we just wanna this page to be write protected, so 
that our device model can be triggered to do some emulation. The content 
written to this page is supposed not to be dropped. This way, if 
sequentially a read operation is performed by guest to this page, the 
guest will still see its anticipated value.

Maybe I need to update my commit message to explain this more clearly. :)

>> @@ -5922,6 +5923,8 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>                   a.mem_type =  HVMMEM_ram_rw;
>>               else if ( p2m_is_grant(t) )
>>                   a.mem_type =  HVMMEM_ram_rw;
>> +            else if ( t == p2m_mmio_write_dm )
>> +                a.mem_type = HVMMEM_mmio_write_dm;
>>               else
>>                   a.mem_type =  HVMMEM_mmio_dm;
>>               rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>> @@ -5941,7 +5944,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) )
>> @@ -5987,14 +5991,17 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>                   rc = -EAGAIN;
>>                   goto param_fail4;
>>               }
>> +
>
> Stray addition of a blank line?
>
>>               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 )
>
> Do you really want to permit e.g. transitions between mmio_dm and
> mmio_write_dm? We should be as restrictive as possible here to not
> open up paths to security problems.
>
>>               {
>>                   put_gfn(d, pfn);
>>                   goto param_fail4;
>>               }
>>
>>               rc = p2m_change_type_one(d, pfn, t, memtype[a.hvmmem_type]);
>> +
>>               put_gfn(d, pfn);
>>               if ( rc )
>>                   goto param_fail4;
>
> Another stray newline addition.
>
>> --- 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 */
>>
>
> If the new type is really needed, shouldn't this get added to
> P2M_RO_TYPES?
>
Well, previsouly, I wished to differenciate the HVMMEM_ram_ro and the 
newly added HVMMEM_mmio_write_dm in the 
HVMOP_get_mem_type/HVMOP_set_mem_type hypercall. I'd rather think of 
this new type as write protected than read only.
But I'll take a look, to see if this can be added to P2M_RO_TYPES. Thanks.

Yu

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

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Jan Beulich]

>>> On 01.12.14 at 09:49, <yu.c.zhang@linux.intel.com> wrote:
> On 11/28/2014 5:57 PM, Jan Beulich wrote:
>>>>> On 28.11.14 at 08:59, <yu.c.zhang@linux.intel.com> wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -2838,7 +2838,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 && (p2mt == p2m_ram_ro)) )
>>> +         (npfec.write_access &&
>>> +		((p2mt == p2m_ram_ro) || (p2mt == p2m_mmio_write_dm))) )
>>
>> Why are you corrupting indentation here?
> Thanks for your comments, Jan.
> The indentation here is to make sure the
> ((p2mt == p2m_ram_ro) || (p2mt == p2m_mmio_write_dm)) are grouped 
> together. But I am not sure if this is correct according to xen coding 
> style. I may have misunderstood your previous comments on Sep 3, which 
> said "the indentation would need adjustment" in reply to "[Xen-devel] 
> [PATCH v3 1/2] x86: add p2m_mmio_write_dm"

Getting the alignment right is needed, but no via using hard tabs.

>> Furthermore the code you modify here suggests that p2m_ram_ro
>> already has the needed semantics - writes get passed to the DM.
>> None of the other changes you make, and none of the other uses
>> of p2m_ram_ro appear to be in conflict with your intentions, so
>> you'd really need to explain better why you need the new type.
>>
> Thanks Jan.
> To my understanding, pages with p2m_ram_ro are not supposed to be 
> modified by guest. So in __hvm_copy(), when p2m type of a page is 
> p2m_ram_rom, no copy will occur.
> However, to our usage, we just wanna this page to be write protected, so 
> that our device model can be triggered to do some emulation. The content 
> written to this page is supposed not to be dropped. This way, if 
> sequentially a read operation is performed by guest to this page, the 
> guest will still see its anticipated value.

__hvm_copy() is only a helper function, and doesn't write to
mmio_dm space either; instead its (indirect) callers would invoke
hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn
returns. The question hence is about the apparent inconsistency
resulting from writes to ram_ro being dropped here but getting
passed to the DM by hvm_hap_nested_page_fault(). Tim - is
that really intentional?

> Maybe I need to update my commit message to explain this more clearly. :)

At the very least, yes.

Jan

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Tim Deegan]

At 09:32 +0000 on 01 Dec (1417422746), Jan Beulich wrote:
> >>> On 01.12.14 at 09:49, <yu.c.zhang@linux.intel.com> wrote:
> > On 11/28/2014 5:57 PM, Jan Beulich wrote:
> >>>>> On 28.11.14 at 08:59, <yu.c.zhang@linux.intel.com> wrote:
> >>> --- a/xen/arch/x86/hvm/hvm.c
> >>> +++ b/xen/arch/x86/hvm/hvm.c
> >>> @@ -2838,7 +2838,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 && (p2mt == p2m_ram_ro)) )
> >>> +         (npfec.write_access &&
> >>> +		((p2mt == p2m_ram_ro) || (p2mt == p2m_mmio_write_dm))) )
> >>
> >> Why are you corrupting indentation here?
> > Thanks for your comments, Jan.
> > The indentation here is to make sure the
> > ((p2mt == p2m_ram_ro) || (p2mt == p2m_mmio_write_dm)) are grouped 
> > together. But I am not sure if this is correct according to xen coding 
> > style. I may have misunderstood your previous comments on Sep 3, which 
> > said "the indentation would need adjustment" in reply to "[Xen-devel] 
> > [PATCH v3 1/2] x86: add p2m_mmio_write_dm"
> 
> Getting the alignment right is needed, but no via using hard tabs.
> 
> >> Furthermore the code you modify here suggests that p2m_ram_ro
> >> already has the needed semantics - writes get passed to the DM.
> >> None of the other changes you make, and none of the other uses
> >> of p2m_ram_ro appear to be in conflict with your intentions, so
> >> you'd really need to explain better why you need the new type.
> >>
> > Thanks Jan.
> > To my understanding, pages with p2m_ram_ro are not supposed to be 
> > modified by guest. So in __hvm_copy(), when p2m type of a page is 
> > p2m_ram_rom, no copy will occur.
> > However, to our usage, we just wanna this page to be write protected, so 
> > that our device model can be triggered to do some emulation. The content 
> > written to this page is supposed not to be dropped. This way, if 
> > sequentially a read operation is performed by guest to this page, the 
> > guest will still see its anticipated value.
> 
> __hvm_copy() is only a helper function, and doesn't write to
> mmio_dm space either; instead its (indirect) callers would invoke
> hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn
> returns. The question hence is about the apparent inconsistency
> resulting from writes to ram_ro being dropped here but getting
> passed to the DM by hvm_hap_nested_page_fault(). Tim - is
> that really intentional?

No - and AFAICT it shouldn't be happening.  It _is_ how it was
implemented originally, because it involved fewer moving parts and
didn't need to be efficient (and after all, writes to entirely missing
addresses go to the device model too).

But the code was later updated to log and discard writes to read-only
memory (see 4d8aa29 from Trolle Selander).

Early version of p2m_ram_ro were documented in the internal headers as
sending the writes to the DM, but the public interface (HVMMEM_ram_ro)
has always said that writes are discarded.

During this bit of archaeology I realised that either this new type
should _not_ be made part of P2M_RO_TYPES, or, better, we need a new
class of P2M types (P2M_DISCARD_WRITE_TYPES, say) that should be used
for these paths in emulate_gva_to_mfn() and __hvm_copy(), containing
just p2m_ram_ro and p2m_grant_map_ro.

Cheers,

Tim.

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Jan Beulich]

>>> On 01.12.14 at 11:30, <tim@xen.org> wrote:
> At 09:32 +0000 on 01 Dec (1417422746), Jan Beulich wrote:
>> >>> On 01.12.14 at 09:49, <yu.c.zhang@linux.intel.com> wrote:
>> > To my understanding, pages with p2m_ram_ro are not supposed to be 
>> > modified by guest. So in __hvm_copy(), when p2m type of a page is 
>> > p2m_ram_rom, no copy will occur.
>> > However, to our usage, we just wanna this page to be write protected, so 
>> > that our device model can be triggered to do some emulation. The content 
>> > written to this page is supposed not to be dropped. This way, if 
>> > sequentially a read operation is performed by guest to this page, the 
>> > guest will still see its anticipated value.
>> 
>> __hvm_copy() is only a helper function, and doesn't write to
>> mmio_dm space either; instead its (indirect) callers would invoke
>> hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn
>> returns. The question hence is about the apparent inconsistency
>> resulting from writes to ram_ro being dropped here but getting
>> passed to the DM by hvm_hap_nested_page_fault(). Tim - is
>> that really intentional?
> 
> No - and AFAICT it shouldn't be happening.  It _is_ how it was
> implemented originally, because it involved fewer moving parts and
> didn't need to be efficient (and after all, writes to entirely missing
> addresses go to the device model too).
> 
> But the code was later updated to log and discard writes to read-only
> memory (see 4d8aa29 from Trolle Selander).
> 
> Early version of p2m_ram_ro were documented in the internal headers as
> sending the writes to the DM, but the public interface (HVMMEM_ram_ro)
> has always said that writes are discarded.

Hmm, so which way do you recommend resolving the inconsistency
then - match what the public interface says or what the apparent
original intention for the internal type was? Presumably we need to
follow the public interface mandated model, and hence require the
new type to be introduced.

> During this bit of archaeology I realised that either this new type
> should _not_ be made part of P2M_RO_TYPES, or, better, we need a new
> class of P2M types (P2M_DISCARD_WRITE_TYPES, say) that should be used
> for these paths in emulate_gva_to_mfn() and __hvm_copy(), containing
> just p2m_ram_ro and p2m_grant_map_ro.

And I suppose in that latter case the new type could be made part
of P2M_RO_TYPES()?

Jan

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Tim Deegan]

At 11:17 +0000 on 01 Dec (1417429027), Jan Beulich wrote:
> >>> On 01.12.14 at 11:30, <tim@xen.org> wrote:
> > At 09:32 +0000 on 01 Dec (1417422746), Jan Beulich wrote:
> >> >>> On 01.12.14 at 09:49, <yu.c.zhang@linux.intel.com> wrote:
> >> > To my understanding, pages with p2m_ram_ro are not supposed to be 
> >> > modified by guest. So in __hvm_copy(), when p2m type of a page is 
> >> > p2m_ram_rom, no copy will occur.
> >> > However, to our usage, we just wanna this page to be write protected, so 
> >> > that our device model can be triggered to do some emulation. The content 
> >> > written to this page is supposed not to be dropped. This way, if 
> >> > sequentially a read operation is performed by guest to this page, the 
> >> > guest will still see its anticipated value.
> >> 
> >> __hvm_copy() is only a helper function, and doesn't write to
> >> mmio_dm space either; instead its (indirect) callers would invoke
> >> hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn
> >> returns. The question hence is about the apparent inconsistency
> >> resulting from writes to ram_ro being dropped here but getting
> >> passed to the DM by hvm_hap_nested_page_fault(). Tim - is
> >> that really intentional?
> > 
> > No - and AFAICT it shouldn't be happening.  It _is_ how it was
> > implemented originally, because it involved fewer moving parts and
> > didn't need to be efficient (and after all, writes to entirely missing
> > addresses go to the device model too).
> > 
> > But the code was later updated to log and discard writes to read-only
> > memory (see 4d8aa29 from Trolle Selander).
> > 
> > Early version of p2m_ram_ro were documented in the internal headers as
> > sending the writes to the DM, but the public interface (HVMMEM_ram_ro)
> > has always said that writes are discarded.
> 
> Hmm, so which way do you recommend resolving the inconsistency
> then - match what the public interface says or what the apparent
> original intention for the internal type was? Presumably we need to
> follow the public interface mandated model, and hence require the
> new type to be introduced.

Sorry, I was unclear -- there isn't an inconsistency; both internal
and public headers currently say that writes are discarded and AFAICT
that is what the code does.

But yes, we ought to follow the established hypercall interface, and
so we need the new type.

> > During this bit of archaeology I realised that either this new type
> > should _not_ be made part of P2M_RO_TYPES, or, better, we need a new
> > class of P2M types (P2M_DISCARD_WRITE_TYPES, say) that should be used
> > for these paths in emulate_gva_to_mfn() and __hvm_copy(), containing
> > just p2m_ram_ro and p2m_grant_map_ro.
> 
> And I suppose in that latter case the new type could be made part
> of P2M_RO_TYPES()?

Yes indeed, as P2M_RO_TYPES is defined as "must have the _PAGE_RW bit
clear in their PTEs".

Cheers,

Tim.

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Jan Beulich]

>>> On 01.12.14 at 13:13, <tim@xen.org> wrote:
> At 11:17 +0000 on 01 Dec (1417429027), Jan Beulich wrote:
>> >>> On 01.12.14 at 11:30, <tim@xen.org> wrote:
>> > At 09:32 +0000 on 01 Dec (1417422746), Jan Beulich wrote:
>> >> >>> On 01.12.14 at 09:49, <yu.c.zhang@linux.intel.com> wrote:
>> >> > To my understanding, pages with p2m_ram_ro are not supposed to be 
>> >> > modified by guest. So in __hvm_copy(), when p2m type of a page is 
>> >> > p2m_ram_rom, no copy will occur.
>> >> > However, to our usage, we just wanna this page to be write protected, so 
>> >> > that our device model can be triggered to do some emulation. The content 
>> >> > written to this page is supposed not to be dropped. This way, if 
>> >> > sequentially a read operation is performed by guest to this page, the 
>> >> > guest will still see its anticipated value.
>> >> 
>> >> __hvm_copy() is only a helper function, and doesn't write to
>> >> mmio_dm space either; instead its (indirect) callers would invoke
>> >> hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn
>> >> returns. The question hence is about the apparent inconsistency
>> >> resulting from writes to ram_ro being dropped here but getting
>> >> passed to the DM by hvm_hap_nested_page_fault(). Tim - is
>> >> that really intentional?
>> > 
>> > No - and AFAICT it shouldn't be happening.  It _is_ how it was
>> > implemented originally, because it involved fewer moving parts and
>> > didn't need to be efficient (and after all, writes to entirely missing
>> > addresses go to the device model too).
>> > 
>> > But the code was later updated to log and discard writes to read-only
>> > memory (see 4d8aa29 from Trolle Selander).
>> > 
>> > Early version of p2m_ram_ro were documented in the internal headers as
>> > sending the writes to the DM, but the public interface (HVMMEM_ram_ro)
>> > has always said that writes are discarded.
>> 
>> Hmm, so which way do you recommend resolving the inconsistency
>> then - match what the public interface says or what the apparent
>> original intention for the internal type was? Presumably we need to
>> follow the public interface mandated model, and hence require the
>> new type to be introduced.
> 
> Sorry, I was unclear -- there isn't an inconsistency; both internal
> and public headers currently say that writes are discarded and AFAICT
> that is what the code does.

Not for hvm_hap_nested_page_fault() afaict - the forwarding to
DM there contradicts the "writes are discarded" model that other
code paths follow.

Jan

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Yu, Zhang]

On 12/1/2014 8:13 PM, Tim Deegan wrote:
> At 11:17 +0000 on 01 Dec (1417429027), Jan Beulich wrote:
>>>>> On 01.12.14 at 11:30, <tim@xen.org> wrote:
>>> At 09:32 +0000 on 01 Dec (1417422746), Jan Beulich wrote:
>>>>>>> On 01.12.14 at 09:49, <yu.c.zhang@linux.intel.com> wrote:
>>>>> To my understanding, pages with p2m_ram_ro are not supposed to be
>>>>> modified by guest. So in __hvm_copy(), when p2m type of a page is
>>>>> p2m_ram_rom, no copy will occur.
>>>>> However, to our usage, we just wanna this page to be write protected, so
>>>>> that our device model can be triggered to do some emulation. The content
>>>>> written to this page is supposed not to be dropped. This way, if
>>>>> sequentially a read operation is performed by guest to this page, the
>>>>> guest will still see its anticipated value.
>>>>
>>>> __hvm_copy() is only a helper function, and doesn't write to
>>>> mmio_dm space either; instead its (indirect) callers would invoke
>>>> hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn
>>>> returns. The question hence is about the apparent inconsistency
>>>> resulting from writes to ram_ro being dropped here but getting
>>>> passed to the DM by hvm_hap_nested_page_fault(). Tim - is
>>>> that really intentional?
>>>
>>> No - and AFAICT it shouldn't be happening.  It _is_ how it was
>>> implemented originally, because it involved fewer moving parts and
>>> didn't need to be efficient (and after all, writes to entirely missing
>>> addresses go to the device model too).
>>>
>>> But the code was later updated to log and discard writes to read-only
>>> memory (see 4d8aa29 from Trolle Selander).
>>>
>>> Early version of p2m_ram_ro were documented in the internal headers as
>>> sending the writes to the DM, but the public interface (HVMMEM_ram_ro)
>>> has always said that writes are discarded.
>>
>> Hmm, so which way do you recommend resolving the inconsistency
>> then - match what the public interface says or what the apparent
>> original intention for the internal type was? Presumably we need to
>> follow the public interface mandated model, and hence require the
>> new type to be introduced.
>
> Sorry, I was unclear -- there isn't an inconsistency; both internal
> and public headers currently say that writes are discarded and AFAICT
> that is what the code does.
>
> But yes, we ought to follow the established hypercall interface, and
> so we need the new type.
>
>>> During this bit of archaeology I realised that either this new type
>>> should _not_ be made part of P2M_RO_TYPES, or, better, we need a new
>>> class of P2M types (P2M_DISCARD_WRITE_TYPES, say) that should be used
>>> for these paths in emulate_gva_to_mfn() and __hvm_copy(), containing
>>> just p2m_ram_ro and p2m_grant_map_ro.
>>
>> And I suppose in that latter case the new type could be made part
>> of P2M_RO_TYPES()?
>
> Yes indeed, as P2M_RO_TYPES is defined as "must have the _PAGE_RW bit
> clear in their PTEs".
>

Thanks Tim.
Following are my understanding of the P2M_RO_TYPES and your comments.
Not sure if I get it right. Please correct me if anything wrong:
1> The P2M_RO_TYPES now bears 2 meanings: one is "w bit is clear in the 
pte"; and another being to discard the write operations;
2> We better define another class to bear the second meaning.

Also some questions for the new p2m class, say P2M_DISCARD_WRITE_TYPES, 
and the new predicates, say p2m_is_discard_write:
1> You mentioned emulate_gva_to_mfn() and __hvm_copy() should discard 
the write ops, yet I also noticed many other places using the 
p2m_is_readonly, or only the "p2mt == p2m_ram_ro" judgement(in 
__hvm_copy/__hvm_clear). Among all these other places, is there any ones 
also supposed to use the p2m_is_discard_write?
2> p2m_grant_map_ro is also supposed to be discarded? Will handling of 
this type of pages goes into __hvm_copy()/__hvm_clear(), or should?

I'm a new guy of this area, and sorry for my messed questions. :)

Yu




> Cheers,
>
> Tim.
>
>

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Yu, Zhang]

On 12/1/2014 8:31 PM, Jan Beulich wrote:
>>>> On 01.12.14 at 13:13, <tim@xen.org> wrote:
>> At 11:17 +0000 on 01 Dec (1417429027), Jan Beulich wrote:
>>>>>> On 01.12.14 at 11:30, <tim@xen.org> wrote:
>>>> At 09:32 +0000 on 01 Dec (1417422746), Jan Beulich wrote:
>>>>>>>> On 01.12.14 at 09:49, <yu.c.zhang@linux.intel.com> wrote:
>>>>>> To my understanding, pages with p2m_ram_ro are not supposed to be
>>>>>> modified by guest. So in __hvm_copy(), when p2m type of a page is
>>>>>> p2m_ram_rom, no copy will occur.
>>>>>> However, to our usage, we just wanna this page to be write protected, so
>>>>>> that our device model can be triggered to do some emulation. The content
>>>>>> written to this page is supposed not to be dropped. This way, if
>>>>>> sequentially a read operation is performed by guest to this page, the
>>>>>> guest will still see its anticipated value.
>>>>>
>>>>> __hvm_copy() is only a helper function, and doesn't write to
>>>>> mmio_dm space either; instead its (indirect) callers would invoke
>>>>> hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn
>>>>> returns. The question hence is about the apparent inconsistency
>>>>> resulting from writes to ram_ro being dropped here but getting
>>>>> passed to the DM by hvm_hap_nested_page_fault(). Tim - is
>>>>> that really intentional?
>>>>
>>>> No - and AFAICT it shouldn't be happening.  It _is_ how it was
>>>> implemented originally, because it involved fewer moving parts and
>>>> didn't need to be efficient (and after all, writes to entirely missing
>>>> addresses go to the device model too).
>>>>
>>>> But the code was later updated to log and discard writes to read-only
>>>> memory (see 4d8aa29 from Trolle Selander).
>>>>
>>>> Early version of p2m_ram_ro were documented in the internal headers as
>>>> sending the writes to the DM, but the public interface (HVMMEM_ram_ro)
>>>> has always said that writes are discarded.
>>>
>>> Hmm, so which way do you recommend resolving the inconsistency
>>> then - match what the public interface says or what the apparent
>>> original intention for the internal type was? Presumably we need to
>>> follow the public interface mandated model, and hence require the
>>> new type to be introduced.
>>
>> Sorry, I was unclear -- there isn't an inconsistency; both internal
>> and public headers currently say that writes are discarded and AFAICT
>> that is what the code does.
>
> Not for hvm_hap_nested_page_fault() afaict - the forwarding to
> DM there contradicts the "writes are discarded" model that other
> code paths follow.
>
Thanks, Jan.
By "inconsistency", do you mean the p2m_ram_ro shall not trigger the 
handle_mmio_with_translation() in hvm_hap_nested_page_fault()?
I'm also a bit confused with the "writes are discarded/dropped" comments 
in the code. Does this mean writes to the p2m_ram_ro pages should be 
abandoned without going to the dm, or going to the dm and  ignored 
later? The code seems to be the second one.

> Jan
>
>
>

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Jan Beulich]

>>> On 02.12.14 at 08:48, <yu.c.zhang@linux.intel.com> wrote:

> 
> On 12/1/2014 8:31 PM, Jan Beulich wrote:
>>>>> On 01.12.14 at 13:13, <tim@xen.org> wrote:
>>> At 11:17 +0000 on 01 Dec (1417429027), Jan Beulich wrote:
>>>>>>> On 01.12.14 at 11:30, <tim@xen.org> wrote:
>>>>> At 09:32 +0000 on 01 Dec (1417422746), Jan Beulich wrote:
>>>>>>>>> On 01.12.14 at 09:49, <yu.c.zhang@linux.intel.com> wrote:
>>>>>>> To my understanding, pages with p2m_ram_ro are not supposed to be
>>>>>>> modified by guest. So in __hvm_copy(), when p2m type of a page is
>>>>>>> p2m_ram_rom, no copy will occur.
>>>>>>> However, to our usage, we just wanna this page to be write protected, so
>>>>>>> that our device model can be triggered to do some emulation. The content
>>>>>>> written to this page is supposed not to be dropped. This way, if
>>>>>>> sequentially a read operation is performed by guest to this page, the
>>>>>>> guest will still see its anticipated value.
>>>>>>
>>>>>> __hvm_copy() is only a helper function, and doesn't write to
>>>>>> mmio_dm space either; instead its (indirect) callers would invoke
>>>>>> hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn
>>>>>> returns. The question hence is about the apparent inconsistency
>>>>>> resulting from writes to ram_ro being dropped here but getting
>>>>>> passed to the DM by hvm_hap_nested_page_fault(). Tim - is
>>>>>> that really intentional?
>>>>>
>>>>> No - and AFAICT it shouldn't be happening.  It _is_ how it was
>>>>> implemented originally, because it involved fewer moving parts and
>>>>> didn't need to be efficient (and after all, writes to entirely missing
>>>>> addresses go to the device model too).
>>>>>
>>>>> But the code was later updated to log and discard writes to read-only
>>>>> memory (see 4d8aa29 from Trolle Selander).
>>>>>
>>>>> Early version of p2m_ram_ro were documented in the internal headers as
>>>>> sending the writes to the DM, but the public interface (HVMMEM_ram_ro)
>>>>> has always said that writes are discarded.
>>>>
>>>> Hmm, so which way do you recommend resolving the inconsistency
>>>> then - match what the public interface says or what the apparent
>>>> original intention for the internal type was? Presumably we need to
>>>> follow the public interface mandated model, and hence require the
>>>> new type to be introduced.
>>>
>>> Sorry, I was unclear -- there isn't an inconsistency; both internal
>>> and public headers currently say that writes are discarded and AFAICT
>>> that is what the code does.
>>
>> Not for hvm_hap_nested_page_fault() afaict - the forwarding to
>> DM there contradicts the "writes are discarded" model that other
>> code paths follow.
>>
> Thanks, Jan.
> By "inconsistency", do you mean the p2m_ram_ro shall not trigger the 
> handle_mmio_with_translation() in hvm_hap_nested_page_fault()?

Yes, pending Tim's agreement.

> I'm also a bit confused with the "writes are discarded/dropped" comments 
> in the code. Does this mean writes to the p2m_ram_ro pages should be 
> abandoned without going to the dm, or going to the dm and  ignored 
> later? The code seems to be the second one.

And it would seem to me that it should be the former.

Jan

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Tim Deegan]

At 12:31 +0000 on 01 Dec (1417433464), Jan Beulich wrote:
> >>> On 01.12.14 at 13:13, <tim@xen.org> wrote:
> > At 11:17 +0000 on 01 Dec (1417429027), Jan Beulich wrote:
> >> >>> On 01.12.14 at 11:30, <tim@xen.org> wrote:
> >> > At 09:32 +0000 on 01 Dec (1417422746), Jan Beulich wrote:
> >> >> >>> On 01.12.14 at 09:49, <yu.c.zhang@linux.intel.com> wrote:
> >> >> > To my understanding, pages with p2m_ram_ro are not supposed to be 
> >> >> > modified by guest. So in __hvm_copy(), when p2m type of a page is 
> >> >> > p2m_ram_rom, no copy will occur.
> >> >> > However, to our usage, we just wanna this page to be write protected, so 
> >> >> > that our device model can be triggered to do some emulation. The content 
> >> >> > written to this page is supposed not to be dropped. This way, if 
> >> >> > sequentially a read operation is performed by guest to this page, the 
> >> >> > guest will still see its anticipated value.
> >> >> 
> >> >> __hvm_copy() is only a helper function, and doesn't write to
> >> >> mmio_dm space either; instead its (indirect) callers would invoke
> >> >> hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn
> >> >> returns. The question hence is about the apparent inconsistency
> >> >> resulting from writes to ram_ro being dropped here but getting
> >> >> passed to the DM by hvm_hap_nested_page_fault(). Tim - is
> >> >> that really intentional?
> >> > 
> >> > No - and AFAICT it shouldn't be happening.  It _is_ how it was
> >> > implemented originally, because it involved fewer moving parts and
> >> > didn't need to be efficient (and after all, writes to entirely missing
> >> > addresses go to the device model too).
> >> > 
> >> > But the code was later updated to log and discard writes to read-only
> >> > memory (see 4d8aa29 from Trolle Selander).
> >> > 
> >> > Early version of p2m_ram_ro were documented in the internal headers as
> >> > sending the writes to the DM, but the public interface (HVMMEM_ram_ro)
> >> > has always said that writes are discarded.
> >> 
> >> Hmm, so which way do you recommend resolving the inconsistency
> >> then - match what the public interface says or what the apparent
> >> original intention for the internal type was? Presumably we need to
> >> follow the public interface mandated model, and hence require the
> >> new type to be introduced.
> > 
> > Sorry, I was unclear -- there isn't an inconsistency; both internal
> > and public headers currently say that writes are discarded and AFAICT
> > that is what the code does.
> 
> Not for hvm_hap_nested_page_fault() afaict - the forwarding to
> DM there contradicts the "writes are discarded" model that other
> code paths follow.

It calls handle_mmio() to emulate the instruction for it, but
handle_mmio() ought not to send anything to the DM.  hvmemul_write()
will call hvm_copy(), which will drop the write and report success.

The shadow code has its own emulator framework for emulting PT writes,
which does much the same thing.

Tim.

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Jan Beulich]

>>> On 02.12.14 at 11:37, <tim@xen.org> wrote:
> At 12:31 +0000 on 01 Dec (1417433464), Jan Beulich wrote:
>> >>> On 01.12.14 at 13:13, <tim@xen.org> wrote:
>> > At 11:17 +0000 on 01 Dec (1417429027), Jan Beulich wrote:
>> >> >>> On 01.12.14 at 11:30, <tim@xen.org> wrote:
>> >> > At 09:32 +0000 on 01 Dec (1417422746), Jan Beulich wrote:
>> >> >> >>> On 01.12.14 at 09:49, <yu.c.zhang@linux.intel.com> wrote:
>> >> >> > To my understanding, pages with p2m_ram_ro are not supposed to be 
>> >> >> > modified by guest. So in __hvm_copy(), when p2m type of a page is 
>> >> >> > p2m_ram_rom, no copy will occur.
>> >> >> > However, to our usage, we just wanna this page to be write protected, so 
>> >> >> > that our device model can be triggered to do some emulation. The content 
>> >> >> > written to this page is supposed not to be dropped. This way, if 
>> >> >> > sequentially a read operation is performed by guest to this page, the 
>> >> >> > guest will still see its anticipated value.
>> >> >> 
>> >> >> __hvm_copy() is only a helper function, and doesn't write to
>> >> >> mmio_dm space either; instead its (indirect) callers would invoke
>> >> >> hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn
>> >> >> returns. The question hence is about the apparent inconsistency
>> >> >> resulting from writes to ram_ro being dropped here but getting
>> >> >> passed to the DM by hvm_hap_nested_page_fault(). Tim - is
>> >> >> that really intentional?
>> >> > 
>> >> > No - and AFAICT it shouldn't be happening.  It _is_ how it was
>> >> > implemented originally, because it involved fewer moving parts and
>> >> > didn't need to be efficient (and after all, writes to entirely missing
>> >> > addresses go to the device model too).
>> >> > 
>> >> > But the code was later updated to log and discard writes to read-only
>> >> > memory (see 4d8aa29 from Trolle Selander).
>> >> > 
>> >> > Early version of p2m_ram_ro were documented in the internal headers as
>> >> > sending the writes to the DM, but the public interface (HVMMEM_ram_ro)
>> >> > has always said that writes are discarded.
>> >> 
>> >> Hmm, so which way do you recommend resolving the inconsistency
>> >> then - match what the public interface says or what the apparent
>> >> original intention for the internal type was? Presumably we need to
>> >> follow the public interface mandated model, and hence require the
>> >> new type to be introduced.
>> > 
>> > Sorry, I was unclear -- there isn't an inconsistency; both internal
>> > and public headers currently say that writes are discarded and AFAICT
>> > that is what the code does.
>> 
>> Not for hvm_hap_nested_page_fault() afaict - the forwarding to
>> DM there contradicts the "writes are discarded" model that other
>> code paths follow.
> 
> It calls handle_mmio() to emulate the instruction for it, but
> handle_mmio() ought not to send anything to the DM.  hvmemul_write()
> will call hvm_copy(), which will drop the write and report success.

Oh, of course - it's really just the wording of the comment there
that mislead me to assume the operation gets passed on for
actual handling. Thanks for clarifying!

Jan

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Tim Deegan]

At 15:38 +0800 on 02 Dec (1417531126), Yu, Zhang wrote:
> On 12/1/2014 8:13 PM, Tim Deegan wrote:
> > At 11:17 +0000 on 01 Dec (1417429027), Jan Beulich wrote:
> >>>>> On 01.12.14 at 11:30, <tim@xen.org> wrote:
> >>> During this bit of archaeology I realised that either this new type
> >>> should _not_ be made part of P2M_RO_TYPES, or, better, we need a new
> >>> class of P2M types (P2M_DISCARD_WRITE_TYPES, say) that should be used
> >>> for these paths in emulate_gva_to_mfn() and __hvm_copy(), containing
> >>> just p2m_ram_ro and p2m_grant_map_ro.
> >>
> >> And I suppose in that latter case the new type could be made part
> >> of P2M_RO_TYPES()?
> >
> > Yes indeed, as P2M_RO_TYPES is defined as "must have the _PAGE_RW bit
> > clear in their PTEs".
> >
> 
> Thanks Tim.
> Following are my understanding of the P2M_RO_TYPES and your comments.
> Not sure if I get it right. Please correct me if anything wrong:
> 1> The P2M_RO_TYPES now bears 2 meanings: one is "w bit is clear in the 
> pte"; and another being to discard the write operations;
> 2> We better define another class to bear the second meaning.

Yes, that's what I meant.

Answering your other questions in reverse order:

> 2> p2m_grant_map_ro is also supposed to be discarded? Will handling of 
> this type of pages goes into __hvm_copy()/__hvm_clear(), or should?

I think so, yes.  At the moment we inject #GP when the guest writes to
a read-only grant, which is OK: the guest really ought to know better.
But I think we'll probably end up with neater code if we handle
read-only grants the same way as p2m_ram_ro.

Anyone else have an opinion on the right thing to do here?

> Also some questions for the new p2m class, say P2M_DISCARD_WRITE_TYPES, 
> and the new predicates, say p2m_is_discard_write:
> 1> You mentioned emulate_gva_to_mfn() and __hvm_copy() should discard 
> the write ops, yet I also noticed many other places using the 
> p2m_is_readonly, or only the "p2mt == p2m_ram_ro" judgement(in 
> __hvm_copy/__hvm_clear). Among all these other places, is there any ones 
> also supposed to use the p2m_is_discard_write?

I've just had a look through them all, and I can see exactly four
places that should be using the new p2m_is_discard_write() test:

 - emulate_gva_to_mfn() (Though in fact it's a no-op as shadow-mode
   guests never have p2m_ram_shared or p2m_ram_logdirty mappings.)
 - __hvm_copy() 
 - __hvm_clear() and
 - hvm_hap_nested_page_fault() (where you should also remove the
   explicit handling of p2m_grant_map_ro below.)

Looking through that turned up a few other oddities, which I'm
listing here to remind myself to look at them later (i.e. you don't
need to worry about them for this patch):

 - nsvm_get_nvmcb_page() and nestedhap_walk_L0_p2m() need to handle
   p2m_ram_logdirty or they might spuriously fail duiring live
   migration.
 - __hvm_copy() and __hvm_clear are probably over-strict in their
   failure to handle grant types.
 - P2M_UNMAP_TYPES in vmce.c is a mess.  It's not the right place to
   define this, since it definitely won't be seen my anyone
   adding a new type, and it already has an 'XXX' comment that says
   it doesn't cover a lot of cases. :(

I'll have a look at those another time.

Cheers,

Tim.

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Jan Beulich]

>>> On 02.12.14 at 12:40, <tim@xen.org> wrote:
> At 15:38 +0800 on 02 Dec (1417531126), Yu, Zhang wrote:
>> 2> p2m_grant_map_ro is also supposed to be discarded? Will handling of 
>> this type of pages goes into __hvm_copy()/__hvm_clear(), or should?
> 
> I think so, yes.  At the moment we inject #GP when the guest writes to
> a read-only grant, which is OK: the guest really ought to know better.
> But I think we'll probably end up with neater code if we handle
> read-only grants the same way as p2m_ram_ro.
> 
> Anyone else have an opinion on the right thing to do here?

I actually came to the same conclusion.

Jan

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Yu, Zhang]

On 12/2/2014 7:40 PM, Tim Deegan wrote:
> At 15:38 +0800 on 02 Dec (1417531126), Yu, Zhang wrote:
>> On 12/1/2014 8:13 PM, Tim Deegan wrote:
>>> At 11:17 +0000 on 01 Dec (1417429027), Jan Beulich wrote:
>>>>>>> On 01.12.14 at 11:30, <tim@xen.org> wrote:
>>>>> During this bit of archaeology I realised that either this new type
>>>>> should _not_ be made part of P2M_RO_TYPES, or, better, we need a new
>>>>> class of P2M types (P2M_DISCARD_WRITE_TYPES, say) that should be used
>>>>> for these paths in emulate_gva_to_mfn() and __hvm_copy(), containing
>>>>> just p2m_ram_ro and p2m_grant_map_ro.
>>>>
>>>> And I suppose in that latter case the new type could be made part
>>>> of P2M_RO_TYPES()?
>>>
>>> Yes indeed, as P2M_RO_TYPES is defined as "must have the _PAGE_RW bit
>>> clear in their PTEs".
>>>
>>
>> Thanks Tim.
>> Following are my understanding of the P2M_RO_TYPES and your comments.
>> Not sure if I get it right. Please correct me if anything wrong:
>> 1> The P2M_RO_TYPES now bears 2 meanings: one is "w bit is clear in the
>> pte"; and another being to discard the write operations;
>> 2> We better define another class to bear the second meaning.
>
> Yes, that's what I meant.
>
> Answering your other questions in reverse order:
>
>> 2> p2m_grant_map_ro is also supposed to be discarded? Will handling of
>> this type of pages goes into __hvm_copy()/__hvm_clear(), or should?
>
> I think so, yes.  At the moment we inject #GP when the guest writes to
> a read-only grant, which is OK: the guest really ought to know better.
> But I think we'll probably end up with neater code if we handle
> read-only grants the same way as p2m_ram_ro.
>
> Anyone else have an opinion on the right thing to do here?
>
>> Also some questions for the new p2m class, say P2M_DISCARD_WRITE_TYPES,
>> and the new predicates, say p2m_is_discard_write:
>> 1> You mentioned emulate_gva_to_mfn() and __hvm_copy() should discard
>> the write ops, yet I also noticed many other places using the
>> p2m_is_readonly, or only the "p2mt == p2m_ram_ro" judgement(in
>> __hvm_copy/__hvm_clear). Among all these other places, is there any ones
>> also supposed to use the p2m_is_discard_write?
>
> I've just had a look through them all, and I can see exactly four
> places that should be using the new p2m_is_discard_write() test:
>
>   - emulate_gva_to_mfn() (Though in fact it's a no-op as shadow-mode
>     guests never have p2m_ram_shared or p2m_ram_logdirty mappings.)
>   - __hvm_copy()
>   - __hvm_clear() and
>   - hvm_hap_nested_page_fault() (where you should also remove the
>     explicit handling of p2m_grant_map_ro below.)
>
Thank you, Tim & Jan.
To give a summary for all the comments:

1> new p2m type p2m_mmio_write_dm is to be added;
2> new p2m type need to be added to P2M_RO_TYPES class;
3> new p2m class, say P2M_DISCARD_WRITE_TYPES(which only include 
p2m_ram_ro and p2m_grant_map_ro), and the new predicates, say 
p2m_is_discard_write are needed to in these 4 places to discard the 
write op;
4> and of cause hvm_hap_nested_page_fault() do not need the special 
handling for p2m_grant_map_ro anymore;
5> coding style changes pointed out by Jan
6> clear the commit message

I'll prepare the patch and thanks! :)

Yu

> Looking through that turned up a few other oddities, which I'm
> listing here to remind myself to look at them later (i.e. you don't
> need to worry about them for this patch):
>
>   - nsvm_get_nvmcb_page() and nestedhap_walk_L0_p2m() need to handle
>     p2m_ram_logdirty or they might spuriously fail duiring live
>     migration.
>   - __hvm_copy() and __hvm_clear are probably over-strict in their
>     failure to handle grant types.
>   - P2M_UNMAP_TYPES in vmce.c is a mess.  It's not the right place to
>     define this, since it definitely won't be seen my anyone
>     adding a new type, and it already has an 'XXX' comment that says
>     it doesn't cover a lot of cases. :(
>
> I'll have a look at those another time.
>
> Cheers,
>
> Tim.
>
>

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Yu, Zhang]

On 12/2/2014 7:40 PM, Tim Deegan wrote:
> At 15:38 +0800 on 02 Dec (1417531126), Yu, Zhang wrote:
>> On 12/1/2014 8:13 PM, Tim Deegan wrote:
>>> At 11:17 +0000 on 01 Dec (1417429027), Jan Beulich wrote:
>>>>>>> On 01.12.14 at 11:30, <tim@xen.org> wrote:
>>>>> During this bit of archaeology I realised that either this new type
>>>>> should _not_ be made part of P2M_RO_TYPES, or, better, we need a new
>>>>> class of P2M types (P2M_DISCARD_WRITE_TYPES, say) that should be used
>>>>> for these paths in emulate_gva_to_mfn() and __hvm_copy(), containing
>>>>> just p2m_ram_ro and p2m_grant_map_ro.
>>>>
>>>> And I suppose in that latter case the new type could be made part
>>>> of P2M_RO_TYPES()?
>>>
>>> Yes indeed, as P2M_RO_TYPES is defined as "must have the _PAGE_RW bit
>>> clear in their PTEs".
>>>
>>
>> Thanks Tim.
>> Following are my understanding of the P2M_RO_TYPES and your comments.
>> Not sure if I get it right. Please correct me if anything wrong:
>> 1> The P2M_RO_TYPES now bears 2 meanings: one is "w bit is clear in the
>> pte"; and another being to discard the write operations;
>> 2> We better define another class to bear the second meaning.
>
> Yes, that's what I meant.
>
> Answering your other questions in reverse order:
>
>> 2> p2m_grant_map_ro is also supposed to be discarded? Will handling of
>> this type of pages goes into __hvm_copy()/__hvm_clear(), or should?
>
> I think so, yes.  At the moment we inject #GP when the guest writes to
> a read-only grant, which is OK: the guest really ought to know better.
> But I think we'll probably end up with neater code if we handle
> read-only grants the same way as p2m_ram_ro.
>
> Anyone else have an opinion on the right thing to do here?
>
>> Also some questions for the new p2m class, say P2M_DISCARD_WRITE_TYPES,
>> and the new predicates, say p2m_is_discard_write:
>> 1> You mentioned emulate_gva_to_mfn() and __hvm_copy() should discard
>> the write ops, yet I also noticed many other places using the
>> p2m_is_readonly, or only the "p2mt == p2m_ram_ro" judgement(in
>> __hvm_copy/__hvm_clear). Among all these other places, is there any ones
>> also supposed to use the p2m_is_discard_write?
>
> I've just had a look through them all, and I can see exactly four
> places that should be using the new p2m_is_discard_write() test:
>
>   - emulate_gva_to_mfn() (Though in fact it's a no-op as shadow-mode
>     guests never have p2m_ram_shared or p2m_ram_logdirty mappings.)
>   - __hvm_copy()
>   - __hvm_clear() and
>   - hvm_hap_nested_page_fault() (where you should also remove the
>     explicit handling of p2m_grant_map_ro below.)
>
> Looking through that turned up a few other oddities, which I'm
> listing here to remind myself to look at them later (i.e. you don't
> need to worry about them for this patch):
>
>   - nsvm_get_nvmcb_page() and nestedhap_walk_L0_p2m() need to handle
>     p2m_ram_logdirty or they might spuriously fail duiring live
>     migration.
>   - __hvm_copy() and __hvm_clear are probably over-strict in their
>     failure to handle grant types.

Hi Tim. Sorry to bother you. :)
I just noticed that in __hvm_copy()/__hvm_clear(), the grant types are 
handled before the p2m_ram_ro - will return HVMCOPY_unhandleable. So if
p2m_is_discard_write() is supposed to replace the handling of 
p2m_ram_ro, handling of p2m_grant_map_ro will still return 
HVMCOPY_unhandleable, before the p2m_is_discard_write() predicate.
Even we move the testing of p2m_is_discard_write() before the handling 
of grant types, it seems not quite clean.
By "over-strict in their failure to handle grant types.", do you also 
mean this?

Thanks
Yu

>   - P2M_UNMAP_TYPES in vmce.c is a mess.  It's not the right place to
>     define this, since it definitely won't be seen my anyone
>     adding a new type, and it already has an 'XXX' comment that says
>     it doesn't cover a lot of cases. :(
>
> I'll have a look at those another time.
>
> Cheers,
>
> Tim.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Tim Deegan]

Hi, 

At 17:01 +0800 on 04 Dec (1417708878), Yu, Zhang wrote:
> I just noticed that in __hvm_copy()/__hvm_clear(), the grant types are 
> handled before the p2m_ram_ro - will return HVMCOPY_unhandleable. So if
> p2m_is_discard_write() is supposed to replace the handling of 
> p2m_ram_ro, handling of p2m_grant_map_ro will still return 
> HVMCOPY_unhandleable, before the p2m_is_discard_write() predicate.
> Even we move the testing of p2m_is_discard_write() before the handling 
> of grant types, it seems not quite clean.
> By "over-strict in their failure to handle grant types.", do you also 
> mean this?

Yes, that's the sort of thing I meant.  I'll try to write a patch for
that later today or next week -- in the meantime I think you should
ignore it. :)

An unrelated thought: when you send your next version can you send it
as a two-patch series, where the first patch does the
p2m_is_discard_write() changes and the second adds your new type?

Cheers,

Tim.

Re: [PATCH v4] x86: add p2m_mmio_write_dm

[Yu, Zhang]

On 12/4/2014 5:36 PM, Tim Deegan wrote:
> Hi,
>
> At 17:01 +0800 on 04 Dec (1417708878), Yu, Zhang wrote:
>> I just noticed that in __hvm_copy()/__hvm_clear(), the grant types are
>> handled before the p2m_ram_ro - will return HVMCOPY_unhandleable. So if
>> p2m_is_discard_write() is supposed to replace the handling of
>> p2m_ram_ro, handling of p2m_grant_map_ro will still return
>> HVMCOPY_unhandleable, before the p2m_is_discard_write() predicate.
>> Even we move the testing of p2m_is_discard_write() before the handling
>> of grant types, it seems not quite clean.
>> By "over-strict in their failure to handle grant types.", do you also
>> mean this?
>
> Yes, that's the sort of thing I meant.  I'll try to write a patch for
> that later today or next week -- in the meantime I think you should
> ignore it. :)
>
> An unrelated thought: when you send your next version can you send it
> as a two-patch series, where the first patch does the
> p2m_is_discard_write() changes and the second adds your new type?

Sure, and thank you, Tim. :)
>
> Cheers,
>
> Tim.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>