Linux PCI subsystem development
 help / color / mirror / Atom feed
* [PATCH v3] PCI: vmd: Only assign OSC features in bare metal OS
@ 2026-07-13 22:08 Nirmal Patel
  2026-07-13 22:18 ` sashiko-bot
  2026-07-20 21:22 ` Nirmal Patel
  0 siblings, 2 replies; 3+ messages in thread
From: Nirmal Patel @ 2026-07-13 22:08 UTC (permalink / raw)
  To: nirmal.patel, linux-pci
  Cc: bhelgaas, kwilczynski, mani, robh, lpieralisi, Nirmal Patel,
	Andy Shevchenko

Copy valid OSC flags from root bridge only when OS can provide correct
values. In virtual machine, all of the control features are disabled as
hypervisor will not passthrough root bridge information to VM.

Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
Reviewed-by:  Andy Shevchenko <andriy.shevchenko@intel.com>
---
v3 : Removed period from commit and added a black line.
v2 : Added descriptive boolean and removed period from commit message.
---
---
 drivers/pci/controller/vmd.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index d4ae250d4bc6..7c7d110290d6 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -782,6 +782,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 	resource_size_t membar2_offset = 0x2000;
 	struct pci_bus *child;
 	struct pci_dev *dev;
+	bool vmd_in_guest;
 	int ret;
 
 	/*
@@ -862,14 +863,16 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 		.parent = res,
 	};
 
+	/* Non-zero offset means guest/direct assign view. */
+	vmd_in_guest = offset[0] || offset[1];
+
 	/*
 	 * Currently MSI remapping must be enabled in guest passthrough mode
 	 * due to some missing interrupt remapping plumbing. This is probably
 	 * acceptable because the guest is usually CPU-limited and MSI
 	 * remapping doesn't become a performance bottleneck.
 	 */
-	if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
-	    offset[0] || offset[1]) {
+	if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) || vmd_in_guest) {
 		ret = vmd_alloc_irqs(vmd);
 		if (ret)
 			return ret;
@@ -910,8 +913,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
 		return -ENODEV;
 	}
 
-	vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
-				   to_pci_host_bridge(vmd->bus->bridge));
+	/* Don't copy _OSC control flags in VM, it disables features. */
+	if (!vmd_in_guest)
+		vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
+					 to_pci_host_bridge(vmd->bus->bridge));
 
 	vmd_attach_resources(vmd);
 	if (vmd->irq_domain)
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] PCI: vmd: Only assign OSC features in bare metal OS
  2026-07-13 22:08 [PATCH v3] PCI: vmd: Only assign OSC features in bare metal OS Nirmal Patel
@ 2026-07-13 22:18 ` sashiko-bot
  2026-07-20 21:22 ` Nirmal Patel
  1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-07-13 22:18 UTC (permalink / raw)
  To: Nirmal Patel; +Cc: linux-pci

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Flawed VM detection heuristic breaks intended OSC flag assignment in edge cases.

Pre-existing issues:
- [Medium] Memory leak of dynamically allocated SRCU structures on error paths.
- [Medium] Leak of dynamically allocated resource entries and IRQ domain on early return.
--

--- Patch [1]: [PATCH v3] PCI: vmd: Only assign OSC features in bare metal OS ---
commit 7b1a0a2085cd70ccc13f43497ed84956b2e0637c
Author: Nirmal Patel <nirmal.patel@intel.com>

PCI: vmd: Only assign OSC features in bare metal OS

Copy valid OSC flags from root bridge only when OS can provide correct
values. In virtual machine, all of the control features are disabled as
hypervisor will not passthrough root bridge information to VM.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c

[ ... ]

> @@ -862,14 +863,16 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  		.parent = res,
>  	};
>  
> +	/* Non-zero offset means guest/direct assign view. */
> +	vmd_in_guest = offset[0] || offset[1];
> +

[Severity: High]
Does this heuristic correctly account for bare-metal BAR reassignment or 
identity-mapped VMs?

If the Linux PCI subsystem reassigns the VMD device's BARs on bare metal,
for example due to resource conflicts, the offsets would become non-zero.
This would cause the driver to incorrectly identify the system as a guest 
and skip copying OSC control flags on bare metal.

Conversely, if a VM uses 1:1 identity-mapped direct assignment where the
guest physical address equals the host physical address, the offset would 
be zero. The driver would then treat the VM as bare metal, copying the 
disabled or invalid OSC flags from the virtual host bridge.

>  	/*
>  	 * Currently MSI remapping must be enabled in guest passthrough mode
>  	 * due to some missing interrupt remapping plumbing. This is probably
>  	 * acceptable because the guest is usually CPU-limited and MSI
>  	 * remapping doesn't become a performance bottleneck.
>  	 */
> -	if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
> -	    offset[0] || offset[1]) {
> +	if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) || vmd_in_guest) {
>  		ret = vmd_alloc_irqs(vmd);

[Severity: Medium]
This is a pre-existing issue, but does vmd_alloc_irqs() leak dynamically
allocated SRCU structures on error paths?

If devm_request_irq() fails inside vmd_alloc_irqs(), the function returns
the error directly without calling cleanup_srcu_struct() on the SRCU
structures initialized in previous loop iterations or the current one.

Additionally, if vmd_enable_domain() fails and propagates an error back to
vmd_probe(), the out_release_instance error path in vmd_probe() does not
call vmd_cleanup_srcu(), which seems to leak the SRCU per-CPU data.

>  		if (ret)
>  			return ret;

[ ... ]

> @@ -910,8 +913,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  		return -ENODEV;
>  	}
>  
> -	vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
> -				   to_pci_host_bridge(vmd->bus->bridge));
> +	/* Don't copy _OSC control flags in VM, it disables features. */
> +	if (!vmd_in_guest)
> +		vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
> +					 to_pci_host_bridge(vmd->bus->bridge));

[Severity: Medium]
This is a pre-existing issue, but does vmd_enable_domain() leak dynamically
allocated resource entries and the IRQ domain on early return?

When pci_bus_find_emul_domain_nr() is called a few lines before this hunk
in vmd_enable_domain():

        sd->domain = pci_bus_find_emul_domain_nr(0, 0x10000, INT_MAX);
        if (sd->domain < 0)
                return sd->domain;

If this fails, it returns early without calling pci_free_resource_list() 
or vmd_remove_irq_domain(), stranding the memory allocated for the 
resources list and the IRQ domain created earlier in the function.

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3] PCI: vmd: Only assign OSC features in bare metal OS
  2026-07-13 22:08 [PATCH v3] PCI: vmd: Only assign OSC features in bare metal OS Nirmal Patel
  2026-07-13 22:18 ` sashiko-bot
@ 2026-07-20 21:22 ` Nirmal Patel
  1 sibling, 0 replies; 3+ messages in thread
From: Nirmal Patel @ 2026-07-20 21:22 UTC (permalink / raw)
  To: Nirmal Patel
  Cc: linux-pci, bhelgaas, kwilczynski, mani, robh, lpieralisi,
	Andy Shevchenko

On Mon, 13 Jul 2026 22:08:44 +0000
Nirmal Patel <nirmal.patel@intel.com> wrote:

> Copy valid OSC flags from root bridge only when OS can provide correct
> values. In virtual machine, all of the control features are disabled
> as hypervisor will not passthrough root bridge information to VM.
> 
> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> Reviewed-by:  Andy Shevchenko <andriy.shevchenko@intel.com>
> ---
> v3 : Removed period from commit and added a black line.
> v2 : Added descriptive boolean and removed period from commit message.
> ---
> ---
>  drivers/pci/controller/vmd.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/vmd.c
> b/drivers/pci/controller/vmd.c index d4ae250d4bc6..7c7d110290d6 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -782,6 +782,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd,
> unsigned long features) resource_size_t membar2_offset = 0x2000;
>  	struct pci_bus *child;
>  	struct pci_dev *dev;
> +	bool vmd_in_guest;
>  	int ret;
>  
>  	/*
> @@ -862,14 +863,16 @@ static int vmd_enable_domain(struct vmd_dev
> *vmd, unsigned long features) .parent = res,
>  	};
>  
> +	/* Non-zero offset means guest/direct assign view. */
> +	vmd_in_guest = offset[0] || offset[1];
> +
>  	/*
>  	 * Currently MSI remapping must be enabled in guest
> passthrough mode
>  	 * due to some missing interrupt remapping plumbing. This is
> probably
>  	 * acceptable because the guest is usually CPU-limited and
> MSI
>  	 * remapping doesn't become a performance bottleneck.
>  	 */
> -	if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
> -	    offset[0] || offset[1]) {
> +	if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
> vmd_in_guest) { ret = vmd_alloc_irqs(vmd);
>  		if (ret)
>  			return ret;
> @@ -910,8 +913,10 @@ static int vmd_enable_domain(struct vmd_dev
> *vmd, unsigned long features) return -ENODEV;
>  	}
>  
> -
> vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
> -
> to_pci_host_bridge(vmd->bus->bridge));
> +	/* Don't copy _OSC control flags in VM, it disables
> features. */
> +	if (!vmd_in_guest)
> +
> vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
> +
> to_pci_host_bridge(vmd->bus->bridge)); 
>  	vmd_attach_resources(vmd);
>  	if (vmd->irq_domain)

Gentle reminder. 

Thanks
nirmal

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-07-20 21:22 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-13 22:08 [PATCH v3] PCI: vmd: Only assign OSC features in bare metal OS Nirmal Patel
2026-07-13 22:18 ` sashiko-bot
2026-07-20 21:22 ` Nirmal Patel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox