Re: [Xen-devel] [RFC v4 8/8] drivers/passthrough/arm: Refactor code for arm smmu drivers

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

Hi Sameer,


On 12/19/2017 08:47 AM, Sameer Goel wrote:
> Pull common defines for SMMU drivers in a local header.
>
> Signed-off-by: Sameer Goel <sameer.goel@xxxxxxxxxx>
> ---
>   xen/drivers/passthrough/arm/arm_smmu.h | 113 +++++++++++++++++++++++++++++++++
>   xen/drivers/passthrough/arm/smmu-v3.c  |  96 ++--------------------------
>   xen/drivers/passthrough/arm/smmu.c     | 104 +-----------------------------
>   3 files changed, 121 insertions(+), 192 deletions(-)
>   create mode 100644 xen/drivers/passthrough/arm/arm_smmu.h
>
> diff --git a/xen/drivers/passthrough/arm/arm_smmu.h 
> b/xen/drivers/passthrough/arm/arm_smmu.h
> new file mode 100644
> index 0000000000..70f97e7d50
> --- /dev/null
> +++ b/xen/drivers/passthrough/arm/arm_smmu.h
> @@ -0,0 +1,113 @@
> +/******************************************************************************
> + * arm_smmu.h
> + *
> + * Common compatibility defines and data_structures for porting arm smmu
> + * drivers from Linux.
> + *
> + * 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 __ARM_SMMU_H__
> +#define __ARM_SMMU_H__
> +
> +/* Helpers to get device MMIO and IRQs */
> +struct resource {
> +    u64 addr;
> +    u64 size;
> +    unsigned int type;
> +};
> +
> +#define resource_size(res) ((res)->size)
> +
> +#define platform_device device
> +
> +#define IORESOURCE_MEM 0
> +#define IORESOURCE_IRQ 1
> +
> +/* Stub out DMA domain related functions */
> +#define iommu_get_dma_cookie(dom) 0
> +#define iommu_put_dma_cookie(dom)
> +
> +#define VA_BITS     0   /* Only used for configuring stage-1 input size */
> +
> +#define MODULE_DEVICE_TABLE(type, name)
> +#define module_param_named(name, value, type, perm)
> +#define MODULE_PARM_DESC(_parm, desc)
> +
> +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;
> +}
> +
> +/*
> + * Domain type definitions. Not really needed for Xen, defining to port
> + * Linux code as-is
> + */
> +#define IOMMU_DOMAIN_UNMANAGED 0
> +#define IOMMU_DOMAIN_DMA 1
> +#define IOMMU_DOMAIN_IDENTITY 2
> +
> +/* 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? */
> +};
> +
> +/* Xen: Dummy iommu_domain */
> +struct iommu_domain {
> +    /* Runtime SMMU configuration for this iommu_domain */
> +    struct arm_smmu_domain      *priv;
Do we need to call it priv? IMHO as there are too many structures wtih 
_domain and in the bigger goal of making the code intuitive
can we remove priv to something more verbose as smmu_domain.

> +    unsigned int            type;
> +
> +    /* Dummy compatibility defines */
> +    unsigned long pgsize_bitmap;
> +    struct iommu_domain_geometry geometry;
> +
> +    atomic_t ref;
> +    /* Used to link iommu_domain contexts for a same domain.
> +     * There is at least one per-SMMU to used by the domain.
> +     */
> +    struct list_head        list;
> +};
> +
> +/* Xen: Describes information required for a Xen domain */
> +struct arm_smmu_xen_domain {
> +    spinlock_t          lock;
> +    /* List of iommu domains associated to this domain */
> +    struct list_head        contexts;
Could we use a more verbose name, How about 
%s/contexts/iommu_domain_contexts/g ?
> +};
> +
> +#endif /* __ARM_SMMU_H__ */
> +
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> b/xen/drivers/passthrough/arm/smmu-v3.c
> index 3488184ad4..6e705f63a3 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -49,20 +49,7 @@
>   #include <asm/io.h>
>   #include <asm/platform.h>
>   
> -
> -/* Xen: Helpers to get device MMIO and IRQs */
> -struct resource {
> -       u64 addr;
> -       u64 size;
> -       unsigned int type;
> -};
> -
> -#define resource_size(res) ((res)->size)
> -
> -#define platform_device device
> -
> -#define IORESOURCE_MEM 0
> -#define IORESOURCE_IRQ 1
> +#include "arm_smmu.h" /* Not a self contained header. So last in the list */
>   
>   static struct resource *platform_get_resource(struct platform_device *pdev,
>                                               unsigned int type,
> @@ -192,81 +179,10 @@ void dmam_free_coherent(struct device *dev, size_t size, 
> void *vaddr,
>         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)
> -#define of_device_id dt_device_match
> -
> -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? */
> -};
> -
> -
> -/* Xen: Type definitions for iommu_domain */
> -#define IOMMU_DOMAIN_UNMANAGED 0
> -#define IOMMU_DOMAIN_DMA 1
> -#define IOMMU_DOMAIN_IDENTITY 2
> -
> -/* Xen: Dummy iommu_domain */
> -struct iommu_domain {
> -       /* Runtime SMMU configuration for this iommu_domain */
> -       struct arm_smmu_domain          *priv;
> -       unsigned int type;
> -
> -       /* Dummy compatibility defines */
> -       unsigned long pgsize_bitmap;
> -       struct iommu_domain_geometry geometry;
> -
> -       atomic_t ref;
> -       /*
> -        * Used to link iommu_domain contexts for a same domain.
> -        * There is at least one per-SMMU to used by the domain.
> -        */
> -       struct list_head                list;
> -};
> -
> -
> -/* Xen: Describes information required for a Xen domain */
> -struct arm_smmu_xen_domain {
> -       spinlock_t                      lock;
> -       /* List of iommu domains associated to this domain */
> -       struct list_head                iommu_domains;
> -};
> -
>   /*
>    * Xen: Information about each device stored in dev->archdata.iommu
>    *
> @@ -3396,7 +3312,7 @@ static int __must_check arm_smmu_iotlb_flush_all(struct 
> domain *d)
>         struct iommu_domain *cfg;
>   
>   	spin_lock(&smmu_domain->lock);
> -       list_for_each_entry(cfg, &smmu_domain->iommu_domains, list) {
> +       list_for_each_entry(cfg, &smmu_domain->contexts, list) {
>                 /*
>                  * Only invalidate the context when SMMU is present.
>                  * This is because the context initialization is delayed
> @@ -3435,7 +3351,7 @@ static struct iommu_domain *arm_smmu_get_domain(struct 
> domain *d,
>          * Loop through the &xen_domain->contexts to locate a context
>          * assigned to this SMMU
>          */
> -       list_for_each_entry(domain, &xen_domain->iommu_domains, list) {
> +       list_for_each_entry(domain, &xen_domain->contexts, list) {
>                 smmu_domain = to_smmu_domain(domain);
>                 if (smmu_domain->smmu == smmu)
>                         return domain;
> @@ -3489,7 +3405,7 @@ static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
>                 arm_smmu->s2_cfg.domain = d;
>   
>   		/* Chain the new context to the domain */
> -               list_add(&domain->list, &xen_domain->iommu_domains);
> +               list_add(&domain->list, &xen_domain->contexts);
>   
>   	}
>   
> @@ -3569,7 +3485,7 @@ static int arm_smmu_iommu_domain_init(struct domain *d)
>                 return -ENOMEM;
>   
>   	spin_lock_init(&xen_domain->lock);
> -       INIT_LIST_HEAD(&xen_domain->iommu_domains);
> +       INIT_LIST_HEAD(&xen_domain->contexts);
>   
>   	dom_iommu(d)->arch.priv = xen_domain;
>   
> @@ -3584,7 +3500,7 @@ static void arm_smmu_iommu_domain_teardown(struct domain *d)
>   {
>         struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
>   
> -	ASSERT(list_empty(&xen_domain->iommu_domains));
> +       ASSERT(list_empty(&xen_domain->contexts));
>         xfree(xen_domain);
>   }
>   
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index ad956d5b8d..4c04391e21 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -41,6 +41,7 @@
>   #include <xen/irq.h>
>   #include <xen/lib.h>
>   #include <xen/list.h>
> +#include <xen/linux_compat.h>
>   #include <xen/mm.h>
>   #include <xen/vmap.h>
>   #include <xen/rbtree.h>
> @@ -51,36 +52,13 @@
>   #include <asm/io.h>
>   #include <asm/platform.h>
>   
> +#include "arm_smmu.h" /* Not a self contained header. So last in the list */
>   /* Xen: The below defines are redefined within the file. Undef it */
>   #undef SCTLR_AFE
>   #undef SCTLR_TRE
>   #undef SCTLR_M
>   #undef TTBCR_EAE
>   
> -/* Alias to Xen device tree helpers */
> -#define device_node dt_device_node
> -#define of_phandle_args dt_phandle_args
> -#define of_device_id dt_device_match
> -#define of_match_node dt_match_node
> -#define of_property_read_u32(np, pname, out) (!dt_property_read_u32(np, pname, 
> out))
> -#define of_property_read_bool dt_property_read_bool
> -#define of_parse_phandle_with_args dt_parse_phandle_with_args
> -
> -/* Xen: Helpers to get device MMIO and IRQs */
> -struct resource
> -{
> -       u64 addr;
> -       u64 size;
> -       unsigned int type;
> -};
> -
> -#define resource_size(res) (res)->size;
> -
> -#define platform_device device
> -
> -#define IORESOURCE_MEM 0
> -#define IORESOURCE_IRQ 1
> -
>   static struct resource *platform_get_resource(struct platform_device *pdev,
>                                               unsigned int type,
>                                               unsigned int num)
> @@ -118,58 +96,6 @@ static struct resource *platform_get_resource(struct 
> platform_device *pdev,
>   
>   /* Xen: Helpers for IRQ functions */
>   #define request_irq(irq, func, flags, name, dev) request_irq(irq, flags, 
> func, name, dev)
> -#define free_irq release_irq
> -
> -enum irqreturn {
> -       IRQ_NONE        = (0 << 0),
> -       IRQ_HANDLED     = (1 << 0),
> -};
> -
> -typedef enum irqreturn irqreturn_t;
> -
> -/* Device logger functions
> - * TODO: Handle PCI
> - */
> -#define dev_print(dev, lvl, fmt, ...)                                          
> \
> -        printk(lvl "smmu: %s: " fmt, dt_node_full_name(dev_to_dt(dev)), ## 
> __VA_ARGS__)
> -
> -#define dev_dbg(dev, fmt, ...) dev_print(dev, XENLOG_DEBUG, fmt, ## 
> __VA_ARGS__)
> -#define dev_notice(dev, fmt, ...) dev_print(dev, XENLOG_INFO, fmt, ## 
> __VA_ARGS__)
> -#define dev_warn(dev, fmt, ...) dev_print(dev, XENLOG_WARNING, fmt, ## 
> __VA_ARGS__)
> -#define dev_err(dev, fmt, ...) dev_print(dev, XENLOG_ERR, fmt, ## __VA_ARGS__)
> -
> -#define dev_err_ratelimited(dev, fmt, ...)                                     
> \
> -        dev_print(dev, 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)           _xmalloc(size, sizeof(void *))
> -#define kzalloc(size, flags)           _xzalloc(size, sizeof(void *))
> -#define devm_kzalloc(dev, size, flags) _xzalloc(size, sizeof(void *))
> -#define kmalloc_array(size, n, flags)  _xmalloc_array(size, sizeof(void *), n)
> -
> -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 doesn't handle IOMMU fault */
>   #define report_iommu_fault(...)       1
> @@ -196,32 +122,6 @@ static inline int pci_for_each_dma_alias(struct pci_dev 
> *pdev,
>   #define PHYS_MASK_SHIFT               PADDR_BITS
>   typedef paddr_t phys_addr_t;
>   
> -#define VA_BITS		0	/* Only used for configuring stage-1 input size */
> -
> -#define MODULE_DEVICE_TABLE(type, name)
> -#define module_param_named(name, value, type, perm)
> -#define MODULE_PARM_DESC(_parm, desc)
> -
> -/* Xen: Dummy iommu_domain */
> -struct iommu_domain
> -{
> -       /* Runtime SMMU configuration for this iommu_domain */
> -       struct arm_smmu_domain          *priv;
> -
> -       atomic_t ref;
> -       /* Used to link iommu_domain contexts for a same domain.
> -        * There is at least one per-SMMU to used by the domain.
> -        * */
> -       struct list_head                list;
> -};
> -
> -/* Xen: Describes informations required for a Xen domain */
> -struct arm_smmu_xen_domain {
> -       spinlock_t                      lock;
> -       /* List of context (i.e iommu_domain) associated to this domain */
> -       struct list_head                contexts;
> -};
> -
>   /*
>    * Xen: Information about each device stored in dev->archdata.iommu
>    *

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