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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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