diff mbox series

[v2,2/4] x86/vioapic: issue EOI to dpci when switching pin to edge trigger mode

Message ID 20210115142820.35224-3-roger.pau@citrix.com (mailing list archive)
State New
Headers show
Series x86/intr: guest interrupt handling fixes/cleanup | expand

Commit Message

Roger Pau Monné Jan. 15, 2021, 2:28 p.m. UTC
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(+)

Comments

Jan Beulich Jan. 21, 2021, 4:23 p.m. UTC | #1
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
Roger Pau Monné Jan. 21, 2021, 5:45 p.m. UTC | #2
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.
Jan Beulich Jan. 22, 2021, 9:13 a.m. UTC | #3
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 mbox series

Patch

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 )
     {
         /*