Re: [PATCH] drm/xe: Add force_p2p module/configfs parameter

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Global force_p2p parameter incorrectly overrides P2P safety checks for all non-Xe PCI devices.
- [Low] The `force_p2p` attribute is missing from `dump_custom_dev_config()`, resulting in incomplete debug logs.
--

--- Patch [1]: [PATCH] drm/xe: Add force_p2p module/configfs parameter ---
commit 7f80ed3c08c50eca10a0100021e6454c59749548
Author: Nathan Ciobanu <[email protected]>

drm/xe: Add force_p2p module/configfs parameter

Add force_p2p module and configfs parameter to give users an option to
allow P2P DMA between GPU cards that are connected off of the root bus
with no switch/bridge in between, as seen in some virtual machine
topologies.

> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
> index 052cce9621611..187352a8b4199 100644
> --- a/drivers/gpu/drm/xe/xe_configfs.c
> +++ b/drivers/gpu/drm/xe/xe_configfs.c
[ ... ]
> @@ -856,6 +923,7 @@ CONFIGFS_ATTR(, ctx_restore_post_bb);
>  CONFIGFS_ATTR(, enable_multi_queue);
>  CONFIGFS_ATTR(, enable_psmi);
>  CONFIGFS_ATTR(, engines_allowed);
> +CONFIGFS_ATTR(, force_p2p);
>  CONFIGFS_ATTR(, gt_types_allowed);
>  CONFIGFS_ATTR(, survivability_mode);
>  
> @@ -865,6 +933,7 @@ static struct configfs_attribute *xe_config_device_attrs[] = {
>  	&attr_enable_multi_queue,
>  	&attr_enable_psmi,
>  	&attr_engines_allowed,
> +	&attr_force_p2p,
>  	&attr_gt_types_allowed,
>  	&attr_survivability_mode,
>  	NULL,

[Severity: Low]
Should the new force_p2p attribute also be added to dump_custom_dev_config()?

When a user modifies the force_p2p attribute via configfs and subsequently
binds the device, the driver successfully detects that custom settings were
applied but fails to log the force_p2p modification. This leads to confusing
debug logs where the driver announces custom settings but prints an
incomplete list.

[ ... ]
> @@ -1269,6 +1338,25 @@ bool xe_configfs_get_psmi_enabled(struct pci_dev *pdev)
>  	return ret;
>  }
>  
> +/** xe_configfs_get_force_p2p - get configfs force_p2p setting
> + * @pdev: pci device
> + *
> + * Return: force_p2p setting in configfs
> + */
> +bool xe_configfs_get_force_p2p(struct pci_dev *pdev)
> +{
> +	struct xe_config_group_device *dev = find_xe_config_group_device(pdev);
> +	bool ret;
> +
> +	if (!dev)
> +		return xe_modparam.force_p2p;

[Severity: High]
Does returning xe_modparam.force_p2p for non-Xe devices bypass PCIe topology
safety checks for unrelated hardware?

Since find_xe_config_group_device() will return NULL for non-Xe PCI devices,
this fallback unconditionally causes any other PCI device (like NVMe drives
or network cards) to inherit the global xe.force_p2p setting.

> +
> +	ret = dev->config.force_p2p;
> +
> +	config_group_put(&dev->group);
> +	return ret;
> +}
[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_dma_buf.c b/drivers/gpu/drm/xe/xe_dma_buf.c
> index bf0728838ead5..bd8f8ad70bcd3 100644
> --- a/drivers/gpu/drm/xe/xe_dma_buf.c
> +++ b/drivers/gpu/drm/xe/xe_dma_buf.c
[ ... ]
> @@ -26,9 +27,14 @@ static int xe_dma_buf_attach(struct dma_buf *dmabuf,
>  			     struct dma_buf_attachment *attach)
>  {
>  	struct drm_gem_object *obj = attach->dmabuf->priv;
> +	struct pci_dev *a_pdev = to_pci_dev(obj->dev->dev);
> +	struct pci_dev *b_pdev = dev_is_pci(attach->dev) ?
> +				 to_pci_dev(attach->dev) : NULL;
>  
>  	if (attach->peer2peer &&
> -	    pci_p2pdma_distance(to_pci_dev(obj->dev->dev), attach->dev, false) < 0)
> +	    !(b_pdev && xe_configfs_get_force_p2p(a_pdev) &&
> +	      xe_configfs_get_force_p2p(b_pdev)) &&
> +	    pci_p2pdma_distance(a_pdev, attach->dev, false) < 0)

[Severity: High]
Because xe_configfs_get_force_p2p(b_pdev) falls back to the global module
parameter when find_xe_config_group_device() fails, doesn't this condition
evaluate to true even when b_pdev is a completely unrelated device?

This effectively skips the pci_p2pdma_distance() safety checks for any device
attempting P2P with the Xe GPU when xe.force_p2p=1 is enabled, which risks
data corruption if the bridge drops or corrupts transactions.

>  		attach->peer2peer = false;
>  
>  	if (!attach->peer2peer && !xe_bo_can_migrate(gem_to_xe_bo(obj), XE_PL_TT))
[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
> index 627a741293d59..3cd53bb166afd 100644
> --- a/drivers/gpu/drm/xe/xe_svm.c
> +++ b/drivers/gpu/drm/xe/xe_svm.c
[ ... ]
> @@ -892,6 +893,10 @@ static bool xe_has_interconnect(struct drm_pagemap_peer *peer1,
>  	if (dev1 == dev2)
>  		return true;
>  
> +	if (xe_configfs_get_force_p2p(to_pci_dev(dev1)) &&
> +	    xe_configfs_get_force_p2p(to_pci_dev(dev2)))
> +		return true;
> +

[Severity: High]
Similar to the xe_dma_buf_attach() logic above, does this improperly bypass
the interconnect peer distance checks for non-Xe devices when the global
module parameter is set?

>  	return pci_p2pdma_distance(to_pci_dev(dev1), dev2, true) >= 0;
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1