[PATCH 0/3] x86/HVM: honor r/o p2m types, in particular during emulation

[PATCH 0/3] x86/HVM: honor r/o p2m types, in particular during emulation

[Jan Beulich]

1: __hvm_copy() should not write to p2m_ioreq_server pages
2: make hvmemul_map_linear_addr() honor p2m_ioreq_server
3: hvmemul_cmpxchg() should also use known_gla()

Jan



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

[PATCH 1/3] x86/HVM: __hvm_copy() should not write to p2m_ioreq_server pages

[Jan Beulich]

Commit 3bdec530a5 ("x86/HVM: split page straddling emulated accesses in
more cases") introduced a hvm_copy_to_guest_linear() attempt before
falling back to hvmemul_linear_mmio_write(). This is wrong for the
p2m_ioreq_server special case. That change widened a pre-existing issue
though: Other writes to such pages also need to be failed (or forced
through emulation), in particular hypercall buffer writes.

Reported-by: ??? <???@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3202,6 +3202,12 @@ static enum hvm_translation_result __hvm
         if ( res != HVMTRANS_okay )
             return res;
 
+        if ( (flags & HVMCOPY_to_guest) && p2mt == p2m_ioreq_server )
+        {
+            put_page(page);
+            return HVMTRANS_bad_gfn_to_mfn;
+        }
+
         p = (char *)__map_domain_page(page) + (addr & ~PAGE_MASK);
 
         if ( flags & HVMCOPY_to_guest )



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

[PATCH 2/3] x86/HVM: make hvmemul_map_linear_addr() honor p2m_ioreq_server

[Jan Beulich]

Write accesses to p2m_ioreq_server pages should get redirected to the
emulator also when using the mapping approach. Extend the
p2m_is_discard_write() check there, and restrict both to the write
access case (this is just a latent bug as currently we go this route
only for write accesses).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -613,10 +613,21 @@ static void *hvmemul_map_linear_addr(
 
         *mfn++ = page_to_mfn(page);
 
-        if ( p2m_is_discard_write(p2mt) )
+        if ( pfec & PFEC_write_access )
         {
-            err = ERR_PTR(~X86EMUL_OKAY);
-            goto out;
+            if ( p2m_is_discard_write(p2mt) )
+            {
+                err = ERR_PTR(~X86EMUL_OKAY);
+                goto out;
+            }
+
+            if ( p2mt == p2m_ioreq_server )
+            {
+                err = NULL;
+                goto out;
+            }
+
+            ASSERT(p2mt == p2m_ram_logdirty || !p2m_is_readonly(p2mt));
         }
     }
 





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

[PATCH 3/3] x86/HVM: hvmemul_cmpxchg() should also use known_gla()

[Jan Beulich]

To be consistent with the write and rmw cases the mapping approach
should not be used when the guest linear address translation is known.
This in particular excludes the discard-write case from bypassing the
emulation path. This also means that now EFLAGS should actually get
properly updated, despite the discarded write portion of the memory
access.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Without "make hvmemul_map_linear_addr() honor p2m_ioreq_server" this
also helps the p2m_ioreq_server case.

TBD: While hvmemul_do_io() takes care of p2m_ioreq_server, I don't see
     it taking care of p2m_is_discard_write() types. Am I overlooking
     something?

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1472,9 +1472,12 @@ static int hvmemul_cmpxchg(
     else if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 )
         pfec |= PFEC_user_mode;
 
-    mapping = hvmemul_map_linear_addr(addr, bytes, pfec, hvmemul_ctxt);
-    if ( IS_ERR(mapping) )
-        return ~PTR_ERR(mapping);
+    if ( !known_gla(addr, bytes, pfec) )
+    {
+        mapping = hvmemul_map_linear_addr(addr, bytes, pfec, hvmemul_ctxt);
+        if ( IS_ERR(mapping) )
+            return ~PTR_ERR(mapping);
+    }
 
     if ( !mapping )
     {





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

Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to p2m_ioreq_server pages

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 13 November 2018 10:14
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>
> Subject: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to
> p2m_ioreq_server pages
> 
> Commit 3bdec530a5 ("x86/HVM: split page straddling emulated accesses in
> more cases") introduced a hvm_copy_to_guest_linear() attempt before
> falling back to hvmemul_linear_mmio_write(). This is wrong for the
> p2m_ioreq_server special case. That change widened a pre-existing issue
> though: Other writes to such pages also need to be failed (or forced
> through emulation), in particular hypercall buffer writes.
> 
> Reported-by: ??? <???@citrix.com>

I think this should be: Igor Druzhinin <igor.druzhinin@citrix.com>

> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> 
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3202,6 +3202,12 @@ static enum hvm_translation_result __hvm
>          if ( res != HVMTRANS_okay )
>              return res;
> 
> +        if ( (flags & HVMCOPY_to_guest) && p2mt == p2m_ioreq_server )
> +        {
> +            put_page(page);
> +            return HVMTRANS_bad_gfn_to_mfn;
> +        }
> +
>          p = (char *)__map_domain_page(page) + (addr & ~PAGE_MASK);
> 
>          if ( flags & HVMCOPY_to_guest )
> 


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

Re: [PATCH 2/3] x86/HVM: make hvmemul_map_linear_addr() honor p2m_ioreq_server

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 13 November 2018 10:14
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>
> Subject: [PATCH 2/3] x86/HVM: make hvmemul_map_linear_addr() honor
> p2m_ioreq_server
> 
> Write accesses to p2m_ioreq_server pages should get redirected to the
> emulator also when using the mapping approach. Extend the
> p2m_is_discard_write() check there, and restrict both to the write
> access case (this is just a latent bug as currently we go this route
> only for write accesses).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -613,10 +613,21 @@ static void *hvmemul_map_linear_addr(
> 
>          *mfn++ = page_to_mfn(page);
> 
> -        if ( p2m_is_discard_write(p2mt) )
> +        if ( pfec & PFEC_write_access )
>          {
> -            err = ERR_PTR(~X86EMUL_OKAY);
> -            goto out;
> +            if ( p2m_is_discard_write(p2mt) )
> +            {
> +                err = ERR_PTR(~X86EMUL_OKAY);
> +                goto out;
> +            }
> +
> +            if ( p2mt == p2m_ioreq_server )
> +            {
> +                err = NULL;
> +                goto out;
> +            }
> +
> +            ASSERT(p2mt == p2m_ram_logdirty || !p2m_is_readonly(p2mt));
>          }
>      }
> 
> 
> 
> 


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

Re: [PATCH 3/3] x86/HVM: hvmemul_cmpxchg() should also use known_gla()

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 13 November 2018 10:15
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>
> Subject: [PATCH 3/3] x86/HVM: hvmemul_cmpxchg() should also use
> known_gla()
> 
> To be consistent with the write and rmw cases the mapping approach
> should not be used when the guest linear address translation is known.
> This in particular excludes the discard-write case from bypassing the
> emulation path. This also means that now EFLAGS should actually get
> properly updated, despite the discarded write portion of the memory
> access.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
> Without "make hvmemul_map_linear_addr() honor p2m_ioreq_server" this
> also helps the p2m_ioreq_server case.
> 
> TBD: While hvmemul_do_io() takes care of p2m_ioreq_server, I don't see
>      it taking care of p2m_is_discard_write() types. Am I overlooking
>      something?

If such an access gets to hvmemul_do_io() I would have thought it should fall through to the null_handler case, which will sink the write.

> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1472,9 +1472,12 @@ static int hvmemul_cmpxchg(
>      else if ( hvmemul_ctxt->seg_reg[x86_seg_ss].dpl == 3 )
>          pfec |= PFEC_user_mode;
> 
> -    mapping = hvmemul_map_linear_addr(addr, bytes, pfec, hvmemul_ctxt);
> -    if ( IS_ERR(mapping) )
> -        return ~PTR_ERR(mapping);
> +    if ( !known_gla(addr, bytes, pfec) )
> +    {
> +        mapping = hvmemul_map_linear_addr(addr, bytes, pfec,
> hvmemul_ctxt);
> +        if ( IS_ERR(mapping) )
> +            return ~PTR_ERR(mapping);
> +    }
> 
>      if ( !mapping )
>      {
> 
> 
> 


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

Re: [PATCH 3/3] x86/HVM: hvmemul_cmpxchg() should also use known_gla()

[Jan Beulich]

>>> On 13.11.18 at 11:34, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 13 November 2018 10:15
>> To: xen-devel <xen-devel@lists.xenproject.org>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
>> <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>
>> Subject: [PATCH 3/3] x86/HVM: hvmemul_cmpxchg() should also use
>> known_gla()
>> 
>> To be consistent with the write and rmw cases the mapping approach
>> should not be used when the guest linear address translation is known.
>> This in particular excludes the discard-write case from bypassing the
>> emulation path. This also means that now EFLAGS should actually get
>> properly updated, despite the discarded write portion of the memory
>> access.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

Thanks.

>> ---
>> Without "make hvmemul_map_linear_addr() honor p2m_ioreq_server" this
>> also helps the p2m_ioreq_server case.
>> 
>> TBD: While hvmemul_do_io() takes care of p2m_ioreq_server, I don't see
>>      it taking care of p2m_is_discard_write() types. Am I overlooking
>>      something?
> 
> If such an access gets to hvmemul_do_io() I would have thought it should 
> fall through to the null_handler case, which will sink the write.

Ah, right - that indeed works without special casing the type(s).

Jan



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

[PATCH 4/3] x86/HVM: hvm_map_guest_frame_rw() should respect p2m_ioreq_server

[Jan Beulich]

Writes to such pages would need to be handed to the emulator, which we're
not prepared to do at this point.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2556,7 +2556,8 @@ static void *_hvm_map_guest_frame(unsign
 
     if ( writable )
     {
-        if ( unlikely(p2m_is_discard_write(p2mt)) )
+        if ( unlikely(p2m_is_discard_write(p2mt)) ||
+             unlikely(p2mt == p2m_ioreq_server) )
             *writable = 0;
         else if ( !permanent )
             paging_mark_pfn_dirty(d, _pfn(gfn));



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

[PATCH 5/3] x86/shadow: emulate_gva_to_mfn() should respect p2m_ioreq_server

[Jan Beulich]

Writes to such pages would need to be handed to the emulator, which we're
not prepared to do at this point.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/hvm.c
+++ b/xen/arch/x86/mm/shadow/hvm.c
@@ -338,7 +338,7 @@ static mfn_t emulate_gva_to_mfn(struct v
     {
         return _mfn(BAD_GFN_TO_MFN);
     }
-    if ( p2m_is_discard_write(p2mt) )
+    if ( p2m_is_discard_write(p2mt) || p2mt == p2m_ioreq_server )
     {
         put_page(page);
         return _mfn(READONLY_GFN);



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

Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to p2m_ioreq_server pages

[Andrew Cooper]

On 13/11/18 10:13, Jan Beulich wrote:
> Commit 3bdec530a5 ("x86/HVM: split page straddling emulated accesses in
> more cases") introduced a hvm_copy_to_guest_linear() attempt before
> falling back to hvmemul_linear_mmio_write(). This is wrong for the
> p2m_ioreq_server special case. That change widened a pre-existing issue
> though: Other writes to such pages also need to be failed (or forced
> through emulation), in particular hypercall buffer writes.
>
> Reported-by: ??? <???@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3202,6 +3202,12 @@ static enum hvm_translation_result __hvm
>          if ( res != HVMTRANS_okay )
>              return res;
>  
> +        if ( (flags & HVMCOPY_to_guest) && p2mt == p2m_ioreq_server )

While this does address the issue, I'm concerned about hardcoding the
behaviour here.

p2m_ioreq_server doesn't mean "I want shadowing properties". It has an
as-yet unspecified per-ioreq-client meaning.

We either want to rename p2m_ioreq_server to something which indicates
its "allow-reads/emulate writes" behaviour, or design a way for the
ioreq client to specify the behaviour it wants.

~Andrew

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

Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to p2m_ioreq_server pages

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 13 November 2018 10:47
> To: Jan Beulich <JBeulich@suse.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>
> Subject: Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to
> p2m_ioreq_server pages
> 
> On 13/11/18 10:13, Jan Beulich wrote:
> > Commit 3bdec530a5 ("x86/HVM: split page straddling emulated accesses in
> > more cases") introduced a hvm_copy_to_guest_linear() attempt before
> > falling back to hvmemul_linear_mmio_write(). This is wrong for the
> > p2m_ioreq_server special case. That change widened a pre-existing issue
> > though: Other writes to such pages also need to be failed (or forced
> > through emulation), in particular hypercall buffer writes.
> >
> > Reported-by: ??? <???@citrix.com>
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -3202,6 +3202,12 @@ static enum hvm_translation_result __hvm
> >          if ( res != HVMTRANS_okay )
> >              return res;
> >
> > +        if ( (flags & HVMCOPY_to_guest) && p2mt == p2m_ioreq_server )
> 
> While this does address the issue, I'm concerned about hardcoding the
> behaviour here.
> 
> p2m_ioreq_server doesn't mean "I want shadowing properties". It has an
> as-yet unspecified per-ioreq-client meaning.
> 
> We either want to rename p2m_ioreq_server to something which indicates
> its "allow-reads/emulate writes" behaviour, or design a way for the
> ioreq client to specify the behaviour it wants.
> 

The comment in the public header is:

/*                                                                           
 * XEN_DMOP_map_mem_type_to_ioreq_server : map or unmap the IOREQ Server <id>                                                                            
 *                                      to specific memory type <type>       
 *                                      for specific accesses <flags>        
 *                                                                           
 * For now, flags only accept the value of XEN_DMOP_IOREQ_MEM_ACCESS_WRITE,  
 * which means only write operations are to be forwarded to an ioreq server. 
 * Support for the emulation of read operations can be added when an ioreq   
 * server has such requirement in future.                                    
 */

...so the write-intercept-only behaviour is baked in. Whilst I agree it would be nice not to proliferate this, I don't think it needs addressing in the short term.

  Paul

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

Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to p2m_ioreq_server pages

[Jan Beulich]

>>> On 13.11.18 at 11:47, <andrew.cooper3@citrix.com> wrote:
> On 13/11/18 10:13, Jan Beulich wrote:
>> Commit 3bdec530a5 ("x86/HVM: split page straddling emulated accesses in
>> more cases") introduced a hvm_copy_to_guest_linear() attempt before
>> falling back to hvmemul_linear_mmio_write(). This is wrong for the
>> p2m_ioreq_server special case. That change widened a pre-existing issue
>> though: Other writes to such pages also need to be failed (or forced
>> through emulation), in particular hypercall buffer writes.
>>
>> Reported-by: ??? <???@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3202,6 +3202,12 @@ static enum hvm_translation_result __hvm
>>          if ( res != HVMTRANS_okay )
>>              return res;
>>  
>> +        if ( (flags & HVMCOPY_to_guest) && p2mt == p2m_ioreq_server )
> 
> While this does address the issue, I'm concerned about hardcoding the
> behaviour here.
> 
> p2m_ioreq_server doesn't mean "I want shadowing properties". It has an
> as-yet unspecified per-ioreq-client meaning.

Why/how is this different from mmio_dm, which then could be
considered having unspecified meaning for reads _and_ writes?
Aren't we simply saying "consider this RAM for reads but MMIO
for writes"?

> We either want to rename p2m_ioreq_server to something which indicates
> its "allow-reads/emulate writes" behaviour, or design a way for the
> ioreq client to specify the behaviour it wants.

Renaming might be worthwhile, but is orthogonal imo. Iirc we had
struggled to find a really suitable (and not overly long) name back
then already.

Jan



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

Re: [PATCH 4/3] x86/HVM: hvm_map_guest_frame_rw() should respect p2m_ioreq_server

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 13 November 2018 10:47
> To: xen-devel <xen-devel@lists.xenproject.org>; Jan Beulich
> <JBeulich@suse.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>
> Subject: [PATCH 4/3] x86/HVM: hvm_map_guest_frame_rw() should respect
> p2m_ioreq_server
> 
> Writes to such pages would need to be handed to the emulator, which we're
> not prepared to do at this point.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Patch #4 out of 3? :-)

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> 
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2556,7 +2556,8 @@ static void *_hvm_map_guest_frame(unsign
> 
>      if ( writable )
>      {
> -        if ( unlikely(p2m_is_discard_write(p2mt)) )
> +        if ( unlikely(p2m_is_discard_write(p2mt)) ||
> +             unlikely(p2mt == p2m_ioreq_server) )
>              *writable = 0;
>          else if ( !permanent )
>              paging_mark_pfn_dirty(d, _pfn(gfn));
> 


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

Re: [PATCH 5/3] x86/shadow: emulate_gva_to_mfn() should respect p2m_ioreq_server

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 13 November 2018 10:47
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Tim (Xen.org)
> <tim@xen.org>
> Subject: [PATCH 5/3] x86/shadow: emulate_gva_to_mfn() should respect
> p2m_ioreq_server
> 
> Writes to such pages would need to be handed to the emulator, which we're
> not prepared to do at this point.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/mm/shadow/hvm.c
> +++ b/xen/arch/x86/mm/shadow/hvm.c
> @@ -338,7 +338,7 @@ static mfn_t emulate_gva_to_mfn(struct v
>      {
>          return _mfn(BAD_GFN_TO_MFN);
>      }
> -    if ( p2m_is_discard_write(p2mt) )
> +    if ( p2m_is_discard_write(p2mt) || p2mt == p2m_ioreq_server )
>      {
>          put_page(page);
>          return _mfn(READONLY_GFN);

Is this what we want here? I would have thought we want to return BAD_GFN_TO_MFN in the p2m_ioreq_server case so that the caller treats this in the same way it would MMIO.

  Paul

> 


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

Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to p2m_ioreq_server pages

[Andrew Cooper]

On 13/11/2018 10:53, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper
>> Sent: 13 November 2018 10:47
>> To: Jan Beulich <JBeulich@suse.com>; xen-devel <xen-
>> devel@lists.xenproject.org>
>> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>
>> Subject: Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to
>> p2m_ioreq_server pages
>>
>> On 13/11/18 10:13, Jan Beulich wrote:
>>> Commit 3bdec530a5 ("x86/HVM: split page straddling emulated accesses in
>>> more cases") introduced a hvm_copy_to_guest_linear() attempt before
>>> falling back to hvmemul_linear_mmio_write(). This is wrong for the
>>> p2m_ioreq_server special case. That change widened a pre-existing issue
>>> though: Other writes to such pages also need to be failed (or forced
>>> through emulation), in particular hypercall buffer writes.
>>>
>>> Reported-by: ??? <???@citrix.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -3202,6 +3202,12 @@ static enum hvm_translation_result __hvm
>>>          if ( res != HVMTRANS_okay )
>>>              return res;
>>>
>>> +        if ( (flags & HVMCOPY_to_guest) && p2mt == p2m_ioreq_server )
>> While this does address the issue, I'm concerned about hardcoding the
>> behaviour here.
>>
>> p2m_ioreq_server doesn't mean "I want shadowing properties". It has an
>> as-yet unspecified per-ioreq-client meaning.
>>
>> We either want to rename p2m_ioreq_server to something which indicates
>> its "allow-reads/emulate writes" behaviour, or design a way for the
>> ioreq client to specify the behaviour it wants.
>>
> The comment in the public header is:
>
> /*                                                                           
>  * XEN_DMOP_map_mem_type_to_ioreq_server : map or unmap the IOREQ Server <id>                                                                            
>  *                                      to specific memory type <type>       
>  *                                      for specific accesses <flags>        
>  *                                                                           
>  * For now, flags only accept the value of XEN_DMOP_IOREQ_MEM_ACCESS_WRITE,  
>  * which means only write operations are to be forwarded to an ioreq server. 
>  * Support for the emulation of read operations can be added when an ioreq   
>  * server has such requirement in future.                                    
>  */
>
> ...so the write-intercept-only behaviour is baked in. Whilst I agree it would be nice not to proliferate this, I don't think it needs addressing in the short term.

Lovely :(

~Andrew

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

Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to p2m_ioreq_server pages

[Jan Beulich]

>>> On 13.11.18 at 12:08, <andrew.cooper3@citrix.com> wrote:
> On 13/11/2018 10:53, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Andrew Cooper
>>> Sent: 13 November 2018 10:47
>>> To: Jan Beulich <JBeulich@suse.com>; xen-devel <xen-
>>> devel@lists.xenproject.org>
>>> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>
>>> Subject: Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to
>>> p2m_ioreq_server pages
>>>
>>> On 13/11/18 10:13, Jan Beulich wrote:
>>>> Commit 3bdec530a5 ("x86/HVM: split page straddling emulated accesses in
>>>> more cases") introduced a hvm_copy_to_guest_linear() attempt before
>>>> falling back to hvmemul_linear_mmio_write(). This is wrong for the
>>>> p2m_ioreq_server special case. That change widened a pre-existing issue
>>>> though: Other writes to such pages also need to be failed (or forced
>>>> through emulation), in particular hypercall buffer writes.
>>>>
>>>> Reported-by: ??? <???@citrix.com>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -3202,6 +3202,12 @@ static enum hvm_translation_result __hvm
>>>>          if ( res != HVMTRANS_okay )
>>>>              return res;
>>>>
>>>> +        if ( (flags & HVMCOPY_to_guest) && p2mt == p2m_ioreq_server )
>>> While this does address the issue, I'm concerned about hardcoding the
>>> behaviour here.
>>>
>>> p2m_ioreq_server doesn't mean "I want shadowing properties". It has an
>>> as-yet unspecified per-ioreq-client meaning.
>>>
>>> We either want to rename p2m_ioreq_server to something which indicates
>>> its "allow-reads/emulate writes" behaviour, or design a way for the
>>> ioreq client to specify the behaviour it wants.
>>>
>> The comment in the public header is:
>>
>> /*                                                                           
>>  * XEN_DMOP_map_mem_type_to_ioreq_server : map or unmap the IOREQ Server <id>                                                                            
>>  *                                      to specific memory type <type>       
>>  *                                      for specific accesses <flags>        
>>  *                                                                           
>>  * For now, flags only accept the value of XEN_DMOP_IOREQ_MEM_ACCESS_WRITE,  
>>  * which means only write operations are to be forwarded to an ioreq server. 
>>  * Support for the emulation of read operations can be added when an ioreq   
>>  * server has such requirement in future.                                    
>>  */
>>
>> ...so the write-intercept-only behaviour is baked in. Whilst I agree it 
> would be nice not to proliferate this, I don't think it needs addressing in 
> the short term.
> 
> Lovely :(

Wasn't the rationale back then that we'd add further p2m types if we
needed new distinct behavior (and in particular accept flags other than
the single form currently accepted)?

Jan



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

Re: [PATCH 4/3] x86/HVM: hvm_map_guest_frame_rw() should respect p2m_ioreq_server

[Jan Beulich]

>>> On 13.11.18 at 11:54, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 13 November 2018 10:47
>> To: xen-devel <xen-devel@lists.xenproject.org>; Jan Beulich
>> <JBeulich@suse.com>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
>> <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>
>> Subject: [PATCH 4/3] x86/HVM: hvm_map_guest_frame_rw() should respect
>> p2m_ioreq_server
>> 
>> Writes to such pages would need to be handed to the emulator, which we're
>> not prepared to do at this point.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Patch #4 out of 3? :-)

Well - I had realized I should do a pattern search only after sending
the first three patches, when seeing further input from Andrew. Yet
it didn't seem to make sense to send a v2 right away, or an entirely
separate series.

> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

Thanks.

Jan



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

Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to p2m_ioreq_server pages

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 13 November 2018 11:15
> To: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to
> p2m_ioreq_server pages
> 
> >>> On 13.11.18 at 12:08, <andrew.cooper3@citrix.com> wrote:
> > On 13/11/2018 10:53, Paul Durrant wrote:
> >>> -----Original Message-----
> >>> From: Andrew Cooper
> >>> Sent: 13 November 2018 10:47
> >>> To: Jan Beulich <JBeulich@suse.com>; xen-devel <xen-
> >>> devel@lists.xenproject.org>
> >>> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>
> >>> Subject: Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to
> >>> p2m_ioreq_server pages
> >>>
> >>> On 13/11/18 10:13, Jan Beulich wrote:
> >>>> Commit 3bdec530a5 ("x86/HVM: split page straddling emulated accesses
> in
> >>>> more cases") introduced a hvm_copy_to_guest_linear() attempt before
> >>>> falling back to hvmemul_linear_mmio_write(). This is wrong for the
> >>>> p2m_ioreq_server special case. That change widened a pre-existing
> issue
> >>>> though: Other writes to such pages also need to be failed (or forced
> >>>> through emulation), in particular hypercall buffer writes.
> >>>>
> >>>> Reported-by: ??? <???@citrix.com>
> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>>
> >>>> --- a/xen/arch/x86/hvm/hvm.c
> >>>> +++ b/xen/arch/x86/hvm/hvm.c
> >>>> @@ -3202,6 +3202,12 @@ static enum hvm_translation_result __hvm
> >>>>          if ( res != HVMTRANS_okay )
> >>>>              return res;
> >>>>
> >>>> +        if ( (flags & HVMCOPY_to_guest) && p2mt == p2m_ioreq_server
> )
> >>> While this does address the issue, I'm concerned about hardcoding the
> >>> behaviour here.
> >>>
> >>> p2m_ioreq_server doesn't mean "I want shadowing properties". It has an
> >>> as-yet unspecified per-ioreq-client meaning.
> >>>
> >>> We either want to rename p2m_ioreq_server to something which indicates
> >>> its "allow-reads/emulate writes" behaviour, or design a way for the
> >>> ioreq client to specify the behaviour it wants.
> >>>
> >> The comment in the public header is:
> >>
> >> /*
> >>  * XEN_DMOP_map_mem_type_to_ioreq_server : map or unmap the IOREQ
> Server <id>
> >>  *                                      to specific memory type <type>
> >>  *                                      for specific accesses <flags>
> >>  *
> >>  * For now, flags only accept the value of
> XEN_DMOP_IOREQ_MEM_ACCESS_WRITE,
> >>  * which means only write operations are to be forwarded to an ioreq
> server.
> >>  * Support for the emulation of read operations can be added when an
> ioreq
> >>  * server has such requirement in future.
> >>  */
> >>
> >> ...so the write-intercept-only behaviour is baked in. Whilst I agree it
> > would be nice not to proliferate this, I don't think it needs addressing
> in
> > the short term.
> >
> > Lovely :(
> 
> Wasn't the rationale back then that we'd add further p2m types if we
> needed new distinct behavior (and in particular accept flags other than
> the single form currently accepted)?
> 

Yes, the idea was to have page-to-type map so that we could have dedicate types for each ioreq server and behaviour.

  Paul

> Jan
> 


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

Re: [PATCH 5/3] x86/shadow: emulate_gva_to_mfn() should respect p2m_ioreq_server

[Jan Beulich]

>>> On 13.11.18 at 11:59, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 13 November 2018 10:47
>> To: xen-devel <xen-devel@lists.xenproject.org>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
>> <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Tim (Xen.org)
>> <tim@xen.org>
>> Subject: [PATCH 5/3] x86/shadow: emulate_gva_to_mfn() should respect
>> p2m_ioreq_server
>> 
>> Writes to such pages would need to be handed to the emulator, which we're
>> not prepared to do at this point.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> --- a/xen/arch/x86/mm/shadow/hvm.c
>> +++ b/xen/arch/x86/mm/shadow/hvm.c
>> @@ -338,7 +338,7 @@ static mfn_t emulate_gva_to_mfn(struct v
>>      {
>>          return _mfn(BAD_GFN_TO_MFN);
>>      }
>> -    if ( p2m_is_discard_write(p2mt) )
>> +    if ( p2m_is_discard_write(p2mt) || p2mt == p2m_ioreq_server )
>>      {
>>          put_page(page);
>>          return _mfn(READONLY_GFN);
> 
> Is this what we want here? I would have thought we want to return 
> BAD_GFN_TO_MFN in the p2m_ioreq_server case so that the caller treats this in 
> the same way it would MMIO.

I'm not sure which behavior is better; I'm certainly fine with switching
as you say, but I'd first like to see Tim's opinion as well.

Jan



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

Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to p2m_ioreq_server pages

[Igor Druzhinin]

On 13/11/2018 10:27, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 13 November 2018 10:14
>> To: xen-devel <xen-devel@lists.xenproject.org>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
>> <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>
>> Subject: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to
>> p2m_ioreq_server pages
>>
>> Commit 3bdec530a5 ("x86/HVM: split page straddling emulated accesses in
>> more cases") introduced a hvm_copy_to_guest_linear() attempt before
>> falling back to hvmemul_linear_mmio_write(). This is wrong for the
>> p2m_ioreq_server special case. That change widened a pre-existing issue
>> though: Other writes to such pages also need to be failed (or forced
>> through emulation), in particular hypercall buffer writes.
>>
>> Reported-by: ??? <???@citrix.com>
> 
> I think this should be: Igor Druzhinin <igor.druzhinin@citrix.com>
> 

Yes, please add me to CC so I could pull and test the patches.

Igor

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

Re: [PATCH 4/3] x86/HVM: hvm_map_guest_frame_rw() should respect p2m_ioreq_server

[Igor Druzhinin]

On 13/11/2018 10:46, Jan Beulich wrote:
> Writes to such pages would need to be handed to the emulator, which we're
> not prepared to do at this point.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2556,7 +2556,8 @@ static void *_hvm_map_guest_frame(unsign
>  
>      if ( writable )
>      {
> -        if ( unlikely(p2m_is_discard_write(p2mt)) )
> +        if ( unlikely(p2m_is_discard_write(p2mt)) ||
> +             unlikely(p2mt == p2m_ioreq_server) )

Shouldn't we introduce p2m_is_ioreq_server() for consistency and use it
everywhere?

Igor

>              *writable = 0;
>          else if ( !permanent )
>              paging_mark_pfn_dirty(d, _pfn(gfn));
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
> 

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

Re: [PATCH 4/3] x86/HVM: hvm_map_guest_frame_rw() should respect p2m_ioreq_server

[Jan Beulich]

>>> On 13.11.18 at 14:39, <igor.druzhinin@citrix.com> wrote:
> On 13/11/2018 10:46, Jan Beulich wrote:
>> Writes to such pages would need to be handed to the emulator, which we're
>> not prepared to do at this point.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2556,7 +2556,8 @@ static void *_hvm_map_guest_frame(unsign
>>  
>>      if ( writable )
>>      {
>> -        if ( unlikely(p2m_is_discard_write(p2mt)) )
>> +        if ( unlikely(p2m_is_discard_write(p2mt)) ||
>> +             unlikely(p2mt == p2m_ioreq_server) )
> 
> Shouldn't we introduce p2m_is_ioreq_server() for consistency and use it
> everywhere?

I think such abstractions help if multiple types are to be covered;
I don't mind them to be used also for single types, but I don't
thinks that's overly important. Plus doing so is of course unrelated
to this series, as other checks using == already exist.

Jan



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

Re: [PATCH 5/3] x86/shadow: emulate_gva_to_mfn() should respect p2m_ioreq_server

[Tim Deegan]

At 04:22 -0700 on 13 Nov (1542082936), Jan Beulich wrote:
> >>> On 13.11.18 at 11:59, <Paul.Durrant@citrix.com> wrote:
> >> Subject: [PATCH 5/3] x86/shadow: emulate_gva_to_mfn() should respect
> >> p2m_ioreq_server
> >> 
> >> Writes to such pages would need to be handed to the emulator, which we're
> >> not prepared to do at this point.
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> 
> >> --- a/xen/arch/x86/mm/shadow/hvm.c
> >> +++ b/xen/arch/x86/mm/shadow/hvm.c
> >> @@ -338,7 +338,7 @@ static mfn_t emulate_gva_to_mfn(struct v
> >>      {
> >>          return _mfn(BAD_GFN_TO_MFN);
> >>      }
> >> -    if ( p2m_is_discard_write(p2mt) )
> >> +    if ( p2m_is_discard_write(p2mt) || p2mt == p2m_ioreq_server )
> >>      {
> >>          put_page(page);
> >>          return _mfn(READONLY_GFN);
> > 
> > Is this what we want here? I would have thought we want to return 
> > BAD_GFN_TO_MFN in the p2m_ioreq_server case so that the caller treats this in 
> > the same way it would MMIO.
> 
> I'm not sure which behavior is better; I'm certainly fine with switching
> as you say, but I'd first like to see Tim's opinion as well.

I'm not clear on what behaviour you want for this kind of page in
general -- I suspect I have missed or forgotten some context.  If the
guest's not supposed to write to it, then IMO you should add it to
P2M_DISCARD_WRITE_TYPES rather than special-casing it here.

Cheers,

Tim.


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

Re: [PATCH 5/3] x86/shadow: emulate_gva_to_mfn() should respect p2m_ioreq_server

[Paul Durrant]

> -----Original Message-----
> From: Tim Deegan [mailto:tim@xen.org]
> Sent: 14 November 2018 12:42
> To: Jan Beulich <JBeulich@suse.com>
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; xen-devel
> <xen-devel@lists.xenproject.org>
> Subject: Re: [PATCH 5/3] x86/shadow: emulate_gva_to_mfn() should respect
> p2m_ioreq_server
> 
> At 04:22 -0700 on 13 Nov (1542082936), Jan Beulich wrote:
> > >>> On 13.11.18 at 11:59, <Paul.Durrant@citrix.com> wrote:
> > >> Subject: [PATCH 5/3] x86/shadow: emulate_gva_to_mfn() should respect
> > >> p2m_ioreq_server
> > >>
> > >> Writes to such pages would need to be handed to the emulator, which
> we're
> > >> not prepared to do at this point.
> > >>
> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > >>
> > >> --- a/xen/arch/x86/mm/shadow/hvm.c
> > >> +++ b/xen/arch/x86/mm/shadow/hvm.c
> > >> @@ -338,7 +338,7 @@ static mfn_t emulate_gva_to_mfn(struct v
> > >>      {
> > >>          return _mfn(BAD_GFN_TO_MFN);
> > >>      }
> > >> -    if ( p2m_is_discard_write(p2mt) )
> > >> +    if ( p2m_is_discard_write(p2mt) || p2mt == p2m_ioreq_server )
> > >>      {
> > >>          put_page(page);
> > >>          return _mfn(READONLY_GFN);
> > >
> > > Is this what we want here? I would have thought we want to return
> > > BAD_GFN_TO_MFN in the p2m_ioreq_server case so that the caller treats
> this in
> > > the same way it would MMIO.
> >
> > I'm not sure which behavior is better; I'm certainly fine with switching
> > as you say, but I'd first like to see Tim's opinion as well.
> 
> I'm not clear on what behaviour you want for this kind of page in
> general -- I suspect I have missed or forgotten some context.  If the
> guest's not supposed to write to it, then IMO you should add it to
> P2M_DISCARD_WRITE_TYPES rather than special-casing it here.

The type has an odd semantic... it needs to read as normal RAM but writes need to hit emulation. The use-case is for GVT-g where the emulator needs to intercept writes to the graphics pagetables in guest RAM.

  Paul

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

Re: [PATCH 5/3] x86/shadow: emulate_gva_to_mfn() should respect p2m_ioreq_server

[Tim Deegan]

At 12:44 +0000 on 14 Nov (1542199496), Paul Durrant wrote:
> > -----Original Message-----
> > From: Tim Deegan [mailto:tim@xen.org]
> > Sent: 14 November 2018 12:42
> > To: Jan Beulich <JBeulich@suse.com>
> > Cc: Paul Durrant <Paul.Durrant@citrix.com>; Andrew Cooper
> > <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; xen-devel
> > <xen-devel@lists.xenproject.org>
> > Subject: Re: [PATCH 5/3] x86/shadow: emulate_gva_to_mfn() should respect
> > p2m_ioreq_server
> > 
> > At 04:22 -0700 on 13 Nov (1542082936), Jan Beulich wrote:
> > > >>> On 13.11.18 at 11:59, <Paul.Durrant@citrix.com> wrote:
> > > >> Subject: [PATCH 5/3] x86/shadow: emulate_gva_to_mfn() should respect
> > > >> p2m_ioreq_server
> > > >>
> > > >> Writes to such pages would need to be handed to the emulator, which
> > we're
> > > >> not prepared to do at this point.
> > > >>
> > > >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > > >>
> > > >> --- a/xen/arch/x86/mm/shadow/hvm.c
> > > >> +++ b/xen/arch/x86/mm/shadow/hvm.c
> > > >> @@ -338,7 +338,7 @@ static mfn_t emulate_gva_to_mfn(struct v
> > > >>      {
> > > >>          return _mfn(BAD_GFN_TO_MFN);
> > > >>      }
> > > >> -    if ( p2m_is_discard_write(p2mt) )
> > > >> +    if ( p2m_is_discard_write(p2mt) || p2mt == p2m_ioreq_server )
> > > >>      {
> > > >>          put_page(page);
> > > >>          return _mfn(READONLY_GFN);
> > > >
> > > > Is this what we want here? I would have thought we want to return
> > > > BAD_GFN_TO_MFN in the p2m_ioreq_server case so that the caller treats
> > this in
> > > > the same way it would MMIO.
> > >
> > > I'm not sure which behavior is better; I'm certainly fine with switching
> > > as you say, but I'd first like to see Tim's opinion as well.
> > 
> > I'm not clear on what behaviour you want for this kind of page in
> > general -- I suspect I have missed or forgotten some context.  If the
> > guest's not supposed to write to it, then IMO you should add it to
> > P2M_DISCARD_WRITE_TYPES rather than special-casing it here.
> 
> The type has an odd semantic... it needs to read as normal RAM but writes need to hit emulation. The use-case is for GVT-g where the emulator needs to intercept writes to the graphics pagetables in guest RAM.

I see, thanks.  In that case, I agree with you that you should signal
this as BAD_GFN_TO_MFN here to trigger the MMIO path.

Cheers,

Tim.

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

[PATCH v2 3/3] x86/HVM: honor p2m_ioreq_server type

[Jan Beulich]

1: __hvm_copy() should not write to p2m_ioreq_server pages
2: hvm_map_guest_frame_rw() should respect p2m_ioreq_server
3: emulate_gva_to_mfn() should respect p2m_ioreq_server

Jan



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

[PATCH v2 0/3] x86/HVM: honor p2m_ioreq_server type

[Jan Beulich]

1: __hvm_copy() should not write to p2m_ioreq_server pages
2: hvm_map_guest_frame_rw() should respect p2m_ioreq_server
3: emulate_gva_to_mfn() should respect p2m_ioreq_server

Jan



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

[PATCH v2 1/3] x86/HVM: __hvm_copy() should not write to p2m_ioreq_server pages

[Jan Beulich]

Commit 3bdec530a5 ("x86/HVM: split page straddling emulated accesses in
more cases") introduced a hvm_copy_to_guest_linear() attempt before
falling back to hvmemul_linear_mmio_write(). This is wrong for the
p2m_ioreq_server special case. That change widened a pre-existing issue
though: Other writes to such pages also need to be failed (or forced
through emulation), in particular hypercall buffer writes.

Reported-by: Igor Druzhinin <igor.druzhinin@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3204,6 +3204,12 @@ static enum hvm_translation_result __hvm
         if ( res != HVMTRANS_okay )
             return res;
 
+        if ( (flags & HVMCOPY_to_guest) && p2mt == p2m_ioreq_server )
+        {
+            put_page(page);
+            return HVMTRANS_bad_gfn_to_mfn;
+        }
+
         p = (char *)__map_domain_page(page) + (addr & ~PAGE_MASK);
 
         if ( flags & HVMCOPY_to_guest )



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

[PATCH v2 2/3] x86/HVM: hvm_map_guest_frame_rw() should respect p2m_ioreq_server

[Jan Beulich]

Writes to such pages would need to be handed to the emulator, which we're
not prepared to do at this point.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2558,7 +2558,8 @@ static void *_hvm_map_guest_frame(unsign
 
     if ( writable )
     {
-        if ( unlikely(p2m_is_discard_write(p2mt)) )
+        if ( unlikely(p2m_is_discard_write(p2mt)) ||
+             unlikely(p2mt == p2m_ioreq_server) )
             *writable = 0;
         else if ( !permanent )
             paging_mark_pfn_dirty(d, _pfn(gfn));



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

[PATCH v2 3/3] x86/shadow: emulate_gva_to_mfn() should respect p2m_ioreq_server

[Jan Beulich]

Writes to such pages need to be handed to the emulator.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Return BAD_GFN_TO_MFN instead.

--- a/xen/arch/x86/mm/shadow/hvm.c
+++ b/xen/arch/x86/mm/shadow/hvm.c
@@ -338,6 +338,11 @@ static mfn_t emulate_gva_to_mfn(struct v
     {
         return _mfn(BAD_GFN_TO_MFN);
     }
+    if ( p2mt == p2m_ioreq_server )
+    {
+        put_page(page);
+        return _mfn(BAD_GFN_TO_MFN);
+    }
     if ( p2m_is_discard_write(p2mt) )
     {
         put_page(page);



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

Re: [PATCH v2 3/3] x86/shadow: emulate_gva_to_mfn() should respect p2m_ioreq_server

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 15 November 2018 12:52
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Tim (Xen.org) <tim@xen.org>
> Subject: [PATCH v2 3/3] x86/shadow: emulate_gva_to_mfn() should respect
> p2m_ioreq_server
> 
> Writes to such pages need to be handed to the emulator.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
> v2: Return BAD_GFN_TO_MFN instead.
> 
> --- a/xen/arch/x86/mm/shadow/hvm.c
> +++ b/xen/arch/x86/mm/shadow/hvm.c
> @@ -338,6 +338,11 @@ static mfn_t emulate_gva_to_mfn(struct v
>      {
>          return _mfn(BAD_GFN_TO_MFN);
>      }
> +    if ( p2mt == p2m_ioreq_server )
> +    {
> +        put_page(page);
> +        return _mfn(BAD_GFN_TO_MFN);
> +    }
>      if ( p2m_is_discard_write(p2mt) )
>      {
>          put_page(page);
> 


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

Re: [PATCH v2 0/3] x86/HVM: honor p2m_ioreq_server type

[Andrew Cooper]

On 15/11/2018 12:45, Jan Beulich wrote:
> 1: __hvm_copy() should not write to p2m_ioreq_server pages
> 2: hvm_map_guest_frame_rw() should respect p2m_ioreq_server
> 3: emulate_gva_to_mfn() should respect p2m_ioreq_server

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 3/3] x86/shadow: emulate_gva_to_mfn() should respect p2m_ioreq_server

[Tim Deegan]

At 05:51 -0700 on 15 Nov (1542261108), Jan Beulich wrote:
> Writes to such pages need to be handed to the emulator.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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