[QUERY] Using same ITS device ID for two PCI devices (two PCI Requestor ID)

[QUERY] Using same ITS device ID for two PCI devices (two PCI Requestor ID)

[Kishon Vijay Abraham I]

Hi Marc, Thomas,

TI's K3 platforms use GIT ITS for PCIe MSI/MSI-X interrupts. It uses
*pre_its_window* as implemented by *its_enable_quirk_socionext_synquacer* in
irq-gic-v3-its.c. So PCIe controller instead of directly writing to
GITS_TRANSLATER, will write to a separate window and the device ID is taken from
the offset to which the PCIe device writes (instead of dedicated lines from PCIe
controller to GIC ITS). So every 4-byte register address maps in this window
maps to a unique ITS device id.

All of this is already implemented in Linux Kernel
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-gic-v3-its.c#n4645

Now TI's AM64 has an issue in that it doesn't trigger interrupt if the address
in the *pre_its_window* is not aligned to 8-bytes (this is due to an invalid
bridge configuration in HW).

This means there will not be interrupts for devices with PCIe requestor ID, 0x1,
0x3, 0x5..., as the address in the pre-ITS window would be 4 (1 << 2), 12 (3 <<
2), 20 (5 << 2) respectively.

So in order to provide 8 byte aligned address always, I mapped the PCIe
requestor ID to ITS device ID such that it always provides 8-byte aligned
address. The DT property like below helped me achieve that.

msi-map = <0x0 &gic_its 0x0 0x10000>;
msi-map-mask = <0xfffe>;

So this would result in creating one "struct its_device" for 2 PCIe devices and
the pre-ITS address will be aligned to 8-bytes.

However with this, its_alloc_device_irq() for the 2nd PCIe device is failing
since we create "struct its_device" with the number of interrupt vectors
requested by the 1st PCIe device.

Would like to get your opinion on what would be the best way to workaround this
for AM64.

One option would be to create a new compatible for AM64 ("ti,am64,gic-v3-its")
allocate a minimal number of interrupt vector while creating "struct
its_device". Would that be acceptable? Any other ideas?

Thanks,
Kishon

Re: [QUERY] Using same ITS device ID for two PCI devices (two PCI Requestor ID)

[Kishon Vijay Abraham I]

Hi Marc,

On 16/09/21 11:43 pm, Marc Zyngier wrote:
> Kishon,
> 
> On Thu, 16 Sep 2021 14:02:58 +0100,
> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> Hi Marc, Thomas,
>>
>> TI's K3 platforms use GIT ITS for PCIe MSI/MSI-X interrupts. It uses
>> *pre_its_window* as implemented by
>> *its_enable_quirk_socionext_synquacer* in irq-gic-v3-its.c.
> 
> I see it coming... If it sounds like a car crash, if it smells like a
> car crash, it probably is a car crash...
> 
>> So PCIe controller instead of directly writing to GITS_TRANSLATER,
>> will write to a separate window and the device ID is taken from the
>> offset to which the PCIe device writes (instead of dedicated lines
>> from PCIe controller to GIC ITS). So every 4-byte register address
>> maps in this window maps to a unique ITS device id.
>>
>> All of this is already implemented in Linux Kernel
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-gic-v3-its.c#n4645
>>
>> Now TI's AM64 has an issue in that it doesn't trigger interrupt if
>> the address in the *pre_its_window* is not aligned to 8-bytes (this
>> is due to an invalid bridge configuration in HW).
>>
>> This means there will not be interrupts for devices with PCIe
>> requestor ID, 0x1, 0x3, 0x5..., as the address in the pre-ITS window
>> would be 4 (1 << 2), 12 (3 << 2), 20 (5 << 2) respectively.
> 
> Let me get this straight: instead of using the existing infrastructure
> and propagating the RIDs to the ITS, TI decided to go our of their way
> to copy the Socionext utter madness, something that was already a
> glaring bug, and managed to add a bug of their own on top of it?

unfortunately yes.
> 
> Not only this completely breaks device isolation (the Socionext bug),
> but you can't even have an odd-numbered function generating MSIs? Good

Yes.
> thing I'm on holiday, I can have an early drink to forget...
> 
>> So in order to provide 8 byte aligned address always, I mapped the PCIe
>> requestor ID to ITS device ID such that it always provides 8-byte aligned
>> address. The DT property like below helped me achieve that.
>>
>> msi-map = <0x0 &gic_its 0x0 0x10000>;
>> msi-map-mask = <0xfffe>;
>>
>> So this would result in creating one "struct its_device" for 2 PCIe
>> devices and the pre-ITS address will be aligned to 8-bytes.
>>
>> However with this, its_alloc_device_irq() for the 2nd PCIe device is failing
>> since we create "struct its_device" with the number of interrupt vectors
>> requested by the 1st PCIe device.
>>
>> Would like to get your opinion on what would be the best way to
>> workaround this for AM64.
> 
> My preferred workaround would be to take a drill and end the life of
> this thing right now. I guess this isn't an option, so see below for
> the next best thing.
> 
>> One option would be to create a new compatible for AM64 ("ti,am64,gic-v3-its")
>> allocate a minimal number of interrupt vector while creating "struct
>> its_device". Would that be acceptable? Any other ideas?
> 
> No. Messing with the core ITS allocation isn't acceptable. If there is
> such a hack, it has to be dealt within the ITS PCI backend (I assume
> you don't have anything but PCI devices targeting the ITS, right?).

That's right!
> 
> We already have to deal with cases where the endpoints are behind
> aliasing bridges, meaning that we can't distinguish between them, and
> have to account for all the endpoints behind this bridge. You will
> have to perform something similar and compute the upper bound for
> these two devices (functions of the same device actually). See
> its_pci_msi_prepare() for the gory details.

Thanks for the hint. Let me take a stab at it.

Regards,
Kishon