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

[Jan Beulich <jbeulich@xxxxxxxx>]

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