All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context()
@ 2017-11-23 15:09 Jan Beulich
  2017-11-23 15:22 ` Razvan Cojocaru
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Jan Beulich @ 2017-11-23 15:09 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, Paul Durrant, Razvan Cojocaru

There were two issues with this function: Its use of
hvmemul_do_pio_buffer() was wrong (the function deals only with
individual port accesses, not repeated ones, i.e. passing it
"*reps * bytes_per_rep" does not have the intended effect). And it
could have processed a larger set of operations in one go than was
probably intended (limited just by the size that xmalloc() can hand
back).

By converting to proper use of hvmemul_do_pio_buffer(), no intermediate
buffer is needed at all. As a result a preemption check is being added.

Also drop unused parameters from the function.

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

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1348,28 +1348,41 @@ static int hvmemul_rep_ins(
 }
 
 static int hvmemul_rep_outs_set_context(
-    enum x86_segment src_seg,
-    unsigned long src_offset,
     uint16_t dst_port,
     unsigned int bytes_per_rep,
-    unsigned long *reps,
-    struct x86_emulate_ctxt *ctxt)
+    unsigned long *reps)
 {
-    unsigned int bytes = *reps * bytes_per_rep;
-    char *buf;
-    int rc;
-
-    buf = xmalloc_array(char, bytes);
+    const struct arch_vm_event *ev = current->arch.vm_event;
+    const uint8_t *ptr;
+    unsigned int avail;
+    unsigned long done;
+    int rc = X86EMUL_OKAY;
 
-    if ( buf == NULL )
+    ASSERT(bytes_per_rep <= 4);
+    if ( !ev )
         return X86EMUL_UNHANDLEABLE;
 
-    rc = set_context_data(buf, bytes);
+    ptr = ev->emul.read.data;
+    avail = ev->emul.read.size;
 
-    if ( rc == X86EMUL_OKAY )
-        rc = hvmemul_do_pio_buffer(dst_port, bytes, IOREQ_WRITE, buf);
+    for ( done = 0; done < *reps; ++done )
+    {
+        unsigned int size = min(bytes_per_rep, avail);
+        uint32_t data = 0;
+
+        if ( done && hypercall_preempt_check() )
+            break;
+
+        memcpy(&data, ptr, size);
+        avail -= size;
+        ptr += size;
+
+        rc = hvmemul_do_pio_buffer(dst_port, bytes_per_rep, IOREQ_WRITE, &data);
+        if ( rc != X86EMUL_OKAY )
+            break;
+    }
 
-    xfree(buf);
+    *reps = done;
 
     return rc;
 }
@@ -1391,8 +1404,7 @@ static int hvmemul_rep_outs(
     int rc;
 
     if ( unlikely(hvmemul_ctxt->set_context) )
-        return hvmemul_rep_outs_set_context(src_seg, src_offset, dst_port,
-                                            bytes_per_rep, reps, ctxt);
+        return hvmemul_rep_outs_set_context(dst_port, bytes_per_rep, reps);
 
     rc = hvmemul_virtual_to_linear(
         src_seg, src_offset, bytes_per_rep, reps, hvm_access_read,




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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context()
  2017-11-23 15:09 [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context() Jan Beulich
@ 2017-11-23 15:22 ` Razvan Cojocaru
  2017-11-23 18:37 ` Andrew Cooper
  2017-12-04 10:12 ` Ping: [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context() Jan Beulich
  2 siblings, 0 replies; 6+ messages in thread
From: Razvan Cojocaru @ 2017-11-23 15:22 UTC (permalink / raw)
  To: Jan Beulich, xen-devel; +Cc: Julien Grall, Paul Durrant

On 11/23/2017 05:09 PM, Jan Beulich wrote:
> There were two issues with this function: Its use of
> hvmemul_do_pio_buffer() was wrong (the function deals only with
> individual port accesses, not repeated ones, i.e. passing it
> "*reps * bytes_per_rep" does not have the intended effect). And it
> could have processed a larger set of operations in one go than was
> probably intended (limited just by the size that xmalloc() can hand
> back).
> 
> By converting to proper use of hvmemul_do_pio_buffer(), no intermediate
> buffer is needed at all. As a result a preemption check is being added.
> 
> Also drop unused parameters from the function.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Thank you for the patch!

FWIW, Reviewed-by: Razvan Cojocaru <rcojocaru@bitdefender.com>


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context()
  2017-11-23 15:09 [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context() Jan Beulich
  2017-11-23 15:22 ` Razvan Cojocaru
@ 2017-11-23 18:37 ` Andrew Cooper
  2017-11-24  7:55   ` MMIO emulation failure on REP OUTS (was: [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context()) Jan Beulich
  2017-12-04 10:12 ` Ping: [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context() Jan Beulich
  2 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2017-11-23 18:37 UTC (permalink / raw)
  To: Jan Beulich, xen-devel; +Cc: Julien Grall, Paul Durrant, Razvan Cojocaru

[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]

On 23/11/17 15:09, Jan Beulich wrote:
> There were two issues with this function: Its use of
> hvmemul_do_pio_buffer() was wrong (the function deals only with
> individual port accesses, not repeated ones, i.e. passing it
> "*reps * bytes_per_rep" does not have the intended effect). And it
> could have processed a larger set of operations in one go than was
> probably intended (limited just by the size that xmalloc() can hand
> back).
>
> By converting to proper use of hvmemul_do_pio_buffer(), no intermediate
> buffer is needed at all. As a result a preemption check is being added.
>
> Also drop unused parameters from the function.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

While this does look like real bug, and bugfix, it isn't the issue I'm
hitting.  I've distilled the repro scenario down to a tiny XTF test,
which is just a `rep outsb` with a buffer which crosses a page boundary.

The results are reliably:

(d1) --- Xen Test Framework ---
(d1) Environment: HVM 32bit (No paging)
(d1) Test hvm-print
(d1) String crossing a page boundary
(XEN) MMIO emulation failed (1): d1v0 32bit @ 0010:001032b0 -> 5e c3 8d
b4 26 00 00 00 00 8d bc 27 00 00 00 00
(d1) Test result: SUCCESS

The Port IO hits a retry because of hitting the page boundary, and the
retry logic successes, as evident by all data hitting hvm_print_line(). 
Somewhere however, the PIO turns into MMIO, and a failure is reported
after the PIO completed successfully.  %rip in the failure message
points after the `rep outsb`, rather than at it.

If anyone has any ideas, I'm all ears.  If not, I will try to find some
time to look deeper into the issue.

~Andrew

[-- Attachment #2: 0001-MMIO-failure-trigger.patch --]
[-- Type: text/x-patch, Size: 1674 bytes --]

>From 9141a36374f52434a291e3be41bd259cfb9bda72 Mon Sep 17 00:00:00 2001
From: Andrew Cooper <andrew.cooper3@citrix.com>
Date: Thu, 23 Nov 2017 18:31:40 +0000
Subject: [PATCH] MMIO failure trigger

---
 tests/hvm-print/Makefile |  9 +++++++++
 tests/hvm-print/main.c   | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)
 create mode 100644 tests/hvm-print/Makefile
 create mode 100644 tests/hvm-print/main.c

diff --git a/tests/hvm-print/Makefile b/tests/hvm-print/Makefile
new file mode 100644
index 0000000..c70bede
--- /dev/null
+++ b/tests/hvm-print/Makefile
@@ -0,0 +1,9 @@
+include $(ROOT)/build/common.mk
+
+NAME      := hvm-print
+CATEGORY  := utility
+TEST-ENVS := hvm32
+
+obj-perenv += main.o
+
+include $(ROOT)/build/gen.mk
diff --git a/tests/hvm-print/main.c b/tests/hvm-print/main.c
new file mode 100644
index 0000000..882b716
--- /dev/null
+++ b/tests/hvm-print/main.c
@@ -0,0 +1,40 @@
+/**
+ * @file tests/hvm-print/main.c
+ * @ref test-hvm-print
+ *
+ * @page test-hvm-print hvm-print
+ *
+ * @todo Docs for test-hvm-print
+ *
+ * @see tests/hvm-print/main.c
+ */
+#include <xtf.h>
+
+const char test_title[] = "Test hvm-print";
+
+static char buf[2 * PAGE_SIZE] __page_aligned_bss;
+
+void test_main(void)
+{
+    char *ptr = &buf[4090];
+    size_t len;
+
+    strcpy(ptr, "String crossing a page boundary\n");
+    len = strlen(ptr);
+
+    asm volatile("rep; outsb"
+                 : "+S" (ptr), "+c" (len)
+                 : "d" (0xe9));
+
+    xtf_success(NULL);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.1.4


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: MMIO emulation failure on REP OUTS (was: [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context())
  2017-11-23 18:37 ` Andrew Cooper
@ 2017-11-24  7:55   ` Jan Beulich
  0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2017-11-24  7:55 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: xen-devel, Paul Durrant

(shrinking Cc list)

>>> On 23.11.17 at 19:37, <andrew.cooper3@citrix.com> wrote:
> On 23/11/17 15:09, Jan Beulich wrote:
>> There were two issues with this function: Its use of
>> hvmemul_do_pio_buffer() was wrong (the function deals only with
>> individual port accesses, not repeated ones, i.e. passing it
>> "*reps * bytes_per_rep" does not have the intended effect). And it
>> could have processed a larger set of operations in one go than was
>> probably intended (limited just by the size that xmalloc() can hand
>> back).
>>
>> By converting to proper use of hvmemul_do_pio_buffer(), no intermediate
>> buffer is needed at all. As a result a preemption check is being added.
>>
>> Also drop unused parameters from the function.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> While this does look like real bug, and bugfix, it isn't the issue I'm
> hitting.  I've distilled the repro scenario down to a tiny XTF test,
> which is just a `rep outsb` with a buffer which crosses a page boundary.
> 
> The results are reliably:
> 
> (d1) --- Xen Test Framework ---
> (d1) Environment: HVM 32bit (No paging)
> (d1) Test hvm-print
> (d1) String crossing a page boundary
> (XEN) MMIO emulation failed (1): d1v0 32bit @ 0010:001032b0 -> 5e c3 8d
> b4 26 00 00 00 00 8d bc 27 00 00 00 00
> (d1) Test result: SUCCESS
> 
> The Port IO hits a retry because of hitting the page boundary, and the
> retry logic successes, as evident by all data hitting hvm_print_line(). 
> Somewhere however, the PIO turns into MMIO, and a failure is reported
> after the PIO completed successfully.  %rip in the failure message
> points after the `rep outsb`, rather than at it.
> 
> If anyone has any ideas, I'm all ears.  If not, I will try to find some
> time to look deeper into the issue.

The failure being UNHANDLEABLE, I have another possibility in
mind: What if there's a bogus extra retry attempt after the REP
OUTS was already handled? The POP which is the next insn
would result in x86_insn_is_mem_access() returning false (its
memory access is an implicit stack one, which the function
intentionally produces false for). I'll see if I can find time later
today to debug this a little - thanks for shrinking it down to an
XTF test.

Jan


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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Ping: [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context()
  2017-11-23 15:09 [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context() Jan Beulich
  2017-11-23 15:22 ` Razvan Cojocaru
  2017-11-23 18:37 ` Andrew Cooper
@ 2017-12-04 10:12 ` Jan Beulich
  2017-12-04 10:33   ` Paul Durrant
  2 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-12-04 10:12 UTC (permalink / raw)
  To: Paul Durrant; +Cc: xen-devel, Razvan Cojocaru

>>> On 23.11.17 at 16:09, <JBeulich@suse.com> wrote:
> There were two issues with this function: Its use of
> hvmemul_do_pio_buffer() was wrong (the function deals only with
> individual port accesses, not repeated ones, i.e. passing it
> "*reps * bytes_per_rep" does not have the intended effect). And it
> could have processed a larger set of operations in one go than was
> probably intended (limited just by the size that xmalloc() can hand
> back).
> 
> By converting to proper use of hvmemul_do_pio_buffer(), no intermediate
> buffer is needed at all. As a result a preemption check is being added.
> 
> Also drop unused parameters from the function.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Ping?

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1348,28 +1348,41 @@ static int hvmemul_rep_ins(
>  }
>  
>  static int hvmemul_rep_outs_set_context(
> -    enum x86_segment src_seg,
> -    unsigned long src_offset,
>      uint16_t dst_port,
>      unsigned int bytes_per_rep,
> -    unsigned long *reps,
> -    struct x86_emulate_ctxt *ctxt)
> +    unsigned long *reps)
>  {
> -    unsigned int bytes = *reps * bytes_per_rep;
> -    char *buf;
> -    int rc;
> -
> -    buf = xmalloc_array(char, bytes);
> +    const struct arch_vm_event *ev = current->arch.vm_event;
> +    const uint8_t *ptr;
> +    unsigned int avail;
> +    unsigned long done;
> +    int rc = X86EMUL_OKAY;
>  
> -    if ( buf == NULL )
> +    ASSERT(bytes_per_rep <= 4);
> +    if ( !ev )
>          return X86EMUL_UNHANDLEABLE;
>  
> -    rc = set_context_data(buf, bytes);
> +    ptr = ev->emul.read.data;
> +    avail = ev->emul.read.size;
>  
> -    if ( rc == X86EMUL_OKAY )
> -        rc = hvmemul_do_pio_buffer(dst_port, bytes, IOREQ_WRITE, buf);
> +    for ( done = 0; done < *reps; ++done )
> +    {
> +        unsigned int size = min(bytes_per_rep, avail);
> +        uint32_t data = 0;
> +
> +        if ( done && hypercall_preempt_check() )
> +            break;
> +
> +        memcpy(&data, ptr, size);
> +        avail -= size;
> +        ptr += size;
> +
> +        rc = hvmemul_do_pio_buffer(dst_port, bytes_per_rep, IOREQ_WRITE, 
> &data);
> +        if ( rc != X86EMUL_OKAY )
> +            break;
> +    }
>  
> -    xfree(buf);
> +    *reps = done;
>  
>      return rc;
>  }
> @@ -1391,8 +1404,7 @@ static int hvmemul_rep_outs(
>      int rc;
>  
>      if ( unlikely(hvmemul_ctxt->set_context) )
> -        return hvmemul_rep_outs_set_context(src_seg, src_offset, dst_port,
> -                                            bytes_per_rep, reps, ctxt);
> +        return hvmemul_rep_outs_set_context(dst_port, bytes_per_rep, reps);
>  
>      rc = hvmemul_virtual_to_linear(
>          src_seg, src_offset, bytes_per_rep, reps, hvm_access_read,
> 
> 
> 
> 
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: Ping: [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context()
  2017-12-04 10:12 ` Ping: [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context() Jan Beulich
@ 2017-12-04 10:33   ` Paul Durrant
  0 siblings, 0 replies; 6+ messages in thread
From: Paul Durrant @ 2017-12-04 10:33 UTC (permalink / raw)
  To: 'Jan Beulich'; +Cc: xen-devel, Razvan Cojocaru

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 04 December 2017 10:13
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: Ping: [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context()
> 
> >>> On 23.11.17 at 16:09, <JBeulich@suse.com> wrote:
> > There were two issues with this function: Its use of
> > hvmemul_do_pio_buffer() was wrong (the function deals only with
> > individual port accesses, not repeated ones, i.e. passing it
> > "*reps * bytes_per_rep" does not have the intended effect). And it
> > could have processed a larger set of operations in one go than was
> > probably intended (limited just by the size that xmalloc() can hand
> > back).
> >
> > By converting to proper use of hvmemul_do_pio_buffer(), no
> intermediate
> > buffer is needed at all. As a result a preemption check is being added.
> >
> > Also drop unused parameters from the function.
> >
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Ping?

Apologies. In the midst of the discussion of the issue Andrew was hitting, I forgot about this one.

> 
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -1348,28 +1348,41 @@ static int hvmemul_rep_ins(
> >  }
> >
> >  static int hvmemul_rep_outs_set_context(
> > -    enum x86_segment src_seg,
> > -    unsigned long src_offset,
> >      uint16_t dst_port,
> >      unsigned int bytes_per_rep,
> > -    unsigned long *reps,
> > -    struct x86_emulate_ctxt *ctxt)
> > +    unsigned long *reps)
> >  {
> > -    unsigned int bytes = *reps * bytes_per_rep;
> > -    char *buf;
> > -    int rc;
> > -
> > -    buf = xmalloc_array(char, bytes);
> > +    const struct arch_vm_event *ev = current->arch.vm_event;
> > +    const uint8_t *ptr;
> > +    unsigned int avail;
> > +    unsigned long done;
> > +    int rc = X86EMUL_OKAY;
> >
> > -    if ( buf == NULL )
> > +    ASSERT(bytes_per_rep <= 4);
> > +    if ( !ev )
> >          return X86EMUL_UNHANDLEABLE;
> >
> > -    rc = set_context_data(buf, bytes);
> > +    ptr = ev->emul.read.data;
> > +    avail = ev->emul.read.size;
> >
> > -    if ( rc == X86EMUL_OKAY )
> > -        rc = hvmemul_do_pio_buffer(dst_port, bytes, IOREQ_WRITE, buf);
> > +    for ( done = 0; done < *reps; ++done )
> > +    {
> > +        unsigned int size = min(bytes_per_rep, avail);
> > +        uint32_t data = 0;
> > +
> > +        if ( done && hypercall_preempt_check() )
> > +            break;
> > +
> > +        memcpy(&data, ptr, size);
> > +        avail -= size;
> > +        ptr += size;
> > +
> > +        rc = hvmemul_do_pio_buffer(dst_port, bytes_per_rep,
> IOREQ_WRITE,
> > &data);
> > +        if ( rc != X86EMUL_OKAY )
> > +            break;
> > +    }

This is a sub-optimal way to be handling this, but I don't see a lot of choice. It would, of course, be nicer to have the context buffer be mappable by QEMU then it could handle this as a single ioreq with count > 1 and indirect data but that would involve fixing the current problem of who owns the guest memory map so...

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

> >
> > -    xfree(buf);
> > +    *reps = done;
> >
> >      return rc;
> >  }
> > @@ -1391,8 +1404,7 @@ static int hvmemul_rep_outs(
> >      int rc;
> >
> >      if ( unlikely(hvmemul_ctxt->set_context) )
> > -        return hvmemul_rep_outs_set_context(src_seg, src_offset, dst_port,
> > -                                            bytes_per_rep, reps, ctxt);
> > +        return hvmemul_rep_outs_set_context(dst_port, bytes_per_rep,
> reps);
> >
> >      rc = hvmemul_virtual_to_linear(
> >          src_seg, src_offset, bytes_per_rep, reps, hvm_access_read,
> >
> >
> >
> >
> > _______________________________________________
> > 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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-04 10:34 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-23 15:09 [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context() Jan Beulich
2017-11-23 15:22 ` Razvan Cojocaru
2017-11-23 18:37 ` Andrew Cooper
2017-11-24  7:55   ` MMIO emulation failure on REP OUTS (was: [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context()) Jan Beulich
2017-12-04 10:12 ` Ping: [PATCH] x86/HVM: fix hvmemul_rep_outs_set_context() Jan Beulich
2017-12-04 10:33   ` Paul Durrant

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.