Re: [Xen-devel] [PATCH 5/7] xen/iommu: smmu-v3: Add Xen specific code to enable the ported driver

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 09/02/18 03:10, Sameer Goel wrote:
> This driver follows an approach similar to smmu driver. The intent here
> is to reuse as much Linux code as possible.
> - Glue code has been introduced to bridge the API calls.
> - Called Linux functions from the Xen IOMMU function calls.
> - Xen modifications are preceded by /*Xen: comment */
> - xen/linux_compat: Add a Linux compat header
>    For porting files directly from Linux it is useful to have a function mapping
>    definitions from Linux to Xen. This file adds common API functions and
>    other defines that are needed for porting arm SMMU drivers.
I understand Roger asked for it, but that was not a really wise choice 
given the size of this patch. Anyway, let's keep it like that.
> Signed-off-by: Sameer Goel <sameer.goel@xxxxxxxxxx>
> ---
>   xen/arch/arm/p2m.c                    |   1 +
>   xen/drivers/Kconfig                   |   2 +
>   xen/drivers/passthrough/arm/Kconfig   |   8 +
>   xen/drivers/passthrough/arm/Makefile  |   1 +
>   xen/drivers/passthrough/arm/smmu-v3.c | 892 ++++++++++++++++++++++++++++++++--
>   xen/include/xen/linux_compat.h        |  84 ++++
You need to CC the REST maintainers for that.

>   6 files changed, 959 insertions(+), 29 deletions(-)
>   create mode 100644 xen/drivers/passthrough/arm/Kconfig
>   create mode 100644 xen/include/xen/linux_compat.h
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 65e8b9c6ea..fef7605fd6 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1460,6 +1460,7 @@ err:
>   static void __init setup_virt_paging_one(void *data)
>   {
>       unsigned long val = (unsigned long)data;
> +    /* SMMUv3 S2 cfg vtcr reuses the following value */
>       WRITE_SYSREG32(val, VTCR_EL2);
>       isb();
>   }
> diff --git a/xen/drivers/Kconfig b/xen/drivers/Kconfig
> index bc3a54f0ea..612655386d 100644
> --- a/xen/drivers/Kconfig
> +++ b/xen/drivers/Kconfig
> @@ -12,4 +12,6 @@ source "drivers/pci/Kconfig"
>   
>   source "drivers/video/Kconfig"
>   
> +source "drivers/passthrough/arm/Kconfig"
> +
>   endmenu
> diff --git a/xen/drivers/passthrough/arm/Kconfig 
> b/xen/drivers/passthrough/arm/Kconfig
> new file mode 100644
> index 0000000000..cda899f608
> --- /dev/null
> +++ b/xen/drivers/passthrough/arm/Kconfig
> @@ -0,0 +1,8 @@
> +
> +config ARM_SMMU_v3
> +       bool "ARM SMMUv3 Support"
> +       depends on ARM_64
Why the dependency on Arm64 here?

> +       help
> +        Support for implementations of the ARM System MMU architecture
> +        version 3.
> +
> diff --git a/xen/drivers/passthrough/arm/Makefile 
> b/xen/drivers/passthrough/arm/Makefile
> index f4cd26e15d..e14732b55c 100644
> --- a/xen/drivers/passthrough/arm/Makefile
> +++ b/xen/drivers/passthrough/arm/Makefile
> @@ -1,2 +1,3 @@
>   obj-y += iommu.o
>   obj-y += smmu.o
> +obj-$(CONFIG_ARM_SMMU_v3) += smmu-v3.o
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> b/xen/drivers/passthrough/arm/smmu-v3.c
> index e67ba6c40f..f43485fe6e 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -18,28 +18,414 @@
>    * Author: Will Deacon <will.deacon@xxxxxxx>
>    *
>    * This driver is powered by bad coffee and bombay mix.
> + *
> + *
> + * Based on Linux drivers/iommu/arm-smmu-v3.c
> + * => commit 7aa8619a66aea52b145e04cbab4f8d6a4e5f3f3b
> + *
> + * Xen modifications:
> + * Sameer Goel <sameer.goel@xxxxxxxxxx>
> + * Copyright (C) 2017, The Linux Foundation, All rights reserved.
> + *
> + */
[...]

> +static void *dmam_alloc_coherent(struct device *dev, size_t size,
> +                          dma_addr_t *dma_handle, gfp_t gfp)
> +{
> +       void *vaddr;
> +       unsigned long alignment = size;
> +
> +       /*
> +        * _xzalloc requires that the (align & (align -1)) = 0. Most of the
> +        * allocations in SMMU code should send the right value for size. In
> +        * case this is not true print a warning and align to the size of a
> +        * (void *)
> +        */
> +       if (size & (size - 1)) {
> +               dev_warn(dev, "Fixing alignment for the DMA buffer\n");
> +               alignment = sizeof(void *);
> +       }
> +
> +       vaddr = _xzalloc(size, alignment);
> +       if (!vaddr) {
> +               dev_err(dev, "DMA allocation failed\n");
> +               return NULL;
> +       }
> +
> +       *dma_handle = virt_to_maddr(vaddr);
> +
> +       return vaddr;
> +}
> +
> +
One newline should be enough.

> +static void dmam_free_coherent(struct device *dev, size_t size, void *vaddr,
> +                       dma_addr_t dma_handle)
> +{
> +       xfree(vaddr);
> +}
> +
> +/* Xen: Stub out DMA domain related functions */
> +#define iommu_get_dma_cookie(dom) 0
> +#define iommu_put_dma_cookie(dom)
> +
> +/* Xen: Stub out module param related function */
> +#define module_param_named(a, b, c, d)
> +#define MODULE_PARM_DESC(a, b)
> +
> +#define dma_set_mask_and_coherent(d, b) 0
> +
> +#define of_dma_is_coherent(n) 0
> +
> +#define MODULE_DEVICE_TABLE(type, name)
> +
> +static void __iomem *devm_ioremap_resource(struct device *dev,
> +                                          struct resource *res)
> +{
> +       void __iomem *ptr;
> +
> +       if (!res || res->type != IORESOURCE_MEM) {
> +               dev_err(dev, "Invalid resource\n");
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       ptr = ioremap_nocache(res->addr, res->size);
> +       if (!ptr) {
> +               dev_err(dev,
> +                       "ioremap failed (addr 0x%"PRIx64" size 0x%"PRIx64")\n",
> +                       res->addr, res->size);
> +               return ERR_PTR(-ENOMEM);
> +       }
> +
> +       return ptr;
> +}
> +
> +/* Xen: Compatibility define for iommu_domain_geometry.*/
> +struct iommu_domain_geometry {
> +       dma_addr_t aperture_start; /* First address that can be mapped    */
> +       dma_addr_t aperture_end;   /* Last address that can be mapped     */
> +       bool force_aperture;       /* DMA only allowed in mappable range? */
> +};
> +
> +
Same here.

[...]

> +
> +/*
> + * Xen: The pgtable_ops are used by the S1 translations, so return the dummy
> + * address.
> + */
> +#define alloc_io_pgtable_ops(f, c, o) ((struct io_pgtable_ops *)0x0)
I am slightly confused, on a previous e-mail you suggested that 0x0 is 
not possible to use. The comment in arm_smmu-domain_finalise seems to 
confirm that. So why the 0x0 here?
> +#define free_io_pgtable_ops(o)
Please use do { } while (0)

[...]

> @@ -1232,7 +1634,7 @@ static void arm_smmu_handle_ppr(struct arm_smmu_device 
> *smmu, u64 *evt)
>   
>   	dev_info(smmu->dev, "unexpected PRI request received:\n");
>         dev_info(smmu->dev,
> -                "\tsid 0x%08x.0x%05x: [%u%s] %sprivileged %s%s%s access at iova 
> 0x%016llx\n",
> +                "\tsid 0x%08x.0x%05x: [%u%s] %sprivileged %s%s%s access at iova %#" 
> PRIx64 "\n",
>                  sid, ssid, grpid, last ? "L" : "",
>                  evt[0] & PRIQ_0_PERM_PRIV ? "" : "un",
>                  evt[0] & PRIQ_0_PERM_READ ? "R" : "",
> @@ -1346,6 +1748,8 @@ static irqreturn_t arm_smmu_combined_irq_handler(int irq, 
> void *dev)
>   {
>         arm_smmu_gerror_handler(irq, dev);
>         arm_smmu_cmdq_sync_handler(irq, dev);
> +       /*Xen: No threaded irq. So call the required function from here */
NIT: /* Xen: ... */

> +       arm_smmu_combined_irq_thread(irq, dev);
>         return IRQ_WAKE_THREAD;
>   }
>   
[...]

> @@ -1783,7 +2239,14 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device 
> *smmu, u32 sid)
>         return sid < limit;
>   }
>   
> +/* Xen: Unused */
> +#if 0
>   static struct iommu_ops arm_smmu_ops;
> +#endif
> +
> +/* Xen: Redefine arm_smmu_ops to what fwspec should evaluate */
> +static const struct iommu_ops arm_smmu_iommu_ops;
> +#define arm_smmu_ops arm_smmu_iommu_ops
Hmmmmm. Why is that necessary? arm_smmu_iommu_ops is added in this 
patch. So can't you just name the structure arm_smmu_ops?
Furthermore, I would be ok to leave to remove the const as Linux does 
not do it.
>   
>   static int arm_smmu_add_device(struct device *dev)
>   {
> @@ -1791,8 +2254,11 @@ static int arm_smmu_add_device(struct device *dev)
>         struct arm_smmu_device *smmu;
>         struct arm_smmu_master_data *master;
>         struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +#if 0 /*Xen: iommu_group is not needed */
>         struct iommu_group *group;
> +#endif
>   
> +	/* Xen: fwspec->ops are not needed */
You don't change this code. So why this comment?

>         if (!fwspec || fwspec->ops != &arm_smmu_ops)
>                 return -ENODEV;
>         /*
> @@ -1830,6 +2296,11 @@ static int arm_smmu_add_device(struct device *dev)
>                 }
>         }
>   
> +/*
> + * Xen: Do not need an iommu group as the stream data is carried by the SMMU
NIT: "We don't need...".

> + * master device object
NIT: Missing full stop.

> + */
> +#if 0
>         group = iommu_group_get_for_dev(dev);
>         if (!IS_ERR(group)) {
>                 iommu_group_put(group);
[...]

> @@ -2316,9 +2800,13 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device 
> *smmu)
>                  * Cavium ThunderX2 implementation doesn't not support unique
>                  * irq lines. Use single irq line for all the SMMUv3 interrupts.
>                  */
> -               ret = devm_request_threaded_irq(smmu->dev, irq,
> +               /*
> +                * Xen: Does not support threaded irqs, so serialise the setup.
> +                * This is the same for pris and event interrupt lines on other
> +                * systems
> +                */
> +               ret = devm_request_irq(smmu->dev, irq,
>                                         arm_smmu_combined_irq_handler,
> -                                       arm_smmu_combined_irq_thread,
>                                         IRQF_ONESHOT,
>                                         "arm-smmu-v3-combined-irq", smmu);
On "RFC v4", I asked a question which was left unanswered. Here the 
conversation:
Me: Above you did implemented a dummy implementation of
    devm_request_threaded_irq(...). So why did you replace the code here?

You: The replacement worked well for other functions, where the handler 
was not defined. So, the wrapper function calls devm_request_irq with 
the argument passed in as thread. In this case really the handler hits 
first and it calls the thread in response. I can modify the code to make 
this fit into the api but in that case I will need to swap around the 
functions so number of line changes will stay the same. Tell me your 
preference.
Me: I don't understand what you mean here. Would it be possible to give 
a concrete example?
>                 if (ret < 0)
[...]

> @@ -2703,7 +3200,7 @@ static inline int arm_smmu_device_acpi_probe(struct 
> platform_device *pdev,
>   static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>                                     struct arm_smmu_device *smmu)
>   {
> -       struct device *dev = &pdev->dev;
> +       struct device *dev = pdev;
>         u32 cells;
>         int ret = -EINVAL;
>   
> @@ -2716,6 +3213,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>   
>   	parse_driver_options(smmu);
>   
> +	/* Xen: of_dma_is_coherent is a stub till dt support is introduced */
>         if (of_dma_is_coherent(dev->of_node))
On RFC v4, I requested to move the message on top of of_dma_is_coherent 
stub and add a WARN/WARN_ON(). Please address it.
>                 smmu->features |= ARM_SMMU_FEAT_COHERENCY;
>   
[...]

> @@ -2844,9 +3351,20 @@ static int arm_smmu_device_probe(struct platform_device 
> *pdev)
>                 if (ret)
>                         return ret;
>         }
> +#endif
> +       /*
> +        * Xen: Keep a list of all probed devices. This will be used to query
> +        * the smmu devices based on the fwnode.
> +        */
> +       INIT_LIST_HEAD(&smmu->devices);
> +       spin_lock(&arm_smmu_devices_lock);
> +       list_add(&smmu->devices, &arm_smmu_devices);
> +       spin_unlock(&arm_smmu_devices_lock);
>         return 0;
>   }
>   
> +/* Xen: Unused function */
> +#if 0
>   static int arm_smmu_device_remove(struct platform_device *pdev)
>   {
>         struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
> @@ -2860,6 +3378,8 @@ static void arm_smmu_device_shutdown(struct 
> platform_device *pdev)
>   {
>         arm_smmu_device_remove(pdev);
>   }
> +#endif
> +
Newline not necessary.

>    >   static const struct of_device_id arm_smmu_of_match[] = {
>         { .compatible = "arm,smmu-v3", },
[...]

> diff --git a/xen/include/xen/linux_compat.h b/xen/include/xen/linux_compat.h
> new file mode 100644
> index 0000000000..8037be0a3e
> --- /dev/null
> +++ b/xen/include/xen/linux_compat.h
> @@ -0,0 +1,84 @@
> +/******************************************************************************
> + * include/xen/linux_compat.h
> + *
> + * Compatibility defines for porting code from Linux to Xen
> + *
> + * Copyright (c) 2017 Linaro Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __XEN_LINUX_COMPAT_H__
> +#define __XEN_LINUX_COMPAT_H__
> +
> +#include <asm/types.h>
> +
> +typedef paddr_t phys_addr_t;
> +typedef paddr_t dma_addr_t;
> +
> +typedef unsigned int gfp_t;
> +#define GFP_KERNEL 0
> +#define __GFP_ZERO 0x01U
No need to the hexa here. 1U is much clearer.

> +
> +/* Helpers for IRQ functions */
> +#define free_irq release_irq
> +
> +enum irqreturn {
> +    IRQ_NONE,
> +    IRQ_HANDLED,
> +    IRQ_WAKE_THREAD,
> +};
> +
> +typedef enum irqreturn irqreturn_t;
> +
> +/* Device logger functions */
> +#define dev_dbg(dev, fmt, ...) printk(XENLOG_DEBUG fmt, ## __VA_ARGS__)
> +#define dev_notice(dev, fmt, ...) printk(XENLOG_INFO fmt, ## __VA_ARGS__)
> +#define dev_warn(dev, fmt, ...) printk(XENLOG_WARNING fmt, ## __VA_ARGS__)
> +#define dev_err(dev, fmt, ...) printk(XENLOG_ERR fmt, ## __VA_ARGS__)
> +#define dev_info(dev, fmt, ...) printk(XENLOG_INFO fmt, ## __VA_ARGS__)
> +
> +#define dev_err_ratelimited(dev, fmt, ...)                  \
> +     printk(XENLOG_ERR fmt, ## __VA_ARGS__)
> +
> +#define dev_name(dev) dt_node_full_name(dev_to_dt(dev))
> +
> +/* Alias to Xen allocation helpers */
> +#define kfree xfree
> +#define kmalloc(size, flags) ({\
> +       void *__ret_alloc = NULL; \
> +       if (flags & __GFP_ZERO) \
> +               __ret_alloc = _xzalloc(size, sizeof(void *)); \
That's Xen code, so please avoid using hard tabs.

> +       else \
> +               __ret_alloc = _xmalloc(size, sizeof(void *)); \
> +       __ret_alloc; \
> +})
Could we make at least kmalloc and kmalloc_array static inline? This 
will add safety and make easier to read (the \ are not indented at all).
> +#define kzalloc(size, flags)        _xzalloc(size, sizeof(void *))
> +#define devm_kzalloc(dev, size, flags)  _xzalloc(size, sizeof(void *))
> +#define kmalloc_array(size, n, flags) ({\
> +       void *__ret_alloc = NULL; \
> +       if (flags & __GFP_ZERO) \
> +               __ret_alloc = _xzalloc_array(size, sizeof(void *), n); \
> +       else \
> +               __ret_alloc = _xmalloc_array(size, sizeof(void *), n); \
> +       __ret_alloc; \
> +})
> +
> +/* Alias to Xen time functions */
> +#define ktime_t s_time_t
> +#define ktime_get()             (NOW())
> +#define ktime_add_us(t,i)       (t + MICROSECS(i))
> +#define ktime_compare(t,i)      (t > (i))
> +
> +#endif /* __XEN_LINUX_COMPAT_H__ */
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel