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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

Jan