[PATCH V3 0/2] Fix vm_event resume path race condition

[PATCH V3 0/2] Fix vm_event resume path race condition

[Razvan Cojocaru]

This small series first creates hvm/vm_event.{h,c}, in order to bring
under vm_event maintainership the code that has previously lived in
hvm_do_resume(), and then fixes a __context_switch()-related race
condition when attempting to set registers via vm_event reply.

[PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h,c}
[PATCH V3 2/2] x86/vm_event: fix race between __context_switch() and


Thanks,
Razvan

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

[PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h,c}

[Razvan Cojocaru]

Created arch/x86/hvm/vm_event.c and include/asm-x86/hvm/vm_event.h,
where HVM-specific vm_event-related code will live. This cleans up
hvm_do_resume() and ensures that the vm_event maintainers are
responsible for changes to that code.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

---
Changes since V2:
 - Added if ( unlikely(v->arch.vm_event) ) before the hvm_do_resume()
   call.
 - Added unlikely()s in hvm_vm_event_do_resume() for the register
   write cases.
 - Moved the copyright lines over from hvm.c.
---
 MAINTAINERS                        |   2 +
 xen/arch/x86/hvm/Makefile          |   1 +
 xen/arch/x86/hvm/hvm.c             |  63 +----------------------
 xen/arch/x86/hvm/vm_event.c        | 103 +++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/hvm/vm_event.h |  34 ++++++++++++
 5 files changed, 142 insertions(+), 61 deletions(-)
 create mode 100644 xen/arch/x86/hvm/vm_event.c
 create mode 100644 xen/include/asm-x86/hvm/vm_event.h

diff --git a/MAINTAINERS b/MAINTAINERS
index c345a50..10863bc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -404,6 +404,7 @@ S:	Supported
 F:	tools/tests/xen-access
 F:	xen/arch/*/monitor.c
 F:	xen/arch/*/vm_event.c
+F:	xen/arch/*/hvm/vm_event.c
 F:	xen/arch/arm/mem_access.c
 F:	xen/arch/x86/mm/mem_access.c
 F:	xen/arch/x86/hvm/monitor.c
@@ -413,6 +414,7 @@ F:	xen/common/vm_event.c
 F:	xen/include/*/mem_access.h
 F:	xen/include/*/monitor.h
 F:	xen/include/*/vm_event.h
+F:	xen/include/*/hvm/vm_event.h
 F:	xen/include/asm-x86/hvm/monitor.h
 
 VTPM
diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 0a3d0f4..02f0add 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -24,6 +24,7 @@ obj-y += stdvga.o
 obj-y += vioapic.o
 obj-y += viridian.o
 obj-y += vlapic.o
+obj-y += vm_event.o
 obj-y += vmsi.o
 obj-y += vpic.o
 obj-y += vpt.o
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index a441955..81691e2 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -34,7 +34,6 @@
 #include <xen/wait.h>
 #include <xen/mem_access.h>
 #include <xen/rangeset.h>
-#include <xen/vm_event.h>
 #include <xen/monitor.h>
 #include <xen/warning.h>
 #include <asm/shadow.h>
@@ -61,6 +60,7 @@
 #include <asm/hvm/nestedhvm.h>
 #include <asm/hvm/monitor.h>
 #include <asm/hvm/ioreq.h>
+#include <asm/hvm/vm_event.h>
 #include <asm/altp2m.h>
 #include <asm/mtrr.h>
 #include <asm/apic.h>
@@ -484,66 +484,7 @@ void hvm_do_resume(struct vcpu *v)
         return;
 
     if ( unlikely(v->arch.vm_event) )
-    {
-        struct monitor_write_data *w = &v->arch.vm_event->write_data;
-
-        if ( unlikely(v->arch.vm_event->emulate_flags) )
-        {
-            enum emul_kind kind = EMUL_KIND_NORMAL;
-
-            /*
-             * Please observ the order here to match the flag descriptions
-             * provided in public/vm_event.h
-             */
-            if ( v->arch.vm_event->emulate_flags &
-                 VM_EVENT_FLAG_SET_EMUL_READ_DATA )
-                kind = EMUL_KIND_SET_CONTEXT_DATA;
-            else if ( v->arch.vm_event->emulate_flags &
-                      VM_EVENT_FLAG_EMULATE_NOWRITE )
-                kind = EMUL_KIND_NOWRITE;
-            else if ( v->arch.vm_event->emulate_flags &
-                      VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
-                kind = EMUL_KIND_SET_CONTEXT_INSN;
-
-            hvm_emulate_one_vm_event(kind, TRAP_invalid_op,
-                                     X86_EVENT_NO_EC);
-
-            v->arch.vm_event->emulate_flags = 0;
-        }
-
-        if ( w->do_write.msr )
-        {
-            if ( hvm_msr_write_intercept(w->msr, w->value, 0) ==
-                 X86EMUL_EXCEPTION )
-                hvm_inject_hw_exception(TRAP_gp_fault, 0);
-
-            w->do_write.msr = 0;
-        }
-
-        if ( w->do_write.cr0 )
-        {
-            if ( hvm_set_cr0(w->cr0, 0) == X86EMUL_EXCEPTION )
-                hvm_inject_hw_exception(TRAP_gp_fault, 0);
-
-            w->do_write.cr0 = 0;
-        }
-
-        if ( w->do_write.cr4 )
-        {
-            if ( hvm_set_cr4(w->cr4, 0) == X86EMUL_EXCEPTION )
-                hvm_inject_hw_exception(TRAP_gp_fault, 0);
-
-            w->do_write.cr4 = 0;
-        }
-
-        if ( w->do_write.cr3 )
-        {
-            if ( hvm_set_cr3(w->cr3, 0) == X86EMUL_EXCEPTION )
-                hvm_inject_hw_exception(TRAP_gp_fault, 0);
-
-            w->do_write.cr3 = 0;
-        }
-    }
+        hvm_vm_event_do_resume(v);
 
     /* Inject pending hw/sw event */
     if ( v->arch.hvm_vcpu.inject_event.vector >= 0 )
diff --git a/xen/arch/x86/hvm/vm_event.c b/xen/arch/x86/hvm/vm_event.c
new file mode 100644
index 0000000..1934556
--- /dev/null
+++ b/xen/arch/x86/hvm/vm_event.c
@@ -0,0 +1,103 @@
+/*
+ * arch/x86/hvm/vm_event.c
+ *
+ * HVM vm_event handling routines
+ *
+ * Copyright (c) 2004, Intel Corporation.
+ * Copyright (c) 2005, International Business Machines Corporation.
+ * Copyright (c) 2008, Citrix Systems, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/sched.h>
+#include <xen/vm_event.h>
+#include <asm/hvm/support.h>
+#include <asm/vm_event.h>
+
+void hvm_vm_event_do_resume(struct vcpu *v)
+{
+    struct monitor_write_data *w;
+
+    if ( likely(!v->arch.vm_event) )
+        return;
+
+    w = &v->arch.vm_event->write_data;
+
+    if ( unlikely(v->arch.vm_event->emulate_flags) )
+    {
+        enum emul_kind kind = EMUL_KIND_NORMAL;
+
+        /*
+         * Please observe the order here to match the flag descriptions
+         * provided in public/vm_event.h
+         */
+        if ( v->arch.vm_event->emulate_flags &
+             VM_EVENT_FLAG_SET_EMUL_READ_DATA )
+            kind = EMUL_KIND_SET_CONTEXT_DATA;
+        else if ( v->arch.vm_event->emulate_flags &
+                  VM_EVENT_FLAG_EMULATE_NOWRITE )
+            kind = EMUL_KIND_NOWRITE;
+        else if ( v->arch.vm_event->emulate_flags &
+                  VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
+            kind = EMUL_KIND_SET_CONTEXT_INSN;
+
+        hvm_emulate_one_vm_event(kind, TRAP_invalid_op,
+                                 X86_EVENT_NO_EC);
+
+        v->arch.vm_event->emulate_flags = 0;
+    }
+
+    if ( unlikely(w->do_write.cr0) )
+    {
+        if ( hvm_set_cr0(w->cr0, 0) == X86EMUL_EXCEPTION )
+            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+
+        w->do_write.cr0 = 0;
+    }
+
+    if ( unlikely(w->do_write.cr4) )
+    {
+        if ( hvm_set_cr4(w->cr4, 0) == X86EMUL_EXCEPTION )
+            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+
+        w->do_write.cr4 = 0;
+    }
+
+    if ( unlikely(w->do_write.cr3) )
+    {
+        if ( hvm_set_cr3(w->cr3, 0) == X86EMUL_EXCEPTION )
+            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+
+        w->do_write.cr3 = 0;
+    }
+
+    if ( unlikely(w->do_write.msr) )
+    {
+        if ( hvm_msr_write_intercept(w->msr, w->value, 0) ==
+             X86EMUL_EXCEPTION )
+            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+
+        w->do_write.msr = 0;
+    }
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-x86/hvm/vm_event.h b/xen/include/asm-x86/hvm/vm_event.h
new file mode 100644
index 0000000..28cb07c
--- /dev/null
+++ b/xen/include/asm-x86/hvm/vm_event.h
@@ -0,0 +1,34 @@
+/*
+ * include/asm-x86/hvm/vm_event.h
+ *
+ * Hardware virtual machine vm_event abstractions.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __ASM_X86_HVM_VM_EVENT_H__
+#define __ASM_X86_HVM_VM_EVENT_H__
+
+void hvm_vm_event_do_resume(struct vcpu *v);
+
+#endif /* __ASM_X86_HVM_VM_EVENT_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.9.1


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

Re: [PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h, c}

[Jan Beulich]

>>> On 04.05.17 at 11:00, <rcojocaru@bitdefender.com> wrote:
> Created arch/x86/hvm/vm_event.c and include/asm-x86/hvm/vm_event.h,
> where HVM-specific vm_event-related code will live. This cleans up
> hvm_do_resume() and ensures that the vm_event maintainers are
> responsible for changes to that code.
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
albeit I wonder ...

> +void hvm_vm_event_do_resume(struct vcpu *v)
> +{
> +    struct monitor_write_data *w;
> +
> +    if ( likely(!v->arch.vm_event) )
> +        return;

... whether this now wouldn't better be an ASSERT().

Jan


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

Re: [PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h, c}

[Razvan Cojocaru]

On 05/04/17 12:11, Jan Beulich wrote:
>>>> On 04.05.17 at 11:00, <rcojocaru@bitdefender.com> wrote:
>> Created arch/x86/hvm/vm_event.c and include/asm-x86/hvm/vm_event.h,
>> where HVM-specific vm_event-related code will live. This cleans up
>> hvm_do_resume() and ensures that the vm_event maintainers are
>> responsible for changes to that code.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> albeit I wonder ...
> 
>> +void hvm_vm_event_do_resume(struct vcpu *v)
>> +{
>> +    struct monitor_write_data *w;
>> +
>> +    if ( likely(!v->arch.vm_event) )
>> +        return;
> 
> ... whether this now wouldn't better be an ASSERT().

I have no objections (can this be done on commit or should I re-send V4?).


Thanks,
Razvan

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

Re: [PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h, c}

[Jan Beulich]

>>> On 04.05.17 at 11:14, <rcojocaru@bitdefender.com> wrote:
> On 05/04/17 12:11, Jan Beulich wrote:
>>>>> On 04.05.17 at 11:00, <rcojocaru@bitdefender.com> wrote:
>>> Created arch/x86/hvm/vm_event.c and include/asm-x86/hvm/vm_event.h,
>>> where HVM-specific vm_event-related code will live. This cleans up
>>> hvm_do_resume() and ensures that the vm_event maintainers are
>>> responsible for changes to that code.
>>>
>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
>> 
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>> albeit I wonder ...
>> 
>>> +void hvm_vm_event_do_resume(struct vcpu *v)
>>> +{
>>> +    struct monitor_write_data *w;
>>> +
>>> +    if ( likely(!v->arch.vm_event) )
>>> +        return;
>> 
>> ... whether this now wouldn't better be an ASSERT().
> 
> I have no objections (can this be done on commit or should I re-send V4?).

Let's first see what Tamas thinks. If he agrees, I see not problem
doing the adjustment while committing.

Jan


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

Re: [PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h, c}

[Tamas K Lengyel]

On Thu, May 4, 2017 at 5:22 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 04.05.17 at 11:14, <rcojocaru@bitdefender.com> wrote:
>> On 05/04/17 12:11, Jan Beulich wrote:
>>>>>> On 04.05.17 at 11:00, <rcojocaru@bitdefender.com> wrote:
>>>> Created arch/x86/hvm/vm_event.c and include/asm-x86/hvm/vm_event.h,
>>>> where HVM-specific vm_event-related code will live. This cleans up
>>>> hvm_do_resume() and ensures that the vm_event maintainers are
>>>> responsible for changes to that code.
>>>>
>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
>>>
>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>> albeit I wonder ...
>>>
>>>> +void hvm_vm_event_do_resume(struct vcpu *v)
>>>> +{
>>>> +    struct monitor_write_data *w;
>>>> +
>>>> +    if ( likely(!v->arch.vm_event) )
>>>> +        return;
>>>
>>> ... whether this now wouldn't better be an ASSERT().
>>
>> I have no objections (can this be done on commit or should I re-send V4?).
>
> Let's first see what Tamas thinks. If he agrees, I see not problem
> doing the adjustment while committing.

I'm not quite sure how converting that to an ASSERT would work. It
looks fine to me as is tbh.

Tamas

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

Re: [PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h, c}

[Razvan Cojocaru]

On 05/04/17 17:57, Tamas K Lengyel wrote:
> On Thu, May 4, 2017 at 5:22 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 04.05.17 at 11:14, <rcojocaru@bitdefender.com> wrote:
>>> On 05/04/17 12:11, Jan Beulich wrote:
>>>>>>> On 04.05.17 at 11:00, <rcojocaru@bitdefender.com> wrote:
>>>>> Created arch/x86/hvm/vm_event.c and include/asm-x86/hvm/vm_event.h,
>>>>> where HVM-specific vm_event-related code will live. This cleans up
>>>>> hvm_do_resume() and ensures that the vm_event maintainers are
>>>>> responsible for changes to that code.
>>>>>
>>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>>> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
>>>>
>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>> albeit I wonder ...
>>>>
>>>>> +void hvm_vm_event_do_resume(struct vcpu *v)
>>>>> +{
>>>>> +    struct monitor_write_data *w;
>>>>> +
>>>>> +    if ( likely(!v->arch.vm_event) )
>>>>> +        return;
>>>>
>>>> ... whether this now wouldn't better be an ASSERT().
>>>
>>> I have no objections (can this be done on commit or should I re-send V4?).
>>
>> Let's first see what Tamas thinks. If he agrees, I see not problem
>> doing the adjustment while committing.
> 
> I'm not quite sure how converting that to an ASSERT would work. It
> looks fine to me as is tbh.

I think Jan means that, since currently the only caller is
hvm_do_resume() where there's already that check now (to avoid the
call), we could here simply replace the if() with
ASSERT(v->arch.vm_event). I could be wrong. :)


Thanks,
Razvan

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

Re: [PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h, c}

[Tamas K Lengyel]

On Thu, May 4, 2017 at 11:17 AM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 05/04/17 17:57, Tamas K Lengyel wrote:
>> On Thu, May 4, 2017 at 5:22 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 04.05.17 at 11:14, <rcojocaru@bitdefender.com> wrote:
>>>> On 05/04/17 12:11, Jan Beulich wrote:
>>>>>>>> On 04.05.17 at 11:00, <rcojocaru@bitdefender.com> wrote:
>>>>>> Created arch/x86/hvm/vm_event.c and include/asm-x86/hvm/vm_event.h,
>>>>>> where HVM-specific vm_event-related code will live. This cleans up
>>>>>> hvm_do_resume() and ensures that the vm_event maintainers are
>>>>>> responsible for changes to that code.
>>>>>>
>>>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>>>> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
>>>>>
>>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>> albeit I wonder ...
>>>>>
>>>>>> +void hvm_vm_event_do_resume(struct vcpu *v)
>>>>>> +{
>>>>>> +    struct monitor_write_data *w;
>>>>>> +
>>>>>> +    if ( likely(!v->arch.vm_event) )
>>>>>> +        return;
>>>>>
>>>>> ... whether this now wouldn't better be an ASSERT().
>>>>
>>>> I have no objections (can this be done on commit or should I re-send V4?).
>>>
>>> Let's first see what Tamas thinks. If he agrees, I see not problem
>>> doing the adjustment while committing.
>>
>> I'm not quite sure how converting that to an ASSERT would work. It
>> looks fine to me as is tbh.
>
> I think Jan means that, since currently the only caller is
> hvm_do_resume() where there's already that check now (to avoid the
> call), we could here simply replace the if() with
> ASSERT(v->arch.vm_event). I could be wrong. :)
>

Ah OK, yea that would be fine.

Tamas

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

Re: [PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h, c}

[Jan Beulich]

>>> On 04.05.17 at 17:17, <rcojocaru@bitdefender.com> wrote:
> On 05/04/17 17:57, Tamas K Lengyel wrote:
>> On Thu, May 4, 2017 at 5:22 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 04.05.17 at 11:14, <rcojocaru@bitdefender.com> wrote:
>>>> On 05/04/17 12:11, Jan Beulich wrote:
>>>>>>>> On 04.05.17 at 11:00, <rcojocaru@bitdefender.com> wrote:
>>>>>> Created arch/x86/hvm/vm_event.c and include/asm-x86/hvm/vm_event.h,
>>>>>> where HVM-specific vm_event-related code will live. This cleans up
>>>>>> hvm_do_resume() and ensures that the vm_event maintainers are
>>>>>> responsible for changes to that code.
>>>>>>
>>>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>>>> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
>>>>>
>>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>> albeit I wonder ...
>>>>>
>>>>>> +void hvm_vm_event_do_resume(struct vcpu *v)
>>>>>> +{
>>>>>> +    struct monitor_write_data *w;
>>>>>> +
>>>>>> +    if ( likely(!v->arch.vm_event) )
>>>>>> +        return;
>>>>>
>>>>> ... whether this now wouldn't better be an ASSERT().
>>>>
>>>> I have no objections (can this be done on commit or should I re-send V4?).
>>>
>>> Let's first see what Tamas thinks. If he agrees, I see not problem
>>> doing the adjustment while committing.
>> 
>> I'm not quite sure how converting that to an ASSERT would work. It
>> looks fine to me as is tbh.
> 
> I think Jan means that, since currently the only caller is
> hvm_do_resume() where there's already that check now (to avoid the
> call), we could here simply replace the if() with
> ASSERT(v->arch.vm_event). I could be wrong. :)

You aren't - that's precisely my reasoning.

Jan


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

Re: [PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h, c}

[Tamas K Lengyel]

On Thu, May 4, 2017 at 11:37 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 04.05.17 at 17:17, <rcojocaru@bitdefender.com> wrote:
>> On 05/04/17 17:57, Tamas K Lengyel wrote:
>>> On Thu, May 4, 2017 at 5:22 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 04.05.17 at 11:14, <rcojocaru@bitdefender.com> wrote:
>>>>> On 05/04/17 12:11, Jan Beulich wrote:
>>>>>>>>> On 04.05.17 at 11:00, <rcojocaru@bitdefender.com> wrote:
>>>>>>> Created arch/x86/hvm/vm_event.c and include/asm-x86/hvm/vm_event.h,
>>>>>>> where HVM-specific vm_event-related code will live. This cleans up
>>>>>>> hvm_do_resume() and ensures that the vm_event maintainers are
>>>>>>> responsible for changes to that code.
>>>>>>>
>>>>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>>>>> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
>>>>>>
>>>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>>> albeit I wonder ...
>>>>>>
>>>>>>> +void hvm_vm_event_do_resume(struct vcpu *v)
>>>>>>> +{
>>>>>>> +    struct monitor_write_data *w;
>>>>>>> +
>>>>>>> +    if ( likely(!v->arch.vm_event) )
>>>>>>> +        return;
>>>>>>
>>>>>> ... whether this now wouldn't better be an ASSERT().
>>>>>
>>>>> I have no objections (can this be done on commit or should I re-send V4?).
>>>>
>>>> Let's first see what Tamas thinks. If he agrees, I see not problem
>>>> doing the adjustment while committing.
>>>
>>> I'm not quite sure how converting that to an ASSERT would work. It
>>> looks fine to me as is tbh.
>>
>> I think Jan means that, since currently the only caller is
>> hvm_do_resume() where there's already that check now (to avoid the
>> call), we could here simply replace the if() with
>> ASSERT(v->arch.vm_event). I could be wrong. :)
>
> You aren't - that's precisely my reasoning.

So if we are changing this to an ASSERT here then a check needs to be
added on the caller site. That would work for me.

Tamas

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

Re: [PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h, c}

[Jan Beulich]

>>> On 05.05.17 at 16:42, <tamas@tklengyel.com> wrote:
> On Thu, May 4, 2017 at 11:37 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 04.05.17 at 17:17, <rcojocaru@bitdefender.com> wrote:
>>> On 05/04/17 17:57, Tamas K Lengyel wrote:
>>>> On Thu, May 4, 2017 at 5:22 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 04.05.17 at 11:14, <rcojocaru@bitdefender.com> wrote:
>>>>>> On 05/04/17 12:11, Jan Beulich wrote:
>>>>>>>>>> On 04.05.17 at 11:00, <rcojocaru@bitdefender.com> wrote:
>>>>>>>> Created arch/x86/hvm/vm_event.c and include/asm-x86/hvm/vm_event.h,
>>>>>>>> where HVM-specific vm_event-related code will live. This cleans up
>>>>>>>> hvm_do_resume() and ensures that the vm_event maintainers are
>>>>>>>> responsible for changes to that code.
>>>>>>>>
>>>>>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>>>>>> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
>>>>>>>
>>>>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>>>> albeit I wonder ...
>>>>>>>
>>>>>>>> +void hvm_vm_event_do_resume(struct vcpu *v)
>>>>>>>> +{
>>>>>>>> +    struct monitor_write_data *w;
>>>>>>>> +
>>>>>>>> +    if ( likely(!v->arch.vm_event) )
>>>>>>>> +        return;
>>>>>>>
>>>>>>> ... whether this now wouldn't better be an ASSERT().
>>>>>>
>>>>>> I have no objections (can this be done on commit or should I re-send V4?).
>>>>>
>>>>> Let's first see what Tamas thinks. If he agrees, I see not problem
>>>>> doing the adjustment while committing.
>>>>
>>>> I'm not quite sure how converting that to an ASSERT would work. It
>>>> looks fine to me as is tbh.
>>>
>>> I think Jan means that, since currently the only caller is
>>> hvm_do_resume() where there's already that check now (to avoid the
>>> call), we could here simply replace the if() with
>>> ASSERT(v->arch.vm_event). I could be wrong. :)
>>
>> You aren't - that's precisely my reasoning.
> 
> So if we are changing this to an ASSERT here then a check needs to be
> added on the caller site. That would work for me.

I don't follow - the reason I did ask for converting the if() here
was because (upon my request) a check in the caller has been
added (or actually, is being kept from the original code instead
of deleting it) in this version.

Jan


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

Re: [PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h, c}

[Tamas K Lengyel]

On Fri, May 5, 2017 at 10:47 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 05.05.17 at 16:42, <tamas@tklengyel.com> wrote:
>> On Thu, May 4, 2017 at 11:37 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 04.05.17 at 17:17, <rcojocaru@bitdefender.com> wrote:
>>>> On 05/04/17 17:57, Tamas K Lengyel wrote:
>>>>> On Thu, May 4, 2017 at 5:22 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>> On 04.05.17 at 11:14, <rcojocaru@bitdefender.com> wrote:
>>>>>>> On 05/04/17 12:11, Jan Beulich wrote:
>>>>>>>>>>> On 04.05.17 at 11:00, <rcojocaru@bitdefender.com> wrote:
>>>>>>>>> Created arch/x86/hvm/vm_event.c and include/asm-x86/hvm/vm_event.h,
>>>>>>>>> where HVM-specific vm_event-related code will live. This cleans up
>>>>>>>>> hvm_do_resume() and ensures that the vm_event maintainers are
>>>>>>>>> responsible for changes to that code.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>>>>>>>>> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
>>>>>>>>
>>>>>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>>>>>>> albeit I wonder ...
>>>>>>>>
>>>>>>>>> +void hvm_vm_event_do_resume(struct vcpu *v)
>>>>>>>>> +{
>>>>>>>>> +    struct monitor_write_data *w;
>>>>>>>>> +
>>>>>>>>> +    if ( likely(!v->arch.vm_event) )
>>>>>>>>> +        return;
>>>>>>>>
>>>>>>>> ... whether this now wouldn't better be an ASSERT().
>>>>>>>
>>>>>>> I have no objections (can this be done on commit or should I re-send V4?).
>>>>>>
>>>>>> Let's first see what Tamas thinks. If he agrees, I see not problem
>>>>>> doing the adjustment while committing.
>>>>>
>>>>> I'm not quite sure how converting that to an ASSERT would work. It
>>>>> looks fine to me as is tbh.
>>>>
>>>> I think Jan means that, since currently the only caller is
>>>> hvm_do_resume() where there's already that check now (to avoid the
>>>> call), we could here simply replace the if() with
>>>> ASSERT(v->arch.vm_event). I could be wrong. :)
>>>
>>> You aren't - that's precisely my reasoning.
>>
>> So if we are changing this to an ASSERT here then a check needs to be
>> added on the caller site. That would work for me.
>
> I don't follow - the reason I did ask for converting the if() here
> was because (upon my request) a check in the caller has been
> added (or actually, is being kept from the original code instead
> of deleting it) in this version.

If there is already a check in the caller, then just go ahead and
convert this to an ASSERT.

Tamas

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

Re: [PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h, c}

[Jan Beulich]

>>> On 04.05.17 at 11:00, <rcojocaru@bitdefender.com> wrote:
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -404,6 +404,7 @@ S:	Supported
>  F:	tools/tests/xen-access
>  F:	xen/arch/*/monitor.c
>  F:	xen/arch/*/vm_event.c
> +F:	xen/arch/*/hvm/vm_event.c
>  F:	xen/arch/arm/mem_access.c
>  F:	xen/arch/x86/mm/mem_access.c
>  F:	xen/arch/x86/hvm/monitor.c
> @@ -413,6 +414,7 @@ F:	xen/common/vm_event.c
>  F:	xen/include/*/mem_access.h
>  F:	xen/include/*/monitor.h
>  F:	xen/include/*/vm_event.h
> +F:	xen/include/*/hvm/vm_event.h
>  F:	xen/include/asm-x86/hvm/monitor.h

Btw., I've noticed only now that these additions would better be
x86-specific, just like their hvm/monitor.[ch] counterparts. I intend
to adjust this while committing.

Jan


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

Re: [PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h, c}

[Razvan Cojocaru]

On 05/08/17 15:44, Jan Beulich wrote:
>>>> On 04.05.17 at 11:00, <rcojocaru@bitdefender.com> wrote:
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -404,6 +404,7 @@ S:	Supported
>>  F:	tools/tests/xen-access
>>  F:	xen/arch/*/monitor.c
>>  F:	xen/arch/*/vm_event.c
>> +F:	xen/arch/*/hvm/vm_event.c
>>  F:	xen/arch/arm/mem_access.c
>>  F:	xen/arch/x86/mm/mem_access.c
>>  F:	xen/arch/x86/hvm/monitor.c
>> @@ -413,6 +414,7 @@ F:	xen/common/vm_event.c
>>  F:	xen/include/*/mem_access.h
>>  F:	xen/include/*/monitor.h
>>  F:	xen/include/*/vm_event.h
>> +F:	xen/include/*/hvm/vm_event.h
>>  F:	xen/include/asm-x86/hvm/monitor.h
> 
> Btw., I've noticed only now that these additions would better be
> x86-specific, just like their hvm/monitor.[ch] counterparts. I intend
> to adjust this while committing.

Fair enough.


Thanks,
Razvan

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

[PATCH V3 2/2] x86/vm_event: fix race between __context_switch() and vm_event_resume()

[Razvan Cojocaru]

The introspection agent can reply to a vm_event faster than
vmx_vmexit_handler() can complete in some cases, where it is then
not safe for vm_event_set_registers() to modify v->arch.user_regs.
In the test scenario, we were stepping over an INT3 breakpoint by
setting RIP += 1. The quick reply tended to complete before the VCPU
triggering the introspection event had properly paused and been
descheduled. If the reply occurs before __context_switch() happens,
__context_switch() clobbers the reply by overwriting
v->arch.user_regs from the stack. If we don't pass through
__context_switch() (due to switching to the idle vCPU), reply data
wouldn't be picked up when switching back straight to the original
vCPU.

This patch ensures that vm_event_resume() code only sets per-VCPU
data to be used for the actual setting of registers later in
hvm_do_resume() (similar to the model used to control setting of CRs
and MSRs).

The patch additionally removes the sync_vcpu_execstate(v) call from
vm_event_resume(), which is no longer necessary, which removes the
associated broadcast TLB flush (read: performance improvement).

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

---
Changes since V2:
 - Corrected commit message.
---
 xen/arch/x86/hvm/vm_event.c    | 35 +++++++++++++++++++++++++++++++++++
 xen/arch/x86/vm_event.c        | 22 ++--------------------
 xen/common/vm_event.c          | 17 ++++++++++-------
 xen/include/asm-x86/vm_event.h |  2 ++
 4 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/xen/arch/x86/hvm/vm_event.c b/xen/arch/x86/hvm/vm_event.c
index 1934556..8eeb210 100644
--- a/xen/arch/x86/hvm/vm_event.c
+++ b/xen/arch/x86/hvm/vm_event.c
@@ -25,6 +25,39 @@
 #include <asm/hvm/support.h>
 #include <asm/vm_event.h>
 
+static void hvm_vm_event_set_registers(const struct vcpu *v)
+{
+    ASSERT(v == current);
+
+    if ( unlikely(v->arch.vm_event->set_gprs) )
+    {
+        struct cpu_user_regs *regs = guest_cpu_user_regs();
+
+        regs->rax = v->arch.vm_event->gprs.rax;
+        regs->rbx = v->arch.vm_event->gprs.rbx;
+        regs->rcx = v->arch.vm_event->gprs.rcx;
+        regs->rdx = v->arch.vm_event->gprs.rdx;
+        regs->rsp = v->arch.vm_event->gprs.rsp;
+        regs->rbp = v->arch.vm_event->gprs.rbp;
+        regs->rsi = v->arch.vm_event->gprs.rsi;
+        regs->rdi = v->arch.vm_event->gprs.rdi;
+
+        regs->r8 = v->arch.vm_event->gprs.r8;
+        regs->r9 = v->arch.vm_event->gprs.r9;
+        regs->r10 = v->arch.vm_event->gprs.r10;
+        regs->r11 = v->arch.vm_event->gprs.r11;
+        regs->r12 = v->arch.vm_event->gprs.r12;
+        regs->r13 = v->arch.vm_event->gprs.r13;
+        regs->r14 = v->arch.vm_event->gprs.r14;
+        regs->r15 = v->arch.vm_event->gprs.r15;
+
+        regs->rflags = v->arch.vm_event->gprs.rflags;
+        regs->rip = v->arch.vm_event->gprs.rip;
+
+        v->arch.vm_event->set_gprs = false;
+    }
+}
+
 void hvm_vm_event_do_resume(struct vcpu *v)
 {
     struct monitor_write_data *w;
@@ -32,6 +65,8 @@ void hvm_vm_event_do_resume(struct vcpu *v)
     if ( likely(!v->arch.vm_event) )
         return;
 
+    hvm_vm_event_set_registers(v);
+
     w = &v->arch.vm_event->write_data;
 
     if ( unlikely(v->arch.vm_event->emulate_flags) )
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 502ccc2..a6ea42c 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -113,26 +113,8 @@ void vm_event_set_registers(struct vcpu *v, vm_event_response_t *rsp)
 {
     ASSERT(atomic_read(&v->vm_event_pause_count));
 
-    v->arch.user_regs.rax = rsp->data.regs.x86.rax;
-    v->arch.user_regs.rbx = rsp->data.regs.x86.rbx;
-    v->arch.user_regs.rcx = rsp->data.regs.x86.rcx;
-    v->arch.user_regs.rdx = rsp->data.regs.x86.rdx;
-    v->arch.user_regs.rsp = rsp->data.regs.x86.rsp;
-    v->arch.user_regs.rbp = rsp->data.regs.x86.rbp;
-    v->arch.user_regs.rsi = rsp->data.regs.x86.rsi;
-    v->arch.user_regs.rdi = rsp->data.regs.x86.rdi;
-
-    v->arch.user_regs.r8 = rsp->data.regs.x86.r8;
-    v->arch.user_regs.r9 = rsp->data.regs.x86.r9;
-    v->arch.user_regs.r10 = rsp->data.regs.x86.r10;
-    v->arch.user_regs.r11 = rsp->data.regs.x86.r11;
-    v->arch.user_regs.r12 = rsp->data.regs.x86.r12;
-    v->arch.user_regs.r13 = rsp->data.regs.x86.r13;
-    v->arch.user_regs.r14 = rsp->data.regs.x86.r14;
-    v->arch.user_regs.r15 = rsp->data.regs.x86.r15;
-
-    v->arch.user_regs.rflags = rsp->data.regs.x86.rflags;
-    v->arch.user_regs.rip = rsp->data.regs.x86.rip;
+    v->arch.vm_event->gprs = rsp->data.regs.x86;
+    v->arch.vm_event->set_gprs = true;
 }
 
 void vm_event_monitor_next_interrupt(struct vcpu *v)
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 0fe9a53..9291db6 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -357,6 +357,16 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
 {
     vm_event_response_t rsp;
 
+    /*
+     * vm_event_resume() runs in either XEN_DOMCTL_VM_EVENT_OP_*, or
+     * EVTCHN_send context from the introspection consumer. Both contexts
+     * are guaranteed not to be the subject of vm_event responses.
+     * While we could ASSERT(v != current) for each VCPU in d in the loop
+     * below, this covers the case where we would need to iterate over all
+     * of them more succintly.
+     */
+    ASSERT(d != current->domain);
+
     /* Pull all responses off the ring. */
     while ( vm_event_get_response(d, ved, &rsp) )
     {
@@ -375,13 +385,6 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
         v = d->vcpu[rsp.vcpu_id];
 
         /*
-         * Make sure the vCPU state has been synchronized for the custom
-         * handlers.
-         */
-        if ( atomic_read(&v->vm_event_pause_count) )
-            sync_vcpu_execstate(v);
-
-        /*
          * In some cases the response type needs extra handling, so here
          * we call the appropriate handlers.
          */
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index ca73f99..39e73c8 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -32,6 +32,8 @@ struct arch_vm_event {
         struct vm_event_emul_insn_data insn;
     } emul;
     struct monitor_write_data write_data;
+    struct vm_event_regs_x86 gprs;
+    bool set_gprs;
 };
 
 int vm_event_init_domain(struct domain *d);
-- 
1.9.1


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

Re: [PATCH V3 0/2] Fix vm_event resume path race condition

[Andrew Cooper]

On 04/05/17 10:00, Razvan Cojocaru wrote:
> This small series first creates hvm/vm_event.{h,c}, in order to bring
> under vm_event maintainership the code that has previously lived in
> hvm_do_resume(), and then fixes a __context_switch()-related race
> condition when attempting to set registers via vm_event reply.
>
> [PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h,c}
> [PATCH V3 2/2] x86/vm_event: fix race between __context_switch() and

Julien: Could we get this fix into 4.9?  The content is reviewed/acked
(subject to a fixup on commit).

~Andrew

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

Re: [PATCH V3 0/2] Fix vm_event resume path race condition

[Julien Grall]

On 05/05/17 15:19, Andrew Cooper wrote:
> On 04/05/17 10:00, Razvan Cojocaru wrote:
>> This small series first creates hvm/vm_event.{h,c}, in order to bring
>> under vm_event maintainership the code that has previously lived in
>> hvm_do_resume(), and then fixes a __context_switch()-related race
>> condition when attempting to set registers via vm_event reply.
>>
>> [PATCH V3 1/2] x86/vm_event: added hvm/vm_event.{h,c}
>> [PATCH V3 2/2] x86/vm_event: fix race between __context_switch() and
>
> Julien: Could we get this fix into 4.9?  The content is reviewed/acked
> (subject to a fixup on commit).

Release-acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

>
> ~Andrew
>

-- 
Julien Grall

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