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.
