diff mbox series

[V3] xen/virtio: Handle PCI devices which Host controller is described in DT

Message ID 20221021172408.77397-1-olekstysh@gmail.com
State New
Headers
Series [V3] xen/virtio: Handle PCI devices which Host controller is described in DT |

Commit Message

Oleksandr Tyshchenko Oct. 21, 2022, 5:24 p.m. UTC 
  From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Use the same "xen-grant-dma" device concept for the PCI devices
behind device-tree based PCI Host controller, but with one modification.
Unlike for platform devices, we cannot use generic IOMMU bindings
(iommus property), as we need to support more flexible configuration.
The problem is that PCI devices under the single PCI Host controller
may have the backends running in different Xen domains and thus have
different endpoints ID (backend domains ID).

Add ability to deal with generic PCI-IOMMU bindings (iommu-map/
iommu-map-mask properties) which allows us to describe relationship
between PCI devices and backend domains ID properly.

To avoid having to look up for the PCI Host bridge twice and reduce
the amount of checks pass an extra struct device_node *np to both
xen_dt_grant_init_backend_domid() and xen_is_dt_grant_dma_device().
While at it also pass domid_t *backend_domid instead of
struct xen_grant_dma_data *data to the former.

So with current patch the code expects iommus property for the platform
devices and iommu-map/iommu-map-mask properties for PCI devices.

The example of generated by the toolstack iommu-map property
for two PCI devices 0000:00:01.0 and 0000:00:02.0 whose
backends are running in different Xen domains with IDs 1 and 2
respectively:
iommu-map = <0x08 0xfde9 0x01 0x08 0x10 0xfde9 0x02 0x08>;

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
Slightly RFC. This is needed to support Xen grant mappings for virtio-pci devices
on Arm at some point in the future. The Xen toolstack side is not completely ready yet.
Here, for PCI devices we use more flexible way to pass backend domid to the guest
than for platform devices.

Changes V1 -> V2:
   - update commit description
   - rebase
   - rework to use generic PCI-IOMMU bindings instead of generic IOMMU bindings

Changes V2 -> V3:
   - update commit description, add an example
   - drop xen_dt_map_id() and squash xen_dt_get_pci_host_node() with
     xen_dt_get_node()
   - pass struct device_node *np to xen_is_dt_grant_dma_device() and
     xen_dt_grant_init_backend_domid()
   - pass domid_t *backend_domid instead of struct xen_grant_dma_data *data
     to xen_dt_grant_init_backend_domid()

Previous discussion is at:
https://lore.kernel.org/xen-devel/20221006174804.2003029-1-olekstysh@gmail.com/
https://lore.kernel.org/xen-devel/20221015153409.918775-1-olekstysh@gmail.com/

Based on:
https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git/log/?h=for-linus-6.1
---
 drivers/xen/grant-dma-ops.c | 80 ++++++++++++++++++++++++++++++-------
 1 file changed, 66 insertions(+), 14 deletions(-)

Comments

Stefano Stabellini Oct. 21, 2022, 8:08 p.m. UTC | #1 
On Fri, 21 Oct 2022, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> Use the same "xen-grant-dma" device concept for the PCI devices
> behind device-tree based PCI Host controller, but with one modification.
> Unlike for platform devices, we cannot use generic IOMMU bindings
> (iommus property), as we need to support more flexible configuration.
> The problem is that PCI devices under the single PCI Host controller
> may have the backends running in different Xen domains and thus have
> different endpoints ID (backend domains ID).
> 
> Add ability to deal with generic PCI-IOMMU bindings (iommu-map/
> iommu-map-mask properties) which allows us to describe relationship
> between PCI devices and backend domains ID properly.
> 
> To avoid having to look up for the PCI Host bridge twice and reduce
> the amount of checks pass an extra struct device_node *np to both
> xen_dt_grant_init_backend_domid() and xen_is_dt_grant_dma_device().
> While at it also pass domid_t *backend_domid instead of
> struct xen_grant_dma_data *data to the former.
> 
> So with current patch the code expects iommus property for the platform
> devices and iommu-map/iommu-map-mask properties for PCI devices.
> 
> The example of generated by the toolstack iommu-map property
> for two PCI devices 0000:00:01.0 and 0000:00:02.0 whose
> backends are running in different Xen domains with IDs 1 and 2
> respectively:
> iommu-map = <0x08 0xfde9 0x01 0x08 0x10 0xfde9 0x02 0x08>;
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
> Slightly RFC. This is needed to support Xen grant mappings for virtio-pci devices
> on Arm at some point in the future. The Xen toolstack side is not completely ready yet.
> Here, for PCI devices we use more flexible way to pass backend domid to the guest
> than for platform devices.
> 
> Changes V1 -> V2:
>    - update commit description
>    - rebase
>    - rework to use generic PCI-IOMMU bindings instead of generic IOMMU bindings
> 
> Changes V2 -> V3:
>    - update commit description, add an example
>    - drop xen_dt_map_id() and squash xen_dt_get_pci_host_node() with
>      xen_dt_get_node()
>    - pass struct device_node *np to xen_is_dt_grant_dma_device() and
>      xen_dt_grant_init_backend_domid()
>    - pass domid_t *backend_domid instead of struct xen_grant_dma_data *data
>      to xen_dt_grant_init_backend_domid()
> 
> Previous discussion is at:
> https://lore.kernel.org/xen-devel/20221006174804.2003029-1-olekstysh@gmail.com/
> https://lore.kernel.org/xen-devel/20221015153409.918775-1-olekstysh@gmail.com/
> 
> Based on:
> https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git/log/?h=for-linus-6.1
> ---
>  drivers/xen/grant-dma-ops.c | 80 ++++++++++++++++++++++++++++++-------
>  1 file changed, 66 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index daa525df7bdc..76b29d20aeee 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -10,6 +10,7 @@
>  #include <linux/module.h>
>  #include <linux/dma-map-ops.h>
>  #include <linux/of.h>
> +#include <linux/pci.h>
>  #include <linux/pfn.h>
>  #include <linux/xarray.h>
>  #include <linux/virtio_anchor.h>
> @@ -292,12 +293,37 @@ static const struct dma_map_ops xen_grant_dma_ops = {
>  	.dma_supported = xen_grant_dma_supported,
>  };
>  
> -static bool xen_is_dt_grant_dma_device(struct device *dev)
> +static struct device_node *xen_dt_get_node(struct device *dev)
>  {
> -	struct device_node *iommu_np;
> +	if (dev_is_pci(dev)) {
> +		struct pci_dev *pdev = to_pci_dev(dev);
> +		struct pci_bus *bus = pdev->bus;
> +
> +		/* Walk up to the root bus to look for PCI Host controller */
> +		while (!pci_is_root_bus(bus))
> +			bus = bus->parent;
> +
> +		return of_node_get(bus->bridge->parent->of_node);
> +	}
> +
> +	return of_node_get(dev->of_node);
> +}
> +
> +static bool xen_is_dt_grant_dma_device(struct device *dev,
> +					struct device_node *np)
> +{
> +	struct device_node *iommu_np = NULL;
>  	bool has_iommu;
>  
> -	iommu_np = of_parse_phandle(dev->of_node, "iommus", 0);
> +	if (dev_is_pci(dev)) {
> +		struct pci_dev *pdev = to_pci_dev(dev);
> +		u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
> +
> +		if (of_map_id(np, rid, "iommu-map", "iommu-map-mask", &iommu_np, NULL))
> +			return false;
> +	} else
> +		iommu_np = of_parse_phandle(np, "iommus", 0);
> +
>  	has_iommu = iommu_np &&
>  		    of_device_is_compatible(iommu_np, "xen,grant-dma");
>  	of_node_put(iommu_np);

I think we can remove xen_is_dt_grant_dma_device and just call
xen_dt_grant_init_backend_domid passing a NULL backend_domid?

It is a bit annoying that we are basically doing the same device tree
parsing twice in a row given that the callers do:

	if (xen_is_grant_dma_device(dev))
		xen_grant_setup_dma_ops(dev);

Maybe we could move the backend_domid allocation and setting to
xen_dt_grant_init_backend_domid, which would end up being done from
the xen_is_grant_dma_device() call chain, and only leave setting
dev->dma_ops from xen_grant_setup_dma_ops().

This way the parsing would be done only once? What do you think? This
suggestion is optional, I am OK also with only removing
xen_is_dt_grant_dma_device.


> @@ -307,9 +333,17 @@ static bool xen_is_dt_grant_dma_device(struct device *dev)
>  
>  bool xen_is_grant_dma_device(struct device *dev)
>  {
> +	struct device_node *np;
> +
>  	/* XXX Handle only DT devices for now */
> -	if (dev->of_node)
> -		return xen_is_dt_grant_dma_device(dev);
> +	np = xen_dt_get_node(dev);
> +	if (np) {
> +		bool ret;
> +
> +		ret = xen_is_dt_grant_dma_device(dev, np);
> +		of_node_put(np);
> +		return ret;
> +	}
>  
>  	return false;
>  }
> @@ -323,14 +357,26 @@ bool xen_virtio_mem_acc(struct virtio_device *dev)
>  }
>  
>  static int xen_dt_grant_init_backend_domid(struct device *dev,
> -					   struct xen_grant_dma_data *data)
> +					   struct device_node *np,
> +					   domid_t *backend_domid)
>  {
> -	struct of_phandle_args iommu_spec;
> +	struct of_phandle_args iommu_spec = { .args_count = 1 };
>  
> -	if (of_parse_phandle_with_args(dev->of_node, "iommus", "#iommu-cells",
> -			0, &iommu_spec)) {
> -		dev_err(dev, "Cannot parse iommus property\n");
> -		return -ESRCH;
> +	if (dev_is_pci(dev)) {
> +		struct pci_dev *pdev = to_pci_dev(dev);
> +		u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
> +
> +		if (of_map_id(np, rid, "iommu-map", "iommu-map-mask", &iommu_spec.np,
> +				iommu_spec.args)) {
> +			dev_err(dev, "Cannot translate ID\n");
> +			return -ESRCH;
> +		}
> +	} else {
> +		if (of_parse_phandle_with_args(np, "iommus", "#iommu-cells",
> +				0, &iommu_spec)) {
> +			dev_err(dev, "Cannot parse iommus property\n");
> +			return -ESRCH;
> +		}
>  	}
>  
>  	if (!of_device_is_compatible(iommu_spec.np, "xen,grant-dma") ||
> @@ -346,7 +392,7 @@ static int xen_dt_grant_init_backend_domid(struct device *dev,
>  	 * The endpoint ID here means the ID of the domain where the
>  	 * corresponding backend is running
>  	 */
> -	data->backend_domid = iommu_spec.args[0];
> +	*backend_domid = iommu_spec.args[0];
>  
>  	return 0;
>  }
> @@ -354,6 +400,7 @@ static int xen_dt_grant_init_backend_domid(struct device *dev,
>  void xen_grant_setup_dma_ops(struct device *dev)
>  {
>  	struct xen_grant_dma_data *data;
> +	struct device_node *np;
>  
>  	data = find_xen_grant_dma_data(dev);
>  	if (data) {
> @@ -365,8 +412,13 @@ void xen_grant_setup_dma_ops(struct device *dev)
>  	if (!data)
>  		goto err;
>  
> -	if (dev->of_node) {
> -		if (xen_dt_grant_init_backend_domid(dev, data))
> +	np = xen_dt_get_node(dev);
> +	if (np) {
> +		int ret;
> +
> +		ret = xen_dt_grant_init_backend_domid(dev, np, &data->backend_domid);
> +		of_node_put(np);
> +		if (ret)
>  			goto err;
>  	} else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT)) {
>  		dev_info(dev, "Using dom0 as backend\n");
> -- 
> 2.25.1
>
Oleksandr Tyshchenko Oct. 22, 2022, 6:44 a.m. UTC | #2 
On 21.10.22 23:08, Stefano Stabellini wrote:

Hello Stefano

> On Fri, 21 Oct 2022, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> Use the same "xen-grant-dma" device concept for the PCI devices
>> behind device-tree based PCI Host controller, but with one modification.
>> Unlike for platform devices, we cannot use generic IOMMU bindings
>> (iommus property), as we need to support more flexible configuration.
>> The problem is that PCI devices under the single PCI Host controller
>> may have the backends running in different Xen domains and thus have
>> different endpoints ID (backend domains ID).
>>
>> Add ability to deal with generic PCI-IOMMU bindings (iommu-map/
>> iommu-map-mask properties) which allows us to describe relationship
>> between PCI devices and backend domains ID properly.
>>
>> To avoid having to look up for the PCI Host bridge twice and reduce
>> the amount of checks pass an extra struct device_node *np to both
>> xen_dt_grant_init_backend_domid() and xen_is_dt_grant_dma_device().
>> While at it also pass domid_t *backend_domid instead of
>> struct xen_grant_dma_data *data to the former.
>>
>> So with current patch the code expects iommus property for the platform
>> devices and iommu-map/iommu-map-mask properties for PCI devices.
>>
>> The example of generated by the toolstack iommu-map property
>> for two PCI devices 0000:00:01.0 and 0000:00:02.0 whose
>> backends are running in different Xen domains with IDs 1 and 2
>> respectively:
>> iommu-map = <0x08 0xfde9 0x01 0x08 0x10 0xfde9 0x02 0x08>;
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>> Slightly RFC. This is needed to support Xen grant mappings for virtio-pci devices
>> on Arm at some point in the future. The Xen toolstack side is not completely ready yet.
>> Here, for PCI devices we use more flexible way to pass backend domid to the guest
>> than for platform devices.
>>
>> Changes V1 -> V2:
>>     - update commit description
>>     - rebase
>>     - rework to use generic PCI-IOMMU bindings instead of generic IOMMU bindings
>>
>> Changes V2 -> V3:
>>     - update commit description, add an example
>>     - drop xen_dt_map_id() and squash xen_dt_get_pci_host_node() with
>>       xen_dt_get_node()
>>     - pass struct device_node *np to xen_is_dt_grant_dma_device() and
>>       xen_dt_grant_init_backend_domid()
>>     - pass domid_t *backend_domid instead of struct xen_grant_dma_data *data
>>       to xen_dt_grant_init_backend_domid()
>>
>> Previous discussion is at:
>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/20221006174804.2003029-1-olekstysh@gmail.com/__;!!GF_29dbcQIUBPA!3cZiRy0Scq8-dibrxyFGUlAIhwa7UwRmrCAG-qdvkAG5NInYPZ_mbLTMtsZ_F4Gonowkettr-dcRO3TAs_gn-1xcLk77xg$  [lore[.]kernel[.]org]
>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/20221015153409.918775-1-olekstysh@gmail.com/__;!!GF_29dbcQIUBPA!3cZiRy0Scq8-dibrxyFGUlAIhwa7UwRmrCAG-qdvkAG5NInYPZ_mbLTMtsZ_F4Gonowkettr-dcRO3TAs_gn-1xwRjX9GQ$  [lore[.]kernel[.]org]
>>
>> Based on:
>> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git/log/?h=for-linus-6.1__;!!GF_29dbcQIUBPA!3cZiRy0Scq8-dibrxyFGUlAIhwa7UwRmrCAG-qdvkAG5NInYPZ_mbLTMtsZ_F4Gonowkettr-dcRO3TAs_gn-1zVOy8WKg$  [git[.]kernel[.]org]
>> ---
>>   drivers/xen/grant-dma-ops.c | 80 ++++++++++++++++++++++++++++++-------
>>   1 file changed, 66 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>> index daa525df7bdc..76b29d20aeee 100644
>> --- a/drivers/xen/grant-dma-ops.c
>> +++ b/drivers/xen/grant-dma-ops.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/module.h>
>>   #include <linux/dma-map-ops.h>
>>   #include <linux/of.h>
>> +#include <linux/pci.h>
>>   #include <linux/pfn.h>
>>   #include <linux/xarray.h>
>>   #include <linux/virtio_anchor.h>
>> @@ -292,12 +293,37 @@ static const struct dma_map_ops xen_grant_dma_ops = {
>>   	.dma_supported = xen_grant_dma_supported,
>>   };
>>   
>> -static bool xen_is_dt_grant_dma_device(struct device *dev)
>> +static struct device_node *xen_dt_get_node(struct device *dev)
>>   {
>> -	struct device_node *iommu_np;
>> +	if (dev_is_pci(dev)) {
>> +		struct pci_dev *pdev = to_pci_dev(dev);
>> +		struct pci_bus *bus = pdev->bus;
>> +
>> +		/* Walk up to the root bus to look for PCI Host controller */
>> +		while (!pci_is_root_bus(bus))
>> +			bus = bus->parent;
>> +
>> +		return of_node_get(bus->bridge->parent->of_node);
>> +	}
>> +
>> +	return of_node_get(dev->of_node);
>> +}
>> +
>> +static bool xen_is_dt_grant_dma_device(struct device *dev,
>> +					struct device_node *np)
>> +{
>> +	struct device_node *iommu_np = NULL;
>>   	bool has_iommu;
>>   
>> -	iommu_np = of_parse_phandle(dev->of_node, "iommus", 0);
>> +	if (dev_is_pci(dev)) {
>> +		struct pci_dev *pdev = to_pci_dev(dev);
>> +		u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>> +
>> +		if (of_map_id(np, rid, "iommu-map", "iommu-map-mask", &iommu_np, NULL))
>> +			return false;
>> +	} else
>> +		iommu_np = of_parse_phandle(np, "iommus", 0);
>> +
>>   	has_iommu = iommu_np &&
>>   		    of_device_is_compatible(iommu_np, "xen,grant-dma");
>>   	of_node_put(iommu_np);
> I think we can remove xen_is_dt_grant_dma_device and just call
> xen_dt_grant_init_backend_domid passing a NULL backend_domid?
>
> It is a bit annoying that we are basically doing the same device tree
> parsing twice in a row given that the callers do:
>
> 	if (xen_is_grant_dma_device(dev))
> 		xen_grant_setup_dma_ops(dev);
>
> Maybe we could move the backend_domid allocation and setting to
> xen_dt_grant_init_backend_domid, which would end up being done from
> the xen_is_grant_dma_device() call chain, and only leave setting
> dev->dma_ops from xen_grant_setup_dma_ops().
>
> This way the parsing would be done only once? What do you think? This
> suggestion is optional, I am OK also with only removing
> xen_is_dt_grant_dma_device.

ok, I will think about such optimization and will provide my thoughts 
about it or a patch. But, I think it should be a separate patch (if 
needs be),

as in current patch we don't change anything in that regard, we just 
implement

"if (dev_is_pci(dev)) { ... }" condition for 
xen_is_dt_grant_dma_device() and xen_dt_grant_init_backend_domid().



>
>
>> @@ -307,9 +333,17 @@ static bool xen_is_dt_grant_dma_device(struct device *dev)
>>   
>>   bool xen_is_grant_dma_device(struct device *dev)
>>   {
>> +	struct device_node *np;
>> +
>>   	/* XXX Handle only DT devices for now */
>> -	if (dev->of_node)
>> -		return xen_is_dt_grant_dma_device(dev);
>> +	np = xen_dt_get_node(dev);
>> +	if (np) {
>> +		bool ret;
>> +
>> +		ret = xen_is_dt_grant_dma_device(dev, np);
>> +		of_node_put(np);
>> +		return ret;
>> +	}
>>   
>>   	return false;
>>   }
>> @@ -323,14 +357,26 @@ bool xen_virtio_mem_acc(struct virtio_device *dev)
>>   }
>>   
>>   static int xen_dt_grant_init_backend_domid(struct device *dev,
>> -					   struct xen_grant_dma_data *data)
>> +					   struct device_node *np,
>> +					   domid_t *backend_domid)
>>   {
>> -	struct of_phandle_args iommu_spec;
>> +	struct of_phandle_args iommu_spec = { .args_count = 1 };
>>   
>> -	if (of_parse_phandle_with_args(dev->of_node, "iommus", "#iommu-cells",
>> -			0, &iommu_spec)) {
>> -		dev_err(dev, "Cannot parse iommus property\n");
>> -		return -ESRCH;
>> +	if (dev_is_pci(dev)) {
>> +		struct pci_dev *pdev = to_pci_dev(dev);
>> +		u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>> +
>> +		if (of_map_id(np, rid, "iommu-map", "iommu-map-mask", &iommu_spec.np,
>> +				iommu_spec.args)) {
>> +			dev_err(dev, "Cannot translate ID\n");
>> +			return -ESRCH;
>> +		}
>> +	} else {
>> +		if (of_parse_phandle_with_args(np, "iommus", "#iommu-cells",
>> +				0, &iommu_spec)) {
>> +			dev_err(dev, "Cannot parse iommus property\n");
>> +			return -ESRCH;
>> +		}
>>   	}
>>   
>>   	if (!of_device_is_compatible(iommu_spec.np, "xen,grant-dma") ||
>> @@ -346,7 +392,7 @@ static int xen_dt_grant_init_backend_domid(struct device *dev,
>>   	 * The endpoint ID here means the ID of the domain where the
>>   	 * corresponding backend is running
>>   	 */
>> -	data->backend_domid = iommu_spec.args[0];
>> +	*backend_domid = iommu_spec.args[0];
>>   
>>   	return 0;
>>   }
>> @@ -354,6 +400,7 @@ static int xen_dt_grant_init_backend_domid(struct device *dev,
>>   void xen_grant_setup_dma_ops(struct device *dev)
>>   {
>>   	struct xen_grant_dma_data *data;
>> +	struct device_node *np;
>>   
>>   	data = find_xen_grant_dma_data(dev);
>>   	if (data) {
>> @@ -365,8 +412,13 @@ void xen_grant_setup_dma_ops(struct device *dev)
>>   	if (!data)
>>   		goto err;
>>   
>> -	if (dev->of_node) {
>> -		if (xen_dt_grant_init_backend_domid(dev, data))
>> +	np = xen_dt_get_node(dev);
>> +	if (np) {
>> +		int ret;
>> +
>> +		ret = xen_dt_grant_init_backend_domid(dev, np, &data->backend_domid);
>> +		of_node_put(np);
>> +		if (ret)
>>   			goto err;
>>   	} else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT)) {
>>   		dev_info(dev, "Using dom0 as backend\n");
>> -- 
>> 2.25.1
>>
-- 
Regards,

Oleksandr Tyshchenko
Oleksandr Tyshchenko Oct. 25, 2022, 4:25 p.m. UTC | #3 
On 22.10.22 09:44, Oleksandr wrote:

Hello Stefano.

>
> On 21.10.22 23:08, Stefano Stabellini wrote:
>
> Hello Stefano
>
>> On Fri, 21 Oct 2022, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> Use the same "xen-grant-dma" device concept for the PCI devices
>>> behind device-tree based PCI Host controller, but with one 
>>> modification.
>>> Unlike for platform devices, we cannot use generic IOMMU bindings
>>> (iommus property), as we need to support more flexible configuration.
>>> The problem is that PCI devices under the single PCI Host controller
>>> may have the backends running in different Xen domains and thus have
>>> different endpoints ID (backend domains ID).
>>>
>>> Add ability to deal with generic PCI-IOMMU bindings (iommu-map/
>>> iommu-map-mask properties) which allows us to describe relationship
>>> between PCI devices and backend domains ID properly.
>>>
>>> To avoid having to look up for the PCI Host bridge twice and reduce
>>> the amount of checks pass an extra struct device_node *np to both
>>> xen_dt_grant_init_backend_domid() and xen_is_dt_grant_dma_device().
>>> While at it also pass domid_t *backend_domid instead of
>>> struct xen_grant_dma_data *data to the former.
>>>
>>> So with current patch the code expects iommus property for the platform
>>> devices and iommu-map/iommu-map-mask properties for PCI devices.
>>>
>>> The example of generated by the toolstack iommu-map property
>>> for two PCI devices 0000:00:01.0 and 0000:00:02.0 whose
>>> backends are running in different Xen domains with IDs 1 and 2
>>> respectively:
>>> iommu-map = <0x08 0xfde9 0x01 0x08 0x10 0xfde9 0x02 0x08>;
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> ---
>>> Slightly RFC. This is needed to support Xen grant mappings for 
>>> virtio-pci devices
>>> on Arm at some point in the future. The Xen toolstack side is not 
>>> completely ready yet.
>>> Here, for PCI devices we use more flexible way to pass backend domid 
>>> to the guest
>>> than for platform devices.
>>>
>>> Changes V1 -> V2:
>>>     - update commit description
>>>     - rebase
>>>     - rework to use generic PCI-IOMMU bindings instead of generic 
>>> IOMMU bindings
>>>
>>> Changes V2 -> V3:
>>>     - update commit description, add an example
>>>     - drop xen_dt_map_id() and squash xen_dt_get_pci_host_node() with
>>>       xen_dt_get_node()
>>>     - pass struct device_node *np to xen_is_dt_grant_dma_device() and
>>>       xen_dt_grant_init_backend_domid()
>>>     - pass domid_t *backend_domid instead of struct 
>>> xen_grant_dma_data *data
>>>       to xen_dt_grant_init_backend_domid()
>>>
>>> Previous discussion is at:
>>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/20221006174804.2003029-1-olekstysh@gmail.com/__;!!GF_29dbcQIUBPA!3cZiRy0Scq8-dibrxyFGUlAIhwa7UwRmrCAG-qdvkAG5NInYPZ_mbLTMtsZ_F4Gonowkettr-dcRO3TAs_gn-1xcLk77xg$ 
>>> [lore[.]kernel[.]org]
>>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/20221015153409.918775-1-olekstysh@gmail.com/__;!!GF_29dbcQIUBPA!3cZiRy0Scq8-dibrxyFGUlAIhwa7UwRmrCAG-qdvkAG5NInYPZ_mbLTMtsZ_F4Gonowkettr-dcRO3TAs_gn-1xwRjX9GQ$ 
>>> [lore[.]kernel[.]org]
>>>
>>> Based on:
>>> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git/log/?h=for-linus-6.1__;!!GF_29dbcQIUBPA!3cZiRy0Scq8-dibrxyFGUlAIhwa7UwRmrCAG-qdvkAG5NInYPZ_mbLTMtsZ_F4Gonowkettr-dcRO3TAs_gn-1zVOy8WKg$ 
>>> [git[.]kernel[.]org]
>>> ---
>>>   drivers/xen/grant-dma-ops.c | 80 
>>> ++++++++++++++++++++++++++++++-------
>>>   1 file changed, 66 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>>> index daa525df7bdc..76b29d20aeee 100644
>>> --- a/drivers/xen/grant-dma-ops.c
>>> +++ b/drivers/xen/grant-dma-ops.c
>>> @@ -10,6 +10,7 @@
>>>   #include <linux/module.h>
>>>   #include <linux/dma-map-ops.h>
>>>   #include <linux/of.h>
>>> +#include <linux/pci.h>
>>>   #include <linux/pfn.h>
>>>   #include <linux/xarray.h>
>>>   #include <linux/virtio_anchor.h>
>>> @@ -292,12 +293,37 @@ static const struct dma_map_ops 
>>> xen_grant_dma_ops = {
>>>       .dma_supported = xen_grant_dma_supported,
>>>   };
>>>   -static bool xen_is_dt_grant_dma_device(struct device *dev)
>>> +static struct device_node *xen_dt_get_node(struct device *dev)
>>>   {
>>> -    struct device_node *iommu_np;
>>> +    if (dev_is_pci(dev)) {
>>> +        struct pci_dev *pdev = to_pci_dev(dev);
>>> +        struct pci_bus *bus = pdev->bus;
>>> +
>>> +        /* Walk up to the root bus to look for PCI Host controller */
>>> +        while (!pci_is_root_bus(bus))
>>> +            bus = bus->parent;
>>> +
>>> +        return of_node_get(bus->bridge->parent->of_node);
>>> +    }
>>> +
>>> +    return of_node_get(dev->of_node);
>>> +}
>>> +
>>> +static bool xen_is_dt_grant_dma_device(struct device *dev,
>>> +                    struct device_node *np)
>>> +{
>>> +    struct device_node *iommu_np = NULL;
>>>       bool has_iommu;
>>>   -    iommu_np = of_parse_phandle(dev->of_node, "iommus", 0);
>>> +    if (dev_is_pci(dev)) {
>>> +        struct pci_dev *pdev = to_pci_dev(dev);
>>> +        u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>>> +
>>> +        if (of_map_id(np, rid, "iommu-map", "iommu-map-mask", 
>>> &iommu_np, NULL))
>>> +            return false;
>>> +    } else
>>> +        iommu_np = of_parse_phandle(np, "iommus", 0);
>>> +
>>>       has_iommu = iommu_np &&
>>>               of_device_is_compatible(iommu_np, "xen,grant-dma");
>>>       of_node_put(iommu_np);
>> I think we can remove xen_is_dt_grant_dma_device and just call
>> xen_dt_grant_init_backend_domid passing a NULL backend_domid?
>>
>> It is a bit annoying that we are basically doing the same device tree
>> parsing twice in a row given that the callers do:
>>
>>     if (xen_is_grant_dma_device(dev))
>>         xen_grant_setup_dma_ops(dev);
>>
>> Maybe we could move the backend_domid allocation and setting to
>> xen_dt_grant_init_backend_domid, which would end up being done from
>> the xen_is_grant_dma_device() call chain, and only leave setting
>> dev->dma_ops from xen_grant_setup_dma_ops().
>>
>> This way the parsing would be done only once? What do you think? This
>> suggestion is optional, I am OK also with only removing
>> xen_is_dt_grant_dma_device.
>
> ok, I will think about such optimization and will provide my thoughts 
> about it or a patch. 

I have thought about it. You was right, it is possible to optimize 
things. Please take a look at:

https://lore.kernel.org/xen-devel/20221025162004.8501-1-olekstysh@gmail.com/





> But, I think it should be a separate patch (if needs be),
>
> as in current patch we don't change anything in that regard, we just 
> implement
>
> "if (dev_is_pci(dev)) { ... }" condition for 
> xen_is_dt_grant_dma_device() and xen_dt_grant_init_backend_domid().
>
>
>
>>
>>
>>> @@ -307,9 +333,17 @@ static bool xen_is_dt_grant_dma_device(struct 
>>> device *dev)
>>>     bool xen_is_grant_dma_device(struct device *dev)
>>>   {
>>> +    struct device_node *np;
>>> +
>>>       /* XXX Handle only DT devices for now */
>>> -    if (dev->of_node)
>>> -        return xen_is_dt_grant_dma_device(dev);
>>> +    np = xen_dt_get_node(dev);
>>> +    if (np) {
>>> +        bool ret;
>>> +
>>> +        ret = xen_is_dt_grant_dma_device(dev, np);
>>> +        of_node_put(np);
>>> +        return ret;
>>> +    }
>>>         return false;
>>>   }
>>> @@ -323,14 +357,26 @@ bool xen_virtio_mem_acc(struct virtio_device 
>>> *dev)
>>>   }
>>>     static int xen_dt_grant_init_backend_domid(struct device *dev,
>>> -                       struct xen_grant_dma_data *data)
>>> +                       struct device_node *np,
>>> +                       domid_t *backend_domid)
>>>   {
>>> -    struct of_phandle_args iommu_spec;
>>> +    struct of_phandle_args iommu_spec = { .args_count = 1 };
>>>   -    if (of_parse_phandle_with_args(dev->of_node, "iommus", 
>>> "#iommu-cells",
>>> -            0, &iommu_spec)) {
>>> -        dev_err(dev, "Cannot parse iommus property\n");
>>> -        return -ESRCH;
>>> +    if (dev_is_pci(dev)) {
>>> +        struct pci_dev *pdev = to_pci_dev(dev);
>>> +        u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>>> +
>>> +        if (of_map_id(np, rid, "iommu-map", "iommu-map-mask", 
>>> &iommu_spec.np,
>>> +                iommu_spec.args)) {
>>> +            dev_err(dev, "Cannot translate ID\n");
>>> +            return -ESRCH;
>>> +        }
>>> +    } else {
>>> +        if (of_parse_phandle_with_args(np, "iommus", "#iommu-cells",
>>> +                0, &iommu_spec)) {
>>> +            dev_err(dev, "Cannot parse iommus property\n");
>>> +            return -ESRCH;
>>> +        }
>>>       }
>>>         if (!of_device_is_compatible(iommu_spec.np, "xen,grant-dma") ||
>>> @@ -346,7 +392,7 @@ static int 
>>> xen_dt_grant_init_backend_domid(struct device *dev,
>>>        * The endpoint ID here means the ID of the domain where the
>>>        * corresponding backend is running
>>>        */
>>> -    data->backend_domid = iommu_spec.args[0];
>>> +    *backend_domid = iommu_spec.args[0];
>>>         return 0;
>>>   }
>>> @@ -354,6 +400,7 @@ static int 
>>> xen_dt_grant_init_backend_domid(struct device *dev,
>>>   void xen_grant_setup_dma_ops(struct device *dev)
>>>   {
>>>       struct xen_grant_dma_data *data;
>>> +    struct device_node *np;
>>>         data = find_xen_grant_dma_data(dev);
>>>       if (data) {
>>> @@ -365,8 +412,13 @@ void xen_grant_setup_dma_ops(struct device *dev)
>>>       if (!data)
>>>           goto err;
>>>   -    if (dev->of_node) {
>>> -        if (xen_dt_grant_init_backend_domid(dev, data))
>>> +    np = xen_dt_get_node(dev);
>>> +    if (np) {
>>> +        int ret;
>>> +
>>> +        ret = xen_dt_grant_init_backend_domid(dev, np, 
>>> &data->backend_domid);
>>> +        of_node_put(np);
>>> +        if (ret)
>>>               goto err;
>>>       } else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT)) {
>>>           dev_info(dev, "Using dom0 as backend\n");
>>> -- 
>>> 2.25.1
>>>
-- 
Regards,

Oleksandr Tyshchenko
diff mbox series

Patch

diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
index daa525df7bdc..76b29d20aeee 100644
--- a/drivers/xen/grant-dma-ops.c
+++ b/drivers/xen/grant-dma-ops.c
@@ -10,6 +10,7 @@ 
 #include <linux/module.h>
 #include <linux/dma-map-ops.h>
 #include <linux/of.h>
+#include <linux/pci.h>
 #include <linux/pfn.h>
 #include <linux/xarray.h>
 #include <linux/virtio_anchor.h>
@@ -292,12 +293,37 @@  static const struct dma_map_ops xen_grant_dma_ops = {
 	.dma_supported = xen_grant_dma_supported,
 };
 
-static bool xen_is_dt_grant_dma_device(struct device *dev)
+static struct device_node *xen_dt_get_node(struct device *dev)
 {
-	struct device_node *iommu_np;
+	if (dev_is_pci(dev)) {
+		struct pci_dev *pdev = to_pci_dev(dev);
+		struct pci_bus *bus = pdev->bus;
+
+		/* Walk up to the root bus to look for PCI Host controller */
+		while (!pci_is_root_bus(bus))
+			bus = bus->parent;
+
+		return of_node_get(bus->bridge->parent->of_node);
+	}
+
+	return of_node_get(dev->of_node);
+}
+
+static bool xen_is_dt_grant_dma_device(struct device *dev,
+					struct device_node *np)
+{
+	struct device_node *iommu_np = NULL;
 	bool has_iommu;
 
-	iommu_np = of_parse_phandle(dev->of_node, "iommus", 0);
+	if (dev_is_pci(dev)) {
+		struct pci_dev *pdev = to_pci_dev(dev);
+		u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
+
+		if (of_map_id(np, rid, "iommu-map", "iommu-map-mask", &iommu_np, NULL))
+			return false;
+	} else
+		iommu_np = of_parse_phandle(np, "iommus", 0);
+
 	has_iommu = iommu_np &&
 		    of_device_is_compatible(iommu_np, "xen,grant-dma");
 	of_node_put(iommu_np);
@@ -307,9 +333,17 @@  static bool xen_is_dt_grant_dma_device(struct device *dev)
 
 bool xen_is_grant_dma_device(struct device *dev)
 {
+	struct device_node *np;
+
 	/* XXX Handle only DT devices for now */
-	if (dev->of_node)
-		return xen_is_dt_grant_dma_device(dev);
+	np = xen_dt_get_node(dev);
+	if (np) {
+		bool ret;
+
+		ret = xen_is_dt_grant_dma_device(dev, np);
+		of_node_put(np);
+		return ret;
+	}
 
 	return false;
 }
@@ -323,14 +357,26 @@  bool xen_virtio_mem_acc(struct virtio_device *dev)
 }
 
 static int xen_dt_grant_init_backend_domid(struct device *dev,
-					   struct xen_grant_dma_data *data)
+					   struct device_node *np,
+					   domid_t *backend_domid)
 {
-	struct of_phandle_args iommu_spec;
+	struct of_phandle_args iommu_spec = { .args_count = 1 };
 
-	if (of_parse_phandle_with_args(dev->of_node, "iommus", "#iommu-cells",
-			0, &iommu_spec)) {
-		dev_err(dev, "Cannot parse iommus property\n");
-		return -ESRCH;
+	if (dev_is_pci(dev)) {
+		struct pci_dev *pdev = to_pci_dev(dev);
+		u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
+
+		if (of_map_id(np, rid, "iommu-map", "iommu-map-mask", &iommu_spec.np,
+				iommu_spec.args)) {
+			dev_err(dev, "Cannot translate ID\n");
+			return -ESRCH;
+		}
+	} else {
+		if (of_parse_phandle_with_args(np, "iommus", "#iommu-cells",
+				0, &iommu_spec)) {
+			dev_err(dev, "Cannot parse iommus property\n");
+			return -ESRCH;
+		}
 	}
 
 	if (!of_device_is_compatible(iommu_spec.np, "xen,grant-dma") ||
@@ -346,7 +392,7 @@  static int xen_dt_grant_init_backend_domid(struct device *dev,
 	 * The endpoint ID here means the ID of the domain where the
 	 * corresponding backend is running
 	 */
-	data->backend_domid = iommu_spec.args[0];
+	*backend_domid = iommu_spec.args[0];
 
 	return 0;
 }
@@ -354,6 +400,7 @@  static int xen_dt_grant_init_backend_domid(struct device *dev,
 void xen_grant_setup_dma_ops(struct device *dev)
 {
 	struct xen_grant_dma_data *data;
+	struct device_node *np;
 
 	data = find_xen_grant_dma_data(dev);
 	if (data) {
@@ -365,8 +412,13 @@  void xen_grant_setup_dma_ops(struct device *dev)
 	if (!data)
 		goto err;
 
-	if (dev->of_node) {
-		if (xen_dt_grant_init_backend_domid(dev, data))
+	np = xen_dt_get_node(dev);
+	if (np) {
+		int ret;
+
+		ret = xen_dt_grant_init_backend_domid(dev, np, &data->backend_domid);
+		of_node_put(np);
+		if (ret)
 			goto err;
 	} else if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT)) {
 		dev_info(dev, "Using dom0 as backend\n");