Re: [Xen-devel] [PATCH] x86/upcall: inject a spurious event after setting upcall vector

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jan 04, 2018 at 03:53:39AM -0700, Jan Beulich wrote:
> >>> On 04.01.18 at 10:13, <roger.pau@xxxxxxxxxx> wrote:
> > On Tue, Jan 02, 2018 at 09:47:40AM -0700, Jan Beulich wrote:
> >> >>> On 28.12.17 at 13:57, <roger.pau@xxxxxxxxxx> wrote:
> >> > In case the vCPU has pending events to inject. This fixes a bug that
> >> > happened if the guest mapped the vcpu info area using
> >> > VCPUOP_register_vcpu_info without having setup the event channel
> >> > upcall, and then setup the upcall vector.
> >> > 
> >> > In this scenario the guest would not receive any upcalls, because the
> >> > call to VCPUOP_register_vcpu_info would have marked the vCPU as having
> >> > pending events, but the vector could not be injected because it was
> >> > not yet setup.
> >> > 
> >> > This has not caused issues so far because all the consumers first
> >> > setup the vector callback and then map the vcpu info page, but there's
> >> > no limitation that prevents doing it in the inverse order.
> >> 
> >> Hmm, yes, okay, I can see that we may indeed want to do this for
> >> symmetry reasons. There is a small theoretical risk of this causing
> >> races, though, for not entirely well written guest drivers.
> > 
> > Not exactly. In the scenario described above not injecting this event
> > will cause further events to not be injected also. This is because
> > VCPUOP_register_vcpu_info sets evtchn_upcall_pending and tries to
> > inject an event using arch_evtchn_inject. If the vector is not set at
> > this point, arch_evtchn_inject will do nothing, but
> > evtchn_upcall_pending will be left set.
> > 
> > Further calls to vcpu_mark_events_pending will not call into
> > hvm_assert_evtchn_irq because the pending bit is already set, thus
> > preventing the delivery of any event channel interrupts.
> 
> I understand that, but I don't understand how this relates to my
> comment.

I don't think this applies to "not entirely well written drivers". I
think it's perfectly fine for a guest to map the vcpu_info page first
and then setup the vector callback. With the current code doing this
will result in no interrupts being injected.

> In any event, an option to at least consider would be to clear the
> pending indicator again if the vector was found unset.

That seems more complicated than the solution proposed here.

> >> > @@ -447,6 +448,9 @@ void hvm_set_callback_via(struct domain *d, uint64_t 
> >> > via)
> >> >  
> >> >      spin_unlock(&d->arch.hvm_domain.irq_lock);
> >> >  
> >> > +    for_each_vcpu(d, v)
> >> > +        arch_evtchn_inject(v);
> >> 
> >> Wouldn't it make sense to limit this to actually active vCPU-s?
> > 
> > Since it's not a hot-path I didn't bother, but I guess using
> > is_vcpu_online should be fine?
> 
> I think so, yes. The less work in such for-each-vcpu loops, the
> better.

Thanks, I'm going to send a new version with those two issues fixed
then.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel