lkml.org 
[lkml]   [2022]   [Oct]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V2] xen/virtio: Handle PCI devices which Host controller is described in DT
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://lore.kernel.org/xen-devel/20221006174804.2003029-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 | 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?


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

\
 
 \ /
  Last update: 2022-10-19 02:59    [W:0.112 / U:1.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site