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

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>]

On 19.10.22 03:58, Stefano Stabellini wrote:

Hello Stefano

> On Sat, 15 Oct 2022, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>
>> 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).
>>
>> So use generic PCI-IOMMU bindings instead (iommu-map/iommu-map-mask
>> properties) which allows us to describe relationship between PCI
>> devices and backend domains ID properly.
>>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> Now that I understood the approach and the reasons for it, I can review
> the patch :-)

perfect, thanks.


>
> Please add an example of the bindings in the commit message.

ok, will do


>
>
>> ---
>> 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
>>
>> Previous discussion is at:
>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/20221006174804.2003029-1-olekstysh@xxxxxxxxx/__;!!GF_29dbcQIUBPA!xJPdZO3-3Wmgo_79HuDsD53RkH_eAN96NmwuwFE7dArt_xNYGdD6LeLsq4B_QPrrvX-x23tJl6jQlNqgyNjgzT2NE3Pqjg$
>>   [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!xJPdZO3-3Wmgo_79HuDsD53RkH_eAN96NmwuwFE7dArt_xNYGdD6LeLsq4B_QPrrvX-x23tJl6jQlNqgyNjgzT2J40LOxg$
>>   [git[.]kernel[.]org]
>> ---
>>   drivers/xen/grant-dma-ops.c | 87 ++++++++++++++++++++++++++++++++-----
>>   1 file changed, 76 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
>> index daa525df7bdc..b79d9d6ce154 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,55 @@ static const struct dma_map_ops xen_grant_dma_ops = {
>>      .dma_supported = xen_grant_dma_supported,
>>   };
>>   
>> +static struct device_node *xen_dt_get_pci_host_node(struct device *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);
>> +}
> It seems silly that we need to walk the hierachy that way, but I
> couldn't find another way to do it

I also couldn't, but is it a really problem? This code is only gets 
called during initialization.


>
>
>> +static struct device_node *xen_dt_get_node(struct device *dev)
>> +{
>> +    if (dev_is_pci(dev))
>> +            return xen_dt_get_pci_host_node(dev);
>> +
>> +    return of_node_get(dev->of_node);
>> +}
>> +
>> +static int xen_dt_map_id(struct device *dev, struct device_node **iommu_np,
>> +                     u32 *sid)
>> +{
>> +    struct pci_dev *pdev = to_pci_dev(dev);
>> +    u32 rid = PCI_DEVID(pdev->bus->number, pdev->devfn);
>> +    struct device_node *host_np;
>> +    int ret;
>> +
>> +    host_np = xen_dt_get_pci_host_node(dev);
>> +    if (!host_np)
>> +            return -ENODEV;
>> +
>> +    ret = of_map_id(host_np, rid, "iommu-map", "iommu-map-mask", iommu_np, 
>> sid);
>> +    of_node_put(host_np);
>> +    return ret;
>> +}
>> +
>>   static bool xen_is_dt_grant_dma_device(struct device *dev)
>>   {
>> -    struct device_node *iommu_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)) {
>> +            if (xen_dt_map_id(dev, &iommu_np, NULL))
>> +                    return false;
>> +    } else
>> +            iommu_np = of_parse_phandle(dev->of_node, "iommus", 0);
>> +
>>      has_iommu = iommu_np &&
>>                  of_device_is_compatible(iommu_np, "xen,grant-dma");
>>      of_node_put(iommu_np);
>> @@ -307,9 +351,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);
>> +            of_node_put(np);
>> +            return ret;
>> +    }
> We don't need to walk the PCI hierachy twice. Maybe we can add the
> of_node check directly to xen_is_dt_grant_dma_device?


Good point. I was thinking that we would likely need the following 
construct is the future:


if (np) /* DT device */
    return xen_is_dt_grant_dma_device(dev);
else /* ACPI device */
    return xen_is_acpi_grant_dma_device(dev);


So, if we use the check directly in xen_is_dt_grant_dma_device() and it 
returns false we won't be able to recognize a reason then
(whether dev is not related to DT, or it is related to DT but it is not 
a "xen,grant-dma" device).
But, I am ok to eliminate one walk right now, then we will see.

xen_is_grant_dma_device() will became the following:

bool xen_is_grant_dma_device(struct device *dev)
{
     return xen_is_dt_grant_dma_device(dev);
}

xen_is_dt_grant_dma_device() will need to gain a check that dev->of_node 
is not a NULL.


Shall I?



>
>
>>      return false;
>>   }
>> @@ -325,12 +377,19 @@ 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 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)) {
>> +            if (xen_dt_map_id(dev, &iommu_spec.np, iommu_spec.args)) {
>> +                    dev_err(dev, "Cannot translate ID\n");
>> +                    return -ESRCH;
>> +            }
>> +    } else {
>> +            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 (!of_device_is_compatible(iommu_spec.np, "xen,grant-dma") ||
>> @@ -354,6 +413,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 +425,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, data);
>> +            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