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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pci: vmd: root ports skipped during domain reset due to CFGBAR start offset resize
- [High] pci: vmd: unprotected modification of global resource tree pointers and structures
--

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

This commit introduces support for a second root bus under VMD, which is
needed starting from Intel Arrow Lake. It enumerates BUS1, divides the
MMIO space to be shared between both root buses, and resizes the CFGBAR
accordingly.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index c6b5e693916cb..51b8c2d9b7f0b 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c

[ ... ]

> @@ -632,12 +664,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 manually assigning the .sibling pointers here introduce a data race?

These assignments link the BUS1 resources directly into the global iomem
resource tree without acquiring the resource_lock.

On the vmd_create_bus() error path later in vmd_enable_domain(), these
sibling pointers are cleared and the resource structures are zeroed, also
without locking:

drivers/pci/controller/vmd.c:vmd_enable_domain() {
	...
	vmd->resources[VMD_RES_MBAR_1].sibling = NULL;
	vmd->resources[VMD_RES_MBAR_2].sibling = NULL;
	vmd->resources[VMD_RES_BUS1_CFGBAR] = (struct resource){};
	...
}

Can concurrent readers (like /proc/iomem iterators traversing the tree)
encounter corrupted or zeroed structures, leading to a crash or invalid reads?

>  }

[ ... ]

> @@ -936,6 +997,62 @@ static void vmd_configure_cfgbar(struct vmd_dev *vmd)
>  		.end   = busn_end,
>  		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
>  	};
> +
> +	if (vmd->bus1_rootbus) {

[ ... ]

> +		/*
> +		 * 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 = bus0_start;
> +		vmd->resources[VMD_RES_CFGBAR].end = bus0_end;

[Severity: High]
Will this resize of the CFGBAR start address cause vmd_domain_reset() to
skip the root buses?

When vmd_configure_cfgbar() resizes vmd->resources[VMD_RES_CFGBAR].start to
the secondary bus start (e.g., 0xE2), vmd_domain_reset() will later read
this modified start address:

drivers/pci/controller/vmd.c:vmd_domain_reset() {
	...
	for (bus = 0; bus < max_buses; bus++) {
		ecam_bus = vmd_bus_to_ecam(vmd, vmd->resources[0].start + bus);
	...
}

Consequently, the ECAM loop skips the primary root buses (0xE0 and 0xE1).
Does this leave their bridge windows with potentially stale BIOS configurations
that conflict with OS enumeration?

> +
> +		vmd->resources[VMD_RES_BUS1_CFGBAR] = (struct resource){
> +			.name = "VMD CFGBAR BUS1",
> +			.start = bus1_start,
> +			.end = bus1_end,
> +			.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
> +		};
> +	}
> +
> +	return 0;
>  }

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