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

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

On Mon, Nov 21, 2022 at 09:33:53AM +0100, Jan Beulich wrote:
> On 18.11.2022 15:26, Roger Pau Monné wrote:
> > On Fri, Nov 18, 2022 at 11:31:28AM +0100, Jan Beulich wrote:
> >> Linux'es relatively new use of HVMOP_set_evtchn_upcall_vector has
> >> exposed a problem with the marking of the respective vector as
> >> pending: For quite some time Linux has been checking whether any stale
> >> ISR or IRR bits would still be set while preparing the LAPIC for use.
> >> This check is now triggering on the upcall vector, as the registration,
> >> at least for APs, happens before the LAPIC is actually enabled.
> >>
> >> In software-disabled state an LAPIC would not accept any interrupt
> >> requests and hence no IRR bit would newly become set while in this
> >> state. As a result it is also wrong for us to mark the upcall vector as
> >> having a pending request when the vLAPIC is in this state.
> >>
> >> To compensate for the "enabled" check added to the assertion logic, add
> >> logic to (conditionally) mark the upcall vector as having a request
> >> pending at the time the LAPIC is being software-enabled by the guest.
> >>
> >> Fixes: 7b5b8ca7dffd ("x86/upcall: inject a spurious event after setting 
> >> upcall vector")
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> ---
> >> Don't one or both of the Viridian uses of vlapic_set_irq() need similar
> >> guarding?
> >>
> >> Is it actually necessary for hvmop_set_evtchn_upcall_vector() and
> >> hvm_set_callback_via() to call hvm_assert_evtchn_irq() when
> >> evtchn_upcall_pending is false?
> >>
> >> --- a/xen/arch/x86/hvm/irq.c
> >> +++ b/xen/arch/x86/hvm/irq.c
> >> @@ -321,9 +321,10 @@ void hvm_assert_evtchn_irq(struct vcpu *
> >>  
> >>      if ( v->arch.hvm.evtchn_upcall_vector != 0 )
> >>      {
> >> -        uint8_t vector = v->arch.hvm.evtchn_upcall_vector;
> >> +        struct vlapic *vlapic = vcpu_vlapic(v);
> >>  
> >> -        vlapic_set_irq(vcpu_vlapic(v), vector, 0);
> >> +        if ( vlapic_enabled(vlapic) )
> >> +           vlapic_set_irq(vlapic, v->arch.hvm.evtchn_upcall_vector, 0);
> > 
> > Shouldn't the vlapic_enabled() check itself be in vlapic_set_irq()? We
> > certainly don't want any vectors set until the vlapic is enabled, be
> > it event channel upcalls or any other sources.
> 
> In principle yes, and I did consider doing so, but for several callers
> (potentially used frequently) this would be redundant with other
> checking they do already (first and foremost callers using
> vlapic_lowest_prio()). However, looking again I think vioapic_deliver()
> and vmsi_deliver() violate this as well in their dest_Fixed handling.
> (In both cases I'm actually inclined to also remove the odd *_inj_irq()
> helper functions.)
> 
> > Maybe best to add an ASSERT in vlapic_set_irq() to be sure the lapic is
> > enabled, as other callers already check this before trying to inject?
> 
> Perhaps, yes (once we've fixed paths where the check is presently
> missing).

Another option would be to unconditionally return 0 for IRR and ISR
reads when the LAPIC is disabled, that would avoid having to force the
event channel injection when the LAPIC is enabled, but there could be
more than just the event channel vector queued in that way which would
be against the spec.

> > Also, and not strictly related to your change, isn't this possibly
> > racy, as by the time you evaluate the return of vlapic_enabled() it is
> > already stale, as there's no lock to protect it from changing?
> 
> Wouldn't this simply match a signal arriving to a physical LAPIC just
> the moment before it is enabled?

Hm, yes, any guest trying to play this kind of games with the APIC is
free to keep the pieces.

Thanks, Roger.