Messages in this thread Patch in this message | | | Date | Thu, 20 Oct 2022 11:24:06 +0300 | Subject | Re: [PATCH V2] xen/virtio: Handle PCI devices which Host controller is described in DT | From | Xenia Ragiadakou <> |
| |
On 10/19/22 22:41, Oleksandr Tyshchenko wrote:
Hi Oleksandr
> > On 19.10.22 11:47, Xenia Ragiadakou wrote: > > Hello Xenia > >> On 10/19/22 03:58, Stefano Stabellini wrote: >>> On Sat, 15 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). >>>> >>>> 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@epam.com> >>> >>> Now that I understood the approach and the reasons for it, I can review >>> the patch :-) >>> >>> Please add an example of the bindings in the commit message. >>> >>> >>>> --- >>>> 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@gmail.com/__;!!GF_29dbcQIUBPA!3-vq7Edm3XfKtD5cnNjnOzDQvuo_XrhJ73yH-nPfqOkGGU0IjLG7R7MR_nAJCAPeOutHRLT44wKYwQwz3SauACie_ZAy$ >>>> [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!3-vq7Edm3XfKtD5cnNjnOzDQvuo_XrhJ73yH-nPfqOkGGU0IjLG7R7MR_nAJCAPeOutHRLT44wKYwQwz3SauAEnMDHAq$ >>>> [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 >>> >>> >>>> +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? >>> >> >> I think in general we could pass directly the host bridge device if >> dev_is_pci(dev) (which can be retrieved with >> pci_get_host_bridge_device(to_pci_dev(dev), and after done with it >> pci_put_host_bridge_device(phb)). >> So that, xen_is_dt_grant_dma_device() and >> xen_dt_grant_init_backend_domid() won't need to discover it themselves. >> This will simplify the code. > > > Good point. I have some remark. Can we use pci_find_host_bridge() > instead? This way we don't have to add #include "../pci/pci.h", and have > to drop reference afterwards. > > With that xen_dt_get_pci_host_node() will became the following: > > > static struct device_node *xen_dt_get_pci_host_node(struct device *dev) > { > struct pci_host_bridge *bridge = > pci_find_host_bridge(to_pci_dev(dev)->bus); > > return of_node_get(bridge->dev.parent->of_node); > } >
You are right. I prefer your version instead of the above.
> > With Stefano's suggestion, we won't walk the PCI hierarchy twice when > executing xen_is_grant_dma_device() for PCI device: > > xen_is_grant_dma_device() -> xen_is_dt_grant_dma_device() -> > xen_dt_map_id() -> xen_dt_get_pci_host_node() > > > What do you think? >
I was thinking passing the device_node along with the device in the function arguments. More specifically, of doing this (not tested, just an idea):
bool xen_is_grant_dma_device(struct device *dev) { struct device_node *np; bool has_iommu = false;
/* XXX Handle only DT devices for now */ np = xen_dt_get_node(dev); if (np) has_iommu = xen_is_dt_grant_dma_device(dev, np); of_node_put(np); return has_iommu; }
static bool xen_is_dt_grant_dma_device(struct device *dev, struct device_node *np) { struct device_node *iommu_np = NULL; bool has_iommu;
if (dev_is_pci(dev)) { struct pci_dev *pdev = to_pci_dev(dev); u32 id = PCI_DEVID(pdev->bus->number, pdev->devfn); of_map_id(np, id, "iommu-map", "iommu-map-mask", &iommu_np, NULL); } 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);
return has_iommu; }
I 'm wondering ... is it possible for the host bridge device node to have the iommus property set? meaning that all of its pci devs will have the same backend?
>> >>> >>>> 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 >>>> >>> >>
-- Xenia
| |