[PATCH v2 0/2] MMIO emulation fixes

[PATCH v2 0/2] MMIO emulation fixes

[Paul Durrant]

These are probably both candidates for back-port.

Paul Durrant (2):
  x86/hvm/ioreq: MMIO range checking completely ignores direction flag
  x86/hvm/emulate: make sure rep I/O emulation does not cross GFN
    boundaries

 xen/arch/x86/hvm/emulate.c | 19 +++++++++++++++++++
 xen/arch/x86/hvm/ioreq.c   | 15 ++++++++++-----
 2 files changed, 29 insertions(+), 5 deletions(-)

-- 
2.11.0


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

[PATCH v2 1/2] x86/hvm/ioreq: MMIO range checking completely ignores direction flag

[Paul Durrant]

hvm_select_ioreq_server() is used to route an ioreq to the appropriate
ioreq server. For MMIO this is done by comparing the range of the ioreq
to the ranges registered by the device models of each ioreq server.
Unfortunately the calculation of the range if the ioreq completely ignores
the direction flag and thus may calculate the wrong range for comparison.
Thus the ioreq may either be routed to the wrong server or erroneously
terminated by null_ops.

NOTE: The patch also fixes whitespace in the switch statement to make it
      style compliant.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/hvm/ioreq.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 7c515b3ef7..940a2c9728 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -1353,20 +1353,25 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
 
         switch ( type )
         {
-            unsigned long end;
+            unsigned long start, end;
 
         case XEN_DMOP_IO_RANGE_PORT:
-            end = addr + p->size - 1;
-            if ( rangeset_contains_range(r, addr, end) )
+            start = addr;
+            end = start + p->size - 1;
+            if ( rangeset_contains_range(r, start, end) )
                 return s;
 
             break;
+
         case XEN_DMOP_IO_RANGE_MEMORY:
-            end = addr + (p->size * p->count) - 1;
-            if ( rangeset_contains_range(r, addr, end) )
+            start = hvm_mmio_first_byte(p);
+            end = hvm_mmio_last_byte(p);
+
+            if ( rangeset_contains_range(r, start, end) )
                 return s;
 
             break;
+
         case XEN_DMOP_IO_RANGE_PCI:
             if ( rangeset_contains_singleton(r, addr >> 32) )
             {
-- 
2.11.0


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

[PATCH v2 2/2] x86/hvm/emulate: make sure rep I/O emulation does not cross GFN boundaries

[Paul Durrant]

When emulating a rep I/O operation it is possible that the ioreq will
describe a single operation that spans multiple GFNs. This is fine as long
as all those GFNs fall within an MMIO region covered by a single device
model, but unfortunately the higher levels of the emulation code do not
guarantee that. This is something that should almost certainly be fixed,
but in the meantime this patch makes sure that MMIO is truncated at GFN
boundaries and hence the appropriate device model is re-evaluated for each
target GFN.

NOTE: This patch does not deal with the case of a single MMIO operation
      spanning a GFN boundary. That is more complex to deal with and is
      deferred to a subsequent patch.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v2:
 - Cosmetic fixes suggested by Andrew.
 - Make sure we don't end up with p.count == 0, and make sure we end
   up with p.count == 1 where a single op spans a GFN boundary.
---
 xen/arch/x86/hvm/emulate.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 8385c62145..602c21ce2a 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -184,6 +184,25 @@ static int hvmemul_do_io(
         hvmtrace_io_assist(&p);
     }
 
+    /*
+     * Make sure that we truncate rep MMIO at any GFN boundary. This is
+     * necessary to ensure that the correct device model is targetted
+     * or that we correctly handle a rep op spanning MMIO and RAM.
+     */
+    if ( unlikely(p.count > 1) && p.type == IOREQ_TYPE_COPY )
+    {
+        unsigned long off = p.addr & ~PAGE_MASK;
+
+        if ( PAGE_SIZE - off < p.size ) /* single rep spans GFN */
+            p.count = 1;
+        else
+            p.count = min_t(unsigned long,
+                            p.count,
+                            ((p.df ? (off + p.size) : (PAGE_SIZE - off)) /
+                             p.size));
+    }
+    ASSERT(p.count);
+
     vio->io_req = p;
 
     rc = hvm_io_intercept(&p);
-- 
2.11.0


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

Re: [PATCH v2 2/2] x86/hvm/emulate: make sure rep I/O emulation does not cross GFN boundaries

[Jan Beulich]

>>> On 10.08.18 at 16:48, <paul.durrant@citrix.com> wrote:
> When emulating a rep I/O operation it is possible that the ioreq will
> describe a single operation that spans multiple GFNs. This is fine as long
> as all those GFNs fall within an MMIO region covered by a single device
> model, but unfortunately the higher levels of the emulation code do not
> guarantee that. This is something that should almost certainly be fixed,
> but in the meantime this patch makes sure that MMIO is truncated at GFN
> boundaries and hence the appropriate device model is re-evaluated for each
> target GFN.
> 
> NOTE: This patch does not deal with the case of a single MMIO operation
>       spanning a GFN boundary. That is more complex to deal with and is
>       deferred to a subsequent patch.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with a type change request:

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -184,6 +184,25 @@ static int hvmemul_do_io(
>          hvmtrace_io_assist(&p);
>      }
>  
> +    /*
> +     * Make sure that we truncate rep MMIO at any GFN boundary. This is
> +     * necessary to ensure that the correct device model is targetted
> +     * or that we correctly handle a rep op spanning MMIO and RAM.
> +     */
> +    if ( unlikely(p.count > 1) && p.type == IOREQ_TYPE_COPY )
> +    {
> +        unsigned long off = p.addr & ~PAGE_MASK;

If this was "unsigned int", all calculations below could be slightly
cheaper 32-bit ones and ...

> +        if ( PAGE_SIZE - off < p.size ) /* single rep spans GFN */
> +            p.count = 1;
> +        else
> +            p.count = min_t(unsigned long,

... this could be just min(), as long as ...

> +                            p.count,
> +                            ((p.df ? (off + p.size) : (PAGE_SIZE - off)) /

... the 3rd arg of the ?: gets cast to unsigned int. If you agree, I'd
be fine doing the adjustments while committing.

Jan



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

Re: [PATCH v2 2/2] x86/hvm/emulate: make sure rep I/O emulation does not cross GFN boundaries

[Andrew Cooper]

On 15/08/18 13:41, Jan Beulich wrote:
>>>> On 10.08.18 at 16:48, <paul.durrant@citrix.com> wrote:
>> When emulating a rep I/O operation it is possible that the ioreq will
>> describe a single operation that spans multiple GFNs. This is fine as long
>> as all those GFNs fall within an MMIO region covered by a single device
>> model, but unfortunately the higher levels of the emulation code do not
>> guarantee that. This is something that should almost certainly be fixed,
>> but in the meantime this patch makes sure that MMIO is truncated at GFN
>> boundaries and hence the appropriate device model is re-evaluated for each
>> target GFN.
>>
>> NOTE: This patch does not deal with the case of a single MMIO operation
>>       spanning a GFN boundary. That is more complex to deal with and is
>>       deferred to a subsequent patch.
>>
>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with a type change request:
>
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -184,6 +184,25 @@ static int hvmemul_do_io(
>>          hvmtrace_io_assist(&p);
>>      }
>>  
>> +    /*
>> +     * Make sure that we truncate rep MMIO at any GFN boundary. This is
>> +     * necessary to ensure that the correct device model is targetted
>> +     * or that we correctly handle a rep op spanning MMIO and RAM.
>> +     */
>> +    if ( unlikely(p.count > 1) && p.type == IOREQ_TYPE_COPY )
>> +    {
>> +        unsigned long off = p.addr & ~PAGE_MASK;
> If this was "unsigned int", all calculations below could be slightly
> cheaper 32-bit ones and ...
>
>> +        if ( PAGE_SIZE - off < p.size ) /* single rep spans GFN */
>> +            p.count = 1;
>> +        else
>> +            p.count = min_t(unsigned long,
> ... this could be just min(), as long as ...
>
>> +                            p.count,
>> +                            ((p.df ? (off + p.size) : (PAGE_SIZE - off)) /
> ... the 3rd arg of the ?: gets cast to unsigned int. If you agree, I'd
> be fine doing the adjustments while committing.

Paul is OoO for a while now.  In lieu, I'd say "yes please" to these
suggestions.

~Andrew

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

Re: [PATCH v2 2/2] x86/hvm/emulate: make sure rep I/O emulation does not cross GFN boundaries

[Jan Beulich]

>>> On 10.08.18 at 16:48, <paul.durrant@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -184,6 +184,25 @@ static int hvmemul_do_io(
>          hvmtrace_io_assist(&p);
>      }
>  
> +    /*
> +     * Make sure that we truncate rep MMIO at any GFN boundary. This is
> +     * necessary to ensure that the correct device model is targetted
> +     * or that we correctly handle a rep op spanning MMIO and RAM.
> +     */
> +    if ( unlikely(p.count > 1) && p.type == IOREQ_TYPE_COPY )
> +    {
> +        unsigned long off = p.addr & ~PAGE_MASK;
> +
> +        if ( PAGE_SIZE - off < p.size ) /* single rep spans GFN */
> +            p.count = 1;
> +        else
> +            p.count = min_t(unsigned long,
> +                            p.count,
> +                            ((p.df ? (off + p.size) : (PAGE_SIZE - off)) /
> +                             p.size));
> +    }
> +    ASSERT(p.count);

I've applied the patch with the earlier suggested changes, but I wonder
if the placement especially wrt the visible in context hvmtrace_io_assist()
isn't sub-optimal.

Considering the other splitting done elsewhere I also wonder whether
some of that can't then go away, or whether the specific code path
where you've found the splitting to be missing wouldn't better be fixed
instead. It certainly feels wrong for splitting to happen in multiple
places for (I think) a single request.

I'll defer the decision whether to backport this until we've settled both
of the above.

Jan



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

Re: [PATCH v2 2/2] x86/hvm/emulate: make sure rep I/O emulation does not cross GFN boundaries

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 16 August 2018 08:34
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: Re: [PATCH v2 2/2] x86/hvm/emulate: make sure rep I/O emulation
> does not cross GFN boundaries
> 
> >>> On 10.08.18 at 16:48, <paul.durrant@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -184,6 +184,25 @@ static int hvmemul_do_io(
> >          hvmtrace_io_assist(&p);
> >      }
> >
> > +    /*
> > +     * Make sure that we truncate rep MMIO at any GFN boundary. This is
> > +     * necessary to ensure that the correct device model is targetted
> > +     * or that we correctly handle a rep op spanning MMIO and RAM.
> > +     */
> > +    if ( unlikely(p.count > 1) && p.type == IOREQ_TYPE_COPY )
> > +    {
> > +        unsigned long off = p.addr & ~PAGE_MASK;
> > +
> > +        if ( PAGE_SIZE - off < p.size ) /* single rep spans GFN */
> > +            p.count = 1;
> > +        else
> > +            p.count = min_t(unsigned long,
> > +                            p.count,
> > +                            ((p.df ? (off + p.size) : (PAGE_SIZE - off)) /
> > +                             p.size));
> > +    }
> > +    ASSERT(p.count);
> 
> I've applied the patch with the earlier suggested changes, but I wonder
> if the placement especially wrt the visible in context hvmtrace_io_assist()
> isn't sub-optimal.
> 
> Considering the other splitting done elsewhere I also wonder whether
> some of that can't then go away, or whether the specific code path
> where you've found the splitting to be missing wouldn't better be fixed
> instead. It certainly feels wrong for splitting to happen in multiple
> places for (I think) a single request.

Agreed. The issue appears to be that the higher layers in emulation are coded to split across a page boundary for the 'definitely memory' operand of a rep mov but not the mmio operand.

  Paul

> 
> I'll defer the decision whether to backport this until we've settled both
> of the above.
> 
> Jan
> 


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