Re: [PATCH for-4.14 3/8] x86/hvm: fix ISA IRQ 0 handling when set as lowest priority mode in IO APIC

From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: xen-devel@lists.xenproject.org,
	Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	paul@xen.org
Subject: Re: [PATCH for-4.14 3/8] x86/hvm: fix ISA IRQ 0 handling when set as lowest priority mode in IO APIC
Date: Thu, 18 Jun 2020 17:20:45 +0200	[thread overview]
Message-ID: <8d3c5b8f-ccf2-cc11-cf74-11e21e80f1a1@suse.com> (raw)
In-Reply-To: <20200618145506.GT735@Air-de-Roger>

On 18.06.2020 16:55, Roger Pau Monné wrote:
> On Thu, Jun 18, 2020 at 04:26:08PM +0200, Jan Beulich wrote:
>> On 12.06.2020 17:56, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/hvm/vioapic.c
>>> +++ b/xen/arch/x86/hvm/vioapic.c
>>> @@ -422,12 +422,13 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, unsigned int pin)
>>>      case dest_LowestPrio:
>>>      {
>>>  #ifdef IRQ0_SPECIAL_ROUTING
>>> -        /* Force round-robin to pick VCPU 0 */
>>> -        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() )
>>> -        {
>>> -            v = d->vcpu ? d->vcpu[0] : NULL;
>>> -            target = v ? vcpu_vlapic(v) : NULL;
>>> -        }
>>> +        struct vlapic *lapic0 = vcpu_vlapic(d->vcpu[0]);
>>> +
>>> +        /* Force to pick vCPU 0 if part of the destination list */
>>> +        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() &&
>>> +             vlapic_match_dest(lapic0, NULL, 0, dest, dest_mode) &&
>>> +             vlapic_enabled(lapic0) )
>>
>> The vlapic_enabled() part needs justification in the commit message
>> (if it is to stay), the more that the other path that patch 2 touched
>> doesn't have / gain it. I'm unconvinced this is a helpful check here
>> (or anywhere when it's not current's LAPIC that gets probed), as its
>> result may be stale right after probing.
> 
> This is modeled after what vlapic_lowest_prio does, which includes the
> vlapic_enabled check. I assumed this was done to prevent injecting to
> disabled lapics if possible.

All understood, but it wants justifying like that in the description,
and the discrepancy to the fixed dest mode wants taking care of
(either again verbally, or by a code change).

> I agree it's stale by the point it gets acted upon, but anyone playing
> with enabling/disabling a lapic part of a destination list shouldn't
> expect anything sensible to happen IMO.

Well, yes, agreed.

>> Having thought about this (including patch 2) some more, I also wonder
>> whether, if no destination match was found, the IRQ0_SPECIAL_ROUTING
>> hack should become to nevertheless deliver to CPU0.
> 
> Hm, that wouldn't match what real hardware would do, but would indeed
> match what old Xen would do for IRQ 0. TBH I would be more comfortable
> with attempting to remove this behaviour, and hence don't inject to
> any vCPU if none match the list.

I agree from an abstract perspective. But at the same time I fear
hard to understand / debug regressions. I'd be curious to know what
others think ...

Jan