Re: [Xen-devel] [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel