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

[Oleksandr Tyshchenko]

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