| Message ID | 20200612155640.4101-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/vpt: fixes for vpt and enable vPIT for PVH dom0 | expand |
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. 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. Jan
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.
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
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c index 67472e5934..e1417cc6a7 100644 --- 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) ) + target = lapic0; else #endif target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode);
Lowest priority destination mode does allow the vIO APIC code to select a vCPU to inject the interrupt to, but the selected vCPU must be part of the possible destinations configured for such IO APIC pin. Fix the code in order to only force vCPU 0 if it's part of the listed destinations. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/hvm/vioapic.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)