Re: pcie-iproc-msi.c: Bug in Multi-MSI support?

[Ray Jui <ray.jui@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 4371 bytes --]



On 5/26/2021 12:57 AM, Marc Zyngier wrote:
> On Tue, 25 May 2021 18:27:54 +0100,
> Ray Jui <ray.jui@broadcom.com> wrote:
>>
>> On 5/24/2021 3:37 AM, Marc Zyngier wrote:
>>> On Thu, 20 May 2021 18:11:32 +0100,
>>> Ray Jui <ray.jui@broadcom.com> wrote:
>>>>
>>>> On 5/20/2021 7:22 AM, Sandor Bodo-Merle wrote:
> 
> [...]
> 
>>>> I guess I'm not too clear on what you mean by "multi-MSI interrupts
>>>> needs to be aligned to number of requested interrupts.". Would you be
>>>> able to plug this into the above explanation so we can have a more clear
>>>> understanding of what you mean here?
>>>
>>> That's a generic PCI requirement: if you are providing a Multi-MSI
>>> configuration, the base vector number has to be size-aligned
>>> (2-aligned for 2 MSIs, 4 aligned for 4, up to 32), and the end-point
>>> supplies up to 5 bits that are orr-ed into the base vector number,
>>> with a *single* doorbell address. You effectively provide a single MSI
>>> number and a single address, and the device knows how to drive 2^n MSIs.
>>>
>>> This is different from MSI-X, which defines multiple individual
>>> vectors, each with their own doorbell address.
>>>
>>> The main problem you have here (other than the broken allocation
>>> mechanism) is that moving an interrupt from one core to another
>>> implies moving the doorbell address to that of another MSI
>>> group. This isn't possible for Multi-MSI, as all the MSIs must have
>>> the same doorbell address. As far as I can see, there is no way to
>>> support Multi-MSI together with affinity change on this HW, and you
>>> should stop advertising support for this feature.
>>>
>>
>> I was not aware of the fact that multi-MSI needs to use the same
>> doorbell address (aka MSI posted write address?). Thank you for helping
>> to point it out. In this case, yes, like you said, we cannot possibly
>> support both multi-MSI and affinity at the same time, since supporting
>> affinity requires us to move from one to another event queue (and irq)
>> that will have different doorbell address.
>>
>> Do you think it makes sense to do the following by only advertising
>> multi-MSI capability in the single CPU core case (detected runtime via
>> 'num_possible_cpus')? This will at least allow multi-MSI to work in
>> platforms with single CPU core that Sandor and Pali use?
> 
> I don't think this makes much sense. Single-CPU machines are an oddity
> these days, and I'd rather you simplify this (already pretty
> complicated) driver.
> 

Here's the thing. All Broadcom ARMv8 based SoCs have migrated to use
either gicv2m or gicv3-its for MSI/MSIX support. The platforms that
still use iProc event queue based MSI are only legacy ARMv7 based
platforms. Out of them:

NSP - dual core
Cygnus - single core
HR2 - single core

So based on this, it seems to me that it still makes sense to allow
multi-msi to be supported on single core platforms, and Sandor's company
seems to need such support in their particular use case. Sandor, can you
confirm?

>>> There is also a more general problem here, which is the atomicity of
>>> the update on affinity change. If you are moving an interrupt from one
>>> CPU to the other, it seems you change both the vector number and the
>>> target address. If that is the case, this isn't atomic, and you may
>>> end-up with the device generating a message based on a half-applied
>>> update.
>>
>> Are you referring to the callback in 'irq_set_addinity" and
>> 'irq_compose_msi_msg'? In such case, can you help to recommend a
>> solution for it (or there's no solution based on such architecture)? It
>> does not appear such atomy can be enforced from the irq framework level.
> 
> irq_compose_msi_msg() is only one part of the problem. The core of the
> issue is that the programming of the end-point is not atomic (you need
> to update a 32bit payload *and* a 64bit address).
> 
> A solution to workaround it would be to rework the way you allocate
> the vectors, making them constant across all CPUs so that only the
> address changes when changing the affinity.
> 

Thanks. This makes sense. And it looks like this can be addressed
separately from the above issue. I'll have to allocate time to work on
this. In addition, I'd also need someone else with the NSP dual-core
platform to test it for me since we don't have these legacy platforms in
our office anymore.

> Thanks,
> 
> 	M.
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4194 bytes --]