Re: [PATCH v4] PCI: vmd: Add feature to add BIOS enumerated devices.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] VMD MEMBAR2 payload resource overlaps with internal hardware shadow registers, exposing them to child devices.
- [High] MSI-X remapping is incorrectly enabled on hardware that no longer supports it when BIOS physical offsets are present.
- [High] Missing `pci_free_irq_vectors()` in error paths causes IRQ capacity leaks.
- [High] Calculated end bus number for the VMD root bus can exceed the absolute PCI limit (255), corrupting bus enumeration.
- [Low] Hardcoding device ID in core routing logic instead of utilizing existing feature flags.
--

commit 68d8397e4bc530fad180d8f900d3ca84607b00a1
Author: Nirmal Patel <nirmal.patel@linux.intel.com>

PCI: vmd: Add feature to add BIOS enumerated devices.

This commit adds support for newer VMD hardware by relying on the BIOS to
enumerate the device tree and assign configurations. It reads the bus hide
range and shadow register values directly from VMD BAR4/BAR5.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index d4ae250d4bc6d..6a5879d099a16 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -393,7 +407,12 @@ 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;
> -	u32 offset = PCIE_ECAM_OFFSET(busnr_ecam, devfn, reg);
> +	u32 offset;
> +
> +	if (vmd->dev->device == PCI_DEVICE_ID_INTEL_VMD_28C1)
> +		busnr_ecam = bus->number;
> +
> +	offset = PCIE_ECAM_OFFSET(busnr_ecam, devfn, reg);

This isn't a bug, but does it make sense to use the explicit device ID check
here instead of using the existing vmd_features bitmask? Adding a feature
flag might scale better for future hardware iterations.

[ ... ]
> @@ -711,6 +770,54 @@ static int vmd_alloc_irqs(struct vmd_dev *vmd)
[ ... ]
> +static int vmd_prepare_offsets_and_bus(struct vmd_dev *vmd,
> +					unsigned long features,
> +					resource_size_t *membar2_offset,
> +					resource_size_t *offset1,
> +					resource_size_t *offset2)
> +{
> +	int ret;
> +
> +	/*
> +	 * Shadow registers may exist in certain VMD device ids which allow
> +	 * guests to correctly assign host physical addresses to the root ports
> +	 * and child devices. These registers will either return the host value
> +	 * or 0, depending on an enable bit in the VMD device.
> +	 */
> +	/*
> +	 * For certain VMD devices (i.e. 0x28C1), BIOS places device info
> +	 * in BAR4 shadow registers to determine the base bus number and memory
> +	 * offsets.
> +	 */
> +	if (features & VMD_FEAT_USE_BIOS_INFO) {
> +		if (resource_type(&vmd->dev->resource[4]) == IORESOURCE_MEM) {
> +			ret = vmd_get_bus_info_from_bar4(vmd, offset1, offset2);
> +			if (ret)
> +				return ret;
> +		}
> +	} else if (features & VMD_FEAT_HAS_MEMBAR_SHADOW) {
> +		*membar2_offset = MB2_SHADOW_OFFSET + MB2_SHADOW_SIZE;

When VMD_FEAT_USE_BIOS_INFO evaluates to true, the else if block is skipped
and membar2_offset remains at its default 0x2000. Could this allow the guest
payload resource to overlap with the new 28C1 hardware shadow registers
located at 0x2818 to 0x2840, exposing them to child devices?

[ ... ]
> @@ -784,32 +891,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
[ ... ]
> +	ret = vmd_prepare_offsets_and_bus(vmd, features, &membar2_offset,
> +					  &offset[0], &offset[1]);
> +	if(ret)
> +		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,

Since the 28C1 CFGBAR size is 256 MB, resource_size(res) >> 20 evaluates to
256. If the BIOS assigns a non-zero vmd->busn_start (e.g., 128), does this
cause the calculated .end to exceed the maximum 8-bit PCI bus limit of 255
and corrupt bus enumeration?

[ ... ]
> @@ -880,7 +965,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  		if (ret)
>  			return ret;
>  	} else {
> -		vmd_set_msi_remapping(vmd, false);
> +		if (!(features & VMD_FEAT_USE_BIOS_INFO))
> +			vmd_set_msi_remapping(vmd, false);
>  	}

The commit message mentions that MSI-X remapping enable/disable is no longer
supported on this hardware. If the BIOS populates non-zero physical offsets,
offset[0] or offset[1] will be non-zero, forcing execution into the if branch
above this code:

drivers/pci/controller/vmd.c:vmd_enable_domain() {
    ...
    if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
        offset[0] || offset[1]) {
        ret = vmd_alloc_irqs(vmd);
        ...
        vmd_set_msi_remapping(vmd, true);

Does this unconditionally enable MSI-X remapping on hardware that no longer
supports it?

Also, looking at vmd_alloc_irqs() called in that same path:

drivers/pci/controller/vmd.c:vmd_alloc_irqs() {
    ...
    vmd->msix_count = pci_alloc_irq_vectors(dev, ...);
    if (vmd->msix_count < 0)
        return vmd->msix_count;

    vmd->irqs = devm_kcalloc(...);
    if (!vmd->irqs)
        return -ENOMEM;
    ...
}

If devm_kcalloc() or devm_request_irq() fails, it returns without freeing
the vectors. Should pci_free_irq_vectors() be called in these error paths
to prevent IRQ capacity leaks? 

Furthermore, if vmd_alloc_irqs() succeeds, but a subsequent function like
vmd_create_irq_domain() fails, does this leak the allocated IRQ vectors since
there is no call to pci_free_irq_vectors() in the error paths?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514215131.179743-1-nirmal.patel@linux.intel.com?part=1