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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] PCI: vmd: overlapping busn_res and unisolated CFGBAR in
  vmd_create_bus()
- [Medium] PCI: vmd: missing VMD_BUSRANGE bounds check in
  vmd_configure_cfgbar()
- [Medium] PCI: vmd: wrong relative bus number for BUS1 in
  vmd_acpi_find_companion()
- [Medium] PCI: vmd: leak of srcu_struct on error paths in
  vmd_enable_domain()
--

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

PCI: vmd: Add support for second rootbus under VMD

This commit adds support for a second VMD root bus (BUS1) exposed via Bus
Restriction type 3 on Intel Arrow Lake and later platforms, partitioning
CFGBAR, MEMBAR1, and MEMBAR2 across two emulated PCI domains.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 5733faf972d10..6831d0a5c443a 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c

[ ... ]

> @@ -989,6 +1133,62 @@ static void vmd_configure_cfgbar(struct vmd_dev *vmd)
>  		.end   = busn_end,
>  		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
>  	};
> +
> +	if (vmd->bus1_rootbus) {
> +		int ret;
> +		u16 bus0_range = 0;
> +		u16 bus1_range = 0;
> +		u8 bus0_start, bus0_end;
> +		u8 bus1_start, bus1_end;
> +
> +		ret = pci_read_config_word(vmd->dev, VMD_BUSRANGE0, &bus0_range);
> +		if (ret) {
> +			pci_err(vmd->dev, "Failed to read VMD_BUSRANGE0: %d\n", ret);
> +			return -EIO;
> +		}
> +
> +		ret = pci_read_config_word(vmd->dev, VMD_BUSRANGE1, &bus1_range);
> +		if (ret) {
> +			pci_err(vmd->dev, "Failed to read VMD_BUSRANGE1: %d\n", ret);
> +			return -EIO;
> +		}
> +
> +		bus0_start = VMD_BUS_START(bus0_range);
> +		bus0_end = VMD_BUS_END(bus0_range);
> +		bus1_start = VMD_BUS_START(bus1_range);
> +		bus1_end = VMD_BUS_END(bus1_range);
> +
> +		if (bus0_start > bus0_end || bus1_start > bus1_end) {
> +			pci_err(vmd->dev,
> +				"Invalid bus range(s): BUS0 [%02x-%02x], BUS1 [%02x-%02x]\n",
> +				bus0_start, bus0_end, bus1_start, bus1_end);
> +			return -EINVAL;
> +		}
> +
> +		if (!(bus0_end < bus1_start || bus1_end < bus0_start)) {
> +			pci_err(vmd->dev,
> +				"Overlapping bus ranges: BUS0 [%02x-%02x], BUS1 [%02x-%02x]\n",
> +				bus0_start, bus0_end, bus1_start, bus1_end);
> +			return -EINVAL;
> +		}
> +
> +		/*
> +		 * 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;
> +
> +		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;
>  }

[Severity: Medium]
Does vmd_configure_cfgbar() need to validate that bus0_start and bus1_start
are above vmd->busn_start[VMD_BUS_1] (0xe1) and that bus0_end and bus1_end
do not exceed busn_end?

If firmware leaves VMD_BUSRANGE0 or VMD_BUSRANGE1 at 0x0000 when one root
bus has no subordinate devices, or reports an end bus beyond the VMD_CFGBAR
aperture, the checks above still pass and overwrite the bus resources with
an inverted range such as [0xe0..0x00] or an out-of-bounds bus number.

[ ... ]

> @@ -1040,55 +1241,132 @@ static int vmd_configure_membar1_membar2(struct vmd_dev *vmd,
>  static int vmd_create_bus(struct vmd_dev *vmd, enum vmd_rootbus bus_number,
>  			  struct pci_sysdata *sd, resource_size_t *offset,
>  			  bool vmd_in_guest, u8 primary)
>  {
> +	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]) {
> +	vmd_bus = pci_create_root_bus(&vmd->dev->dev,
> +				      vmd->busn_start[bus_number], &vmd_ops, sd,
> +				      &resources);

[Severity: High]
Can VMD_BUS_0 and VMD_BUS_1 allocate and scan each other's ECAM bus numbers
when a root port bridge is unconfigured?

vmd_configure_cfgbar() sets vmd->resources[VMD_RES_CFGBAR] to
[bus0_start..bus0_end] (for example [0xe2..0xf0]) and
vmd->resources[VMD_RES_BUS1_CFGBAR] to [bus1_start..bus1_end] (for example
[0xf1..0xff]).  However, pci_create_root_bus() -> pci_register_host_bridge()
ignores res->start and inserts bus->busn_res starting at bus->number:

drivers/pci/probe.c:pci_register_host_bridge() {
    ...
	resource_list_for_each_entry(window, &bridge->windows) {
		res = window->res;
		if (resource_type(res) == IORESOURCE_BUS)
			pci_bus_insert_busn_res(bus, bus->number, res->end);
	}
    ...
}

As a result, VMD_BUS_0 gets busn_res [0xe0..0xf0] (which includes 0xe1, the
root bus of VMD_BUS_1) and VMD_BUS_1 gets busn_res [0xe1..0xff] (which
includes 0xe2..0xf0, the subordinate bus range of VMD_BUS_0), while
vmd_cfg_addr() maps bus->number directly into the shared vmd->cfgbar
without checking which domain owns that bus number.  When
pci_scan_child_bus_extend() encounters an unconfigured bridge on VMD_BUS_1
(or on VMD_BUS_0 before max passes 0xe0), Pass 1 assigns max + 1 (0xe2 on
VMD_BUS_1, or 0xe1 on VMD_BUS_0), scanning and reprogramming devices that
belong to the other VMD root bus.

[ ... ]

> @@ -1213,11 +1511,38 @@ 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, vmd_in_guest);
> +	if (vmd->bus1_rootbus) {
> +		vmd->sysdata[VMD_BUS_1].vmd_dev = vmd->dev;
> +		vmd->sysdata[VMD_BUS_1].node = pcibus_to_node(vmd->dev->bus);
> +		vmd->sysdata[VMD_BUS_1].domain = pci_bus_find_emul_domain_nr(0, 0x10000, INT_MAX);
> +		if (vmd->sysdata[VMD_BUS_1].domain < 0) {
> +			pci_warn(vmd->dev,
> +				 "Can't allocate BUS1 domain: %d, continuing with BUS0 only\n",
> +				 vmd->sysdata[VMD_BUS_1].domain);
> +			vmd_disable_bus1(vmd);
> +		}
> +	}
> +
> +	ret = vmd_attach_resources(vmd);
> +	if (ret) {
> +		pci_bus_release_emul_domain_nr(sd->domain);
> +		if (vmd->bus1_rootbus && vmd->sysdata[VMD_BUS_1].domain >= 0)
> +			pci_bus_release_emul_domain_nr(vmd->sysdata[VMD_BUS_1].domain);
> +
> +		vmd_remove_irq_domain(vmd);
> +		return ret;
> +	}
> +
> +	ret = vmd_create_bus(vmd, VMD_BUS_0, sd, offset, vmd_in_guest,
> +			     VMD_PRIMARY_BUS0);
>  	if (ret) {
>  		pci_err(vmd->dev, "Can't create bus: %d\n", ret);
>  		pci_bus_release_emul_domain_nr(sd->domain);
> +		if (vmd->bus1_rootbus && vmd->sysdata[VMD_BUS_1].domain >= 0)
> +			pci_bus_release_emul_domain_nr(vmd->sysdata[VMD_BUS_1].domain);
> +
>  		vmd_remove_irq_domain(vmd);
> +		vmd_detach_resources(vmd);
>  		return ret;
>  	}

[Severity: Medium]
Do these error paths leak the per-vector srcu_struct instances initialized
by vmd_alloc_irqs()?

When MSI remapping is enabled, vmd_alloc_irqs() calls init_srcu_struct() on
each vmd->irqs[i].srcu, which allocates per-CPU memory not managed by
devres.  If vmd_attach_resources() or vmd_create_bus() fails, returning
without calling vmd_cleanup_srcu(vmd) leaks that memory and leaves active
srcu_struct objects in devm-allocated vmd->irqs memory.

>  	WARN(sysfs_create_link(&vmd->dev->dev.kobj,
>  			       &vmd->bus[VMD_BUS_0]->dev.kobj, "domain"),
>  	     "Can't create symlink to domain\n");
>  
> +	if (vmd->bus1_rootbus && vmd->sysdata[VMD_BUS_1].domain >= 0) {
> +		ret = vmd_create_bus(vmd, VMD_BUS_1, &vmd->sysdata[VMD_BUS_1], offset,
> +				     vmd_in_guest, VMD_PRIMARY_BUS1);
> +		if (ret) {
> +			pci_warn(vmd->dev,
> +				 "Can't create BUS1: %d, continuing with BUS0 only\n",
> +				 ret);
> +
> +			release_resource(&vmd->resources[VMD_RES_BUS1_MBAR_2]);
> +			release_resource(&vmd->resources[VMD_RES_BUS1_MBAR_1]);
> +			pci_bus_release_emul_domain_nr(vmd->sysdata[VMD_BUS_1].domain);
> +			vmd_disable_bus1(vmd);
> +		}
> +
> +		if (vmd->bus1_rootbus && vmd->sysdata[VMD_BUS_1].domain >= 0) {
> +			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_0], features);
>  
> +	if (vmd->bus1_rootbus && vmd->sysdata[VMD_BUS_1].domain >= 0)
> +		vmd_bus_enumeration(vmd->bus[VMD_BUS_1], features);

[Severity: Medium]
Does vmd_acpi_find_companion() compute the wrong relative bus number when
enumerating VMD_BUS_1?

drivers/pci/controller/vmd.c:vmd_acpi_find_companion() {
    ...
	bridge = pci_find_host_bridge(pci_dev->bus);
	busnr = pci_dev->bus->number - bridge->bus->number;
    ...
	addr = (busnr << 24) | ((u32)pci_dev->devfn << 16) | 0x8000FFFFU;

	return acpi_find_child_device(ACPI_COMPANION(bridge->dev.parent), addr,
				      false);
}

Both host bridges share &vmd->dev->dev as bridge->dev.parent, where the
relative bus number in the ACPI _ADR is the offset from VMD_BUS_0's start
bus (0xe0).  Because bridge->bus->number for VMD_BUS_1 is 0xe1, subtracting
bridge->bus->number yields busnr == 0 for root ports on 0xe1 (matching
VMD_BUS_0's ACPI companion objects) and shifts all VMD_BUS_1 subordinate bus
_ADR lookups down by one.

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