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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dynamic bus assignment (e.g., for hotplugged switches or unconfigured bridges) behind VMD BUS1 is broken due to incomplete logical-to-physical bus number translation, leading to configuration space access failures and broken ACPI matching.
- [High] Double free of IRQ domain and emulated domain number on VMD_BUS_0 creation failure.
--

--- Patch [8]: [PATCH v6 8/8] PCI: vmd: Add workaround for bus number hardwired to fixed non-zero value ---
commit 1dce6e841a4c83aa7c67d1d1cd42002341425bb7
Author: Szymon Durawa <szymon.durawa@linux.intel.com>

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

This commit implements a workaround for the VMD BUS1 root bus number
being fixed to 0x80 in hardware. It deconfigures the root bus number in
the first pass of pci_scan_bridge() and translates configuration accesses
and ACPI matching to work around the fixed root bus number.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 7c4379d565eaf..8da2e6727282b 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -440,8 +440,22 @@ 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;
> +
> +	/*
> +	 * BUS1 is registered with logical bus number 0x80 to avoid bridge
> +	 * reconfiguration, but cfg accesses must target the original BUS1
> +	 * restricted range start.
> +	 */
> +	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);

[Severity: High]
Does this logic correctly handle subordinate buses dynamically assigned behind
BUS1?

Since vmd_cfg_addr() only remaps the exact bus number 0x80 to its physical
restricted range (e.g., 0xE1), subordinate buses like 0x81 would fall through
to the else branch.

If bus->number is 0x81, subtracting vmd->busn_start[VMD_BUS_0] (which is
0xE0) would underflow the unsigned busnr_ecam integer. This results in a
massive ECAM offset that silently fails the VMD_CFGBAR bounds check below,
returning all FFs and breaking configuration space access for dynamically
assigned buses.

[ ... ]

> @@ -520,18 +534,37 @@ static struct pci_ops vmd_ops = {
>  static struct acpi_device *vmd_acpi_find_companion(struct pci_dev *pci_dev)
>  {
>  	struct pci_host_bridge *bridge;
> -	u32 busnr, addr;
> +	struct vmd_dev *vmd;
> +	u32 addr;
> +	int busnr;
> +	u8 pci_bus_number;
> +	u8 bridge_bus_number;
>  
>  	if (pci_dev->bus->ops != &vmd_ops)
>  		return NULL;
>  
> +	vmd = vmd_from_bus(pci_dev->bus);
>  	bridge = pci_find_host_bridge(pci_dev->bus);
> -	busnr = pci_dev->bus->number - bridge->bus->number;
> +	pci_bus_number = pci_dev->bus->number;
> +	bridge_bus_number = bridge->bus->number;
> +
> +	/*
> +	 * BUS1 is registered with logical root number 0x80. For ACPI companion
> +	 * matching, compute the relative bus number against the original BUS1
> +	 * restricted range base.
> +	 */
> +	if (vmd->bus1_rootbus && bridge->bus == vmd->bus[VMD_BUS_1]) {
> +		bridge_bus_number = vmd->busn_start[VMD_BUS_1];
> +		if (pci_bus_number == VMD_PRIMARY_BUS1)
> +			pci_bus_number = vmd->busn_start[VMD_BUS_1];
> +	}
> +
> +	busnr = pci_bus_number - bridge_bus_number;
>  	/*
>  	 * The address computation below is only applicable to relative bus
>  	 * numbers below 32.
>  	 */
> -	if (busnr > 31)
> +	if (busnr < 0 || busnr > 31)
>  		return NULL;

[Severity: High]
Can this relative bus depth calculation produce negative values for OS-assigned
subordinate buses?

For a subordinate bus like 0x81, pci_bus_number remains 0x81, while
bridge_bus_number is remapped to the physical base (e.g., 0xE1). This results
in busnr being 0x81 - 0xE1 = -0x60, which fails the new busnr < 0 check and
breaks ACPI companion matching.

[ ... ]

> @@ -1045,6 +1078,7 @@ static int vmd_create_bus(struct vmd_dev *vmd, enum vmd_rootbus bus_number,
>  			  struct pci_sysdata *sd, resource_size_t *offset,
>  			  u8 primary)
>  {
> +	u8 root_busnr;
>  	u8 cfgbar = bus_number * 3;
>  	u8 membar1 = cfgbar + 1;
>  	u8 membar2 = cfgbar + 2;
> @@ -1057,8 +1091,27 @@ static int vmd_create_bus(struct vmd_dev *vmd, enum vmd_rootbus bus_number,
>  	pci_add_resource_offset(&resources, &vmd->resources[membar2],
>  				offset[1]);
>  
> -	vmd_bus = pci_create_root_bus(&vmd->dev->dev,
> -				      vmd->busn_start[bus_number], &vmd_ops, sd,
> +	/*
> +	 * Register BUS1 with its logical root number (0x80) up front so PCI core
> +	 * bridge scanning does not see a post-registration bus-number mutation.
> +	 *
> +	 * 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: VMD_BUS_1 bus number shall be set to VMD_PRIMARY_BUS1 so it has
> +	 * the same value as vmd->bus[VMD_BUS_1]->primary, it will bypass bus number
> +	 * reconfiguration.
> +	 */
> +
> +	if (bus_number == VMD_BUS_1 && vmd->bus1_rootbus)
> +		root_busnr = VMD_PRIMARY_BUS1;
> +	else
> +		root_busnr = vmd->busn_start[bus_number];
> +
> +	vmd_bus = pci_create_root_bus(&vmd->dev->dev, root_busnr, &vmd_ops, sd,
>  				      &resources);
>  
>  	if (!vmd_bus) {

[Severity: High]
Is there a double free of the IRQ domain and emulated domain number when
VMD_BUS_0 creation fails?

Inside vmd_create_bus(), the error path for !vmd_bus explicitly cleans up:

    if (bus_number == VMD_BUS_0) {
        pci_bus_release_emul_domain_nr(sd->domain);
        vmd_remove_irq_domain(vmd);
    }
    return -ENODEV;

However, the caller vmd_enable_domain() intercepts this error return and
mistakenly executes the exact same cleanup operations:

    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;
    }

Because vmd_remove_irq_domain() does not set vmd->irq_domain to NULL, this
appears to trigger an immediate double free of the fwnode and irq domain
structure. The IDA structure also gets freed twice.

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