Re: [PATCH 1/6] drm/xe/pci: log ATS/PASID enablement state at probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/xe/pci: xe_pci_ats_pasid_enabled() always returns false on SR-IOV VFs
- [Low] drm/xe/pci: build failure in __xe_pci_probe() without CONFIG_PCI_ATS/PASID
--

commit 9613705bbc43ffcbfae8d92b0e6cff920caffa87
Author: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>

drm/xe/pci: log ATS/PASID enablement state at probe

This commit logs ATS and PASID enablement states during __xe_pci_probe() when
their PCI capabilities are present, and introduces xe_pci_ats_pasid_enabled()
to check whether both features are enabled on a PCI device.

> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 44587b5f4df60..7c8a4286b657a 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -1281,6 +1281,13 @@ static int __xe_pci_probe(struct pci_dev *pdev, const struct xe_device_desc *des
>  		xe_step_name(xe->info.step.media),
>  		xe_step_name(xe->info.step.basedie));
>  
> +	if (pdev->ats_cap)
> +		drm_dbg(&xe->drm, "Arch ATS = %s",
> +			str_enabled_disabled(pdev->ats_enabled));
> +	if (pdev->pasid_cap)
> +		drm_dbg(&xe->drm, "Arch PASID = %s",
> +			str_enabled_disabled(pdev->pasid_enabled));

[Severity: Low]
Will __xe_pci_probe() in drivers/gpu/drm/xe/xe_pci.c fail to build when
CONFIG_PCI_ATS or CONFIG_PCI_PASID is disabled?

In include/linux/pci.h, struct pci_dev only defines ats_cap and pasid_cap
when their respective Kconfig options are enabled:

#ifdef CONFIG_PCI_ATS
	...
	u16		ats_cap;	/* ATS Capability offset */
	u8		ats_stu;	/* ATS Smallest Translation Unit */
#endif
...
#ifdef CONFIG_PCI_PASID
	u16		pasid_cap;	/* PASID Capability offset */
	u16		pasid_features;
#endif

Since CONFIG_DRM_XE in drivers/gpu/drm/xe/Kconfig only depends on DRM and
PCI without selecting or depending on PCI_ATS or PCI_PASID (for example, in
drivers/gpu/drm/xe/.kunitconfig), accessing pdev->ats_cap and pdev->pasid_cap
directly without IS_ENABLED() guards breaks the build when either option is
not set.

> +
>  	drm_dbg(&xe->drm, "SR-IOV support: %s (mode: %s)\n",
>  		str_yes_no(xe_device_has_sriov(xe)),
>  		xe_sriov_mode_to_string(xe_device_sriov_mode(xe)));
> diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
> index 11bcc5fe2c5b9..5f784229a5ecf 100644
> --- a/drivers/gpu/drm/xe/xe_pci.h
> +++ b/drivers/gpu/drm/xe/xe_pci.h
> @@ -5,6 +5,7 @@
>  
>  #ifndef _XE_PCI_H_
>  #define _XE_PCI_H_
> +#include <linux/pci.h>
>  
>  struct pci_dev;
>  
> @@ -12,4 +13,9 @@ int xe_register_pci_driver(void);
>  void xe_unregister_pci_driver(void);
>  struct xe_device *xe_pci_to_pf_device(struct pci_dev *pdev);
>  
> +static inline bool xe_pci_ats_pasid_enabled(struct pci_dev *pdev)
> +{
> +	return pdev->ats_enabled && pdev->pasid_enabled;
> +}

[Severity: Medium]
How does xe_pci_ats_pasid_enabled() in drivers/gpu/drm/xe/xe_pci.h (as well
as the PASID check in __xe_pci_probe()) behave on SR-IOV Virtual Functions?

In drivers/pci/ats.c, pci_enable_pasid() notes that VFs do not implement
their own PASID capability (so pdev->pasid_cap is 0) and instead share the
PF's PASID configuration:

	if (pdev->is_virtfn) {
		if (pci_physfn(pdev)->pasid_enabled)
			return 0;
		return -EINVAL;
	}

When the IOMMU enables PASID on a VF via pci_enable_pasid(), it checks
pci_physfn(pdev)->pasid_enabled and returns 0 without setting
pdev->pasid_enabled on the VF's pci_dev (whereas pci_enable_ats() sets
pdev->ats_enabled on the VF).

Because xe_pci_ats_pasid_enabled() and __xe_pci_probe() inspect
pdev->pasid_enabled and pdev->pasid_cap directly instead of checking
pci_physfn(pdev) (or calling pci_pasid_status(pdev)), won't
xe_pci_ats_pasid_enabled() always return false on VFs and __xe_pci_probe()
always skip logging the PASID state on VFs?

> +
>  #endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021041117.1774877-1-naresh.kumar.g@intel.com?part=1