On Fri, Jun 25 2021 at 10:43, Thomas Gleixner wrote:
> On Fri, Jun 25 2021 at 05:21, Kevin Tian wrote:
>> p.s. one question to Thomas. As Alex cited above, software must
>> not modify the Address, Data, or Steering Tag fields of an MSI-X
>> entry while it is unmasked. However this rule might be violated
>> today in below flow:
>>
>> request_irq()
>> __setup_irq()
>> irq_startup()
>> __irq_startup()
>> irq_enable()
>> unmask_irq() <<<<<<<<<<<<<
>> irq_setup_affinity()
>> irq_do_set_affinity()
>> msi_set_affinity() // when IR is disabled
>> irq_msi_update_msg()
>> pci_msi_domain_write_msg() <<<<<<<<<<<<<<
>>
>> Isn't above have msi-x entry updated after it's unmasked?
>
> Dammit, I could swear that we had masking at the core or PCI level at
> some point. Let me dig into this.
Indeed, that code path does not check irq_can_move_pcntxt(). It doesn't
blow up in our face by chance because of this:
__setup_irq()
irq_activate()
unmask()
irq_setup_affinity()
irq_activate() assigns a vector based on the affinity mask so
irq_setup_affinity() ends up writing the same data again pointlessly.
For some stupid reason the ordering of startup/setup_affinity is the way
it is for historical reasons. I tried to reorder it at some point but
that caused failure on !x86 so I went back to the status quo.
All other affinity settings happen with the interrupt masked because we
do that from actual interrupt context via irq_move_masked_irq() which
does the right thing.
Let me fix that proper for the startup case.
Thanks,
tglx
