All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
@ 2018-12-10 16:01 Razvan Cojocaru
  2018-12-10 16:49 ` Roger Pau Monné
  0 siblings, 1 reply; 16+ messages in thread
From: Razvan Cojocaru @ 2018-12-10 16:01 UTC (permalink / raw)
  To: xen-devel
  Cc: kevin.tian, tamas, wei.liu2, suravee.suthikulpanit,
	Razvan Cojocaru, jun.nakajima, andrew.cooper3, julien.grall,
	sstabellini, jbeulich, boris.ostrovsky, brian.woods, roger.pau

Block interrupts (in vmx_intr_assist()) for the duration of
processing a sync vm_event (similarly to the strategy
currently used for single-stepping). Otherwise, attempting
to emulate an instruction when requested by a vm_event
reply may legitimately need to call e.g.
hvm_inject_page_fault(), which then overwrites the active
interrupt in the VMCS.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

---
Changes since V1:
 - Added "e.g." to the patch description to counter the
   impression that the only possibility of overwriting the
   active interrupt in the VMCS is hvm_inject_page_fault().
 - Added the early return logic to svm_intr_assist() as well
   (also support AMD).
 - Removed redundant
   vm_event_check_ring(v->domain->vm_event_monitor) test
   (on re-reading the code, it turns out that there's no
   way that v->arch.vm_event == NULL &&
   vm_event_check_ring() == true.
---
 xen/arch/x86/hvm/svm/intr.c    | 5 +++++
 xen/arch/x86/hvm/vm_event.c    | 2 ++
 xen/arch/x86/hvm/vmx/intr.c    | 5 +++++
 xen/arch/x86/vm_event.c        | 5 +++++
 xen/common/monitor.c           | 6 ++++++
 xen/include/asm-arm/vm_event.h | 6 ++++++
 xen/include/asm-x86/vm_event.h | 8 ++++++++
 7 files changed, 37 insertions(+)

diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
index 7967353..e1cc465 100644
--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -32,6 +32,7 @@
 #include <asm/hvm/svm/svm.h>
 #include <asm/hvm/svm/intr.h>
 #include <asm/hvm/nestedhvm.h> /* for nestedhvm_vcpu_in_guestmode */
+#include <asm/vm_event.h>
 #include <xen/event.h>
 #include <xen/kernel.h>
 #include <public/hvm/ioreq.h>
@@ -137,6 +138,10 @@ void svm_intr_assist(void)
     struct hvm_intack intack;
     enum hvm_intblk intblk;
 
+    /* Block event injection while handling a sync vm_event. */
+    if ( unlikely(v->arch.vm_event) && v->arch.vm_event->intr_block )
+        return;
+
     /* Crank the handle on interrupt state. */
     pt_update_irq(v);
 
diff --git a/xen/arch/x86/hvm/vm_event.c b/xen/arch/x86/hvm/vm_event.c
index 0df8ab4..6e4233b 100644
--- a/xen/arch/x86/hvm/vm_event.c
+++ b/xen/arch/x86/hvm/vm_event.c
@@ -124,6 +124,8 @@ void hvm_vm_event_do_resume(struct vcpu *v)
 
         w->do_write.msr = 0;
     }
+
+    vm_event_block_interrupts(v, false);
 }
 
 /*
diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 5e8cbd4..9102064 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -37,6 +37,7 @@
 #include <asm/hvm/nestedhvm.h>
 #include <public/hvm/ioreq.h>
 #include <asm/hvm/trace.h>
+#include <asm/vm_event.h>
 
 /*
  * A few notes on virtual NMI and INTR delivery, and interactions with
@@ -239,6 +240,10 @@ void vmx_intr_assist(void)
         return;
     }
 
+    /* Block event injection while handling a sync vm_event. */
+    if ( unlikely(v->arch.vm_event) && v->arch.vm_event->intr_block )
+        return;
+
     /* Crank the handle on interrupt state. */
     pt_vector = pt_update_irq(v);
 
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 713e684..d71881f 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -122,6 +122,11 @@ void vm_event_monitor_next_interrupt(struct vcpu *v)
     v->arch.monitor.next_interrupt_enabled = true;
 }
 
+void vm_event_block_interrupts(struct vcpu *v, bool value)
+{
+    v->arch.vm_event->intr_block = value;
+}
+
 #ifdef CONFIG_HVM
 static void vm_event_pack_segment_register(enum x86_segment segment,
                                            struct vm_event_regs_x86 *reg)
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index c606683..af52b07 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -113,6 +113,12 @@ int monitor_traps(struct vcpu *v, bool sync, vm_event_request_t *req)
     if ( sync )
     {
         req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
+        /*
+         * It only makes sense to block interrupts for the duration of
+         * processing blocking vm_events, since that is the only case where
+         * emulating the current instruction really applies.
+         */
+        vm_event_block_interrupts(v, true);
         vm_event_vcpu_pause(v);
         rc = 1;
     }
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index 66f2474..b63249e 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -52,4 +52,10 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
     /* Not supported on ARM. */
 }
 
+static inline
+void vm_event_block_interrupts(struct vcpu *v, bool value)
+{
+    /* Not supported on ARM. */
+}
+
 #endif /* __ASM_ARM_VM_EVENT_H__ */
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 39e73c8..0f20cf0 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -34,6 +34,12 @@ struct arch_vm_event {
     struct monitor_write_data write_data;
     struct vm_event_regs_x86 gprs;
     bool set_gprs;
+    /*
+     * Block interrupts until this vm_event is done handling (after the
+     * fashion of single-step). Meant for the cases where the vm_event
+     * reply asks for emulation of the current instruction.
+     */
+    bool intr_block;
 };
 
 int vm_event_init_domain(struct domain *d);
@@ -47,4 +53,6 @@ void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp);
 
 void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp);
 
+void vm_event_block_interrupts(struct vcpu *v, bool value);
+
 #endif /* __ASM_X86_VM_EVENT_H__ */
-- 
2.7.4


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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
  2018-12-10 16:01 [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events Razvan Cojocaru
@ 2018-12-10 16:49 ` Roger Pau Monné
  2018-12-10 16:59   ` Razvan Cojocaru
  0 siblings, 1 reply; 16+ messages in thread
From: Roger Pau Monné @ 2018-12-10 16:49 UTC (permalink / raw)
  To: Razvan Cojocaru
  Cc: kevin.tian, tamas, wei.liu2, suravee.suthikulpanit, jun.nakajima,
	andrew.cooper3, julien.grall, sstabellini, jbeulich, xen-devel,
	boris.ostrovsky, brian.woods

On Mon, Dec 10, 2018 at 06:01:49PM +0200, Razvan Cojocaru wrote:
> diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
> index 66f2474..b63249e 100644
> --- a/xen/include/asm-arm/vm_event.h
> +++ b/xen/include/asm-arm/vm_event.h
> @@ -52,4 +52,10 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
>      /* Not supported on ARM. */
>  }
>  
> +static inline
> +void vm_event_block_interrupts(struct vcpu *v, bool value)
> +{
> +    /* Not supported on ARM. */

ASSERT_UNREACHABLE?

Thanks, Roger.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
  2018-12-10 16:49 ` Roger Pau Monné
@ 2018-12-10 16:59   ` Razvan Cojocaru
  2018-12-11 10:01     ` Razvan Cojocaru
  0 siblings, 1 reply; 16+ messages in thread
From: Razvan Cojocaru @ 2018-12-10 16:59 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: kevin.tian, tamas, wei.liu2, suravee.suthikulpanit, jun.nakajima,
	andrew.cooper3, julien.grall, sstabellini, jbeulich, xen-devel,
	boris.ostrovsky, brian.woods

On 12/10/18 6:49 PM, Roger Pau Monné wrote:
> On Mon, Dec 10, 2018 at 06:01:49PM +0200, Razvan Cojocaru wrote:
>> diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
>> index 66f2474..b63249e 100644
>> --- a/xen/include/asm-arm/vm_event.h
>> +++ b/xen/include/asm-arm/vm_event.h
>> @@ -52,4 +52,10 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
>>      /* Not supported on ARM. */
>>  }
>>  
>> +static inline
>> +void vm_event_block_interrupts(struct vcpu *v, bool value)
>> +{
>> +    /* Not supported on ARM. */
> 
> ASSERT_UNREACHABLE?

Will do (although if you look at the rest of the function in that header
it'll break what appears to be the prior convention).


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
  2018-12-10 16:59   ` Razvan Cojocaru
@ 2018-12-11 10:01     ` Razvan Cojocaru
  2018-12-11 10:14       ` Roger Pau Monné
  0 siblings, 1 reply; 16+ messages in thread
From: Razvan Cojocaru @ 2018-12-11 10:01 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: kevin.tian, tamas, wei.liu2, suravee.suthikulpanit,
	andrew.cooper3, julien.grall, sstabellini, jbeulich,
	jun.nakajima, xen-devel, boris.ostrovsky, brian.woods

On 12/10/18 6:59 PM, Razvan Cojocaru wrote:
> On 12/10/18 6:49 PM, Roger Pau Monné wrote:
>> On Mon, Dec 10, 2018 at 06:01:49PM +0200, Razvan Cojocaru wrote:
>>> diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
>>> index 66f2474..b63249e 100644
>>> --- a/xen/include/asm-arm/vm_event.h
>>> +++ b/xen/include/asm-arm/vm_event.h
>>> @@ -52,4 +52,10 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
>>>      /* Not supported on ARM. */
>>>  }
>>>  
>>> +static inline
>>> +void vm_event_block_interrupts(struct vcpu *v, bool value)
>>> +{
>>> +    /* Not supported on ARM. */
>>
>> ASSERT_UNREACHABLE?
> 
> Will do (although if you look at the rest of the function in that header
> it'll break what appears to be the prior convention).

Sorry, on second thought we can't do that, because that function is
being called from the common code - which is why the function became
necessary. Specifically, this it unconditionally called in
monitor_traps(), which is used for all events (ARM and otherwise).

So it's valid to call monitor_traps() for ARM vm_events and expect it to
run without issue, which ASSERT_UNREACHABLE() would of course break.


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
  2018-12-11 10:01     ` Razvan Cojocaru
@ 2018-12-11 10:14       ` Roger Pau Monné
  2018-12-11 10:21         ` Razvan Cojocaru
  0 siblings, 1 reply; 16+ messages in thread
From: Roger Pau Monné @ 2018-12-11 10:14 UTC (permalink / raw)
  To: Razvan Cojocaru
  Cc: kevin.tian, tamas, wei.liu2, suravee.suthikulpanit,
	andrew.cooper3, julien.grall, sstabellini, jbeulich,
	jun.nakajima, xen-devel, boris.ostrovsky, brian.woods

On Tue, Dec 11, 2018 at 12:01:53PM +0200, Razvan Cojocaru wrote:
> On 12/10/18 6:59 PM, Razvan Cojocaru wrote:
> > On 12/10/18 6:49 PM, Roger Pau Monné wrote:
> >> On Mon, Dec 10, 2018 at 06:01:49PM +0200, Razvan Cojocaru wrote:
> >>> diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
> >>> index 66f2474..b63249e 100644
> >>> --- a/xen/include/asm-arm/vm_event.h
> >>> +++ b/xen/include/asm-arm/vm_event.h
> >>> @@ -52,4 +52,10 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
> >>>      /* Not supported on ARM. */
> >>>  }
> >>>  
> >>> +static inline
> >>> +void vm_event_block_interrupts(struct vcpu *v, bool value)
> >>> +{
> >>> +    /* Not supported on ARM. */
> >>
> >> ASSERT_UNREACHABLE?
> > 
> > Will do (although if you look at the rest of the function in that header
> > it'll break what appears to be the prior convention).
> 
> Sorry, on second thought we can't do that, because that function is
> being called from the common code - which is why the function became
> necessary. Specifically, this it unconditionally called in
> monitor_traps(), which is used for all events (ARM and otherwise).
> 
> So it's valid to call monitor_traps() for ARM vm_events and expect it to
> run without issue, which ASSERT_UNREACHABLE() would of course break.

But then the functionality that makes use of vm_event_block_interrupts
cannot work reliably on ARM and should not be used?

Thanks, Roger.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
  2018-12-11 10:14       ` Roger Pau Monné
@ 2018-12-11 10:21         ` Razvan Cojocaru
  2018-12-11 11:59           ` Julien Grall
  0 siblings, 1 reply; 16+ messages in thread
From: Razvan Cojocaru @ 2018-12-11 10:21 UTC (permalink / raw)
  To: Roger Pau Monné, tamas
  Cc: kevin.tian, sstabellini, wei.liu2, suravee.suthikulpanit,
	andrew.cooper3, julien.grall, jbeulich, jun.nakajima, xen-devel,
	boris.ostrovsky, brian.woods

On 12/11/18 12:14 PM, Roger Pau Monné wrote:
> On Tue, Dec 11, 2018 at 12:01:53PM +0200, Razvan Cojocaru wrote:
>> On 12/10/18 6:59 PM, Razvan Cojocaru wrote:
>>> On 12/10/18 6:49 PM, Roger Pau Monné wrote:
>>>> On Mon, Dec 10, 2018 at 06:01:49PM +0200, Razvan Cojocaru wrote:
>>>>> diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
>>>>> index 66f2474..b63249e 100644
>>>>> --- a/xen/include/asm-arm/vm_event.h
>>>>> +++ b/xen/include/asm-arm/vm_event.h
>>>>> @@ -52,4 +52,10 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
>>>>>      /* Not supported on ARM. */
>>>>>  }
>>>>>  
>>>>> +static inline
>>>>> +void vm_event_block_interrupts(struct vcpu *v, bool value)
>>>>> +{
>>>>> +    /* Not supported on ARM. */
>>>>
>>>> ASSERT_UNREACHABLE?
>>>
>>> Will do (although if you look at the rest of the function in that header
>>> it'll break what appears to be the prior convention).
>>
>> Sorry, on second thought we can't do that, because that function is
>> being called from the common code - which is why the function became
>> necessary. Specifically, this it unconditionally called in
>> monitor_traps(), which is used for all events (ARM and otherwise).
>>
>> So it's valid to call monitor_traps() for ARM vm_events and expect it to
>> run without issue, which ASSERT_UNREACHABLE() would of course break.
> 
> But then the functionality that makes use of vm_event_block_interrupts
> cannot work reliably on ARM and should not be used?

Well, it's currently a no-op on ARM so it doesn't make anything worse.
I don't have access to ARM hardware and am unfamiliar with the specifics
of handling interrupts on ARM with regard to vm_events (or even if this
specific problem applies to ARM) - so it's the best that I am able to do
at the moment.

Of course, this patch can be the basis of a future one for ARM if that
work makes sense (perhaps Tamas has more to say about this), or if an
ARM maintaner can point out what modifications should be done I can
compile-test for ARM with a cross-compiler, _hope_ it works, and
re-submit the patch.


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
  2018-12-11 10:21         ` Razvan Cojocaru
@ 2018-12-11 11:59           ` Julien Grall
  2018-12-11 12:33             ` Razvan Cojocaru
  0 siblings, 1 reply; 16+ messages in thread
From: Julien Grall @ 2018-12-11 11:59 UTC (permalink / raw)
  To: Razvan Cojocaru, Roger Pau Monné, tamas
  Cc: kevin.tian, sstabellini, wei.liu2, suravee.suthikulpanit,
	andrew.cooper3, jbeulich, jun.nakajima, xen-devel,
	boris.ostrovsky, brian.woods

Hi,

On 11/12/2018 10:21, Razvan Cojocaru wrote:
> On 12/11/18 12:14 PM, Roger Pau Monné wrote:
>> On Tue, Dec 11, 2018 at 12:01:53PM +0200, Razvan Cojocaru wrote:
>>> On 12/10/18 6:59 PM, Razvan Cojocaru wrote:
>>>> On 12/10/18 6:49 PM, Roger Pau Monné wrote:
>>>>> On Mon, Dec 10, 2018 at 06:01:49PM +0200, Razvan Cojocaru wrote:
>>>>>> diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
>>>>>> index 66f2474..b63249e 100644
>>>>>> --- a/xen/include/asm-arm/vm_event.h
>>>>>> +++ b/xen/include/asm-arm/vm_event.h
>>>>>> @@ -52,4 +52,10 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
>>>>>>       /* Not supported on ARM. */
>>>>>>   }
>>>>>>   
>>>>>> +static inline
>>>>>> +void vm_event_block_interrupts(struct vcpu *v, bool value)
>>>>>> +{
>>>>>> +    /* Not supported on ARM. */
>>>>>
>>>>> ASSERT_UNREACHABLE?
>>>>
>>>> Will do (although if you look at the rest of the function in that header
>>>> it'll break what appears to be the prior convention).
>>>
>>> Sorry, on second thought we can't do that, because that function is
>>> being called from the common code - which is why the function became
>>> necessary. Specifically, this it unconditionally called in
>>> monitor_traps(), which is used for all events (ARM and otherwise).
>>>
>>> So it's valid to call monitor_traps() for ARM vm_events and expect it to
>>> run without issue, which ASSERT_UNREACHABLE() would of course break.
>>
>> But then the functionality that makes use of vm_event_block_interrupts
>> cannot work reliably on ARM and should not be used?
> 
> Well, it's currently a no-op on ARM so it doesn't make anything worse.
Can an vm-event app rely on the interrupts to be blocked?

> I don't have access to ARM hardware and am unfamiliar with the specifics
> of handling interrupts on ARM with regard to vm_events (or even if this
> specific problem applies to ARM) - so it's the best that I am able to do
> at the moment.

At the first, the name of the function looks quite wrong for Arm. If you block 
interrupts, you will never receive them again. I read the commit message and I 
am not sure to understand the exact behavior of this function.

Do you mind to provide more explanation what you are trying to achieve?

> 
> Of course, this patch can be the basis of a future one for ARM if that
> work makes sense (perhaps Tamas has more to say about this), or if an
> ARM maintaner can point out what modifications should be done I can
> compile-test for ARM with a cross-compiler, _hope_ it works, and
> re-submit the patch.

Before pointing out the modifications, I need to understand what you exactly 
want to achieve with it. But then, I think such code should be tested before 
getting merged.

That's fine by me if you don't want to implement it for Arm. However, we need to 
make sure that vm-event app does not expect that behavior.

In any case, I think you want to rename the function and/or document more that 
expected behavior.

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
  2018-12-11 11:59           ` Julien Grall
@ 2018-12-11 12:33             ` Razvan Cojocaru
  2018-12-13  6:54               ` Tian, Kevin
  0 siblings, 1 reply; 16+ messages in thread
From: Razvan Cojocaru @ 2018-12-11 12:33 UTC (permalink / raw)
  To: Julien Grall, Roger Pau Monné, tamas
  Cc: kevin.tian, sstabellini, wei.liu2, suravee.suthikulpanit,
	andrew.cooper3, jbeulich, jun.nakajima, xen-devel,
	boris.ostrovsky, brian.woods

On 12/11/18 1:59 PM, Julien Grall wrote:
> Hi,
> 
> On 11/12/2018 10:21, Razvan Cojocaru wrote:
>> On 12/11/18 12:14 PM, Roger Pau Monné wrote:
>>> On Tue, Dec 11, 2018 at 12:01:53PM +0200, Razvan Cojocaru wrote:
>>>> On 12/10/18 6:59 PM, Razvan Cojocaru wrote:
>>>>> On 12/10/18 6:49 PM, Roger Pau Monné wrote:
>>>>>> On Mon, Dec 10, 2018 at 06:01:49PM +0200, Razvan Cojocaru wrote:
>>>>>>> diff --git a/xen/include/asm-arm/vm_event.h
>>>>>>> b/xen/include/asm-arm/vm_event.h
>>>>>>> index 66f2474..b63249e 100644
>>>>>>> --- a/xen/include/asm-arm/vm_event.h
>>>>>>> +++ b/xen/include/asm-arm/vm_event.h
>>>>>>> @@ -52,4 +52,10 @@ void vm_event_emulate_check(struct vcpu *v,
>>>>>>> vm_event_response_t *rsp)
>>>>>>>       /* Not supported on ARM. */
>>>>>>>   }
>>>>>>>   +static inline
>>>>>>> +void vm_event_block_interrupts(struct vcpu *v, bool value)
>>>>>>> +{
>>>>>>> +    /* Not supported on ARM. */
>>>>>>
>>>>>> ASSERT_UNREACHABLE?
>>>>>
>>>>> Will do (although if you look at the rest of the function in that
>>>>> header
>>>>> it'll break what appears to be the prior convention).
>>>>
>>>> Sorry, on second thought we can't do that, because that function is
>>>> being called from the common code - which is why the function became
>>>> necessary. Specifically, this it unconditionally called in
>>>> monitor_traps(), which is used for all events (ARM and otherwise).
>>>>
>>>> So it's valid to call monitor_traps() for ARM vm_events and expect
>>>> it to
>>>> run without issue, which ASSERT_UNREACHABLE() would of course break.
>>>
>>> But then the functionality that makes use of vm_event_block_interrupts
>>> cannot work reliably on ARM and should not be used?
>>
>> Well, it's currently a no-op on ARM so it doesn't make anything worse.
> Can an vm-event app rely on the interrupts to be blocked?
> 
>> I don't have access to ARM hardware and am unfamiliar with the specifics
>> of handling interrupts on ARM with regard to vm_events (or even if this
>> specific problem applies to ARM) - so it's the best that I am able to do
>> at the moment.
> 
> At the first, the name of the function looks quite wrong for Arm. If you
> block interrupts, you will never receive them again. I read the commit
> message and I am not sure to understand the exact behavior of this
> function.
> 
> Do you mind to provide more explanation what you are trying to achieve?

So on x86 what happens is this:

1. A sync vm_event is sent out, for an EPT fault. This happens in
xen/arch/x86/hvm/hvm.c, which for VMX / Intel is called in
ept_handle_violation(), which in turn is called in vmx_vmexit_handler(),
as a consequence of handling an EXIT_REASON_EPT_VIOLATION exit.

2. Since this is a _sync_ event, this means that the VCPU is now paused
until the introspection agent replies to it. Let's assume that at this
step, the introspection agent does reply, and tells Xen to emulate the
current instruction in the reply.

3. After Xen receives the reply and re-schedules the VCPU to run, we may
see this backtrace (collected on Xen 4.7, but it applies to staging as
well):

(XEN) [<ffff82d0801fe618>] vmx.c#__vmx_inject_exception+0x74/0xc7
(XEN) [<ffff82d08020184f>] vmx.c#vmx_inject_trap+0x1e1/0x29f
(XEN) [<ffff82d0801d67fb>] hvm_inject_trap+0xb0/0xb5
(XEN) [<ffff82d0801d6858>] hvm_inject_page_fault+0x2a/0x2c
(XEN) [<ffff82d0801d6937>] hvm.c#__hvm_copy+0xdd/0x37f
(XEN) [<ffff82d0801d8557>] hvm_copy_to_guest_virt+0x1d/0x1f
(XEN) [<ffff82d0801d2350>] emulate.c#hvmemul_write+0xe0/0x148
(XEN) [<ffff82d0801b6c5b>] x86_emulate+0xd148/0x11405
(XEN) [<ffff82d0801d146a>] emulate.c#_hvm_emulate_one+0x188/0x2bc
(XEN) [<ffff82d0801d1694>] hvm_emulate_one+0x10/0x12
(XEN) [<ffff82d0801d2999>] hvm_mem_access_emulate_one+0x7a/0xee
(XEN) [<ffff82d0801dbac3>] hvm_do_resume+0x246/0x3c5
(XEN) [<ffff82d0801fb9ff>] vmx_do_resume+0x102/0x119
(XEN) [<ffff82d080170b3e>] context_switch+0xf52/0xfad
(XEN) [<ffff82d0801317d6>] schedule.c#schedule+0x753/0x792
(XEN) [<ffff82d080134eaf>] softirq.c#__do_softirq+0x85/0x90
(XEN) [<ffff82d080134f04>] do_softirq+0x13/0x15
(XEN) [<ffff82d08016b592>] domain.c#idle_loop+0x61/0x6e

Now, vmx_inject_exception() calls __vmwrite(VM_ENTRY_INTR_INFO,
intr_fields);, and _before_ we get here, the asm code has already
previously called vmx_intr_assist(), which may have placed some valid
value into VM_ENTRY_INTR_INFO as well. This, of course, means that the
previous value will now be overwritten, and so lost.

The current patch ensures that vmx_intr_info() will _not_ pick that
pending interrupt up until after the sync vm_event has been handled
(which is also essentially how the interrupt should be processed anyway,
since the current instruction is sort-of-in-progress until the sync
vm_event is handled; it's also the strategy VMX single-step has employed).

>> Of course, this patch can be the basis of a future one for ARM if that
>> work makes sense (perhaps Tamas has more to say about this), or if an
>> ARM maintaner can point out what modifications should be done I can
>> compile-test for ARM with a cross-compiler, _hope_ it works, and
>> re-submit the patch.
> 
> Before pointing out the modifications, I need to understand what you
> exactly want to achieve with it. But then, I think such code should be
> tested before getting merged.
> 
> That's fine by me if you don't want to implement it for Arm. However, we
> need to make sure that vm-event app does not expect that behavior.
> 
> In any case, I think you want to rename the function and/or document
> more that expected behavior.

You're right, I should probably rename that function / variable to
better reflect what it signifies - that sync vm_event processing is in
progress. For VMX and SVM, that simply means that interrupts will be
blocked, and the value of the variable will be correct and possibly
useful for ARM as well.

That may also help Andrew's "x86/hvm: Drop the may_defer boolean from
hvm_* helpers" patch actually.


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
  2018-12-11 12:33             ` Razvan Cojocaru
@ 2018-12-13  6:54               ` Tian, Kevin
  2018-12-13  8:03                 ` Razvan Cojocaru
  0 siblings, 1 reply; 16+ messages in thread
From: Tian, Kevin @ 2018-12-13  6:54 UTC (permalink / raw)
  To: Razvan Cojocaru, Julien Grall, Roger Pau Monné, tamas
  Cc: sstabellini, wei.liu2, Nakajima, Jun, andrew.cooper3, jbeulich,
	xen-devel, boris.ostrovsky, brian.woods, suravee.suthikulpanit

> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com]
> Sent: Tuesday, December 11, 2018 8:33 PM
> 
> > In any case, I think you want to rename the function and/or document
> > more that expected behavior.
> 
> You're right, I should probably rename that function / variable to
> better reflect what it signifies - that sync vm_event processing is in
> progress. For VMX and SVM, that simply means that interrupts will be
> blocked, and the value of the variable will be correct and possibly
> useful for ARM as well.
> 

what about vm_event_block_interrupt_injection? in that case
it's injection instead of interrupt itself being blocked. blocking
injection should mean same thing cross archs?

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
  2018-12-13  6:54               ` Tian, Kevin
@ 2018-12-13  8:03                 ` Razvan Cojocaru
  2018-12-13 12:04                   ` Julien Grall
  0 siblings, 1 reply; 16+ messages in thread
From: Razvan Cojocaru @ 2018-12-13  8:03 UTC (permalink / raw)
  To: Tian, Kevin, Julien Grall, Roger Pau Monné, tamas
  Cc: sstabellini, wei.liu2, Nakajima, Jun, andrew.cooper3, jbeulich,
	xen-devel, boris.ostrovsky, brian.woods, suravee.suthikulpanit

On 12/13/18 8:54 AM, Tian, Kevin wrote:
>> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com]
>> Sent: Tuesday, December 11, 2018 8:33 PM
>>
>>> In any case, I think you want to rename the function and/or document
>>> more that expected behavior.
>>
>> You're right, I should probably rename that function / variable to
>> better reflect what it signifies - that sync vm_event processing is in
>> progress. For VMX and SVM, that simply means that interrupts will be
>> blocked, and the value of the variable will be correct and possibly
>> useful for ARM as well.
>>
> 
> what about vm_event_block_interrupt_injection? in that case
> it's injection instead of interrupt itself being blocked. blocking
> injection should mean same thing cross archs?

Of course, if Julien agrees with the change I'll rename it as suggested.


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
  2018-12-13  8:03                 ` Razvan Cojocaru
@ 2018-12-13 12:04                   ` Julien Grall
  2018-12-13 12:15                     ` Razvan Cojocaru
  0 siblings, 1 reply; 16+ messages in thread
From: Julien Grall @ 2018-12-13 12:04 UTC (permalink / raw)
  To: Razvan Cojocaru, Tian, Kevin, Roger Pau Monné, tamas
  Cc: sstabellini, wei.liu2, Nakajima, Jun, andrew.cooper3, jbeulich,
	xen-devel, boris.ostrovsky, brian.woods, suravee.suthikulpanit

Hi,

On 12/13/18 8:03 AM, Razvan Cojocaru wrote:
> On 12/13/18 8:54 AM, Tian, Kevin wrote:
>>> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com]
>>> Sent: Tuesday, December 11, 2018 8:33 PM
>>>
>>>> In any case, I think you want to rename the function and/or document
>>>> more that expected behavior.
>>>
>>> You're right, I should probably rename that function / variable to
>>> better reflect what it signifies - that sync vm_event processing is in
>>> progress. For VMX and SVM, that simply means that interrupts will be
>>> blocked, and the value of the variable will be correct and possibly
>>> useful for ARM as well.
>>>
>>
>> what about vm_event_block_interrupt_injection? in that case
>> it's injection instead of interrupt itself being blocked. blocking
>> injection should mean same thing cross archs?

Why would you want to block all interrupts injections? When I looked at 
the details, it feels more you want to block exceptions.

I can see use for blocking exception on Arm, blocking all the interrupts 
is likely going to bring more issues than solving anything.

So a better name would be vm_event_block_exception_injection.

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
  2018-12-13 12:04                   ` Julien Grall
@ 2018-12-13 12:15                     ` Razvan Cojocaru
  2018-12-13 12:39                       ` Julien Grall
  0 siblings, 1 reply; 16+ messages in thread
From: Razvan Cojocaru @ 2018-12-13 12:15 UTC (permalink / raw)
  To: Julien Grall, Tian, Kevin, Roger Pau Monné, tamas
  Cc: sstabellini, wei.liu2, Nakajima, Jun, andrew.cooper3, jbeulich,
	xen-devel, boris.ostrovsky, brian.woods, suravee.suthikulpanit

On 12/13/18 2:04 PM, Julien Grall wrote:
> Hi,
> 
> On 12/13/18 8:03 AM, Razvan Cojocaru wrote:
>> On 12/13/18 8:54 AM, Tian, Kevin wrote:
>>>> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com]
>>>> Sent: Tuesday, December 11, 2018 8:33 PM
>>>>
>>>>> In any case, I think you want to rename the function and/or document
>>>>> more that expected behavior.
>>>>
>>>> You're right, I should probably rename that function / variable to
>>>> better reflect what it signifies - that sync vm_event processing is in
>>>> progress. For VMX and SVM, that simply means that interrupts will be
>>>> blocked, and the value of the variable will be correct and possibly
>>>> useful for ARM as well.
>>>>
>>>
>>> what about vm_event_block_interrupt_injection? in that case
>>> it's injection instead of interrupt itself being blocked. blocking
>>> injection should mean same thing cross archs?
> 
> Why would you want to block all interrupts injections? When I looked at
> the details, it feels more you want to block exceptions.
> 
> I can see use for blocking exception on Arm, blocking all the interrupts
> is likely going to bring more issues than solving anything.
> 
> So a better name would be vm_event_block_exception_injection.

I'd like to block the writing of anything, by vmx_intr_assist(), into
VM_ENTRY_INTR_INFO, because an emulation attempt that happens
post-vmx_intr_assist() (because the vm_event client application has
requested it) may write an exception of its own there.

Since vmx_intr_assist() is called on VMX between the time of sending out
the vm_event and the emulation (which happens in
hvm_vm_event_do_resume()), we want to block everything that it may write
in the VMCS until the emulation is done. I think that's more than just
exceptions.

I've probably been confusing when I was talking about the exceptions
that emulating the current instruction may trigger - we don't want to
block those.


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
  2018-12-13 12:15                     ` Razvan Cojocaru
@ 2018-12-13 12:39                       ` Julien Grall
  2018-12-13 13:18                         ` Razvan Cojocaru
  0 siblings, 1 reply; 16+ messages in thread
From: Julien Grall @ 2018-12-13 12:39 UTC (permalink / raw)
  To: Razvan Cojocaru, Tian, Kevin, Roger Pau Monné, tamas
  Cc: sstabellini, wei.liu2, Nakajima, Jun, andrew.cooper3, jbeulich,
	xen-devel, boris.ostrovsky, brian.woods, suravee.suthikulpanit

Hi,

On 12/13/18 12:15 PM, Razvan Cojocaru wrote:
> On 12/13/18 2:04 PM, Julien Grall wrote:
>> Hi,
>>
>> On 12/13/18 8:03 AM, Razvan Cojocaru wrote:
>>> On 12/13/18 8:54 AM, Tian, Kevin wrote:
>>>>> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com]
>>>>> Sent: Tuesday, December 11, 2018 8:33 PM
>>>>>
>>>>>> In any case, I think you want to rename the function and/or document
>>>>>> more that expected behavior.
>>>>>
>>>>> You're right, I should probably rename that function / variable to
>>>>> better reflect what it signifies - that sync vm_event processing is in
>>>>> progress. For VMX and SVM, that simply means that interrupts will be
>>>>> blocked, and the value of the variable will be correct and possibly
>>>>> useful for ARM as well.
>>>>>
>>>>
>>>> what about vm_event_block_interrupt_injection? in that case
>>>> it's injection instead of interrupt itself being blocked. blocking
>>>> injection should mean same thing cross archs?
>>
>> Why would you want to block all interrupts injections? When I looked at
>> the details, it feels more you want to block exceptions.
>>
>> I can see use for blocking exception on Arm, blocking all the interrupts
>> is likely going to bring more issues than solving anything.
>>
>> So a better name would be vm_event_block_exception_injection.
> 
> I'd like to block the writing of anything, by vmx_intr_assist(), into
> VM_ENTRY_INTR_INFO, because an emulation attempt that happens
> post-vmx_intr_assist() (because the vm_event client application has
> requested it) may write an exception of its own there.
> 
> Since vmx_intr_assist() is called on VMX between the time of sending out
> the vm_event and the emulation (which happens in
> hvm_vm_event_do_resume()), we want to block everything that it may write
> in the VMCS until the emulation is done. I think that's more than just
> exceptions.

I don't know in details how x86 virtualization works, so it is a bit 
hard to comment on that. However, it feels to me that you are 
introducing in common code a function that will workaround an 
architecture specific problem.

Can you try to explain it in agnostic word?

To expand what I said above, I think it is reasonable to request 
blocking exception (e.g page-fault...) because they can be generated by 
an instruction. However, all interrupts generated by the interrupt 
controller (e.g device, IPI..) should not be blocked.

AFAIU your description, it is the same path to handle the two on x86, right?

On Arm, there are 2 distinct paths, interrupt generated by the interrupt 
controller are queued. The exceptions will be generated using multiple 
different paths. Yet exceptions can still override each other.

I can't see any reason for Arm to block interrupt generated by the 
interrupt controller. This would actually be dangerous due to the way we 
handle them in Xen currently. Instead we may want to block the exception 
as they can be generated by an instruction.

> 
> I've probably been confusing when I was talking about the exceptions
> that emulating the current instruction may trigger - we don't want to
> block those.

I understood that bit.

> 
> 
> Thanks,
> Razvan
> 

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
  2018-12-13 12:39                       ` Julien Grall
@ 2018-12-13 13:18                         ` Razvan Cojocaru
  2018-12-13 14:58                           ` Jan Beulich
  0 siblings, 1 reply; 16+ messages in thread
From: Razvan Cojocaru @ 2018-12-13 13:18 UTC (permalink / raw)
  To: Julien Grall, Tian, Kevin, Roger Pau Monné, tamas
  Cc: sstabellini, wei.liu2, Nakajima, Jun, andrew.cooper3, jbeulich,
	xen-devel, boris.ostrovsky, brian.woods, suravee.suthikulpanit

On 12/13/18 2:39 PM, Julien Grall wrote:
> Hi,
> 
> On 12/13/18 12:15 PM, Razvan Cojocaru wrote:
>> On 12/13/18 2:04 PM, Julien Grall wrote:
>>> Hi,
>>>
>>> On 12/13/18 8:03 AM, Razvan Cojocaru wrote:
>>>> On 12/13/18 8:54 AM, Tian, Kevin wrote:
>>>>>> From: Razvan Cojocaru [mailto:rcojocaru@bitdefender.com]
>>>>>> Sent: Tuesday, December 11, 2018 8:33 PM
>>>>>>
>>>>>>> In any case, I think you want to rename the function and/or document
>>>>>>> more that expected behavior.
>>>>>>
>>>>>> You're right, I should probably rename that function / variable to
>>>>>> better reflect what it signifies - that sync vm_event processing
>>>>>> is in
>>>>>> progress. For VMX and SVM, that simply means that interrupts will be
>>>>>> blocked, and the value of the variable will be correct and possibly
>>>>>> useful for ARM as well.
>>>>>>
>>>>>
>>>>> what about vm_event_block_interrupt_injection? in that case
>>>>> it's injection instead of interrupt itself being blocked. blocking
>>>>> injection should mean same thing cross archs?
>>>
>>> Why would you want to block all interrupts injections? When I looked at
>>> the details, it feels more you want to block exceptions.
>>>
>>> I can see use for blocking exception on Arm, blocking all the interrupts
>>> is likely going to bring more issues than solving anything.
>>>
>>> So a better name would be vm_event_block_exception_injection.
>>
>> I'd like to block the writing of anything, by vmx_intr_assist(), into
>> VM_ENTRY_INTR_INFO, because an emulation attempt that happens
>> post-vmx_intr_assist() (because the vm_event client application has
>> requested it) may write an exception of its own there.
>>
>> Since vmx_intr_assist() is called on VMX between the time of sending out
>> the vm_event and the emulation (which happens in
>> hvm_vm_event_do_resume()), we want to block everything that it may write
>> in the VMCS until the emulation is done. I think that's more than just
>> exceptions.
> 
> I don't know in details how x86 virtualization works, so it is a bit
> hard to comment on that. However, it feels to me that you are
> introducing in common code a function that will workaround an
> architecture specific problem.
> 
> Can you try to explain it in agnostic word?

I'll certainly do my best. :)

Assume the following scenario:

1. A guest instruction tries to write into read-only memory (as set in
the EPT), with monitoring active for the domain.

2. An EPT violation exit occurs, and in the course of it, the VCPU that
was running the code that produced the violation is paused and a
vm_event is sent to an introspection application.

3. The introspection application is processing the vm_event. During this
time, some event may occur (which will remain pending).

4. The introspection agent replies with "please emulate" the current
instruction - since the emulator (currently) does not care about EPT
restrictions, so this is a cheap way of proceeding without lifting them.

5. With x86, we have {vmx,svm}_intr_assist(), which is guaranteed to be
called _before_ the VCPU is woken up again. This is the designated "pick
up pending interrupts" function on x86. Perhaps this (real-life)
backtrace is helpful:

(XEN) Xen call trace:
(XEN)    [<ffff82d08031b55d>] vmx.c#__vmx_inject_exception+0xa1/0xda
(XEN)    [<ffff82d08031eb5c>] vmx_inject_extint+0x94/0x9f
(XEN)    [<ffff82d080315a0a>] vmx_intr_assist+0x4ee/0x5ad
(XEN)    [<ffff82d0803258ff>] vmx_asm_vmexit_handler+0xff/0x270

On x86, there can (currently) be only one scheduled interrupt /
exception, and that is written into VM_ENTRY_INTR_INFO in the VMCS on Intel.

6. _After_ vmx_intr_assist() has run, we are now trying to emulate the
current instruction, which may cause an exception, which will overwrite
the pending interrupt.

So, long story short, on VMX we first send out the vm_event, while
processing it an interrupt / exception may become pending, before
resuming the VCPU that has sent out the vm_event there's a Xen function
that picks up the pending interrupt and schedules it (writes it in the
VMCS), and only then we attempt the emulation, which may overwrite it
(because there's only one place we can write to schedule interrupts /
exceptions).

> To expand what I said above, I think it is reasonable to request
> blocking exception (e.g page-fault...) because they can be generated by
> an instruction. However, all interrupts generated by the interrupt
> controller (e.g device, IPI..) should not be blocked.
> 
> AFAIU your description, it is the same path to handle the two on x86,
> right?

Pretty much, yes. Technically speaking there are two that I am aware of,
the second of them being the IDT vectoring case just before the EPT
fault exit - but that's outside the scope of this patch. Just mentioning
it for completeness.

Also, it is worth mentioning that:

1. This is the exact same strategy employed by the single-stepping
functionality on VMX / Intel. In fact, if you look at
xen/arch/x86/hvm/vmx/intr.c, in vmx_intr_assist(), you'll see an early
return blocking injection of interrupts for the duration of single
stepping ("if ( unlikely(v->arch.hvm.single_step) )").

2. Interrupts are not blocked indefinitely - only until the emulation is
done. It could be argued that that's really the proper place for them to
be processed anyway - on an instruction boundary, _after_ the
in-progress instruction has finished executing. It's just that with the
vm_event introspection thing you could say that executing the current
instruction may take a bit longer.

I hope I've been able to explain it better this time. :)


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
  2018-12-13 13:18                         ` Razvan Cojocaru
@ 2018-12-13 14:58                           ` Jan Beulich
  2018-12-13 15:11                             ` Razvan Cojocaru
  0 siblings, 1 reply; 16+ messages in thread
From: Jan Beulich @ 2018-12-13 14:58 UTC (permalink / raw)
  To: Razvan Cojocaru
  Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Jun Nakajima,
	Andrew Cooper, Julien Grall, Tamas K Lengyel,
	Suravee Suthikulpanit, xen-devel, Boris Ostrovsky, Brian Woods,
	Roger Pau Monne

>>> On 13.12.18 at 14:18, <rcojocaru@bitdefender.com> wrote:
> So, long story short, on VMX we first send out the vm_event, while
> processing it an interrupt / exception may become pending, before
> resuming the VCPU that has sent out the vm_event there's a Xen function
> that picks up the pending interrupt and schedules it (writes it in the
> VMCS), and only then we attempt the emulation, which may overwrite it
> (because there's only one place we can write to schedule interrupts /
> exceptions).

So perhaps the solution is indeed to change the order of how things
get done, instead of blocking interrupts? You seem to think this way
too, as per ...

> 2. Interrupts are not blocked indefinitely - only until the emulation is
> done. It could be argued that that's really the proper place for them to
> be processed anyway - on an instruction boundary, _after_ the
> in-progress instruction has finished executing. It's just that with the
> vm_event introspection thing you could say that executing the current
> instruction may take a bit longer.

... this.

Jan



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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events
  2018-12-13 14:58                           ` Jan Beulich
@ 2018-12-13 15:11                             ` Razvan Cojocaru
  0 siblings, 0 replies; 16+ messages in thread
From: Razvan Cojocaru @ 2018-12-13 15:11 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Kevin Tian, Stefano Stabellini, Wei Liu, Jun Nakajima,
	Andrew Cooper, Julien Grall, Tamas K Lengyel,
	Suravee Suthikulpanit, xen-devel, Boris Ostrovsky, Brian Woods,
	Roger Pau Monne

On 12/13/18 4:58 PM, Jan Beulich wrote:
>>>> On 13.12.18 at 14:18, <rcojocaru@bitdefender.com> wrote:
>> So, long story short, on VMX we first send out the vm_event, while
>> processing it an interrupt / exception may become pending, before
>> resuming the VCPU that has sent out the vm_event there's a Xen function
>> that picks up the pending interrupt and schedules it (writes it in the
>> VMCS), and only then we attempt the emulation, which may overwrite it
>> (because there's only one place we can write to schedule interrupts /
>> exceptions).
> 
> So perhaps the solution is indeed to change the order of how things
> get done, instead of blocking interrupts? You seem to think this way
> too, as per ...
> 
>> 2. Interrupts are not blocked indefinitely - only until the emulation is
>> done. It could be argued that that's really the proper place for them to
>> be processed anyway - on an instruction boundary, _after_ the
>> in-progress instruction has finished executing. It's just that with the
>> vm_event introspection thing you could say that executing the current
>> instruction may take a bit longer.
> 
> ... this.

Quite possibly, following the lead of the singlestepping code just
seemed like the most straightforward way out of the problem. Of course
an alternative way of handling interrupts would probably be preferrable,
however we'd need a bit of guidance on how to go about it and in the
meantime I don't see how this small fix would hurt.

I remember Andrew suggesting taking something like this on at the Xen
Developer Summit in Budapest but then of course much more important
things happened with Meltdown & al.


Thanks,
Razvan

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-12-13 15:11 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-10 16:01 [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events Razvan Cojocaru
2018-12-10 16:49 ` Roger Pau Monné
2018-12-10 16:59   ` Razvan Cojocaru
2018-12-11 10:01     ` Razvan Cojocaru
2018-12-11 10:14       ` Roger Pau Monné
2018-12-11 10:21         ` Razvan Cojocaru
2018-12-11 11:59           ` Julien Grall
2018-12-11 12:33             ` Razvan Cojocaru
2018-12-13  6:54               ` Tian, Kevin
2018-12-13  8:03                 ` Razvan Cojocaru
2018-12-13 12:04                   ` Julien Grall
2018-12-13 12:15                     ` Razvan Cojocaru
2018-12-13 12:39                       ` Julien Grall
2018-12-13 13:18                         ` Razvan Cojocaru
2018-12-13 14:58                           ` Jan Beulich
2018-12-13 15:11                             ` Razvan Cojocaru

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.