Re: [Xen-devel] [PATCH] x86/vm_event: fix race between vmx_vmexit_handler() and vm_event_resume()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.04.17 at 10:34, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 04/27/17 11:18, Jan Beulich wrote:
>>>>> On 27.04.17 at 10:11, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> On 04/27/17 11:01, Jan Beulich wrote:
>>>>>>> On 27.04.17 at 09:22, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -473,6 +473,39 @@ static bool hvm_get_pending_event(struct vcpu *v, 
> struct x86_event *info)
>>>>>      return hvm_funcs.get_pending_event(v, info);
>>>>>  }
>>>>>  
>>>>> +static void hvm_vm_event_set_registers(struct vcpu *v)
>>>>> +{
>>>>> +    ASSERT(v == current);
>>>>> +
>>>>> +    if ( v->arch.vm_event->set_gprs )
>>>>
>>>> I think we will want an unlikely() here. We anyway can only hope for
>>>> the compiler to always inline this function, such that non-VM-event
>>>> setups don't get penalized by the extra call here. Strictly speaking
>>>> the function doesn't belong into this file ...
>>>
>>> Should I move it to the x86 vm_event code?
>> 
>> If you do, then you'll want to have an inline wrapper with the if()
>> in it, so the actual call at the machine level would be avoided in the
>> common case.
> 
> Sorry, I'm not sure I understand: if moving it is not required, I'd
> prefer to leave it where it is, as we already have
> vm_event_set_registers() in arch/x86/vm_event.c and I feel it would
> complicate matters there (I'd have to perhaps prepend "hvm_" to it in
> which case it wouldn't really belong in vm_event.c either). But if it's
> necessary, I'll move it - do you want me to move it?

Well, logically this is a VM event function, so belongs into vm_event.c.
So I'd _prefer_ for it to be moved, but I don't insist (in the end, by
leaving it where it is, you and Tamas [wrongly imo] are not the
maintainers of it). But I agree, x86/vm_event.c isn't an ideal place
either, better would be x86/hvm/vm_event.c.

Otoh hvm_do_resume() open codes all the CR and MSR writes
(getting the order wrong as it seems, btw, as some MSR writes
depend on certain CR settings), so maybe a separate function isn't
needed at all here? In fact the bulk of the function is VM event
code, so one might also view it the other way around, and most or
all of this should move into a new function, for example
hvm_vm_event_do_resume().

Jan


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