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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.18 at 13:15, <roger.pau@xxxxxxxxxx> wrote:
> On Thu, Jan 04, 2018 at 05:10:52AM -0700, Jan Beulich wrote:
>> >>> On 04.01.18 at 12:37, <roger.pau@xxxxxxxxxx> wrote:
>> > 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.
>> 
>> My meaning of "not entirely well written drivers" is unrelated to
>> the scenario you want to fix: When setting the upcall vector, an
>> event would now be received at a time that the drivers may not
>> expect (iirc there's no requirement to relocate the vcpu info out
>> of the shared info page, and even if there was the issue described
>> would still arise if the vector was changed later on).
> 
> Sorry, I misunderstood the context of that sentence. Regarding your
> point, yes, that's right, drivers should be able to cope with spurious
> interrupts. I would be very surprised if an event channel driver is not
> able to cope with spurious interrupts.

I'm not concerned about spurious interrupts in general (i.e. in
the middle of some other operations). What I'm (slightly) worried
about is a driver getting the setup order wrong, and receiving
an interrupt when its handler isn't fully ready yet.

Jan


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