RE: [EXT] Re: [PATCH v13 2/2] PCI: endpoint: pci-epf-vntb: using platform MSI as doorbell
From: Thomas Gleixner
Date: Mon Nov 28 2022 - 17:39:28 EST
Frank!
On Mon, Nov 28 2022 at 21:25, Frank Li wrote:
Can you please fix your mail client to not copy the whole CC list into
the reply? It's just pointless noise. A simple:
On Mon, Nov 28 200 at 1:15 PM Thomas Gleixner wrote:
instead of:
>> -----Original Message-----
>> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Sent: Monday, November 28, 2022 1:15 PM
>> To: Frank Li <frank.li@xxxxxxx>; lpieralisi@xxxxxxxxxx
>> Cc: Frank Li <frank.li@xxxxxxx>; Aisheng Dong <aisheng.dong@xxxxxxx>;
>> bhelgaas@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; festevam@xxxxxxxxx;
>> imx@xxxxxxxxxxxxxxx; jdmason@xxxxxxxx; kernel@xxxxxxxxxxxxxx;
>> kishon@xxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; kw@xxxxxxxxx; linux-
>> arm-kernel@xxxxxxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; linux-
>> kernel@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx;
>> lorenzo.pieralisi@xxxxxxx; lznuaa@xxxxxxxxx;
>> manivannan.sadhasivam@xxxxxxxxxx; maz@xxxxxxxxxx; ntb@xxxxxxxxxxxxxxx;
>> Peng Fan <peng.fan@xxxxxxx>; robh+dt@xxxxxxxxxx;
>> s.hauer@xxxxxxxxxxxxxx; shawnguo@xxxxxxxxxx
>> Subject: [EXT] Re: [PATCH v13 2/2] PCI: endpoint: pci-epf-vntb: using platform
>> MSI as doorbell
is completely sufficient.
>> Caution: EXT Email
We are neither interested in the oddities of NXP's mail categorization system.
>> On Thu, Nov 24 2022 at 00:50, Frank Li wrote:
>> >
>> > Using platform MSI interrupt controller as endpoint(EP)'s doorbell.
>>
>> Can you please explain what the MSI controller is in this picture? MSI
>> controller is not a term which is common in the interrupt handling
>> landscape despite the fact that it's pretty wide spread in device tree
>> bindings presumably through intensive copy & pasta cargo cult.
>
> I use irq-imx-mu-msi to do test. I supposed it should work for all kinds
> general msi controller.
Sure it works by some definition of "works" because obviously that
implementation does not care about where a particular message originates
from.
But that's still wrong at the conceptual level because it very much
matters where a message originates from. PCIe devices and general
platform devices have very distinct mechanisms to transport that
information.
Just because it "works" does not prove that it is correct.
How are you going to do proper shielding with that approach?
> Our test platform have not GIC ITS supported yet.
And therefore the originating device is irrelevant, right? Get to the
point where you have ITS and it all falls apart.
>> You're explaining what the code does, but fail to explain the underlying
>> mechanisms.
>>
>> Platform MSI is definitely the wrong mechanism here. Why?
>
> This patch use Platform MSI. I never said " Platform MSI is
> definitely the wrong mechanism here".
I did not claim that _you_ said that. _I_ said that this is wrong. See
above.
> Base logic is that, get msi controller's message address by irq API.
> Map this physical address to DB BAR, so PCI host write this DB bar, then
> EP generate irq.
Again, you are explaining what your implementation is doing, but you are
not describing the conceptual level.
>> This is about a PCIe endpoint, which is usually handled by a PCI/MSI
>> interrupt domain. Obviously this usage does not fit into the way how the
>> "global" PCI/MSI domains work.
>
> PCI endpoint have not standard PCI configure space to enable/disable MSI irq and
> MSI address (CAP 05).
I'm very well aware of the fact that a PCI endpoint does not have the
standard MSI configuration space mechanism.
> That's reason why need "platform msi", or you called "global"
Again: platform MSI does not convey the PCIe device originator. It might
be irrelevant for your actual platform, but that does not make it more
correct. Once you have the need to track the origin of a MSI message
then the distinction between platform and MSI matters.
Sure you can argue that you don't care, but that does neither make it
correct nor future proof and there is no point to rework this whole
thing 6 month down the road when you actually have to support GIC-ITS.
>> There is upcoming work and at least the generic parts should show up in
>> 6.2 which addresses exactly the problem you are trying to solve:
>>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.k
>> ernel.org%2Fall%2F20221124225331.464480443%40linutronix.de&data
>> =05%7C01%7CFrank.Li%40nxp.com%7C6a07e33e56af45ffc1ff08dad174d02d
>> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6380525969049530
>> 06%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMz
>> IiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Q8jr
>> eVGGLa2M4yhjGO7Njqwdm59XDC0GyLEwkr0k6B0%3D&reserved=0
>>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.k
>> ernel.org%2Fall%2F20221124230505.073418677%40linutronix.de&data
>> =05%7C01%7CFrank.Li%40nxp.com%7C6a07e33e56af45ffc1ff08dad174d02d
>> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6380525969049530
>> 06%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMz
>> IiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Tc9p
>> XNJ499ETFgNWQBNLViFk8D5GbvrrwYDlBW%2Bf2qg%3D&reserved=0
>>
>> plus the prove that the platform MSI mess can be replaced by this:
>>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.k
>> ernel.org%2Fall%2F20221121135653.208611233%40linutronix.de&data
>> =05%7C01%7CFrank.Li%40nxp.com%7C6a07e33e56af45ffc1ff08dad174d02d
>> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6380525969049530
>> 06%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMz
>> IiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=R5K
>> NVfcGqxoCam%2FYhY57ihsloWGhGLM3Kh9IkyME4lk%3D&reserved=0
Those outlook artifacts are helpful to our conversation in which way?
>> NTB in it's current form should never have happened, but that's water
>> down the bridge.
>>
>> What you really want is:
>>
>> 1) Convert your platform to the new MSI parent model
>>
>> 2) Utilize PCI/IMS which is giving you exactly what you need with
>> proper PCI semantics
>
> Sorry, I still don't understand yet. This patch is just user of msi
> controller.
As I explained to you before: The concept of MSI controller does not
exist in the kernel. It might exist in your NXP nomenclature, but that's
irrelevant here.
> Your patches focus on the msi controller itself.
No. They focus on changing the hierarchy model from "global" MSI domains
to per device MSI domains so that the weird constructs of platform MSI
can be replaced by something which actually matches the hardware and
provides a proper abstraction for PCIe/NTB at the right level.
> Interface platform_msi_domain_alloc_irqs still exist at your devmsi-v2-part3.
Sure, but at:
git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git devmsi-v2-arm
platform_msi_domain_alloc_irqs() does not exist anymore.
You replied to exactly that series which builds on top of devmsi-v2-part3, no?
So what are you trying to tell me?
Thanks,
tglx