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 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.

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?

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?

Thanks, Roger.