Re: [PATCH 1/3] xen/virtio: restructure xen grant dma setup

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>]

On 06.10.22 10:14, Juergen Gross wrote:

Hello Juergen

> In order to prepare supporting other means than device tree for
> setting up virtio devices under Xen, restructure the functions
> xen_is_grant_dma_device() and xen_grant_setup_dma_ops() a little bit.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>


Patch looks good,

one NIT, xen_dt_grant_setup_dma_ops() down the code doesn't actually 
setup DMA OPS, it retrieves the backend domid via device-tree means and 
stores it,

so I would rename to it, maybe something like 
xen_dt_grant_setup_backend_domid() or xen_dt_grant_init_backend_domid(), 
but I am not sure it would be good alternative.


So, w/ or w/o renaming:

Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

also

Tested-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx> # Arm64 
only


> ---
>   drivers/xen/grant-dma-ops.c | 68 +++++++++++++++++++++++--------------
>   1 file changed, 43 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> index 8973fc1e9ccc..f29759d5301f 100644
> --- a/drivers/xen/grant-dma-ops.c
> +++ b/drivers/xen/grant-dma-ops.c
> @@ -273,22 +273,28 @@ static const struct dma_map_ops xen_grant_dma_ops = {
>       .dma_supported = xen_grant_dma_supported,
>   };
>   
> -bool xen_is_grant_dma_device(struct device *dev)
> +static bool xen_is_dt_grant_dma_device(struct device *dev)
>   {
>       struct device_node *iommu_np;
>       bool has_iommu;
>   
> -     /* XXX Handle only DT devices for now */
> -     if (!dev->of_node)
> -             return false;
> -
>       iommu_np = of_parse_phandle(dev->of_node, "iommus", 0);
> -     has_iommu = iommu_np && of_device_is_compatible(iommu_np, 
> "xen,grant-dma");
> +     has_iommu = iommu_np &&
> +                 of_device_is_compatible(iommu_np, "xen,grant-dma");
>       of_node_put(iommu_np);
>   
>       return has_iommu;
>   }
>   
> +bool xen_is_grant_dma_device(struct device *dev)
> +{
> +     /* XXX Handle only DT devices for now */
> +     if (dev->of_node)
> +             return xen_is_dt_grant_dma_device(dev);
> +
> +     return false;
> +}
> +
>   bool xen_virtio_mem_acc(struct virtio_device *dev)
>   {
>       if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT))
> @@ -297,45 +303,56 @@ bool xen_virtio_mem_acc(struct virtio_device *dev)
>       return xen_is_grant_dma_device(dev->dev.parent);
>   }
>   
> -void xen_grant_setup_dma_ops(struct device *dev)
> +static int xen_dt_grant_setup_dma_ops(struct device *dev,
> +                                    struct xen_grant_dma_data *data)
>   {
> -     struct xen_grant_dma_data *data;
>       struct of_phandle_args iommu_spec;
>   
> -     data = find_xen_grant_dma_data(dev);
> -     if (data) {
> -             dev_err(dev, "Xen grant DMA data is already created\n");
> -             return;
> -     }
> -
> -     /* XXX ACPI device unsupported for now */
> -     if (!dev->of_node)
> -             goto err;
> -
>       if (of_parse_phandle_with_args(dev->of_node, "iommus", "#iommu-cells",
>                       0, &iommu_spec)) {
>               dev_err(dev, "Cannot parse iommus property\n");
> -             goto err;
> +             return -ESRCH;
>       }
>   
>       if (!of_device_is_compatible(iommu_spec.np, "xen,grant-dma") ||
>                       iommu_spec.args_count != 1) {
>               dev_err(dev, "Incompatible IOMMU node\n");
>               of_node_put(iommu_spec.np);
> -             goto err;
> +             return -ESRCH;
>       }
>   
>       of_node_put(iommu_spec.np);
>   
> +     /*
> +      * The endpoint ID here means the ID of the domain where the
> +      * corresponding backend is running
> +      */
> +     data->backend_domid = iommu_spec.args[0];
> +
> +     return 0;
> +}
> +
> +void xen_grant_setup_dma_ops(struct device *dev)
> +{
> +     struct xen_grant_dma_data *data;
> +
> +     data = find_xen_grant_dma_data(dev);
> +     if (data) {
> +             dev_err(dev, "Xen grant DMA data is already created\n");
> +             return;
> +     }
> +
>       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>       if (!data)
>               goto err;
>   
> -     /*
> -      * The endpoint ID here means the ID of the domain where the 
> corresponding
> -      * backend is running
> -      */
> -     data->backend_domid = iommu_spec.args[0];
> +     if (dev->of_node) {
> +             if (xen_dt_grant_setup_dma_ops(dev, data))
> +                     goto err;
> +     } else {
> +             /* XXX ACPI device unsupported for now */
> +             goto err;
> +     }
>   
>       if (xa_err(xa_store(&xen_grant_dma_devices, (unsigned long)dev, data,
>                       GFP_KERNEL))) {
> @@ -348,6 +365,7 @@ void xen_grant_setup_dma_ops(struct device *dev)
>       return;
>   
>   err:
> +     devm_kfree(dev, data);
>       dev_err(dev, "Cannot set up Xen grant DMA ops, retain platform DMA 
> ops\n");
>   }
>   

-- 
Regards,

Oleksandr Tyshchenko