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 <jbeul...@suse.com>
>> ---
>> 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
