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

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

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.

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.

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

Thanks, Roger.