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

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

[Nirmal Patel]

Newer VMD  with device ID 0x28c1 has unique settings compared to its
predecessor where BIOS enumerates the entire VMD device tree and
assigns respective configurations.

VMD configuration BAR0 carries over from GNR legacy VMD as the mechanism
to access the configuration space of the devices owned by VMD. The size
of this window is fixed at 256 MB, where each function consumes 4 KB and
every bus consumes 1 MB.

The shadow and scratchpad registers have been relocated from the VMD
configuration space to the VMD MMIO space in VMD BAR4/BAR5, otherwise
refers to as MEMBAR2 or MSI-X bar.

VMD MSI-X remapping enable/disable is no longer supported.

All the VMD driver code needs to do is to obtain bus hide range along
with shadow register values set by BIOS and perform a bus scan.

The patch also involves small refactoring of vmd_enable_domain function.

Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
---
 drivers/pci/controller/vmd.c | 146 ++++++++++++++++++++++++++++-------
 include/linux/pci_ids.h      |   1 +
 2 files changed, 119 insertions(+), 28 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index d4ae250d4bc6..6a5879d099a1 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -37,6 +37,11 @@
 #define MB2_SHADOW_OFFSET	0x2000
 #define MB2_SHADOW_SIZE		16
 
+/* DMR BAR4 register offsets */
+#define SHADOW_MEMBAR1_28C1		0x2818 /* MEMBAR1 physical address */
+#define SHADOW_MEMBAR2_28C1		0x2820 /* MEMBAR2 physical address */
+#define BASE_ID_REG_28C1		0x2840
+
 enum vmd_features {
 	/*
 	 * Device may contain registers which hint the physical location of the
@@ -77,6 +82,15 @@ enum vmd_features {
 	 * proper power management of the SoC.
 	 */
 	VMD_FEAT_BIOS_PM_QUIRK		= (1 << 5),
+
+	/*
+	 * Newer VMD  with device ID 0x28c1 has unique settings compared to its
+	 * predecessor where BIOS enumerates the entire VMD device tree and
+	 * stores respective configurations including bus start range and
+	 * shadow registers in VMD MMIO space in VMD BAR4/BAR5, otherwise refers
+	 * to as MEMBAR2 or MSI-X bar.
+	 */
+	VMD_FEAT_USE_BIOS_INFO		= (1 << 6),
 };
 
 #define VMD_BIOS_PM_QUIRK_LTR	0x1003	/* 3145728 ns */
@@ -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);
 
 	if (offset + len >= resource_size(&vmd->dev->resource[VMD_CFGBAR]))
 		return NULL;
@@ -661,6 +680,46 @@ static int vmd_get_bus_number_start(struct vmd_dev *vmd)
 	return 0;
 }
 
+static int vmd_get_bus_info_from_bar4(struct vmd_dev *vmd,
+				       resource_size_t *offset1,
+				       resource_size_t *offset2)
+{
+	u64 phys1, phys2, bar4_2840;
+	void __iomem *bar4;
+	u32 base_id;
+	u8 base_bus;
+
+
+	bar4 = pci_ioremap_bar(vmd->dev, 4);
+	if (!bar4)
+		return -ENOMEM;
+
+	/* Read shadow registers for MEMBAR1 and MEMBAR2 physical addresses. */
+	phys1 = readq(bar4 + SHADOW_MEMBAR1_28C1);
+	phys2 = readq(bar4 + SHADOW_MEMBAR2_28C1);
+
+	/*
+	 * Read and set bus start number from Base ID register.
+	 * 24-bit Base ID register is part of 64-bit shadowed reqid hide
+	 * range register and holds segement, bus, device and function.
+	 */
+	bar4_2840 = readq(bar4 + BASE_ID_REG_28C1);
+	base_id = bar4_2840 & 0xFFFFFF;
+	base_bus = base_id >> 8;
+	vmd->busn_start = base_bus;
+
+	/* Calculate offsets like vmd_get_phys_offsets() does. */
+	if (phys1)
+		*offset1 = vmd->dev->resource[VMD_MEMBAR1].start -
+			(phys1 & PCI_BASE_ADDRESS_MEM_MASK);
+	if (phys2)
+		*offset2 = vmd->dev->resource[VMD_MEMBAR2].start -
+			(phys2 & PCI_BASE_ADDRESS_MEM_MASK);
+
+	pci_iounmap(vmd->dev, bar4);
+	return 0;
+}
+
 static irqreturn_t vmd_irq(int irq, void *data)
 {
 	struct vmd_irq_list *irqs = data;
@@ -711,6 +770,54 @@ static int vmd_alloc_irqs(struct vmd_dev *vmd)
 	return 0;
 }
 
+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;
+		ret = vmd_get_phys_offsets(vmd, true, offset1, offset2);
+		if (ret)
+			return ret;
+	} else if (features & VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP) {
+		ret = vmd_get_phys_offsets(vmd, false, offset1, offset2);
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * Certain VMD devices may have a root port configuration option which
+	 * limits the bus range to between 0-127, 128-255, or 224-255.
+	 */
+	if (features & VMD_FEAT_HAS_BUS_RESTRICTIONS) {
+		ret = vmd_get_bus_number_start(vmd);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
 /*
  * Since VMD is an aperture to regular PCIe root ports, only allow it to
  * control features that the OS is allowed to control on the physical PCI bus.
@@ -784,32 +891,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	struct pci_dev *dev;
 	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.
-	 */
-	if (features & VMD_FEAT_HAS_MEMBAR_SHADOW) {
-		membar2_offset = MB2_SHADOW_OFFSET + MB2_SHADOW_SIZE;
-		ret = vmd_get_phys_offsets(vmd, true, &offset[0], &offset[1]);
-		if (ret)
-			return ret;
-	} else if (features & VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP) {
-		ret = vmd_get_phys_offsets(vmd, false, &offset[0], &offset[1]);
-		if (ret)
-			return ret;
-	}
-
-	/*
-	 * Certain VMD devices may have a root port configuration option which
-	 * limits the bus range to between 0-127, 128-255, or 224-255
-	 */
-	if (features & VMD_FEAT_HAS_BUS_RESTRICTIONS) {
-		ret = vmd_get_bus_number_start(vmd);
-		if (ret)
-			return ret;
-	}
+	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) {
@@ -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);
 	}
 
 	pci_add_resource(&resources, &vmd->resources[0]);
@@ -1114,6 +1200,10 @@ static const struct pci_device_id vmd_ids[] = {
 		.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW |
 				VMD_FEAT_HAS_BUS_RESTRICTIONS |
 				VMD_FEAT_CAN_BYPASS_MSI_REMAP,},
+	{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_28C1),
+                .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW |
+				VMD_FEAT_CAN_BYPASS_MSI_REMAP |
+				VMD_FEAT_USE_BIOS_INFO,},
 	{PCI_VDEVICE(INTEL, 0x467f),
 		.driver_data = VMD_FEATS_CLIENT,},
 	{PCI_VDEVICE(INTEL, 0x4c3d),
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 24cb42f66e4b..2a8ebe7df92e 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2889,6 +2889,7 @@
 #define PCI_DEVICE_ID_INTEL_HDA_ICH8	0x284b
 #define PCI_DEVICE_ID_INTEL_ICH8_6	0x2850
 #define PCI_DEVICE_ID_INTEL_VMD_28C0	0x28c0
+#define PCI_DEVICE_ID_INTEL_VMD_28C1    0x28c1
 #define PCI_DEVICE_ID_INTEL_ICH9_0	0x2910
 #define PCI_DEVICE_ID_INTEL_ICH9_2	0x2912
 #define PCI_DEVICE_ID_INTEL_ICH9_3	0x2913
-- 
2.52.0

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

[sashiko-bot]

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