linux-kernel - Re: [PATCH v2] PCI/portdrv: Do not setup up IRQs if there are no users

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <5f6bc890-3a49-3056-ccee-210de546688e@siemens.com>
Date:   Mon, 31 Jan 2022 22:22:28 +0100
From:   Jan Kiszka <jan.kiszka@...mens.com>
To:     <linux-pci@...r.kernel.org>, Bjorn Helgaas <bhelgaas@...gle.com>
CC:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] PCI/portdrv: Do not setup up IRQs if there are no
 users

On 30.08.21 10:08, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@...mens.com>
> 
> Avoid registering service IRQs if there is no service that offers them
> or no driver to register a handler against them. This saves IRQ vectors
> when they are limited (e.g. on x86) and also avoids that spurious events
> could hit a missing handler. Such spurious events need to be generated
> by the Jailhouse hypervisor for active MSI vectors when enabling or
> disabling itself.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@...mens.com>
> ---
> 
> Changes in v2:
>   - move initialization of irqs to address test bot finding
> 
>   drivers/pci/pcie/portdrv_core.c | 47 +++++++++++++++++++++------------
>   1 file changed, 30 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index e1fed6649c41..0e2556269429 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -166,9 +166,6 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>   {
>   	int ret, i;
>   
> -	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
> -		irqs[i] = -1;
> -
>   	/*
>   	 * If we support PME but can't use MSI/MSI-X for it, we have to
>   	 * fall back to INTx or other interrupts, e.g., a system shared
> @@ -312,8 +309,10 @@ static int pcie_device_init(struct pci_dev *pdev, int service, int irq)
>    */
>   int pcie_port_device_register(struct pci_dev *dev)
>   {
> -	int status, capabilities, i, nr_service;
> -	int irqs[PCIE_PORT_DEVICE_MAXSERVICES];
> +	int status, capabilities, irq_services, i, nr_service;
> +	int irqs[PCIE_PORT_DEVICE_MAXSERVICES] = {
> +		[0 ... PCIE_PORT_DEVICE_MAXSERVICES-1] = -1
> +	};
>   
>   	/* Enable PCI Express port device */
>   	status = pci_enable_device(dev);
> @@ -326,18 +325,32 @@ int pcie_port_device_register(struct pci_dev *dev)
>   		return 0;
>   
>   	pci_set_master(dev);
> -	/*
> -	 * Initialize service irqs. Don't use service devices that
> -	 * require interrupts if there is no way to generate them.
> -	 * However, some drivers may have a polling mode (e.g. pciehp_poll_mode)
> -	 * that can be used in the absence of irqs.  Allow them to determine
> -	 * if that is to be used.
> -	 */
> -	status = pcie_init_service_irqs(dev, irqs, capabilities);
> -	if (status) {
> -		capabilities &= PCIE_PORT_SERVICE_HP;
> -		if (!capabilities)
> -			goto error_disable;
> +
> +	irq_services = 0;
> +	if (IS_ENABLED(CONFIG_PCIE_PME))
> +		irq_services |= PCIE_PORT_SERVICE_PME;
> +	if (IS_ENABLED(CONFIG_PCIEAER))
> +		irq_services |= PCIE_PORT_SERVICE_AER;
> +	if (IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE))
> +		irq_services |= PCIE_PORT_SERVICE_HP;
> +	if (IS_ENABLED(CONFIG_PCIE_DPC))
> +		irq_services |= PCIE_PORT_SERVICE_DPC;
> +	irq_services &= capabilities;
> +
> +	if (irq_services) {
> +		/*
> +		 * Initialize service irqs. Don't use service devices that
> +		 * require interrupts if there is no way to generate them.
> +		 * However, some drivers may have a polling mode (e.g.
> +		 * pciehp_poll_mode) that can be used in the absence of irqs.
> +		 * Allow them to determine if that is to be used.
> +		 */
> +		status = pcie_init_service_irqs(dev, irqs, irq_services);
> +		if (status) {
> +			irq_services &= PCIE_PORT_SERVICE_HP;
> +			if (!irq_services)
> +				goto error_disable;
> +		}
>   	}
>   
>   	/* Allocate child services if any */

It turns out that this patch causes troubles on some machines, see [1].
That could be "resolved" by doing

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index bda630889f95..68b0013c3662 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -331,7 +331,7 @@ int pcie_port_device_register(struct pci_dev *dev)
  
  	pci_set_master(dev);
  
-	irq_services = 0;
+	irq_services = PCIE_PORT_SERVICE_BWNOTIF;
  	if (IS_ENABLED(CONFIG_PCIE_PME))
  		irq_services |= PCIE_PORT_SERVICE_PME;
  	if (IS_ENABLED(CONFIG_PCIEAER))

thus considering bandwidth notification as an IRQ-providing service as
well. But as far as I can see, there is no driver for this port service,
thus no one should ever request or even handle that interrupt.

I'm not yet seeing the key difference that could explain this effect.
What else happens via pcie_device_init() when called for
PCIE_PORT_SERVICE_BWNOTIF, although there will never be a driver?

Jan

[1] https://bugzilla.kernel.org/show_bug.cgi?id=215533

-- 
Siemens AG, Technology
Competence Center Embedded Linux