Re: [PATCH] x86/HVM: don't mark evtchn upcall vector as pending when vLAPIC is disabled

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

On Thu, Nov 24, 2022 at 12:16:13PM +0100, Jan Beulich wrote:
> On 24.11.2022 10:33, Roger Pau Monné wrote:
> > On Thu, Nov 24, 2022 at 10:11:05AM +0100, Jan Beulich wrote:
> >> On 24.11.2022 10:06, Roger Pau Monné wrote:
> >>> On Thu, Nov 24, 2022 at 09:42:40AM +0100, Roger Pau Monné wrote:
> >>>> On Thu, Nov 24, 2022 at 08:59:00AM +0100, Jan Beulich wrote:
> >>>>> - problematic wrt evtchn_upcall_pending, once set, preventing event
> >>>>>   injection later on.
> >>>>> As you may have inferred already, I'm inclined to suggest to drop the
> >>>>> the is_vcpu_online() check from hvm_set_callback_via().
> >>>>>
> >>>>> One related question here is whether vlapic_do_init() shouldn't have
> >>>>> the non-architectural side effect of clearing evtchn_upcall_pending.
> >>>>> While this again violates the principle of the hypervisor only ever
> >>>>> setting that bit, it would deal with the risk of no further event
> >>>>> injection once the flag is set, considering that vlapic_do_init()
> >>>>> clears IRR (and ISR).
> >>>>
> >>>> That would seem sensible to me, and was kind of what I was suggesting
> >>>> in:
> >>>>
> >>>> https://lore.kernel.org/xen-devel/Y3eO0bMKRPYJc2yQ@Air-de-Roger/
> >>>
> >>> Another option would be for vcpu_mark_events_pending() to
> >>> unconditionally call hvm_assert_evtchn_irq() regardless of the state
> >>> of evtchn_upcall_pending.
> >>
> >> I think you said so before, and ...
> >>
> >>>  This will create some spurious events.
> >>
> >> ... I continue to be afraid of s/some/many/.
> > 
> > Not _that_ many I think, as we can only queue one pending interrupt in
> > IRR.
> 
> We need to be careful here - the kernel treating it as "edge" (like
> any other interrupt coming directly from the LAPIC), it ack-s it
> before calling the handler, i.e. before evtchn_upcall_pending would
> have a chance to be cleared. See Linux'es sysvec_xen_hvm_callback().

Hm, that's not how I handle it on FreeBSD, where the vector is acked
after calling the handler (evtchn_upcall_pending gets cleared before
the EOI).  Maybe there's some corner case I'm missing that requires
the EOI to be performed before clearing evtchn_upcall_pending?

> >> This event delivery is meant
> >> to be kind of edge triggered, and I think it is a reasonable model to treat
> >> the flag going from 0 to 1 as the "edge".
> > 
> > Hm, it's a weird interrupt model I would say.  In some aspects it's
> > similar to level (as the line stays asserted until it's cleared), but
> > we don't get new interrupts injected into the APIC.
> > 
> > Maybe the right mode would be to treat evtchn_upcall_pending as a
> > level triggered line and keep injecting interrupts until the line is
> > deasserted (ie: evtchn_upcall_pending == 0)?
> 
> As said above, and as also pointed out by Andrew on another sub-
> thread when discussing the (dis)similarity with IO-APIC connected
> interrupts, at the LAPIC there's no edge/level distinction anymore,
> as we're not dealing with "asserted" signals there, but just with
> messages sent on the bus.

Right, we could likely fake the behavior I've listed above, but not
sure it's worth it.

We should likely have some of this documented in xen.h next to the
declaration of evtchn_upcall_pending, at least to note that once
evtchn_upcall_pending is set no further vector callbacks will get
injected until the field is cleared.

Roger.