| 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
| ||
|
Message-ID: <c7956816-7744-436c-94f6-fec4fe9d5635@163.com>
Date: Sun, 2 Mar 2025 22:45:39 +0800
From: Hans Zhang <18255117159@....com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: manivannan.sadhasivam@...aro.org, kw@...ux.com, kwilczynski@...nel.org,
bhelgaas@...gle.com, Frank.Li@....com, cassel@...nel.org,
linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [v2] genirq/msi: Add the address and data that show MSI/MSIX
Hi Thomas(tglx),
On 2025/3/2 17:01, Thomas Gleixner wrote:
> Hans!
>
> On Sat, Mar 01 2025 at 20:39, Hans Zhang wrote:
>> The debug_show() callback function is implemented in the MSI core code.
>> And assign it to the domain ops::debug_show() creation.
>>
>> cat /sys/kernel/debug/irq/irqs/msi_irq_num, the address and data stored
>> in the MSI capability or the address and data stored in the MSIX vector
>> table will be displayed.
>
> So this explains what the patch is doing and what the output is. But it
> fails to explain the _why_. Documentation gives proper guidance:
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-changes
Is the following explanation OK?
Displaying the address and data of the MSI/MSIX interrupt in the debugfs
helps with debugging.
>> e.g.
>> root@...t:/sys/kernel/debug/irq/irqs# cat /proc/interrupts | grep ITS
>> 85: 0 0 0 0 0 0 0 0 0 0 0 0 ITS-MSI 75497472 Edge PCIe PME, aerdrv
>> 86: 0 30 0 0 0 0 0 0 0 0 0 0 ITS-MSI 76021760 Edge nvme0q0
>> 87: 287 0 0 0 0 0 0 0 0 0 0 0 ITS-MSI 76021761 Edge nvme0q1
>> 88: 0 265 0 0 0 0 0 0 0 0 0 0 ITS-MSI 76021762 Edge nvme0q2
>> 89: 0 0 177 0 0 0 0 0 0 0 0 0 ITS-MSI 76021763 Edge nvme0q3
>> 90: 0 0 0 76 0 0 0 0 0 0 0 0 ITS-MSI 76021764 Edge nvme0q4
>> 91: 0 0 0 0 161 0 0 0 0 0 0 0 ITS-MSI 76021765 Edge nvme0q5
>> 92: 0 0 0 0 0 991 0 0 0 0 0 0 ITS-MSI 76021766 Edge nvme0q6
>> 93: 0 0 0 0 0 0 194 0 0 0 0 0 ITS-MSI 76021767 Edge nvme0q7
>> 94: 0 0 0 0 0 0 0 94 0 0 0 0 ITS-MSI 76021768 Edge nvme0q8
>> 95: 0 0 0 0 0 0 0 0 148 0 0 0 ITS-MSI 76021769 Edge nvme0q9
>> 96: 0 0 0 0 0 0 0 0 0 261 0 0 ITS-MSI 76021770 Edge nvme0q10
>> 97: 0 0 0 0 0 0 0 0 0 0 127 0 ITS-MSI 76021771 Edge nvme0q11
>> 98: 0 0 0 0 0 0 0 0 0 0 0 317 ITS-MSI 76021772 Edge nvme0q12
>
> How is this relevant to describe the patch?
The preceding information will be deleted in v4 patch.
>
>> root@...t:/sys/kernel/debug/irq/irqs#
>> root@...t:/sys/kernel/debug/irq/irqs# cat 87
>> handler: handle_fasteoi_irq
>> device: 0000:91:00.0
>> status: 0x00000000
>> istate: 0x00004000
>> ddepth: 0
>> wdepth: 0
>> dstate: 0x31600200
>> IRQD_ACTIVATED
>> IRQD_IRQ_STARTED
>> IRQD_SINGLE_TARGET
>> IRQD_AFFINITY_MANAGED
>> IRQD_AFFINITY_ON_ACTIVATE
>> IRQD_HANDLE_ENFORCE_IRQCTX
>> node: 0
>> affinity: 0
>> effectiv: 0
>> domain: :soc@0:interrupt-controller@...01000:its@...50000-3
>> hwirq: 0x4880001
>> chip: ITS-MSI
>
> This output is from a pre 6.11 kernel...
I will delete all other information except for what my patch will display.
>
>> flags: 0x20
>> IRQCHIP_ONESHOT_SAFE
>> msix:
>> address_hi: 0x00000000
>> address_lo: 0x0e060040
>> msg_data: 0x00000001
>
> For demonstration it's enough to stop here, no?
Yes, I will change it to the following:
msix:
address_hi: 0x00000000
address_lo: 0x0e060040
msg_data: 0x00000001
>
>> +static void msi_domain_debug_show(struct seq_file *m, struct irq_domain *d,
>> + struct irq_data *irqd, int ind)
>> +{
>> + struct msi_desc *desc;
>> + bool is_msix;
>> +
>> + desc = irq_get_msi_desc(irqd->irq);
>
> Move this up to the declaration.
>
>> + if (!desc)
>> + return;
>> +
>> + is_msix = desc->pci.msi_attrib.is_msix;
>
> That's not valid for non PCI MSI interrupts.
Do you mean to remove the following two lines of code?
is_msix = desc->pci.msi_attrib.is_msix;
seq_printf(m, "%*s%s:", ind, "", is_msix ? "msix" : "msi");
> This function is used for all types of MSI interrupts. So for non PCI
> MSI interrupts this will output random garbage. Just print the address
> and be done with it. The MSI variant is visible from the chip name on
> current kernels. It's either ITS-PCI-MSI or ITS-PCI-MSIX and not
> ITS-MSI.
>
>> + seq_printf(m, "%*s%s:", ind, "", is_msix ? "msix" : "msi");
>> + seq_printf(m, "\n%*saddress_hi: 0x%08x", ind + 1, "", desc->msg.address_hi);
>> + seq_printf(m, "\n%*saddress_lo: 0x%08x", ind + 1, "", desc->msg.address_lo);
>> + seq_printf(m, "\n%*smsg_data: 0x%08x\n", ind + 1, "", desc->msg.data);
>> +}
>> +
>> static const struct irq_domain_ops msi_domain_ops = {
>> .alloc = msi_domain_alloc,
>> .free = msi_domain_free,
>> .activate = msi_domain_activate,
>> .deactivate = msi_domain_deactivate,
>> .translate = msi_domain_translate,
>> + .debug_show = msi_domain_debug_show,
>
> This does not build when CONFIG_GENERIC_IRQ_DEBUGFS=n.
>
Kernel test robot has reported a compilation error, and I have submitted
v3 patch to solve this problem. I will fix all your questions in v4 patch.
Finally, thank you very much for all your comments.
Best regards
Hans
Powered by blists - more mailing lists