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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incorrect MEMBAR2 resizing logic causes overlapping memory windows for BUS0 and truncated memory windows for BUS1.
- [High] Double free of the PCI emulated domain IDA and redundant removal of the IRQ domain on the VMD_BUS_0 creation error path.
- [High] Lockless modification of global resource tree pointers can race with concurrent readers.
- [High] Incorrect busn_start configuration for restriction type 3 assigns wrong root bus number to BUS0.

Pre-existing issues:
- [High] Missing pci_free_irq_vectors() in error paths and during device removal causes a permanent IRQ vector leak.
--

--- Patch [7]: [PATCH v6 7/8] PCI: vmd: Add support for second rootbus under VMD ---
commit 59d38ef8140ce34f05ff3140bfb153eb7ddbc2c5
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). 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 786374965f74a..7c4379d565eaf 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c

[ ... ]

> @@ -587,12 +619,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];
> +	}
>  }

[Severity: High]
Does modifying the sibling pointers here race with concurrent readers?

Directly modifying the sibling pointers of resources that are part of the
global iomem_resource tree without acquiring the resource_lock rwlock can
cause data races.

Concurrent readers, such as /proc/iomem, traversing the resource tree
while the sibling pointers are being modified here in vmd_attach_resources()
or cleared in the vmd_enable_domain() fallback error path could experience
invalid memory access.

[ ... ]

> @@ -668,6 +712,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;

[Severity: High]
Should the BUS0 start number be VMD_RESTRICT_0_BUS_START (0x00) here?

By assigning VMD_RESTRICT_2_BUS_START (0xE0) to BUS0 for restriction
type 3, vmd_create_bus() is instructed to create the BUS0 root bus
with bus number 0xE0.

However, for Arrow Lake, the BUS0 root bus is physically expected to be
at 0x00, and its CFGBAR resource range is correctly parsed and adjusted
starting at 0x00. Passing 0xE0 to pci_create_root_bus() conflicts with
the CFGBAR start address and can break device discovery on the primary
VMD bus.

[ ... ]

> @@ -850,49 +962,135 @@ static int vmd_configure_membar1_membar2(struct vmd_dev *vmd,
>  {
>  	int ret;
>  
> -	ret = vmd_configure_membar(vmd, VMD_RES_MBAR_1, VMD_MEMBAR1, 0, 0);
> -	if (ret)
> -		return ret;

[ ... ]

> +		/*
> +		 * 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
> +		 */
> +		ret = vmd_configure_membar(vmd, VMD_RES_MBAR_1, VMD_MEMBAR1, 0,
> +					   mbar1_sz - bus1_mbar1_ofs);
> +		if (ret)
> +			return ret;
> +
> +		ret = vmd_configure_membar(vmd, VMD_RES_MBAR_2, VMD_MEMBAR2,
> +					   mbar2_ofs, mbar2_sz - (bus1_mbar2_ofs + mbar2_ofs));
> +		if (ret)
> +			return ret;
> +
> +		ret = vmd_configure_membar(vmd, VMD_RES_BUS1_MBAR_1, VMD_MEMBAR1,
> +					   bus1_mbar1_ofs, 0);
> +		if (ret)
> +			return ret;
> +
> +		ret = vmd_configure_membar(vmd, VMD_RES_BUS1_MBAR_2, VMD_MEMBAR2,
> +					   mbar2_ofs + bus1_mbar2_ofs, 0);

[Severity: High]
Does this logic create overlapping memory windows for BUS0 and truncated
memory windows for BUS1?

The hardware splits MEMBAR2 based on bus1_mbar2_ofs, which is the
absolute offset from the BAR's base address. However, when configuring
vmd_configure_membar1_membar2(), the software-reserved shadow size
(mbar2_ofs) is added to the split point.

This extends the BUS0 region up to bus1_mbar2_ofs + mbar2_ofs - 1, and
sets the BUS1 start offset to mbar2_ofs + bus1_mbar2_ofs. Since hardware
routes based solely on bus1_mbar2_ofs, the OS will assign an overlapping
address range to BUS0 devices that hardware will incorrectly route to BUS1,
potentially breaking MMIO in the overlap region.

[ ... ]

> @@ -1036,7 +1244,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, VMD_PRIMARY_BUS0);
>  	if (ret) {
>  		pci_err(vmd->dev, "Can't create bus: %d\n", ret);
>  		pci_bus_release_emul_domain_nr(sd->domain);
>  		vmd_remove_irq_domain(vmd);
>  		return ret;
>  	}

[Severity: High]
Will this result in a double free of the emulated domain IDA?

When pci_create_root_bus() fails for VMD_BUS_0 inside vmd_create_bus(),
the error path within that function now cleans up by calling
pci_bus_release_emul_domain_nr(sd->domain) and vmd_remove_irq_domain(vmd).

Since vmd_enable_domain() also checks the return code and repeats both
calls here, pci_bus_release_emul_domain_nr() is called twice on the
same domain index. This will trigger a kernel WARNING and corrupt the
IDA state structure.

[ ... ]

> @@ -1147,6 +1381,13 @@ static void vmd_remove(struct pci_dev *dev)
>  	pci_stop_root_bus(vmd->bus[VMD_BUS_0]);
>  	sysfs_remove_link(&vmd->dev->dev.kobj, "domain");
>  	pci_remove_root_bus(vmd->bus[VMD_BUS_0]);
> +
> +	if (vmd->bus1_rootbus && vmd->bus[VMD_BUS_1]) {
> +		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]);
> +	}
> +
>  	vmd_cleanup_srcu(vmd);
>  	vmd_detach_resources(vmd);
>  	vmd_remove_irq_domain(vmd);
>  }

[Severity: High]
This is a pre-existing issue, but do we leak PCI IRQ vectors during device
removal and on probe error paths?

According to PCI subsystem guidelines, every error path after a successful
pci_alloc_irq_vectors() must explicitly call pci_free_irq_vectors() before
returning.

The device removal function vmd_remove() lacks a call to
pci_free_irq_vectors(). Furthermore, early returns in vmd_alloc_irqs()
and probe error paths in vmd_enable_domain() omit this cleanup. Unbinding
and rebinding the driver or failing probe will permanently leak PCI IRQ
vectors.

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