Re: [PATCH v3 03/10] PCI: Allow per function PCI slots

["Cédric Le Goater" <clg@redhat.com>]

Hello Ali,

On 9/11/25 20:33, Farhan Ali wrote:
> On s390 systems, which use a machine level hypervisor, PCI devices are
> always accessed through a form of PCI pass-through which fundamentally
> operates on a per PCI function granularity. This is also reflected in the
> s390 PCI hotplug driver which creates hotplug slots for individual PCI
> functions. Its reset_slot() function, which is a wrapper for
> zpci_hot_reset_device(), thus also resets individual functions.
> 
> Currently, the kernel's PCI_SLOT() macro assigns the same pci_slot object
> to multifunction devices. This approach worked fine on s390 systems that
> only exposed virtual functions as individual PCI domains to the operating
> system.  Since commit 44510d6fa0c0 ("s390/pci: Handling multifunctions")
> s390 supports exposing the topology of multifunction PCI devices by
> grouping them in a shared PCI domain. When attempting to reset a function
> through the hotplug driver, the shared slot assignment causes the wrong
> function to be reset instead of the intended one. It also leaks memory as
> we do create a pci_slot object for the function, but don't correctly free
> it in pci_slot_release().
> 
> Add a flag for struct pci_slot to allow per function PCI slots for
> functions managed through a hypervisor, which exposes individual PCI
> functions while retaining the topology.
> 
> Fixes: 44510d6fa0c0 ("s390/pci: Handling multifunctions")
> Suggested-by: Niklas Schnelle <schnelle@linux.ibm.com>
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>   drivers/pci/hotplug/s390_pci_hpc.c | 10 ++++++++--
>   drivers/pci/pci.c                  |  4 +++-
>   drivers/pci/slot.c                 | 14 +++++++++++---
>   include/linux/pci.h                |  1 +
>   4 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/s390_pci_hpc.c b/drivers/pci/hotplug/s390_pci_hpc.c
> index d9996516f49e..8b547de464bf 100644
> --- a/drivers/pci/hotplug/s390_pci_hpc.c
> +++ b/drivers/pci/hotplug/s390_pci_hpc.c
> @@ -126,14 +126,20 @@ static const struct hotplug_slot_ops s390_hotplug_slot_ops = {
>   
>   int zpci_init_slot(struct zpci_dev *zdev)
>   {
> +	int ret;
>   	char name[SLOT_NAME_SIZE];
>   	struct zpci_bus *zbus = zdev->zbus;
>   
>   	zdev->hotplug_slot.ops = &s390_hotplug_slot_ops;
>   
>   	snprintf(name, SLOT_NAME_SIZE, "%08x", zdev->fid);
> -	return pci_hp_register(&zdev->hotplug_slot, zbus->bus,
> -			       zdev->devfn, name);
> +	ret = pci_hp_register(&zdev->hotplug_slot, zbus->bus,
> +				zdev->devfn, name);
> +	if (ret)
> +		return ret;
> +
> +	zdev->hotplug_slot.pci_slot->per_func_slot = 1;
> +	return 0;
>   }
>   
>   void zpci_exit_slot(struct zpci_dev *zdev)
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 3994fa82df68..70296d3b1cfc 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5061,7 +5061,9 @@ static int pci_reset_hotplug_slot(struct hotplug_slot *hotplug, bool probe)
>   
>   static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe)
>   {
> -	if (dev->multifunction || dev->subordinate || !dev->slot ||
> +	if (dev->multifunction && !dev->slot->per_func_slot)
> +		return -ENOTTY;
> +	if (dev->subordinate || !dev->slot ||
>   	    dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET)
>   		return -ENOTTY;
>   
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 50fb3eb595fe..51ee59e14393 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -63,6 +63,14 @@ static ssize_t cur_speed_read_file(struct pci_slot *slot, char *buf)
>   	return bus_speed_read(slot->bus->cur_bus_speed, buf);
>   }
>   
> +static bool pci_dev_matches_slot(struct pci_dev *dev, struct pci_slot *slot)
> +{
> +	if (slot->per_func_slot)
> +		return dev->devfn == slot->number;
> +
> +	return PCI_SLOT(dev->devfn) == slot->number;
> +}
> +
>   static void pci_slot_release(struct kobject *kobj)
>   {
>   	struct pci_dev *dev;
> @@ -73,7 +81,7 @@ static void pci_slot_release(struct kobject *kobj)
>   
>   	down_read(&pci_bus_sem);
>   	list_for_each_entry(dev, &slot->bus->devices, bus_list)
> -		if (PCI_SLOT(dev->devfn) == slot->number)
> +		if (pci_dev_matches_slot(dev, slot))
>   			dev->slot = NULL;
>   	up_read(&pci_bus_sem);
>   
> @@ -166,7 +174,7 @@ void pci_dev_assign_slot(struct pci_dev *dev)
>   
>   	mutex_lock(&pci_slot_mutex);
>   	list_for_each_entry(slot, &dev->bus->slots, list)
> -		if (PCI_SLOT(dev->devfn) == slot->number)
> +		if (pci_dev_matches_slot(dev, slot))
>   			dev->slot = slot;
>   	mutex_unlock(&pci_slot_mutex);
>   }
> @@ -285,7 +293,7 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
>   
>   	down_read(&pci_bus_sem);
>   	list_for_each_entry(dev, &parent->devices, bus_list)
> -		if (PCI_SLOT(dev->devfn) == slot_nr)
> +		if (pci_dev_matches_slot(dev, slot))
>   			dev->slot = slot;
>   	up_read(&pci_bus_sem);
>   
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 59876de13860..9265f32d9786 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -78,6 +78,7 @@ struct pci_slot {
>   	struct list_head	list;		/* Node in list of slots */
>   	struct hotplug_slot	*hotplug;	/* Hotplug info (move here) */
>   	unsigned char		number;		/* PCI_SLOT(pci_dev->devfn) */
> +	unsigned int		per_func_slot:1; /* Allow per function slot */
>   	struct kobject		kobj;
>   };
>   

This change generates a kernel oops on x86_64. It can be reproduced in a VM.


C.

[    3.073990] BUG: kernel NULL pointer dereference, address: 0000000000000021
[    3.074976] #PF: supervisor read access in kernel mode
[    3.074976] #PF: error_code(0x0000) - not-present page
[    3.074976] PGD 0 P4D 0
[    3.074976] Oops: Oops: 0000 [#1] SMP NOPTI
[    3.074976] CPU: 18 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.17.0-rc6-clg-dirty #8 PREEMPT(voluntary)
[    3.074976] Hardware name: Supermicro Super Server/X13SAE-F, BIOS 4.2 12/17/2024
[    3.074976] RIP: 0010:pci_reset_bus_function+0xdf/0x160
[    3.074976] Code: 4e 08 00 00 40 0f 85 83 00 00 00 48 8b 78 18 e8 27 9d ff ff 83 f8 e7 74 17 48 83 c4 08 5b 5d 41 5c c3 cc cc cc cc 48 8b 43 30 <f6> 40 21 01 75 b6 48 8b 53 10 48 83 7a 10 00 74 5e 48 83 7b 18 00
[    3.074976] RSP: 0000:ffffcd808007b9a8 EFLAGS: 00010202
[    3.074976] RAX: 0000000000000000 RBX: ffff88c4019b8000 RCX: 0000000000000000
[    3.074976] RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffff88c4019b8000
[    3.074976] RBP: 0000000000000001 R08: 0000000000000002 R09: ffffcd808007b99c
[    3.074976] R10: ffffcd808007b950 R11: 0000000000000000 R12: 0000000000000001
[    3.074976] R13: ffff88c4019b80c8 R14: ffff88c401a7e028 R15: ffff88c401a73400
[    3.074976] FS:  0000000000000000(0000) GS:ffff88d38aad5000(0000) knlGS:0000000000000000
[    3.074976] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    3.074976] CR2: 0000000000000021 CR3: 0000000f66222001 CR4: 0000000000770ef0
[    3.074976] PKRU: 55555554
[    3.074976] Call Trace:
[    3.074976]  <TASK>
[    3.074976]  ? pci_pm_reset+0x39/0x180
[    3.074976]  pci_init_reset_methods+0x52/0x80
[    3.074976]  pci_device_add+0x215/0x5d0
[    3.074976]  pci_scan_single_device+0xa2/0xe0
[    3.074976]  pci_scan_slot+0x66/0x1c0
[    3.074976]  ? klist_next+0x145/0x150
[    3.074976]  pci_scan_child_bus_extend+0x3a/0x290
[    3.074976]  acpi_pci_root_create+0x236/0x2a0
[    3.074976]  pci_acpi_scan_root+0x19b/0x1f0
[    3.074976]  acpi_pci_root_add+0x1a5/0x370
[    3.074976]  acpi_bus_attach+0x1a8/0x290
[    3.074976]  ? __pfx_acpi_dev_for_one_check+0x10/0x10
[    3.074976]  device_for_each_child+0x4b/0x80
[    3.074976]  acpi_dev_for_each_child+0x28/0x40
[    3.074976]  ? __pfx_acpi_bus_attach+0x10/0x10
[    3.074976]  acpi_bus_attach+0x7a/0x290
[    3.074976]  ? _raw_spin_unlock_irqrestore+0x23/0x40
[    3.074976]  ? __pfx_acpi_dev_for_one_check+0x10/0x10
[    3.074976]  device_for_each_child+0x4b/0x80
[    3.074976]  acpi_dev_for_each_child+0x28/0x40
[    3.074976]  ? __pfx_acpi_bus_attach+0x10/0x10
[    3.074976]  acpi_bus_attach+0x7a/0x290
[    3.074976]  acpi_bus_scan+0x6a/0x1c0
[    3.074976]  ? __pfx_acpi_init+0x10/0x10
[    3.074976]  acpi_scan_init+0xdc/0x280
[    3.074976]  ? __pfx_acpi_init+0x10/0x10
[    3.074976]  acpi_init+0x218/0x530
[    3.074976]  do_one_initcall+0x40/0x310
[    3.074976]  kernel_init_freeable+0x2fe/0x450
[    3.074976]  ? __pfx_kernel_init+0x10/0x10
[    3.074976]  kernel_init+0x16/0x1d0
[    3.074976]  ret_from_fork+0x1ab/0x1e0
[    3.074976]  ? __pfx_kernel_init+0x10/0x10
[    3.074976]  ret_from_fork_asm+0x1a/0x30
[    3.074976]  </TASK>
[    3.074976] Modules linked in:
[    3.074976] CR2: 0000000000000021
[    3.074976] ---[ end trace 0000000000000000 ]---
[    3.074976] RIP: 0010:pci_reset_bus_function+0xdf/0x160