[PATCH v5 0/8] VMD add second root bus support

[PATCH v5 0/8] VMD add second root bus support

[Szymon Durawa]

This patch series implements second rootbus support inside Intel VMD
module. Current implementation allows VMD to take ownership of devices
only on first bus (Rootbus0). Starting from Intel Arrow Lake, VMD exposes
second bus (Rootbus1) to allow VMD to own devices on this bus as well.
VMD MMIO BARs (CFGBAR. MEMBAR1 and MEMBAR2) are now shared between
Rootbus0 and Rootbus1. Reconfiguration of 3 MMIO BARs is required by
resizing current MMIO BARs ranges. It allows to find/register VMD Rootbus1
and discovers devices or root ports under it.

Patches 1 to 6 introduce code refactoring without functional changes.
Patch 7 implements VMD Rootbus1 support and patch 8 provides workaround
for rootbus number hardwired to fixed non-zero value. Patch 8 is necessary
for correct enumeration attached devices under VMD Rootbus1. Without it
user cannot access those devices as they are not visible in the system,
only drives under VMD Rootbus0 are available to the user.

Changes from v1:
- splitting series into more commits, requested by Bjorn
- adding helper functions, suggested by Bjorn
- minor typos and unclear wording updated, suggested by Bjorn

Changes from v2:
- wording update in commit logs, suggested by Bjorn

Changes from v3:
- using GENMASK() instead of manual bits shifting, suggested by Bjorn
- converting decimal number to hex representation, suggested by Bjorn
- wording update in commit logs, suggested by Bjorn

Changes from v4:
- Update Dan's email address
- Resending the whole series to the correct mailing list

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Dan Williams <djbw@kernel.org>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: linux-pci@vger.kernel.org
Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com>
Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>

Szymon Durawa (8):
  PCI: vmd: Add vmd_bus_enumeration() helper function
  PCI: vmd: Add vmd_configure_cfgbar() helper function
  PCI: vmd: Add vmd_configure_membar() and 
    vmd_configure_membar1_membar2()
  PCI: vmd: Add vmd_create_bus()
  PCI: vmd: Replace hardcoded values with enum and defines
  PCI: vmd: Convert bus and busn_start to an array
  PCI: vmd: Add support for second rootbus under VMD
  PCI: vmd: Add workaround for bus number hardwired to fixed non-zero
    value

 drivers/pci/controller/vmd.c | 488 +++++++++++++++++++++++++++--------
 1 file changed, 376 insertions(+), 112 deletions(-)

-- 
2.43.0

[PATCH v5 1/8] PCI: vmd: Add vmd_bus_enumeration() helper function

[Szymon Durawa]

Move the vmd bus enumeration code to a new helper vmd_bus_enumeration().
No functional changes.

Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com>
Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>
---
 drivers/pci/controller/vmd.c | 89 ++++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 40 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index d4ae250d4bc6..070b247b3fbb 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -771,6 +771,54 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	return 0;
 }
 
+static void vmd_bus_enumeration(struct pci_bus *bus, unsigned long features)
+{
+	struct pci_bus *child;
+	struct pci_dev *dev;
+	int ret;
+
+	vmd_acpi_begin();
+
+	pci_scan_child_bus(bus);
+	vmd_domain_reset(vmd_from_bus(bus));
+
+	/*
+	 * When Intel VMD is enabled, the OS does not discover the Root Ports
+	 * owned by Intel VMD within the MMCFG space. pci_reset_bus() applies
+	 * a reset to the parent of the PCI device supplied as argument. This
+	 * is why we pass a child device, so the reset can be triggered at
+	 * the Intel bridge level and propagated to all the children in the
+	 * hierarchy.
+	 */
+	list_for_each_entry(child, &bus->children, node) {
+		if (!list_empty(&child->devices)) {
+			dev = list_first_entry(&child->devices, struct pci_dev,
+					       bus_list);
+			ret = pci_reset_bus(dev);
+			if (ret)
+				pci_warn(dev, "can't reset device: %d\n", ret);
+
+			break;
+		}
+	}
+
+	pci_assign_unassigned_bus_resources(bus);
+
+	pci_walk_bus(bus, vmd_pm_enable_quirk, &features);
+
+	/*
+	 * VMD root buses are virtual and don't return true on pci_is_pcie()
+	 * and will fail pcie_bus_configure_settings() early. It can instead be
+	 * run on each of the real root ports.
+	 */
+	list_for_each_entry(child, &bus->children, node)
+		pcie_bus_configure_settings(child);
+
+	pci_bus_add_devices(bus);
+
+	vmd_acpi_end();
+}
+
 static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 {
 	struct pci_sysdata *sd = &vmd->sysdata;
@@ -780,8 +828,6 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	LIST_HEAD(resources);
 	resource_size_t offset[2] = {0};
 	resource_size_t membar2_offset = 0x2000;
-	struct pci_bus *child;
-	struct pci_dev *dev;
 	int ret;
 
 	/*
@@ -923,45 +969,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	WARN(sysfs_create_link(&vmd->dev->dev.kobj, &vmd->bus->dev.kobj,
 			       "domain"), "Can't create symlink to domain\n");
 
-	vmd_acpi_begin();
-
-	pci_scan_child_bus(vmd->bus);
-	vmd_domain_reset(vmd);
+	vmd_bus_enumeration(vmd->bus, features);
 
-	/* When Intel VMD is enabled, the OS does not discover the Root Ports
-	 * owned by Intel VMD within the MMCFG space. pci_reset_bus() applies
-	 * a reset to the parent of the PCI device supplied as argument. This
-	 * is why we pass a child device, so the reset can be triggered at
-	 * the Intel bridge level and propagated to all the children in the
-	 * hierarchy.
-	 */
-	list_for_each_entry(child, &vmd->bus->children, node) {
-		if (!list_empty(&child->devices)) {
-			dev = list_first_entry(&child->devices,
-					       struct pci_dev, bus_list);
-			ret = pci_reset_bus(dev);
-			if (ret)
-				pci_warn(dev, "can't reset device: %d\n", ret);
-
-			break;
-		}
-	}
-
-	pci_assign_unassigned_bus_resources(vmd->bus);
-
-	pci_walk_bus(vmd->bus, vmd_pm_enable_quirk, &features);
-
-	/*
-	 * VMD root buses are virtual and don't return true on pci_is_pcie()
-	 * and will fail pcie_bus_configure_settings() early. It can instead be
-	 * run on each of the real root ports.
-	 */
-	list_for_each_entry(child, &vmd->bus->children, node)
-		pcie_bus_configure_settings(child);
-
-	pci_bus_add_devices(vmd->bus);
-
-	vmd_acpi_end();
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH v5 1/8] PCI: vmd: Add vmd_bus_enumeration() helper function

[sashiko-bot]

> Move the vmd bus enumeration code to a new helper vmd_bus_enumeration().
> No functional changes.
> 
> Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625082940.1290740-1-szymon.durawa@linux.intel.com?part=1

[PATCH v5 2/8] PCI: vmd: Add vmd_configure_cfgbar() helper function

[Szymon Durawa]

Move the VMD CFGBAR initialization code to a new helper
vmd_configure_cfgbar(). No functional changes.

Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com>
Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>
---
 drivers/pci/controller/vmd.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 070b247b3fbb..2c824660cabd 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -771,6 +771,18 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
 	return 0;
 }
 
+static void vmd_configure_cfgbar(struct vmd_dev *vmd)
+{
+	struct resource *res = &vmd->dev->resource[VMD_CFGBAR];
+
+	vmd->resources[0] = (struct resource){
+		.name = "VMD CFGBAR",
+		.start = vmd->busn_start,
+		.end = vmd->busn_start + (resource_size(res) >> 20) - 1,
+		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
+	};
+}
+
 static void vmd_bus_enumeration(struct pci_bus *bus, unsigned long features)
 {
 	struct pci_bus *child;
@@ -857,13 +869,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 			return ret;
 	}
 
-	res = &vmd->dev->resource[VMD_CFGBAR];
-	vmd->resources[0] = (struct resource) {
-		.name  = "VMD CFGBAR",
-		.start = vmd->busn_start,
-		.end   = vmd->busn_start + (resource_size(res) >> 20) - 1,
-		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
-	};
+	vmd_configure_cfgbar(vmd);
 
 	/*
 	 * If the window is below 4GB, clear IORESOURCE_MEM_64 so we can
-- 
2.43.0

Re: [PATCH v5 2/8] PCI: vmd: Add vmd_configure_cfgbar() helper function

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Destruction of SRCU struct before freeing IRQ handlers leads to potential Use-After-Free.
--

commit 277c077bdf8fa22a45d1262f68f3e1a7fc357e50
Author: Szymon Durawa <szymon.durawa@linux.intel.com>

PCI: vmd: Add vmd_configure_cfgbar() helper function

Move the VMD CFGBAR initialization code to a new helper
vmd_configure_cfgbar(). No functional changes.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 070b247b3fbbe..2c824660cabd5 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
[ ... ]
> @@ -857,13 +869,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  			return ret;
>  	}
>  
> -	res = &vmd->dev->resource[VMD_CFGBAR];
> -	vmd->resources[0] = (struct resource) {
> -		.name  = "VMD CFGBAR",
> -		.start = vmd->busn_start,
> -		.end   = vmd->busn_start + (resource_size(res) >> 20) - 1,
> -		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
> -	};
> +	vmd_configure_cfgbar(vmd);

[Severity: High]
This is a pre-existing issue, but does vmd_remove() have a use-after-free
race?

In vmd_alloc_irqs(), the interrupts are requested via devm_request_irq(),
which means they are freed automatically by devres after vmd_remove() returns.

However, vmd_remove() explicitly destroys the SRCU structure:

vmd_remove()
  vmd_cleanup_srcu()
    cleanup_srcu_struct(&vmd->irqs[i].srcu)

If an interrupt fires after cleanup_srcu_struct() but before the devres
cleanup unregisters the IRQ, vmd_irq() will unconditionally access the
destroyed SRCU structure:

vmd_irq()
  idx = srcu_read_lock(&irqs->srcu);

Should the IRQs be freed before the SRCU structures are destroyed?

>  
>  	/*
>  	 * If the window is below 4GB, clear IORESOURCE_MEM_64 so we can

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625082940.1290740-1-szymon.durawa@linux.intel.com?part=2

[PATCH v5 3/8] PCI: vmd: Add vmd_configure_membar() and vmd_configure_membar1_membar2()

[Szymon Durawa]

Move the MEMBAR1 and MEMBAR2 registry initialization code to new helpers
vmd_configure_membar() and vmd_configure_membar1_membar2(). No functional
changes.

Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com>
Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>
---
 drivers/pci/controller/vmd.c | 80 ++++++++++++++++++++++--------------
 1 file changed, 50 insertions(+), 30 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 2c824660cabd..443a0cefb7f7 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -783,6 +783,48 @@ static void vmd_configure_cfgbar(struct vmd_dev *vmd)
 	};
 }
 
+/*
+ * vmd_configure_membar - Configure VMD MemBAR register, which points
+ * to MMIO address assigned by the OS or BIOS.
+ * @vmd: the VMD device
+ * @resource_number: resource buffer number to be filled in
+ * @membar_number: number of the MemBAR
+ * @start_offset: 4K aligned offset applied to start of VMD’s MEMBAR MMIO space
+ * @end_offset: 4K aligned offset applied to end of VMD’s MEMBAR MMIO space
+ *
+ * Function fills resource buffer inside the VMD structure.
+ */
+static void vmd_configure_membar(struct vmd_dev *vmd, u8 resource_number,
+				 u8 membar_number, resource_size_t start_offset,
+				 resource_size_t end_offset)
+{
+	u32 upper_bits;
+	unsigned long flags;
+
+	struct resource *res = &vmd->dev->resource[membar_number];
+
+	upper_bits = upper_32_bits(res->end);
+	flags = res->flags & ~IORESOURCE_SIZEALIGN;
+	if (!upper_bits)
+		flags &= ~IORESOURCE_MEM_64;
+
+	vmd->resources[resource_number] = (struct resource){
+		.name = kasprintf(GFP_KERNEL, "VMD MEMBAR%d",
+				  membar_number / 2),
+		.start = res->start + start_offset,
+		.end = res->end - end_offset,
+		.flags = flags,
+		.parent = res,
+	};
+}
+
+static void vmd_configure_membar1_membar2(struct vmd_dev *vmd,
+					  resource_size_t mbar2_ofs)
+{
+	vmd_configure_membar(vmd, 1, VMD_MEMBAR1, 0, 0);
+	vmd_configure_membar(vmd, 2, VMD_MEMBAR2, mbar2_ofs, 0);
+}
+
 static void vmd_bus_enumeration(struct pci_bus *bus, unsigned long features)
 {
 	struct pci_bus *child;
@@ -834,9 +876,6 @@ static void vmd_bus_enumeration(struct pci_bus *bus, unsigned long features)
 static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 {
 	struct pci_sysdata *sd = &vmd->sysdata;
-	struct resource *res;
-	u32 upper_bits;
-	unsigned long flags;
 	LIST_HEAD(resources);
 	resource_size_t offset[2] = {0};
 	resource_size_t membar2_offset = 0x2000;
@@ -883,36 +922,12 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	 *
 	 * The only way we could use a 64-bit non-prefetchable MEMBAR is
 	 * if its address is <4GB so that we can convert it to a 32-bit
-	 * resource.  To be visible to the host OS, all VMD endpoints must
+	 * resource. To be visible to the host OS, all VMD endpoints must
 	 * be initially configured by platform BIOS, which includes setting
-	 * up these resources.  We can assume the device is configured
+	 * up these resources. We can assume the device is configured
 	 * according to the platform needs.
 	 */
-	res = &vmd->dev->resource[VMD_MEMBAR1];
-	upper_bits = upper_32_bits(res->end);
-	flags = res->flags & ~IORESOURCE_SIZEALIGN;
-	if (!upper_bits)
-		flags &= ~IORESOURCE_MEM_64;
-	vmd->resources[1] = (struct resource) {
-		.name  = "VMD MEMBAR1",
-		.start = res->start,
-		.end   = res->end,
-		.flags = flags,
-		.parent = res,
-	};
-
-	res = &vmd->dev->resource[VMD_MEMBAR2];
-	upper_bits = upper_32_bits(res->end);
-	flags = res->flags & ~IORESOURCE_SIZEALIGN;
-	if (!upper_bits)
-		flags &= ~IORESOURCE_MEM_64;
-	vmd->resources[2] = (struct resource) {
-		.name  = "VMD MEMBAR2",
-		.start = res->start + membar2_offset,
-		.end   = res->end,
-		.flags = flags,
-		.parent = res,
-	};
+	vmd_configure_membar1_membar2(vmd, membar2_offset);
 
 	/*
 	 * Currently MSI remapping must be enabled in guest passthrough mode
@@ -1074,6 +1089,11 @@ static void vmd_remove(struct pci_dev *dev)
 	pci_stop_root_bus(vmd->bus);
 	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
 	pci_remove_root_bus(vmd->bus);
+
+	/* CFGBAR is static, does not require releasing memory */
+	kfree(vmd->resources[1].name);
+	kfree(vmd->resources[2].name);
+
 	vmd_cleanup_srcu(vmd);
 	vmd_detach_resources(vmd);
 	vmd_remove_irq_domain(vmd);
-- 
2.43.0

Re: [PATCH v5 3/8] PCI: vmd: Add vmd_configure_membar() and vmd_configure_membar1_membar2()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Critical] Dynamically allocated resource names are freed before the resources are detached from the resource tree, causing a Use-After-Free window.
- [Medium] Dynamically allocated memory for VMD MEMBAR resource names is leaked on error paths.
- [Medium] The return value of kasprintf() is not checked, potentially leaving resource names uninitialized as NULL.

Pre-existing issues:
- [High] Error paths and device removal fail to call pci_free_irq_vectors(), causing IRQ resource leaks.
- [High] SRCU structures are destroyed before the IRQ handlers are unregistered, risking a UAF if an interrupt fires during teardown.
--

commit 295e9b4a624efdd5299bd60f26213363d5cf8c68
Author: Szymon Durawa <szymon.durawa@linux.intel.com>

PCI: vmd: Add vmd_configure_membar() and vmd_configure_membar1_membar2()

This commit moves the MEMBAR1 and MEMBAR2 registry initialization code into
new helper functions. It introduces vmd_configure_membar and
vmd_configure_membar1_membar2 to handle the resource setup.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
[ ... ]
> @@ -783,6 +783,48 @@ static void vmd_configure_cfgbar(struct vmd_dev *vmd)
[ ... ]
> +static void vmd_configure_membar(struct vmd_dev *vmd, u8 resource_number,
> +				 u8 membar_number, resource_size_t start_offset,
> +				 resource_size_t end_offset)
> +{
[ ... ]
> +	vmd->resources[resource_number] = (struct resource){
> +		.name = kasprintf(GFP_KERNEL, "VMD MEMBAR%d",
> +				  membar_number / 2),

[Severity: Medium]
Does this kasprintf call need a check for allocation failure? 

If kasprintf fails under memory pressure and returns a null pointer, the
resource structure will be initialized with a null name, which might violate
kernel API expectations for resource structures.

[Severity: Medium]
Could this dynamic allocation cause a memory leak if vmd_enable_domain fails
later on? 

Looking at vmd_enable_domain, if functions like vmd_alloc_irqs or
vmd_create_bus return an error, the function exits directly. It doesn't
appear to free these allocated strings on those error paths, leaving the
memory leaked.

[ ... ]
> @@ -1074,6 +1089,11 @@ static void vmd_remove(struct pci_dev *dev)
>  	pci_stop_root_bus(vmd->bus);
>  	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
>  	pci_remove_root_bus(vmd->bus);
> +
> +	/* CFGBAR is static, does not require releasing memory */
> +	kfree(vmd->resources[1].name);
> +	kfree(vmd->resources[2].name);

[Severity: Critical]
Can this lead to a use-after-free if a userspace tool reads proc iomem
concurrently? 

By calling kfree on the resource names here, the memory is freed before
vmd_detach_resources unlinks these resources from the parent PCI device
tree. A concurrent read of the global resource tree during this window
might dereference the freed pointer.

> +
>  	vmd_cleanup_srcu(vmd);

[Severity: High]
This is a pre-existing issue, but I noticed we don't seem to free the irq
vectors during teardown. 

In vmd_alloc_irqs, pci_alloc_irq_vectors is called, but vmd_remove completes
without calling pci_free_irq_vectors. Does this leak IRQ resources,
potentially exhausting system IRQ capacity over multiple bind and unbind
cycles?

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to destroy the SRCU
structure here before the interrupt handlers are unregistered? 

The handlers are registered with devm_request_irq, meaning they remain active
until the devres cleanup phase, which happens after vmd_remove finishes. If a
hardware interrupt fires in this window, vmd_irq will call srcu_read_lock on
the already destroyed SRCU object.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625082940.1290740-1-szymon.durawa@linux.intel.com?part=3

[PATCH v5 4/8] PCI: vmd: Add vmd_create_bus()

[Szymon Durawa]

Move the VMD bus initialization code to a new helper vmd_create_bus().
No functional changes.

Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com>
Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>
---
 drivers/pci/controller/vmd.c | 57 ++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 443a0cefb7f7..c0a953df5f23 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -825,6 +825,37 @@ static void vmd_configure_membar1_membar2(struct vmd_dev *vmd,
 	vmd_configure_membar(vmd, 2, VMD_MEMBAR2, mbar2_ofs, 0);
 }
 
+static int vmd_create_bus(struct vmd_dev *vmd, struct pci_sysdata *sd,
+			  resource_size_t *offset)
+{
+	LIST_HEAD(resources);
+
+	pci_add_resource(&resources, &vmd->resources[0]);
+	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);
+	pci_add_resource_offset(&resources, &vmd->resources[2], offset[1]);
+
+	vmd->bus = pci_create_root_bus(&vmd->dev->dev, vmd->busn_start,
+				       &vmd_ops, sd, &resources);
+	if (!vmd->bus) {
+		pci_bus_release_emul_domain_nr(sd->domain);
+		pci_free_resource_list(&resources);
+		vmd_remove_irq_domain(vmd);
+		return -ENODEV;
+	}
+
+	vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
+				   to_pci_host_bridge(vmd->bus->bridge));
+
+	vmd_attach_resources(vmd);
+	if (vmd->irq_domain)
+		dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
+	else
+		dev_set_msi_domain(&vmd->bus->dev,
+				   dev_get_msi_domain(&vmd->dev->dev));
+
+	return 0;
+}
+
 static void vmd_bus_enumeration(struct pci_bus *bus, unsigned long features)
 {
 	struct pci_bus *child;
@@ -876,7 +907,6 @@ static void vmd_bus_enumeration(struct pci_bus *bus, unsigned long features)
 static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 {
 	struct pci_sysdata *sd = &vmd->sysdata;
-	LIST_HEAD(resources);
 	resource_size_t offset[2] = {0};
 	resource_size_t membar2_offset = 0x2000;
 	int ret;
@@ -950,10 +980,6 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 		vmd_set_msi_remapping(vmd, false);
 	}
 
-	pci_add_resource(&resources, &vmd->resources[0]);
-	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);
-	pci_add_resource_offset(&resources, &vmd->resources[2], offset[1]);
-
 	sd->vmd_dev = vmd->dev;
 
 	/*
@@ -968,25 +994,12 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 
 	sd->node = pcibus_to_node(vmd->dev->bus);
 
-	vmd->bus = pci_create_root_bus(&vmd->dev->dev, vmd->busn_start,
-				       &vmd_ops, sd, &resources);
-	if (!vmd->bus) {
-		pci_bus_release_emul_domain_nr(sd->domain);
-		pci_free_resource_list(&resources);
-		vmd_remove_irq_domain(vmd);
-		return -ENODEV;
+	ret = vmd_create_bus(vmd, sd, offset);
+	if (ret) {
+		pci_err(vmd->dev, "Can't create bus: %d\n", ret);
+		return ret;
 	}
 
-	vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
-				   to_pci_host_bridge(vmd->bus->bridge));
-
-	vmd_attach_resources(vmd);
-	if (vmd->irq_domain)
-		dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
-	else
-		dev_set_msi_domain(&vmd->bus->dev,
-				   dev_get_msi_domain(&vmd->dev->dev));
-
 	WARN(sysfs_create_link(&vmd->dev->dev.kobj, &vmd->bus->dev.kobj,
 			       "domain"), "Can't create symlink to domain\n");
 
-- 
2.43.0

Re: [PATCH v5 4/8] PCI: vmd: Add vmd_create_bus()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Memory allocated by kasprintf for resource names is leaked on vmd_enable_domain() error paths.
--

commit 1ab8bc19707a3b648b92906666ae290aa0884407
Author: Szymon Durawa <szymon.durawa@linux.intel.com>

PCI: vmd: Add vmd_create_bus()

This commit moves the VMD bus initialization logic into a new helper function
vmd_create_bus(). It aims to simplify the domain enablement code without
introducing functional changes.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 443a0cefb7f74..c0a953df5f238 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c

[ ... ]

> @@ -968,25 +994,12 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  
>  	sd->node = pcibus_to_node(vmd->dev->bus);
>  
> -	vmd->bus = pci_create_root_bus(&vmd->dev->dev, vmd->busn_start,
> -				       &vmd_ops, sd, &resources);
> -	if (!vmd->bus) {
> -		pci_bus_release_emul_domain_nr(sd->domain);
> -		pci_free_resource_list(&resources);
> -		vmd_remove_irq_domain(vmd);
> -		return -ENODEV;
> +	ret = vmd_create_bus(vmd, sd, offset);
> +	if (ret) {
> +		pci_err(vmd->dev, "Can't create bus: %d\n", ret);
> +		return ret;

[Severity: High]
Does this error path in vmd_enable_domain() leak the resource names allocated
by kasprintf() during the earlier memory bar configuration?

If vmd_create_bus() fails and returns an error, does this early return
lack the necessary rollback logic? Could this leave the previously
initialized bus active while its sysdata pointer (the vmd_dev struct)
is later freed by devres, resulting in a use-after-free?

>  	}
>  
> -	vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
> -				   to_pci_host_bridge(vmd->bus->bridge));
> -
> -	vmd_attach_resources(vmd);
> -	if (vmd->irq_domain)
> -		dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
> -	else
> -		dev_set_msi_domain(&vmd->bus->dev,
> -				   dev_get_msi_domain(&vmd->dev->dev));
> -
>  	WARN(sysfs_create_link(&vmd->dev->dev.kobj, &vmd->bus->dev.kobj,
>  			       "domain"), "Can't create symlink to domain\n");
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625082940.1290740-1-szymon.durawa@linux.intel.com?part=4

[PATCH v5 5/8] PCI: vmd: Replace hardcoded values with enum and defines

[Szymon Durawa]

Add enum vmd_resource type to replace hardcoded values. Add defines for
vmd bus start number based on VMD restriction value. No functional
changes.

Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com>
Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>
---
 drivers/pci/controller/vmd.c | 43 ++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index c0a953df5f23..9ff82a24e859 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -26,6 +26,11 @@
 #define VMD_MEMBAR1	2
 #define VMD_MEMBAR2	4
 
+/* VMD restriction value determines secondary start bus number */
+#define VMD_RESTRICT_0_BUS_START 0x0
+#define VMD_RESTRICT_1_BUS_START 0x80
+#define VMD_RESTRICT_2_BUS_START 0xE0
+
 #define PCI_REG_VMCAP		0x40
 #define BUS_RESTRICT_CAP(vmcap)	(vmcap & 0x1)
 #define PCI_REG_VMCONFIG	0x44
@@ -37,6 +42,13 @@
 #define MB2_SHADOW_OFFSET	0x2000
 #define MB2_SHADOW_SIZE		16
 
+enum vmd_resource {
+	VMD_RES_CFGBAR = 0, /* VMD Bus0 Config BAR */
+	VMD_RES_MBAR_1, /* VMD Bus0 Resource MemBAR 1 */
+	VMD_RES_MBAR_2, /* VMD Bus0 Resource MemBAR 2 */
+	VMD_RES_COUNT
+};
+
 enum vmd_features {
 	/*
 	 * Device may contain registers which hint the physical location of the
@@ -135,7 +147,7 @@ struct vmd_dev {
 	struct vmd_irq_list	*irqs;
 
 	struct pci_sysdata	sysdata;
-	struct resource		resources[3];
+	struct resource		resources[VMD_RES_COUNT];
 	struct irq_domain	*irq_domain;
 	struct pci_bus		*bus;
 	u8			busn_start;
@@ -520,7 +532,7 @@ static inline void vmd_acpi_end(void) { }
 
 static void vmd_domain_reset(struct vmd_dev *vmd)
 {
-	u16 bus, max_buses = resource_size(&vmd->resources[0]);
+	u16 bus, max_buses = resource_size(&vmd->resources[VMD_RES_CFGBAR]);
 	u8 dev, functions, fn, hdr_type;
 	char __iomem *base;
 
@@ -568,8 +580,8 @@ static void vmd_domain_reset(struct vmd_dev *vmd)
 
 static void vmd_attach_resources(struct vmd_dev *vmd)
 {
-	vmd->dev->resource[VMD_MEMBAR1].child = &vmd->resources[1];
-	vmd->dev->resource[VMD_MEMBAR2].child = &vmd->resources[2];
+	vmd->dev->resource[VMD_MEMBAR1].child = &vmd->resources[VMD_RES_MBAR_1];
+	vmd->dev->resource[VMD_MEMBAR2].child = &vmd->resources[VMD_RES_MBAR_2];
 }
 
 static void vmd_detach_resources(struct vmd_dev *vmd)
@@ -643,13 +655,13 @@ static int vmd_get_bus_number_start(struct vmd_dev *vmd)
 
 		switch (BUS_RESTRICT_CFG(reg)) {
 		case 0:
-			vmd->busn_start = 0;
+			vmd->busn_start = VMD_RESTRICT_0_BUS_START;
 			break;
 		case 1:
-			vmd->busn_start = 128;
+			vmd->busn_start = VMD_RESTRICT_1_BUS_START;
 			break;
 		case 2:
-			vmd->busn_start = 224;
+			vmd->busn_start = VMD_RESTRICT_2_BUS_START;
 			break;
 		default:
 			pci_err(dev, "Unknown Bus Offset Setting (%d)\n",
@@ -775,7 +787,7 @@ static void vmd_configure_cfgbar(struct vmd_dev *vmd)
 {
 	struct resource *res = &vmd->dev->resource[VMD_CFGBAR];
 
-	vmd->resources[0] = (struct resource){
+	vmd->resources[VMD_RES_CFGBAR] = (struct resource){
 		.name = "VMD CFGBAR",
 		.start = vmd->busn_start,
 		.end = vmd->busn_start + (resource_size(res) >> 20) - 1,
@@ -794,7 +806,8 @@ static void vmd_configure_cfgbar(struct vmd_dev *vmd)
  *
  * Function fills resource buffer inside the VMD structure.
  */
-static void vmd_configure_membar(struct vmd_dev *vmd, u8 resource_number,
+static void vmd_configure_membar(struct vmd_dev *vmd,
+				 enum vmd_resource resource_number,
 				 u8 membar_number, resource_size_t start_offset,
 				 resource_size_t end_offset)
 {
@@ -830,9 +843,11 @@ static int vmd_create_bus(struct vmd_dev *vmd, struct pci_sysdata *sd,
 {
 	LIST_HEAD(resources);
 
-	pci_add_resource(&resources, &vmd->resources[0]);
-	pci_add_resource_offset(&resources, &vmd->resources[1], offset[0]);
-	pci_add_resource_offset(&resources, &vmd->resources[2], offset[1]);
+	pci_add_resource(&resources, &vmd->resources[VMD_RES_CFGBAR]);
+	pci_add_resource_offset(&resources, &vmd->resources[VMD_RES_MBAR_1],
+				offset[0]);
+	pci_add_resource_offset(&resources, &vmd->resources[VMD_RES_MBAR_2],
+				offset[1]);
 
 	vmd->bus = pci_create_root_bus(&vmd->dev->dev, vmd->busn_start,
 				       &vmd_ops, sd, &resources);
@@ -1104,8 +1119,8 @@ static void vmd_remove(struct pci_dev *dev)
 	pci_remove_root_bus(vmd->bus);
 
 	/* CFGBAR is static, does not require releasing memory */
-	kfree(vmd->resources[1].name);
-	kfree(vmd->resources[2].name);
+	kfree(vmd->resources[VMD_RES_MBAR_1].name);
+	kfree(vmd->resources[VMD_RES_MBAR_2].name);
 
 	vmd_cleanup_srcu(vmd);
 	vmd_detach_resources(vmd);
-- 
2.43.0

Re: [PATCH v5 5/8] PCI: vmd: Replace hardcoded values with enum and defines

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Leak of dynamically allocated resource names on the vmd_probe error path.

Pre-existing issues:
- [High] Missing pci_free_irq_vectors() calls in error paths and during device removal, leading to vector leakage.
- [High] vmd_cleanup_srcu() destroys SRCU structs before devm-managed IRQ handlers are unregistered.
--

commit 415d4461cb0ec06aaac750c476d8975aa2ffae22
Author: Szymon Durawa <szymon.durawa@linux.intel.com>

PCI: vmd: Replace hardcoded values with enum and defines

Add enum vmd_resource type to replace hardcoded values. Add defines for
vmd bus start number based on VMD restriction value. No functional
changes.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index c0a953df5f238..9ff82a24e859c 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
[ ... ]
> @@ -794,7 +806,8 @@ static void vmd_configure_cfgbar(struct vmd_dev *vmd)
>   *
>   * Function fills resource buffer inside the VMD structure.
>   */
> -static void vmd_configure_membar(struct vmd_dev *vmd, u8 resource_number,
> +static void vmd_configure_membar(struct vmd_dev *vmd,
> +				 enum vmd_resource resource_number,
>  				 u8 membar_number, resource_size_t start_offset,
>  				 resource_size_t end_offset)
>  {

[Severity: Medium]
Does this memory allocation lack proper cleanup in error paths?

In vmd_configure_membar(), resource names are allocated dynamically:

    .name = kasprintf(GFP_KERNEL, "VMD MEMBAR%d %s", membar_number / 2, ...),

If vmd_enable_domain() fails after these names are successfully allocated,
the error propagates to vmd_probe(). The error handling block at the
out_release_instance label returns without explicitly freeing these allocated
strings, which leaks memory.

[ ... ]
> @@ -1104,8 +1119,8 @@ static void vmd_remove(struct pci_dev *dev)
>  	pci_remove_root_bus(vmd->bus);
>  
>  	/* CFGBAR is static, does not require releasing memory */
> -	kfree(vmd->resources[1].name);
> -	kfree(vmd->resources[2].name);
> +	kfree(vmd->resources[VMD_RES_MBAR_1].name);
> +	kfree(vmd->resources[VMD_RES_MBAR_2].name);

[Severity: High]
This isn't a bug introduced by this patch, but does this function omit the
required call to pci_free_irq_vectors()?

The PCI subsystem guidelines require that after a successful call to
pci_alloc_irq_vectors(), every error path and device removal must explicitly
call pci_free_irq_vectors() to prevent leaking IRQ vectors.

If init_srcu_struct() or devm_request_irq() fails in vmd_alloc_irqs(), or if
any operation fails in vmd_enable_domain() after allocation, the function
returns without cleanup. 

Similarly, this vmd_remove() function completes tear-down without freeing 
the vectors. Exhausting system IRQ capacity could prevent future allocations.

>  
>  	vmd_cleanup_srcu(vmd);

[Severity: High]
This isn't a bug introduced by this patch, but does cleaning up SRCU here
create a race condition with the devm-managed IRQ handler?

During device removal, vmd_remove() calls vmd_cleanup_srcu() to destroy
the SRCU structs. However, the driver's IRQ handlers were registered using
devm_request_irq(), which means they aren't deregistered until the devres
cleanup phase executes after vmd_remove() returns.

If a spurious MSI interrupt fires during this window, the handler will run
and invoke srcu_read_lock() on the already destroyed srcu_struct:

drivers/pci/controller/vmd.c:vmd_irq() {
    ...
    idx = srcu_read_lock(&irqs->srcu);
    ...
}

Could this use-after-free on per-CPU memory lead to memory corruption?

>  	vmd_detach_resources(vmd);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625082940.1290740-1-szymon.durawa@linux.intel.com?part=5

[PATCH v5 6/8] PCI: vmd: Convert bus and busn_start to an array

[Szymon Durawa]

Convert bus and busn_start from scalar to an array to support
multiple VMD buses in the future. No functional changes.

Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com>
Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>
---
 drivers/pci/controller/vmd.c | 46 +++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 9ff82a24e859..2bfc16875d91 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -49,6 +49,11 @@ enum vmd_resource {
 	VMD_RES_COUNT
 };
 
+enum vmd_rootbus {
+	VMD_BUS_0 = 0,
+	VMD_BUS_COUNT
+};
+
 enum vmd_features {
 	/*
 	 * Device may contain registers which hint the physical location of the
@@ -149,8 +154,8 @@ struct vmd_dev {
 	struct pci_sysdata	sysdata;
 	struct resource		resources[VMD_RES_COUNT];
 	struct irq_domain	*irq_domain;
-	struct pci_bus		*bus;
-	u8			busn_start;
+	struct pci_bus		*bus[VMD_BUS_COUNT];
+	u8			busn_start[VMD_BUS_COUNT];
 	u8			first_vec;
 	char			*name;
 	int			instance;
@@ -404,7 +409,7 @@ static void vmd_remove_irq_domain(struct vmd_dev *vmd)
 static void __iomem *vmd_cfg_addr(struct vmd_dev *vmd, struct pci_bus *bus,
 				  unsigned int devfn, int reg, int len)
 {
-	unsigned int busnr_ecam = bus->number - vmd->busn_start;
+	unsigned int busnr_ecam = bus->number - vmd->busn_start[VMD_BUS_0];
 	u32 offset = PCIE_ECAM_OFFSET(busnr_ecam, devfn, reg);
 
 	if (offset + len >= resource_size(&vmd->dev->resource[VMD_CFGBAR]))
@@ -655,13 +660,13 @@ static int vmd_get_bus_number_start(struct vmd_dev *vmd)
 
 		switch (BUS_RESTRICT_CFG(reg)) {
 		case 0:
-			vmd->busn_start = VMD_RESTRICT_0_BUS_START;
+			vmd->busn_start[VMD_BUS_0] = VMD_RESTRICT_0_BUS_START;
 			break;
 		case 1:
-			vmd->busn_start = VMD_RESTRICT_1_BUS_START;
+			vmd->busn_start[VMD_BUS_0] = VMD_RESTRICT_1_BUS_START;
 			break;
 		case 2:
-			vmd->busn_start = VMD_RESTRICT_2_BUS_START;
+			vmd->busn_start[VMD_BUS_0] = VMD_RESTRICT_2_BUS_START;
 			break;
 		default:
 			pci_err(dev, "Unknown Bus Offset Setting (%d)\n",
@@ -789,8 +794,9 @@ static void vmd_configure_cfgbar(struct vmd_dev *vmd)
 
 	vmd->resources[VMD_RES_CFGBAR] = (struct resource){
 		.name = "VMD CFGBAR",
-		.start = vmd->busn_start,
-		.end = vmd->busn_start + (resource_size(res) >> 20) - 1,
+		.start = vmd->busn_start[VMD_BUS_0],
+		.end = vmd->busn_start[VMD_BUS_0] +
+		       (resource_size(res) >> 20) - 1,
 		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
 	};
 }
@@ -849,9 +855,10 @@ static int vmd_create_bus(struct vmd_dev *vmd, struct pci_sysdata *sd,
 	pci_add_resource_offset(&resources, &vmd->resources[VMD_RES_MBAR_2],
 				offset[1]);
 
-	vmd->bus = pci_create_root_bus(&vmd->dev->dev, vmd->busn_start,
-				       &vmd_ops, sd, &resources);
-	if (!vmd->bus) {
+	vmd->bus[VMD_BUS_0] = pci_create_root_bus(&vmd->dev->dev,
+						  vmd->busn_start[VMD_BUS_0],
+						  &vmd_ops, sd, &resources);
+	if (!vmd->bus[VMD_BUS_0]) {
 		pci_bus_release_emul_domain_nr(sd->domain);
 		pci_free_resource_list(&resources);
 		vmd_remove_irq_domain(vmd);
@@ -859,13 +866,13 @@ static int vmd_create_bus(struct vmd_dev *vmd, struct pci_sysdata *sd,
 	}
 
 	vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
-				   to_pci_host_bridge(vmd->bus->bridge));
+		to_pci_host_bridge(vmd->bus[VMD_BUS_0]->bridge));
 
 	vmd_attach_resources(vmd);
 	if (vmd->irq_domain)
-		dev_set_msi_domain(&vmd->bus->dev, vmd->irq_domain);
+		dev_set_msi_domain(&vmd->bus[VMD_BUS_0]->dev, vmd->irq_domain);
 	else
-		dev_set_msi_domain(&vmd->bus->dev,
+		dev_set_msi_domain(&vmd->bus[VMD_BUS_0]->dev,
 				   dev_get_msi_domain(&vmd->dev->dev));
 
 	return 0;
@@ -1015,10 +1022,11 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 		return ret;
 	}
 
-	WARN(sysfs_create_link(&vmd->dev->dev.kobj, &vmd->bus->dev.kobj,
-			       "domain"), "Can't create symlink to domain\n");
+	WARN(sysfs_create_link(&vmd->dev->dev.kobj,
+			       &vmd->bus[VMD_BUS_0]->dev.kobj, "domain"),
+	     "Can't create symlink to domain\n");
 
-	vmd_bus_enumeration(vmd->bus, features);
+	vmd_bus_enumeration(vmd->bus[VMD_BUS_0], features);
 
 	return 0;
 }
@@ -1114,9 +1122,9 @@ static void vmd_remove(struct pci_dev *dev)
 {
 	struct vmd_dev *vmd = pci_get_drvdata(dev);
 
-	pci_stop_root_bus(vmd->bus);
+	pci_stop_root_bus(vmd->bus[VMD_BUS_0]);
 	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
-	pci_remove_root_bus(vmd->bus);
+	pci_remove_root_bus(vmd->bus[VMD_BUS_0]);
 
 	/* CFGBAR is static, does not require releasing memory */
 	kfree(vmd->resources[VMD_RES_MBAR_1].name);
-- 
2.43.0

Re: [PATCH v5 6/8] PCI: vmd: Convert bus and busn_start to an array

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Failing to call pci_free_irq_vectors() in error paths and driver removal leaks IRQ resources.
- [High] SRCU structures are destroyed before IRQ vectors are explicitly disabled, risking a Use-After-Free on spurious interrupts.
--

commit 5baf3e06c174f82b0781954bbca2834e43441922
Author: Szymon Durawa <szymon.durawa@linux.intel.com>

PCI: vmd: Convert bus and busn_start to an array

Convert bus and busn_start from scalar to an array to support multiple VMD
buses in the future. No functional changes.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 9ff82a24e859c..2bfc16875d912 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c

[ ... ]

> @@ -1114,9 +1122,9 @@ static void vmd_remove(struct pci_dev *dev)
>  {
>  	struct vmd_dev *vmd = pci_get_drvdata(dev);
>  
> -	pci_stop_root_bus(vmd->bus);
> +	pci_stop_root_bus(vmd->bus[VMD_BUS_0]);
>  	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
> -	pci_remove_root_bus(vmd->bus);
> +	pci_remove_root_bus(vmd->bus[VMD_BUS_0]);

[Severity: High]
This is a pre-existing issue, but does device removal or probe failure leak
IRQ resources?

In vmd_alloc_irqs(), vectors are successfully allocated:

vmd->msix_count = pci_alloc_irq_vectors(dev, vmd->first_vec + 1,
                                        vmd->msix_count, PCI_IRQ_MSIX);

However, they do not appear to be explicitly freed in error paths or upon
driver removal in vmd_remove(). The PCI subsystem guidelines state that every
error path or removal after successful allocation must call
pci_free_irq_vectors() to prevent resource leaks and potential exhaustion of
system IRQ capacity.

[Severity: High]
This is also a pre-existing issue, but is there a use-after-free risk during
teardown if a spurious interrupt fires?

In vmd_remove(), the SRCU structures are destroyed via vmd_cleanup_srcu().
However, the IRQ handlers are managed by devres (via devm_request_irq) and
will only be disabled after vmd_remove() returns.

If a spurious or shared interrupt fires after the SRCU structures are
destroyed but before devres cleanup completes, the handler will execute
and access the destroyed struct:

vmd_irq() {
    ...
    idx = srcu_read_lock(&irqs->srcu);
}

Could this race condition lead to memory corruption or a crash? Should the
interrupts be explicitly disabled before destroying the SRCU structures?

>  	/* CFGBAR is static, does not require releasing memory */
>  	kfree(vmd->resources[VMD_RES_MBAR_1].name);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625082940.1290740-1-szymon.durawa@linux.intel.com?part=6

[PATCH v5 7/8] PCI: vmd: Add support for second rootbus under VMD

[Szymon Durawa]

Starting from Intel Arrow Lake VMD enhancement introduces second root bus
support with fixed root bus number (0x80). It means that all 3 MMIO BARs
exposed by VMD are shared now between both buses (current BUS0 and
new BUS1).

Add new BUS1 enumeration and divide MMIO space to be shared between
both root buses. Due to enumeration issues with root bus hardwired to a
fixed non-zero value, this patch will work with a workaround proposed
in next patch. Without workaround user won't see attached devices for BUS1
root bus.

Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com>
Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>
---
 drivers/pci/controller/vmd.c | 225 ++++++++++++++++++++++++++++++-----
 1 file changed, 195 insertions(+), 30 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 2bfc16875d91..c68ea7837620 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2015, Intel Corporation.
  */
 
+#include <linux/bitfield.h>
 #include <linux/device.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
@@ -30,6 +31,7 @@
 #define VMD_RESTRICT_0_BUS_START 0x0
 #define VMD_RESTRICT_1_BUS_START 0x80
 #define VMD_RESTRICT_2_BUS_START 0xE0
+#define VMD_RESTRICT_3_BUS_START 0xE1
 
 #define PCI_REG_VMCAP		0x40
 #define BUS_RESTRICT_CAP(vmcap)	(vmcap & 0x1)
@@ -42,15 +44,36 @@
 #define MB2_SHADOW_OFFSET	0x2000
 #define MB2_SHADOW_SIZE		16
 
+/* Primary Bus Number for VMD devices on root bus 0 */
+#define VMD_PRIMARY_BUS0    0x00
+/* Primary Bus Number for VMD devices on root bus 1 */
+#define VMD_PRIMARY_BUS1    0x80
+
+#define VMD_BUSRANGE0       0xc8
+#define VMD_BUSRANGE1       0xcc
+#define VMD_MEMBAR1_OFFSET  0xd0
+#define VMD_MEMBAR2_OFFSET1 0xd8
+#define VMD_MEMBAR2_OFFSET2 0xdc
+#define VMD_BUS_END(busr) FIELD_GET(GENMASK(15, 8), busr)
+#define VMD_BUS_START(busr) FIELD_GET(GENMASK(7, 0), busr)
+
+/*
+ * Add VMD resources for BUS1, it will share the same MMIO space with
+ * previous VMD resources.
+ */
 enum vmd_resource {
-	VMD_RES_CFGBAR = 0, /* VMD Bus0 Config BAR */
-	VMD_RES_MBAR_1, /* VMD Bus0 Resource MemBAR 1 */
-	VMD_RES_MBAR_2, /* VMD Bus0 Resource MemBAR 2 */
+	VMD_RES_CFGBAR = 0,  /* VMD Bus0 Config BAR */
+	VMD_RES_MBAR_1,      /* VMD Bus0 Resource MemBAR 1 */
+	VMD_RES_MBAR_2,      /* VMD Bus0 Resource MemBAR 2 */
+	VMD_RES_BUS1_CFGBAR, /* VMD Bus1 Config BAR */
+	VMD_RES_BUS1_MBAR_1, /* VMD Bus1 Resource MemBAR 1 */
+	VMD_RES_BUS1_MBAR_2, /* VMD Bus1 Resource MemBAR 2 */
 	VMD_RES_COUNT
 };
 
 enum vmd_rootbus {
 	VMD_BUS_0 = 0,
+	VMD_BUS_1,
 	VMD_BUS_COUNT
 };
 
@@ -94,6 +117,12 @@ enum vmd_features {
 	 * proper power management of the SoC.
 	 */
 	VMD_FEAT_BIOS_PM_QUIRK		= (1 << 5),
+
+	/*
+	 * Starting from Intel Arrow Lake, VMD devices have their VMD root ports
+	 * on the additional BUS1 root bus.
+	 */
+	VMD_FEAT_HAS_BUS1_ROOTBUS	= (1 << 6)
 };
 
 #define VMD_BIOS_PM_QUIRK_LTR	0x1003	/* 3145728 ns */
@@ -101,7 +130,8 @@ enum vmd_features {
 #define VMD_FEATS_CLIENT	(VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |	\
 				 VMD_FEAT_HAS_BUS_RESTRICTIONS |	\
 				 VMD_FEAT_OFFSET_FIRST_VECTOR |		\
-				 VMD_FEAT_BIOS_PM_QUIRK)
+				 VMD_FEAT_BIOS_PM_QUIRK |		\
+				 VMD_FEAT_HAS_BUS1_ROOTBUS)
 
 static DEFINE_IDA(vmd_instance_ida);
 
@@ -159,6 +189,7 @@ struct vmd_dev {
 	u8			first_vec;
 	char			*name;
 	int			instance;
+	bool			bus1_rootbus;
 };
 
 static inline struct vmd_dev *vmd_from_bus(struct pci_bus *bus)
@@ -537,11 +568,23 @@ static inline void vmd_acpi_end(void) { }
 
 static void vmd_domain_reset(struct vmd_dev *vmd)
 {
-	u16 bus, max_buses = resource_size(&vmd->resources[VMD_RES_CFGBAR]);
+	u16 bus, bus_cnt = resource_size(&vmd->resources[VMD_RES_CFGBAR]);
 	u8 dev, functions, fn, hdr_type;
 	char __iomem *base;
 
-	for (bus = 0; bus < max_buses; bus++) {
+	/*
+	 * Total bus-number count (bus_cnt) when BUS1 root bus is present:
+	 * initialized with BUS0 CFGBAR range, extended by BUS1 CFGBAR range plus
+	 * one bus number per root bus' own config space (bus_cnt is increased by 2)
+	 * as the CFGBAR ranges do not include the root bus config space.
+	 */
+
+	if (vmd->bus1_rootbus) {
+		bus_cnt += resource_size(&vmd->resources[VMD_RES_BUS1_CFGBAR]);
+		bus_cnt += 2;
+	}
+
+	for (bus = 0; bus < bus_cnt; bus++) {
 		for (dev = 0; dev < 32; dev++) {
 			base = vmd->cfgbar + PCIE_ECAM_OFFSET(bus,
 						PCI_DEVFN(dev, 0), 0);
@@ -587,12 +630,24 @@ static void vmd_attach_resources(struct vmd_dev *vmd)
 {
 	vmd->dev->resource[VMD_MEMBAR1].child = &vmd->resources[VMD_RES_MBAR_1];
 	vmd->dev->resource[VMD_MEMBAR2].child = &vmd->resources[VMD_RES_MBAR_2];
+
+	if (vmd->bus1_rootbus) {
+		vmd->resources[VMD_RES_MBAR_1].sibling =
+			&vmd->resources[VMD_RES_BUS1_MBAR_1];
+		vmd->resources[VMD_RES_MBAR_2].sibling =
+			&vmd->resources[VMD_RES_BUS1_MBAR_2];
+	}
 }
 
 static void vmd_detach_resources(struct vmd_dev *vmd)
 {
 	vmd->dev->resource[VMD_MEMBAR1].child = NULL;
 	vmd->dev->resource[VMD_MEMBAR2].child = NULL;
+
+	if (vmd->bus1_rootbus) {
+		vmd->resources[VMD_RES_MBAR_1].sibling = NULL;
+		vmd->resources[VMD_RES_MBAR_2].sibling = NULL;
+	}
 }
 
 static int vmd_get_phys_offsets(struct vmd_dev *vmd, bool native_hint,
@@ -649,7 +704,7 @@ static int vmd_get_phys_offsets(struct vmd_dev *vmd, bool native_hint,
 	return 0;
 }
 
-static int vmd_get_bus_number_start(struct vmd_dev *vmd)
+static int vmd_get_bus_number_start(struct vmd_dev *vmd, unsigned long features)
 {
 	struct pci_dev *dev = vmd->dev;
 	u16 reg;
@@ -668,6 +723,19 @@ static int vmd_get_bus_number_start(struct vmd_dev *vmd)
 		case 2:
 			vmd->busn_start[VMD_BUS_0] = VMD_RESTRICT_2_BUS_START;
 			break;
+		case 3:
+			if (!(features & VMD_FEAT_HAS_BUS1_ROOTBUS)) {
+				pci_err(dev, "VMD Bus Restriction detected type %d, but BUS1 Rootbus is not supported, aborting.\n",
+					BUS_RESTRICT_CFG(reg));
+				return -ENODEV;
+			}
+
+			/* VMD (on root bus 0) secondary bus start number */
+			vmd->busn_start[VMD_BUS_0] = VMD_RESTRICT_2_BUS_START;
+			/* VMD (on root bus 1) secondary bus start number */
+			vmd->busn_start[VMD_BUS_1] = VMD_RESTRICT_3_BUS_START;
+			vmd->bus1_rootbus = true;
+			break;
 		default:
 			pci_err(dev, "Unknown Bus Offset Setting (%d)\n",
 				BUS_RESTRICT_CFG(reg));
@@ -799,6 +867,30 @@ static void vmd_configure_cfgbar(struct vmd_dev *vmd)
 		       (resource_size(res) >> 20) - 1,
 		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
 	};
+
+	if (vmd->bus1_rootbus) {
+		u16 bus0_range = 0;
+		u16 bus1_range = 0;
+
+		pci_read_config_word(vmd->dev, VMD_BUSRANGE0, &bus0_range);
+		pci_read_config_word(vmd->dev, VMD_BUSRANGE1, &bus1_range);
+
+		/*
+		 * Resize BUS0 CFGBAR range to make space for BUS1
+		 * owned devices by adjusting range end with value stored in
+		 * VMD_BUSRANGE0 register.
+		 */
+		vmd->resources[VMD_RES_CFGBAR].start = VMD_BUS_START(bus0_range);
+		vmd->resources[VMD_RES_CFGBAR].end = VMD_BUS_END(bus0_range);
+
+		vmd->resources[VMD_RES_BUS1_CFGBAR] = (struct resource){
+			.name = "VMD CFGBAR BUS1",
+			.start = VMD_BUS_START(bus1_range),
+			.end = VMD_BUS_END(bus1_range),
+			.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
+			.parent = res,
+		};
+	}
 }
 
 /*
@@ -828,8 +920,8 @@ static void vmd_configure_membar(struct vmd_dev *vmd,
 		flags &= ~IORESOURCE_MEM_64;
 
 	vmd->resources[resource_number] = (struct resource){
-		.name = kasprintf(GFP_KERNEL, "VMD MEMBAR%d",
-				  membar_number / 2),
+		.name = kasprintf(GFP_KERNEL, "VMD MEMBAR%d %s", membar_number / 2,
+			resource_number > VMD_RES_MBAR_2 ? "BUS1" : ""),
 		.start = res->start + start_offset,
 		.end = res->end - end_offset,
 		.flags = flags,
@@ -840,41 +932,83 @@ static void vmd_configure_membar(struct vmd_dev *vmd,
 static void vmd_configure_membar1_membar2(struct vmd_dev *vmd,
 					  resource_size_t mbar2_ofs)
 {
-	vmd_configure_membar(vmd, 1, VMD_MEMBAR1, 0, 0);
-	vmd_configure_membar(vmd, 2, VMD_MEMBAR2, mbar2_ofs, 0);
+	if (vmd->bus1_rootbus) {
+		u32 bus1_mbar1_ofs = 0;
+		u64 bus1_mbar2_ofs = 0;
+		u32 reg;
+
+		pci_read_config_dword(vmd->dev, VMD_MEMBAR1_OFFSET,
+				      &bus1_mbar1_ofs);
+
+		pci_read_config_dword(vmd->dev, VMD_MEMBAR2_OFFSET1, &reg);
+		bus1_mbar2_ofs = reg;
+
+		pci_read_config_dword(vmd->dev, VMD_MEMBAR2_OFFSET2, &reg);
+		bus1_mbar2_ofs |= (u64)reg << 32;
+
+		/*
+		 * Resize BUS MEMBAR1 and MEMBAR2 ranges to make space
+		 * for BUS1 owned devices by adjusting range end with values
+		 * stored in VMD_MEMBAR1_OFFSET and VMD_MEMBAR2_OFFSET registers
+		 */
+		vmd_configure_membar(vmd, VMD_RES_MBAR_1, VMD_MEMBAR1, 0,
+				     bus1_mbar1_ofs);
+		vmd_configure_membar(vmd, VMD_RES_MBAR_2, VMD_MEMBAR2,
+				     mbar2_ofs, bus1_mbar2_ofs - mbar2_ofs);
+
+		vmd_configure_membar(vmd, VMD_RES_BUS1_MBAR_1, VMD_MEMBAR1,
+				     bus1_mbar1_ofs, 0);
+		vmd_configure_membar(vmd, VMD_RES_BUS1_MBAR_2, VMD_MEMBAR2,
+				     mbar2_ofs + bus1_mbar2_ofs, 0);
+	} else {
+		vmd_configure_membar(vmd, VMD_RES_MBAR_1, VMD_MEMBAR1, 0, 0);
+		vmd_configure_membar(vmd, VMD_RES_MBAR_2, VMD_MEMBAR2,
+				     mbar2_ofs, 0);
+	}
 }
 
-static int vmd_create_bus(struct vmd_dev *vmd, struct pci_sysdata *sd,
-			  resource_size_t *offset)
+static int vmd_create_bus(struct vmd_dev *vmd, enum vmd_rootbus bus_number,
+			  struct pci_sysdata *sd, resource_size_t *offset)
 {
+	u8 cfgbar = bus_number * 3;
+	u8 membar1 = cfgbar + 1;
+	u8 membar2 = cfgbar + 2;
+	struct pci_bus *vmd_bus;
 	LIST_HEAD(resources);
 
-	pci_add_resource(&resources, &vmd->resources[VMD_RES_CFGBAR]);
-	pci_add_resource_offset(&resources, &vmd->resources[VMD_RES_MBAR_1],
+	pci_add_resource(&resources, &vmd->resources[cfgbar]);
+	pci_add_resource_offset(&resources, &vmd->resources[membar1],
 				offset[0]);
-	pci_add_resource_offset(&resources, &vmd->resources[VMD_RES_MBAR_2],
+	pci_add_resource_offset(&resources, &vmd->resources[membar2],
 				offset[1]);
 
-	vmd->bus[VMD_BUS_0] = pci_create_root_bus(&vmd->dev->dev,
-						  vmd->busn_start[VMD_BUS_0],
-						  &vmd_ops, sd, &resources);
-	if (!vmd->bus[VMD_BUS_0]) {
-		pci_bus_release_emul_domain_nr(sd->domain);
+	vmd_bus = pci_create_root_bus(&vmd->dev->dev,
+				      vmd->busn_start[bus_number], &vmd_ops, sd,
+				      &resources);
+
+	if (!vmd_bus) {
 		pci_free_resource_list(&resources);
-		vmd_remove_irq_domain(vmd);
+
+		/* Clean only when BUS0 root bus cannot be created */
+		if (bus_number == VMD_PRIMARY_BUS0) {
+			pci_bus_release_emul_domain_nr(sd->domain);
+			vmd_remove_irq_domain(vmd);
+		}
 		return -ENODEV;
 	}
 
 	vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
-		to_pci_host_bridge(vmd->bus[VMD_BUS_0]->bridge));
+		to_pci_host_bridge(vmd_bus->bridge));
 
 	vmd_attach_resources(vmd);
 	if (vmd->irq_domain)
-		dev_set_msi_domain(&vmd->bus[VMD_BUS_0]->dev, vmd->irq_domain);
+		dev_set_msi_domain(&vmd_bus->dev, vmd->irq_domain);
 	else
-		dev_set_msi_domain(&vmd->bus[VMD_BUS_0]->dev,
+		dev_set_msi_domain(&vmd_bus->dev,
 				   dev_get_msi_domain(&vmd->dev->dev));
 
+	vmd->bus[bus_number] = vmd_bus;
+
 	return 0;
 }
 
@@ -887,7 +1021,9 @@ static void vmd_bus_enumeration(struct pci_bus *bus, unsigned long features)
 	vmd_acpi_begin();
 
 	pci_scan_child_bus(bus);
-	vmd_domain_reset(vmd_from_bus(bus));
+
+	if (bus->primary == VMD_PRIMARY_BUS0)
+		vmd_domain_reset(vmd_from_bus(bus));
 
 	/*
 	 * When Intel VMD is enabled, the OS does not discover the Root Ports
@@ -955,7 +1091,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	 * limits the bus range to between 0-127, 128-255, or 224-255
 	 */
 	if (features & VMD_FEAT_HAS_BUS_RESTRICTIONS) {
-		ret = vmd_get_bus_number_start(vmd);
+		ret = vmd_get_bus_number_start(vmd, features);
 		if (ret)
 			return ret;
 	}
@@ -1016,7 +1152,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 
 	sd->node = pcibus_to_node(vmd->dev->bus);
 
-	ret = vmd_create_bus(vmd, sd, offset);
+	ret = vmd_create_bus(vmd, VMD_BUS_0, sd, offset);
 	if (ret) {
 		pci_err(vmd->dev, "Can't create bus: %d\n", ret);
 		return ret;
@@ -1028,6 +1164,27 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 
 	vmd_bus_enumeration(vmd->bus[VMD_BUS_0], features);
 
+	if (vmd->bus1_rootbus) {
+		ret = vmd_create_bus(vmd, VMD_BUS_1, sd, offset);
+		if (ret) {
+			pci_err(vmd->dev, "Can't create BUS1: %d\n", ret);
+			return ret;
+		}
+
+		/*
+		 * Primary bus is not set by pci_create_root_bus(), it is
+		 * updated here
+		 */
+		vmd->bus[VMD_BUS_1]->primary = VMD_PRIMARY_BUS1;
+
+		WARN(sysfs_create_link(&vmd->dev->dev.kobj,
+				       &vmd->bus[VMD_BUS_1]->dev.kobj,
+				       "domain1"),
+		     "Can't create symlink to domain1\n");
+
+		vmd_bus_enumeration(vmd->bus[VMD_BUS_1], features);
+	}
+
 	return 0;
 }
 
@@ -1126,10 +1283,18 @@ static void vmd_remove(struct pci_dev *dev)
 	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
 	pci_remove_root_bus(vmd->bus[VMD_BUS_0]);
 
-	/* CFGBAR is static, does not require releasing memory */
+	/* CFGBARs are static, do not require releasing memory */
 	kfree(vmd->resources[VMD_RES_MBAR_1].name);
 	kfree(vmd->resources[VMD_RES_MBAR_2].name);
 
+	if (vmd->bus1_rootbus) {
+		pci_stop_root_bus(vmd->bus[VMD_BUS_1]);
+		sysfs_remove_link(&vmd->dev->dev.kobj, "domain1");
+		pci_remove_root_bus(vmd->bus[VMD_BUS_1]);
+		kfree(vmd->resources[VMD_RES_BUS1_MBAR_1].name);
+		kfree(vmd->resources[VMD_RES_BUS1_MBAR_2].name);
+	}
+
 	vmd_cleanup_srcu(vmd);
 	vmd_detach_resources(vmd);
 	vmd_remove_irq_domain(vmd);
@@ -1222,4 +1387,4 @@ module_pci_driver(vmd_drv);
 MODULE_AUTHOR("Intel Corporation");
 MODULE_DESCRIPTION("Volume Management Device driver");
 MODULE_LICENSE("GPL v2");
-MODULE_VERSION("0.6");
+MODULE_VERSION("0.7");
-- 
2.43.0

Re: [PATCH v5 7/8] PCI: vmd: Add support for second rootbus under VMD

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Flawed arithmetic when configuring MEMBAR offsets guarantees overlapping or gapped PCI resource windows between BUS0 and BUS1.
- [High] Failure to clean up VMD_BUS_0 on VMD_BUS_1 creation failure leads to a severe Use-After-Free.
- [High] Iterating over an unvalidated sum of disjoint bus ranges in `vmd_domain_reset` causes missed bus resets and potential out-of-bounds MMIO accesses.
- [Medium] VMD_RES_BUS1_CFGBAR is assigned an IORESOURCE_MEM resource as its parent, corrupting the resource tree.
- [Medium] An uninitialized stack variable is assigned to resource bounds if PCI config space reads fail.
--

commit 46d85add5d9d3ed310b9c5a3c11026a4231b01a9
Author: Szymon Durawa <szymon.durawa@linux.intel.com>
Subject: PCI: vmd: Add support for second rootbus under VMD

Starting from Intel Arrow Lake VMD enhancement introduces second root bus
support with fixed root bus number (0x80). It means that all 3 MMIO BARs
exposed by VMD are shared now between both buses (current BUS0 and
new BUS1).

Add new BUS1 enumeration and divide MMIO space to be shared between
both root buses.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 2bfc16875d912..c68ea7837620d 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c

[ ... ]

> @@ -537,11 +568,23 @@ static inline void vmd_acpi_end(void) { }
>  
>  static void vmd_domain_reset(struct vmd_dev *vmd)
>  {
> -	u16 bus, max_buses = resource_size(&vmd->resources[VMD_RES_CFGBAR]);
> +	u16 bus, bus_cnt = resource_size(&vmd->resources[VMD_RES_CFGBAR]);
>  	u8 dev, functions, fn, hdr_type;
>  	char __iomem *base;
>  
> -	for (bus = 0; bus < max_buses; bus++) {
> +	/*
> +	 * Total bus-number count (bus_cnt) when BUS1 root bus is present:
> +	 * initialized with BUS0 CFGBAR range, extended by BUS1 CFGBAR range plus
> +	 * one bus number per root bus' own config space (bus_cnt is increased by 2)
> +	 * as the CFGBAR ranges do not include the root bus config space.
> +	 */
> +
> +	if (vmd->bus1_rootbus) {
> +		bus_cnt += resource_size(&vmd->resources[VMD_RES_BUS1_CFGBAR]);
> +		bus_cnt += 2;
> +	}
> +
> +	for (bus = 0; bus < bus_cnt; bus++) {
>  		for (dev = 0; dev < 32; dev++) {
>  			base = vmd->cfgbar + PCIE_ECAM_OFFSET(bus,
>  						PCI_DEVFN(dev, 0), 0);

[Severity: High]
The bus_cnt variable combines the sizes of both the BUS0 and BUS1 CFGBAR
ranges. If the ranges for BUS0 and BUS1 are sparse, does this continuous
loop up to the sum of their sizes end up missing the higher bus numbers?

Could this also access out-of-bounds MMIO offsets if the sum exceeds the
mapped region for ECAM?

[ ... ]

> @@ -799,6 +867,30 @@ static void vmd_configure_cfgbar(struct vmd_dev *vmd)
>  		       (resource_size(res) >> 20) - 1,
>  		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
>  	};
> +
> +	if (vmd->bus1_rootbus) {
> +		u16 bus0_range = 0;
> +		u16 bus1_range = 0;
> +
> +		pci_read_config_word(vmd->dev, VMD_BUSRANGE0, &bus0_range);
> +		pci_read_config_word(vmd->dev, VMD_BUSRANGE1, &bus1_range);
> +
> +		/*
> +		 * Resize BUS0 CFGBAR range to make space for BUS1
> +		 * owned devices by adjusting range end with value stored in
> +		 * VMD_BUSRANGE0 register.
> +		 */
> +		vmd->resources[VMD_RES_CFGBAR].start = VMD_BUS_START(bus0_range);
> +		vmd->resources[VMD_RES_CFGBAR].end = VMD_BUS_END(bus0_range);
> +
> +		vmd->resources[VMD_RES_BUS1_CFGBAR] = (struct resource){
> +			.name = "VMD CFGBAR BUS1",
> +			.start = VMD_BUS_START(bus1_range),
> +			.end = VMD_BUS_END(bus1_range),
> +			.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
> +			.parent = res,
> +		};
> +	}
>  }

[Severity: Medium]
The res pointer here refers to VMD_CFGBAR, which is an IORESOURCE_MEM
resource. Is it safe to assign a memory resource as the parent to
VMD_RES_BUS1_CFGBAR, which is an IORESOURCE_BUS resource?

Will this cross-namespace parenting confuse the generic PCI resource
management?

[ ... ]

> @@ -840,41 +932,83 @@ static void vmd_configure_membar(struct vmd_dev *vmd,
>  static void vmd_configure_membar1_membar2(struct vmd_dev *vmd,
>  					  resource_size_t mbar2_ofs)
>  {
> -	vmd_configure_membar(vmd, 1, VMD_MEMBAR1, 0, 0);
> -	vmd_configure_membar(vmd, 2, VMD_MEMBAR2, mbar2_ofs, 0);
> +	if (vmd->bus1_rootbus) {
> +		u32 bus1_mbar1_ofs = 0;
> +		u64 bus1_mbar2_ofs = 0;
> +		u32 reg;
> +
> +		pci_read_config_dword(vmd->dev, VMD_MEMBAR1_OFFSET,
> +				      &bus1_mbar1_ofs);
> +
> +		pci_read_config_dword(vmd->dev, VMD_MEMBAR2_OFFSET1, &reg);
> +		bus1_mbar2_ofs = reg;

[Severity: Medium]
If pci_read_config_dword() fails due to a bus error or unsupported
operation, it leaves reg untouched. Could this result in uninitialized
stack memory being used to configure the physical MMIO allocation for
BUS1?

> +
> +		pci_read_config_dword(vmd->dev, VMD_MEMBAR2_OFFSET2, &reg);
> +		bus1_mbar2_ofs |= (u64)reg << 32;
> +
> +		/*
> +		 * Resize BUS MEMBAR1 and MEMBAR2 ranges to make space
> +		 * for BUS1 owned devices by adjusting range end with values
> +		 * stored in VMD_MEMBAR1_OFFSET and VMD_MEMBAR2_OFFSET registers
> +		 */
> +		vmd_configure_membar(vmd, VMD_RES_MBAR_1, VMD_MEMBAR1, 0,
> +				     bus1_mbar1_ofs);
> +		vmd_configure_membar(vmd, VMD_RES_MBAR_2, VMD_MEMBAR2,
> +				     mbar2_ofs, bus1_mbar2_ofs - mbar2_ofs);
> +
> +		vmd_configure_membar(vmd, VMD_RES_BUS1_MBAR_1, VMD_MEMBAR1,
> +				     bus1_mbar1_ofs, 0);
> +		vmd_configure_membar(vmd, VMD_RES_BUS1_MBAR_2, VMD_MEMBAR2,
> +				     mbar2_ofs + bus1_mbar2_ofs, 0);

[Severity: Critical]
The vmd_configure_membar() function subtracts the last argument from the
end of the resource. By passing bus1_mbar1_ofs as both the start offset
for BUS1 and the amount to subtract from the end of BUS0, won't the
resources overlap?

For example, if the offset is smaller than half the total BAR size, does
this cause identical physical MMIO ranges to be assigned to distinct
devices on BUS0 and BUS1?

[ ... ]

> @@ -1028,6 +1164,27 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  
>  	vmd_bus_enumeration(vmd->bus[VMD_BUS_0], features);
>  
> +	if (vmd->bus1_rootbus) {
> +		ret = vmd_create_bus(vmd, VMD_BUS_1, sd, offset);
> +		if (ret) {
> +			pci_err(vmd->dev, "Can't create BUS1: %d\n", ret);
> +			return ret;
> +		}

[Severity: High]
If vmd_create_bus() fails for VMD_BUS_1, this path returns early. However,
VMD_BUS_0 has already been fully created and enumerated earlier in this
function.

Since vmd_remove() is not called on the probe error path, does this leave
VMD_BUS_0 active while the struct vmd_dev context is subsequently freed by
devres, resulting in a use-after-free?

> +
> +		/*
> +		 * Primary bus is not set by pci_create_root_bus(), it is
> +		 * updated here
> +		 */
> +		vmd->bus[VMD_BUS_1]->primary = VMD_PRIMARY_BUS1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625082940.1290740-1-szymon.durawa@linux.intel.com?part=7

[PATCH v5 8/8] PCI: vmd: Add workaround for bus number hardwired to fixed non-zero value

[Szymon Durawa]

The VMD BUS1 root bus number is fixed in hardware to 0x80. It marks bridge
invalid configuration when detecting a non-zero (0x80) the VMD BUS1 root
bus number.
Thus root bus number is deconfigured in the first pass of pci_scan_bridge()
to be re-assigned to 0x0 in the second pass. As a result no subordinate bus
number behind VMD BUS1 is found.

To avoid bus number reconfiguration, BUS1 number has to be the same
as BUS1 primary number.

Log snippet without workaround:

[    3.752507] vmd 0000:00:0e.0: PCI host bridge to bus 10000:e1
[    3.752510] pci_bus 10000:e1: busn_res: can not insert [bus e1-ff] under domain [bus 00-ff] (conflicts with (null) [bus e0-f0])
[    3.752515] pci_bus 10000:e1: root bus resource [bus f1-ff]
[    3.752517] pci_bus 10000:e1: root bus resource [mem 0x8c800000-0x8cffffff]
[    3.752519] pci_bus 10000:e1: root bus resource [mem 0x701b802000-0x701bffffff 64bit]
[    3.752523] pci_bus 10000:e1: scanning bus
[    3.752732] pci (null): Looking for ACPI companion (address 0x80e0ffff)
[    3.752745] pci 10000:e1:1c.0: [8086:7f38] type 01 class 0x060400 PCIe Root Port
[    3.752779] pci 10000:e1:1c.0: PCI bridge to [bus f1]
[    3.752861] pci 10000:e1:1c.0: PME# supported from D0 D3hot D3cold
[    3.752864] pci 10000:e1:1c.0: PME# disabled
[    3.752909] pci 10000:e1:1c.0: PTM enabled (root), 4ns granularity
[    3.752981] pci 10000:e1:1c.0: vgaarb: pci_notify
[    3.752987] pci_bus 10000:e1: fixups for bus
[    3.752992] pci 10000:e1:1c.0: scanning [bus f1-f1] behind bridge, pass 0
[    3.752993] pci 10000:e1:1c.0: primary 80, bus->number e1.
[    3.752994] pci 10000:e1:1c.0: bridge configuration invalid ([bus f1-f1]), reconfiguring
[    3.753003] pci 10000:e1:1c.0: scanning [bus 00-00] behind bridge, pass 1
[    3.753004] pci 10000:e1:1c.0: primary 00, bus->number e1.

Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com>
Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>
---
 drivers/pci/controller/vmd.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index c68ea7837620..d0527be78ebd 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -440,8 +440,23 @@ static void vmd_remove_irq_domain(struct vmd_dev *vmd)
 static void __iomem *vmd_cfg_addr(struct vmd_dev *vmd, struct pci_bus *bus,
 				  unsigned int devfn, int reg, int len)
 {
-	unsigned int busnr_ecam = bus->number - vmd->busn_start[VMD_BUS_0];
-	u32 offset = PCIE_ECAM_OFFSET(busnr_ecam, devfn, reg);
+	unsigned char bus_number;
+	unsigned int busnr_ecam;
+	u32 offset;
+
+	/*
+	 * VMD workaround: for VMD BUS1, bus->number is set to VMD_PRIMARY_BUS1
+	 * (see comment in vmd_enable_domain() under vmd_create_bus() for BUS1)
+	 * but original value is 0xE1 which is stored in
+	 * vmd->busn_start[VMD_BUS_1].
+	 */
+	if (vmd->bus1_rootbus && bus->number == VMD_PRIMARY_BUS1)
+		bus_number = vmd->busn_start[VMD_BUS_1];
+	else
+		bus_number = bus->number;
+
+	busnr_ecam = bus_number - vmd->busn_start[VMD_BUS_0];
+	offset = PCIE_ECAM_OFFSET(busnr_ecam, devfn, reg);
 
 	if (offset + len >= resource_size(&vmd->dev->resource[VMD_CFGBAR]))
 		return NULL;
@@ -1177,6 +1192,19 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 		 */
 		vmd->bus[VMD_BUS_1]->primary = VMD_PRIMARY_BUS1;
 
+		/*
+		 * This is a workaround for pci_scan_bridge_extend() code.
+		 * It marks bridge invalid configuration when detecting a
+		 * non-zero (0x80) the VMD BUS1 root bus number. Thus Primary Bus Number
+		 * of Root Ports on BUS1 is deconfigured in the first pass of
+		 * pci_scan_bridge() to be re-assigned to 0x0 in the second pass.
+		 * As a result no subordinate bus number behind VMD BUS1 is found.
+		 * Workaround: update vmd->bus[VMD_BUS_1]->number and
+		 * vmd->bus[VMD_BUS_1]->primary to the same value, which
+		 * bypasses bus number reconfiguration.
+		 */
+		vmd->bus[VMD_BUS_1]->number = VMD_PRIMARY_BUS1;
+
 		WARN(sysfs_create_link(&vmd->dev->dev.kobj,
 				       &vmd->bus[VMD_BUS_1]->dev.kobj,
 				       "domain1"),
-- 
2.43.0

Re: [PATCH v5 8/8] PCI: vmd: Add workaround for bus number hardwired to fixed non-zero value

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Modifying `bus->number` dynamically after bus registration breaks PCI device naming conventions and sysfs topology consistency.

Pre-existing issues:
- [High] Missing cleanup of VMD_BUS_0 when VMD_BUS_1 creation fails, leading to a PCI root bus leak and a potential Use-After-Free.
--

commit daf784b1aba7feceb67edffc6d0d8a59f8bc2fb9
Author: Szymon Durawa <szymon.durawa@linux.intel.com>

PCI: vmd: Add workaround for bus number hardwired to fixed non-zero value

The VMD BUS1 root bus number is fixed in hardware to 0x80. This patch adds
a workaround to bypass bus number reconfiguration by updating the BUS1
number to be the same as the BUS1 primary number.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index c68ea7837620d..d0527be78ebd5 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -440,8 +440,23 @@ static void vmd_remove_irq_domain(struct vmd_dev *vmd)
>  static void __iomem *vmd_cfg_addr(struct vmd_dev *vmd, struct pci_bus *bus,
>  				  unsigned int devfn, int reg, int len)
>  {
> -	unsigned int busnr_ecam = bus->number - vmd->busn_start[VMD_BUS_0];
> -	u32 offset = PCIE_ECAM_OFFSET(busnr_ecam, devfn, reg);
> +	unsigned char bus_number;
> +	unsigned int busnr_ecam;
> +	u32 offset;
> +
> +	/*
> +	 * VMD workaround: for VMD BUS1, bus->number is set to VMD_PRIMARY_BUS1
> +	 * (see comment in vmd_enable_domain() under vmd_create_bus() for BUS1)
> +	 * but original value is 0xE1 which is stored in
> +	 * vmd->busn_start[VMD_BUS_1].
> +	 */
> +	if (vmd->bus1_rootbus && bus->number == VMD_PRIMARY_BUS1)
> +		bus_number = vmd->busn_start[VMD_BUS_1];

[Severity: High]
Does this code undo the bus number mutation performed in vmd_enable_domain()
for config space accesses?

While this ensures config space accesses still work, it highlights the
mismatch that leaves the sysfs topology structurally inconsistent for
userspace.

> +	else
> +		bus_number = bus->number;
> +
> +	busnr_ecam = bus_number - vmd->busn_start[VMD_BUS_0];
> +	offset = PCIE_ECAM_OFFSET(busnr_ecam, devfn, reg);
>  
>  	if (offset + len >= resource_size(&vmd->dev->resource[VMD_CFGBAR]))
>  		return NULL;

[ ... ]

> @@ -1177,6 +1192,19 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  		 */
>  		vmd->bus[VMD_BUS_1]->primary = VMD_PRIMARY_BUS1;

[Severity: High]
This is a pre-existing issue, but does the error handling just above this
code leak VMD_BUS_0?

If vmd_create_bus() for VMD_BUS_1 fails:

vmd_enable_domain() {
    ...
    ret = vmd_create_bus(vmd, VMD_BUS_0, sd, offset);
    if (ret)
        return ret; // Succeeds
    ...
    if (vmd->bus1_rootbus) {
        ret = vmd_create_bus(vmd, VMD_BUS_1, sd, offset);
        if (ret) {
            pci_err(vmd->dev, "Can't create BUS1: %d\n", ret);
            return ret; // Leaks VMD_BUS_0
        }
    ...
}

The error path returns directly without calling pci_stop_root_bus() and
pci_remove_root_bus() for VMD_BUS_0.

Could this lead to a use-after-free if the PCI core retains a reference to
the leaked VMD_BUS_0 after the vmd device allocation is freed?

> +		/*
> +		 * This is a workaround for pci_scan_bridge_extend() code.
> +		 * It marks bridge invalid configuration when detecting a
> +		 * non-zero (0x80) the VMD BUS1 root bus number. Thus Primary Bus Number
> +		 * of Root Ports on BUS1 is deconfigured in the first pass of
> +		 * pci_scan_bridge() to be re-assigned to 0x0 in the second pass.
> +		 * As a result no subordinate bus number behind VMD BUS1 is found.
> +		 * Workaround: update vmd->bus[VMD_BUS_1]->number and
> +		 * vmd->bus[VMD_BUS_1]->primary to the same value, which
> +		 * bypasses bus number reconfiguration.
> +		 */
> +		vmd->bus[VMD_BUS_1]->number = VMD_PRIMARY_BUS1;

[Severity: High]
Does modifying the bus number dynamically after bus registration break PCI
device naming conventions and sysfs topology consistency?

Because the bus number is forcibly mutated after the VMD_BUS_1 root bus
device has already been registered with the PCI subsystem, child devices
downstream will be enumerated and registered using names derived from
0x80 (e.g., 0000:80:1c.0).

Meanwhile, their parent bus directory in sysfs remains named using the
original bus number (e.g., 0000:e1), which can break userspace tools
expecting standard PCI naming.

> +
>  		WARN(sysfs_create_link(&vmd->dev->dev.kobj,
>  				       &vmd->bus[VMD_BUS_1]->dev.kobj,
>  				       "domain1"),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625082940.1290740-1-szymon.durawa@linux.intel.com?part=8