Re: [PATCH v3 03/10] PCI: Allow per function PCI slots
From: Farhan Ali
Date: Tue Sep 16 2025 - 14:38:47 EST
On 9/15/2025 11:52 PM, Cédric Le Goater wrote:
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@xxxxxxxxxxxxx>
Signed-off-by: Farhan Ali <alifm@xxxxxxxxxxxxx>
---
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
Hi Cedric,
Thanks for pointing this out. I missed that dev->slot could be NULL and
so the per_func_slot check should be done after the check for
!dev->slot. I tried this change on top of the patch in an x86_64 VM and
was able to boot the VM without the oops.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 70296d3b1cfc..3631f7faa0cf 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5061,10 +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->slot->per_func_slot)
- return -ENOTTY;
if (dev->subordinate || !dev->slot ||
- dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET)
+ dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET ||
+ (dev->multifunction && !dev->slot->per_func_slot))
return -ENOTTY;
Thanks
Farhan