linux-kernel - Re: [PATCH v8 3/3] PCI: uniphier: Add misc interrupt handler to invoke PME and AER

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210728223508.iffjpmk6ipjpvddh@pali>
Date:   Thu, 29 Jul 2021 00:35:08 +0200
From:   Pali Rohár <pali@...nel.org>
To:     Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>
Cc:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Rob Herring <robh@...nel.org>,
        Jingoo Han <jingoohan1@...il.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Marc Zyngier <maz@...nel.org>, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Jassi Brar <jaswinder.singh@...aro.org>,
        Masami Hiramatsu <masami.hiramatsu@...aro.org>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v8 3/3] PCI: uniphier: Add misc interrupt handler to
 invoke PME and AER

On Wednesday 28 July 2021 14:29:15 Kunihiko Hayashi wrote:
> Hi Lorenzo, Pali,
> 
> On 2021/07/23 18:36, Kunihiko Hayashi wrote:
> > Hi Pali,
> 
> [snip]
> 
> > > Just you need to specify that new/private IRQ domain into
> > > irq_find_mapping() call.
> > 
> > I'll try to replace the events with new IRQ domain.
> According to Pali's suggestion, the bridge handles INTX and it isn't difficult
> to change IRQ's map for Root Port like the example.
> It seems that it can't be applied to MSI.

Hm... And it is hard to change mapping also for MSI via custom/new
callback?

> On the other hand, according to Lorenzo's suggestion,
> 
> >>>>>>> IMO this should be modelled with a separate IRQ domain and chip for
> >>>>>>> the root port (yes this implies describing the root port in the dts
> >>>>>>> file with a separate msi-parent).
> 
> Interrupts for PME/AER event is assigned to number 0 of MSI IRQ domain.

Yes. This is because Root Port of your PCIe controller provides this
information to OS.

> (pcie_port_enable_irq_vec() in portdrv_core.c)
> This expects MSI status bit 0 to be set when the event occurs.

Obviously (according to PCIe spec).

> However, in the uniphier PCIe controller, MSI status bit 0 is not set, but
> the PME/AER status bit in the glue logic is set.

And this "violates" PCIe spec and your controller needs custom handling
in driver to work...

> I think that it's hard to associate the new domain and "MSI-IRQ 0" event
> if the new IRQ domain and chip is modelled.

No. It was mean to assign all MSI IRQs (not only MSI number 0) which
comes from Root Port to new domain. Assigning just one MSI number to
separate domain is I guess impossible (and also does not make sense).

This is required to difference between MSI number 0 which comes from
real MSI path and "fake MSI number 0" which is just specific for Root
Port and does not share anything with real MSI interrupts. As MSI
interrupts are not shared it is required to prevent "mixing" interrupt
sources. And kernel can do it via different MSI domains.

So in the end you would have two different MSI numbers 0.

> So, I have no idea to handle both new IRQ domain and cascaded MSI event.

It was mean to define Root Port PCIe device in DTS. Then define a new
IRQ chip / domain in DTS. And specify in DTS that this Root Port PCIe
device should use this new IRQ chip / domain instead of default one.
And then you need to implement "driver" for this "virtual" IRQ chip /
domain to handle specific glue logic in this controller driver.

I was hoping that it is possible to set this mapping directly in
controller driver. But if you checked that only legacy IRQs can be done
in this way, and not MSI then it is either needed to go via this DTS
path OR try to figure out how to define this mapping in PCI subsystem
(maybe needs some changes?) also for MSI.

> Is there any example for that?

I do not know. I think you are the first one who have such buggy PCIe
controller which needs this specific kind of configuration and domains.

In my case in pci-aardvark.c, emulated kernel Root Port does not support
MSI interrupts (yet) so these "fake" interrupts are routed as legacy
INTA. And because legacy INTx are shared interrupts they can be mixed
together with real (as it is done prior my patch). My patch (link sent
in previous email) just separates "fake" INTA from "real" INTA via
separate domains for performance reasons.

But you use MSI interrupts, which means that it is required to have
logic to separate them into different domains to prevent mixing.

> Thank you,
> 
> ---
> Best Regards
> Kunihiko Hayashi