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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 04/27/17 12:00, Jan Beulich wrote:
>>>> 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().

All fair points. If there are no objections, I'll add x86/hvm/vm_event.c
and asm-x86/hvm/vm_event.h, and place hvm_vm_do_resume() in it. I'll
also add the files to MAINTAINERS under vm_event.

In the process of moving the code, I'll also put the MSR write last.

This will then become a 2-patch series, with the 1st patch doing the
above, and the second patch will be this one.


Thanks,
Razvan

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