Re: [Xen-devel] xc_hvm_inject_trap() races

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 11/07/2016 03:53 PM, Jan Beulich wrote:
>>>> On 07.11.16 at 14:44, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 11/07/2016 03:20 PM, Jan Beulich wrote:
>>>>>> On 07.11.16 at 13:27, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>> On 11/07/2016 02:23 PM, Jan Beulich wrote:
>>>>>> At this point it looks like the best solution is to simply discard the
>>>>>>> non-architectural event if there's a pending architectural one, and add
>>>>>>> a new vm_event, as suggested by Tamas, that notifies the application
>>>>>>> that a trap has failed, or succeeded, and let it do the best it can with
>>>>>>> that information.
>>>>> Well, if the two of you think this is something which fits into the VM
>>>>> event model, then I guess that's an option. I, for one, am not
>>>>> convinced: It simply seems too special purpose. If this was a more
>>>>> generic event ("interruption delivered", perhaps with a type or
>>>>> vector qualifier) that can be subscribed to, and the app did that
>>>>> only for such transient periods of time, this would look more
>>>>> reasonable to me.
>>>>
>>>> Indeed, that's the way I think of it: the user is free to subscribe to
>>>> the event or not, and the event is:
>>>>
>>>> struct vm_event_injection_result {
>>>>     uint32_t vector;
>>>>     uint32_t type;
>>>>     uint32_t error_code;
>>>>     uint64_t cr2;
>>>>     uint32_t injected;
>>>> };
>>>>
>>>> with injected 0 for failure and 1 for success. It's as generic as possible.
>>>
>>> Wait, no, that's not what I did describe. I'm not talking about the
>>> "result" of an injection (through hypercall), but about delivering of
>>> events (of any origin). Hence there can't be any "failure" here.
>>> IOW what I'm proposing is a "VM is about to see this interruption"
>>> event.
>>
>> But a a success-only event is hard to interpret with regards to failure,
>> which is what we're really interested in (specifically, failure to
>> inject via hypercall). Do we count it as a failure if we don't receive a
>> "VM is about to see this interruption" event immediately after the
>> vm_event that caused the injection, on the same processor, with the same
>> trap vector? That's a tricky commitment to make.
> 
> If you subscribed to all interruptions, you'd simply see one next
> which is different from the one you've tried to inject.
> 
>> That would also decrease performance, likely noticeably, for a
>> vm_event-based application, as we'd get many such events (most of which
>> we're not interested in at all) which would require treatment while the
>> respective VCPU is paused.
> 
> You should limit the periods of time when you enable the
> subscription (e.g. only from when you inject an event until
> you did see it arrive). Perhaps such a subscription should then
> be per-vCPU ...

I think there might be a design argument against this model: using a
generic event implies that there are (or there might be) users
interested in long-term following interrupt events. However, that's
clearly impractical, since this would effectively render the guest unusable.

So the real use case would be to just use it, as you say, sparingly, for
just a few events - but in this case the event was never meant to be
followed more than figuring out if, for example, outside injection
failed, which can more readily be accomplished with a single, dedicated
event.

My proposal was simply something along the lines of:

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 704fd64..58f5ae4 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -535,8 +535,22 @@ void hvm_do_resume(struct vcpu *v)
     /* Inject pending hw/sw trap */
     if ( v->arch.hvm_vcpu.inject_trap.vector != -1 )
     {
-        hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap);
+        unsigned int success = 0;
+
+        /* Check for already pending interrupts (races). */
+        if ( !hvm_event_pending(v) )
+        {
+            hvm_inject_trap(&v->arch.hvm_vcpu.inject_trap);
+            success = 1;
+        }
+
         v->arch.hvm_vcpu.inject_trap.vector = -1;
+
+        hvm_monitor_injection_result(v->arch.hvm_vcpu.inject_trap.vector,
+                                     v->arch.hvm_vcpu.inject_trap.type,
+
v->arch.hvm_vcpu.inject_trap.error_code,
+                                     v->arch.hvm_vcpu.inject_trap.cr2,
+                                     success);
     }
 }


Thanks,
Razvan

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