| Message ID | 20210115142820.35224-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/intr: guest interrupt handling fixes/cleanup | expand |
On 15.01.2021 15:28, Roger Pau Monne wrote: > --- a/xen/arch/x86/hvm/vioapic.c > +++ b/xen/arch/x86/hvm/vioapic.c > @@ -268,6 +268,17 @@ static void vioapic_write_redirent( > > spin_unlock(&d->arch.hvm.irq_lock); > > + if ( ent.fields.trig_mode == VIOAPIC_EDGE_TRIG && > + ent.fields.remote_irr && is_iommu_enabled(d) ) > + /* > + * Since IRR has been cleared and further interrupts can be > + * injected also attempt to deassert any virtual line of passed > + * through devices using this pin. Switching a pin from level to > + * trigger mode can be used as a way to EOI an interrupt at the > + * IO-APIC level. > + */ > + hvm_dpci_eoi(d, gsi); > + > if ( is_hardware_domain(d) && unmasked ) > { > /* I assume in the comment you mean "... from level to edge mode ...". But this isn't reflected in the if() you add - you do the same also when the mode doesn't change. Or do you build on the assumption that ent.fields.remote_irr can only be set if the prior mode was "level" (in which case an assertion may be warranted, as I don't think this is overly obvious)? Also, looking at this code, is it correct to trigger an IRQ upon the guest writing the upper half of an unmasked RTE with remote_irr clear? I'd assume this needs to be strictly limited to a 1->0 transition of the mask bit. If other code indeed guarantees this in all cases, perhaps another place where an assertion would be warranted? Jan
On Thu, Jan 21, 2021 at 05:23:04PM +0100, Jan Beulich wrote: > On 15.01.2021 15:28, Roger Pau Monne wrote: > > --- a/xen/arch/x86/hvm/vioapic.c > > +++ b/xen/arch/x86/hvm/vioapic.c > > @@ -268,6 +268,17 @@ static void vioapic_write_redirent( > > > > spin_unlock(&d->arch.hvm.irq_lock); > > > > + if ( ent.fields.trig_mode == VIOAPIC_EDGE_TRIG && > > + ent.fields.remote_irr && is_iommu_enabled(d) ) > > + /* > > + * Since IRR has been cleared and further interrupts can be > > + * injected also attempt to deassert any virtual line of passed > > + * through devices using this pin. Switching a pin from level to > > + * trigger mode can be used as a way to EOI an interrupt at the > > + * IO-APIC level. > > + */ > > + hvm_dpci_eoi(d, gsi); > > + > > if ( is_hardware_domain(d) && unmasked ) > > { > > /* > > I assume in the comment you mean "... from level to edge > mode ...". Yes, that's right, I completely missed it, sorry. > But this isn't reflected in the if() you add - > you do the same also when the mode doesn't change. Or do > you build on the assumption that ent.fields.remote_irr can > only be set if the prior mode was "level" (in which case > an assertion may be warranted, as I don't think this is > overly obvious)? Yes, IRR is only set for level triggered interrupts, so it's indeed build on the assumption that a pin can only have had IRR set when in edge mode when it's being switched from level to edge. I can add an assertion. > Also, looking at this code, is it correct to trigger an IRQ > upon the guest writing the upper half of an unmasked RTE > with remote_irr clear? I'd assume this needs to be strictly > limited to a 1->0 transition of the mask bit. If other code > indeed guarantees this in all cases, perhaps another place > where an assertion would be warranted? Indeed. I don't think it should be possible for a write to the upper half to trigger the injection of an interrupt, as having gsi_assert_count > 0 would imply that either IRR is already set, or that the pin is masked when processing an upper write. I can add that a pre-patch if you agree. In fact we could almost short-circuit the logic after the *pent = ent; line for upper writes if it wasn't for the call to vlapic_adjust_i8259_target, the rest of the code there shouldn't matter for upper writes. And the i8259 target logic that we have is very dodgy I would say. I have plans to fix it at some point, but that requires fixing the virtual periodic timers logic first, which I didn't get around to re-posting. Thanks, Roger.
On 21.01.2021 18:45, Roger Pau Monné wrote: > On Thu, Jan 21, 2021 at 05:23:04PM +0100, Jan Beulich wrote: >> On 15.01.2021 15:28, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/hvm/vioapic.c >>> +++ b/xen/arch/x86/hvm/vioapic.c >>> @@ -268,6 +268,17 @@ static void vioapic_write_redirent( >>> >>> spin_unlock(&d->arch.hvm.irq_lock); >>> >>> + if ( ent.fields.trig_mode == VIOAPIC_EDGE_TRIG && >>> + ent.fields.remote_irr && is_iommu_enabled(d) ) >>> + /* >>> + * Since IRR has been cleared and further interrupts can be >>> + * injected also attempt to deassert any virtual line of passed >>> + * through devices using this pin. Switching a pin from level to >>> + * trigger mode can be used as a way to EOI an interrupt at the >>> + * IO-APIC level. >>> + */ >>> + hvm_dpci_eoi(d, gsi); >>> + >>> if ( is_hardware_domain(d) && unmasked ) >>> { >>> /* >> >> I assume in the comment you mean "... from level to edge >> mode ...". > > Yes, that's right, I completely missed it, sorry. > >> But this isn't reflected in the if() you add - >> you do the same also when the mode doesn't change. Or do >> you build on the assumption that ent.fields.remote_irr can >> only be set if the prior mode was "level" (in which case >> an assertion may be warranted, as I don't think this is >> overly obvious)? > > Yes, IRR is only set for level triggered interrupts, so it's indeed > build on the assumption that a pin can only have had IRR set when in > edge mode when it's being switched from level to edge. > > I can add an assertion. > >> Also, looking at this code, is it correct to trigger an IRQ >> upon the guest writing the upper half of an unmasked RTE >> with remote_irr clear? I'd assume this needs to be strictly >> limited to a 1->0 transition of the mask bit. If other code >> indeed guarantees this in all cases, perhaps another place >> where an assertion would be warranted? > > Indeed. I don't think it should be possible for a write to the upper > half to trigger the injection of an interrupt, as having > gsi_assert_count > 0 would imply that either IRR is already set, or > that the pin is masked when processing an upper write. > > I can add that a pre-patch if you agree. I do, yes. > In fact we could almost short-circuit the logic after the *pent = ent; > line for upper writes if it wasn't for the call to > vlapic_adjust_i8259_target, the rest of the code there shouldn't > matter for upper writes. And the i8259 target logic that we have is > very dodgy I would say. I have plans to fix it at some point, but > that requires fixing the virtual periodic timers logic first, which I > didn't get around to re-posting. True. Looking forward to it. Jan
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c index 804bc77279..dc35347afa 100644 --- a/xen/arch/x86/hvm/vioapic.c +++ b/xen/arch/x86/hvm/vioapic.c @@ -268,6 +268,17 @@ static void vioapic_write_redirent( spin_unlock(&d->arch.hvm.irq_lock); + if ( ent.fields.trig_mode == VIOAPIC_EDGE_TRIG && + ent.fields.remote_irr && is_iommu_enabled(d) ) + /* + * Since IRR has been cleared and further interrupts can be + * injected also attempt to deassert any virtual line of passed + * through devices using this pin. Switching a pin from level to + * trigger mode can be used as a way to EOI an interrupt at the + * IO-APIC level. + */ + hvm_dpci_eoi(d, gsi); + if ( is_hardware_domain(d) && unmasked ) { /*
When an IO-APIC pin is switched from level to edge trigger mode the IRR bit is cleared, so it can be used as a way to EOI an interrupt at the IO-APIC level. Such EOI however does not get forwarded to the dpci code like it's done for the local APIC initiated EOI. This change adds the code in order to notify dpci of such EOI, so that dpci and the interrupt controller are in sync. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/hvm/vioapic.c | 11 +++++++++++ 1 file changed, 11 insertions(+)