Re: [Xen-devel] [v3 5/6] drivers/passthrough/arm: Refactor code for arm smmu drivers

["Goel, Sameer" <sgoel@xxxxxxxxxxxxxx>]

Hi Manish,
 Julien had responded back on the removal of code in this patch before 
following is the response to Roger:

"What matters is to know what is common between SMMUv2 and SMMUv3 driver. So it 
can be pulled in a separate headers.

IHMO, the both yours and his way are valid. TBH, I would have done a third way 
and move that patch before #5. But at this stage, this is a matter of taste, 
hence way I didn't push to reshuffle the series. "


At this point do you think these requested changes will change to code in any 
way that will impact your integration?

Thanks,
Sameer
On 6/7/2018 10:36 PM, Manish Jaggi wrote:
> Hi Sameer,
> 
> 
> On 06/08/2018 05:17 AM, Sameer Goel wrote:
>> Pull common defines for SMMU drives in a local header.
> Can add more detail in commit message?
>>
>> Signed-off-by: Sameer Goel <sameer.goel@xxxxxxxxxx>
>> ---
>>   xen/drivers/passthrough/arm/smmu-v3.c |  96 +-------------------
>>   xen/drivers/passthrough/arm/smmu.c    | 104 +--------------------
>>   xen/drivers/passthrough/arm/smmu.h    | 125 ++++++++++++++++++++++++++
>>   3 files changed, 128 insertions(+), 197 deletions(-)
>>   create mode 100644 xen/drivers/passthrough/arm/smmu.h
>>
>> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
>> b/xen/drivers/passthrough/arm/smmu-v3.c
>> index 75c3411ad9..fdf85c1508 100644
>> --- a/xen/drivers/passthrough/arm/smmu-v3.c
>> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
>> @@ -49,28 +49,7 @@
>>   #include <asm/io.h>
>>   #include <asm/platform.h>
>>   -/* 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 */
> Added and removed in subsequent patches? code specific to xen.
> you can remove from patch#4 itself.
>> -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 "smmu.h"
>>     static struct resource *platform_get_resource(struct platform_device 
>> *pdev,
>>                             unsigned int type,
>> @@ -200,79 +179,6 @@ static 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)
>> -
>> -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;
>> -};
>> -
> This xen dummy structure was introduced in patch 4 and now removing in patch5,
> Can you remove it from patch 4 itself?
>> -/* 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;
>> -};
>> -
> Same as above
>>   /*
>>    * Xen: Information about each device stored in dev->archdata.iommu
>>    *
>> diff --git a/xen/drivers/passthrough/arm/smmu.c 
>> b/xen/drivers/passthrough/arm/smmu.c
>> index ad956d5b8d..f7a6b107de 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 "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
>>    *
>> diff --git a/xen/drivers/passthrough/arm/smmu.h 
>> b/xen/drivers/passthrough/arm/smmu.h
>> new file mode 100644
>> index 0000000000..8e38d78c1a
>> --- /dev/null
>> +++ b/xen/drivers/passthrough/arm/smmu.h
>> @@ -0,0 +1,125 @@
>> +/******************************************************************************
>> + * ./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 __SMMU_H__
>> +#define __SMMU_H__
>> +
>> +
>> +/* 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
>> +
>> +/* 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)
>> +
>> +#define dma_set_mask_and_coherent(d, b)    0
>> +#define of_dma_is_coherent(n)    0
>> +
>> +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;
>> +    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;
>> +};
>> +
>> +#endif /* __SMMU_H__ */
>> +
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel

-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, 
Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

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