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

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

On Fri, Nov 25, 2022 at 09:43:59AM +0100, Jan Beulich wrote:
> On 24.11.2022 16:12, Roger Pau Monné wrote:
> > 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?
> 
> I think for the purpose of the one vector doing the EOI late is okay,
> but aiui the goal of doing it early for edge triggered interrupts in
> general (and yet more generally as early as possible) is to unmask
> lower priority vectors as well.

My reasoning for doing it late was in order to avoid adding extra
latency to things like the timer handling, as the EOI will likely
trigger a vmexit.

> Of course that's useful only if IRQs
> as a whole are unmasked during (part of) the handling.

What do you mean with IRQs as a whole?  Are you referring to setting
the interrupt flag?

Thanks for the input, it's appreciated, and sorry for diverging the
conversation so much.

Roger.