Re: [Xen-devel] [PATCH] x86/vm_event: Added support for VM_EVENT_REASON_INTERRUPT

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.11.16 at 12:32, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 11/09/2016 01:17 PM, Jan Beulich wrote:
>>>>> On 09.11.16 at 10:42, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -532,11 +532,23 @@ void hvm_do_resume(struct vcpu *v)
>>>          }
>>>      }
>>>  
>>> -    /* Inject pending hw/sw trap */
>>> -    if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )
>>> -    {
>>> +    /* Inject pending hw/sw trap if there are no other pending interrupts. 
>>> */
>>> +    if ( v->arch.hvm_vcpu.inject_trap.vector != -1 && 
>>> !hvm_event_pending(v) )
>>>          hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap);
>>> -        v->arch.hvm_vcpu.inject_trap.vector = -1;
>>> +
>>> +    v->arch.hvm_vcpu.inject_trap.vector = -1;
>> 
>> I don't see why you pull this out of the if() body.
> 
> That is intended, and covered by the "the patch also fixes the behaviour
> of the xc_hvm_inject_trap() hypercall, which would lead to
> non-architectural interrupts overwriting pending (specifically
> reinjected) architectural ones" part of the patch description.
> 
> If we couldn't inject the trap because there was a pending event (i.e.
> the second if() condition, then not setting
> v->arch.hvm_vcpu.inject_trap.vector to -1 would lead to the trap being
> kept for injection at the first opportunity - and that could be when the
> context has changed and we shouldn't inject it anymore. So
> v->arch.hvm_vcpu.inject_trap.vector is therefore reset either way.

Ah, that's because you extend the condition. How about you leave
the condition as is, and only make the actual call conditonal
upon hvm_event_pending()'s return value? That's also make the
patch better readable.

Jan


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