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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 19 Oct 2022, Oleksandr Tyshchenko wrote:
> On 19.10.22 03:58, Stefano Stabellini wrote:
> > 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).

That problem can easily be solved by having xen_is_dt_grant_dma_device
return more than 2 possible values. It could return an int for example,
with 3 valid values.


> 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?