diff mbox series

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

Message ID 20171219031703.23420-9-sameer.goel@linaro.org
State New
Headers show
Series SMMUv3 driver | expand

Commit Message

Sameer Goel Dec. 19, 2017, 3:17 a.m. UTC 
Pull common defines for SMMU drivers in a local header.

Signed-off-by: Sameer Goel <sameer.goel@linaro.org>
---
 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

Comments

Manish Jaggi Jan. 3, 2018, 5:34 a.m. UTC | #1 
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@linaro.org>
> ---
>   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
>    *
Julien Grall Jan. 15, 2018, 8:41 p.m. UTC | #2 
On 01/03/2018 05:34 AM, Manish Jaggi wrote:
> Hi Sameer,

Hi Manish,

>> +    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 ?

How about a much more verbose name...? This name is 21 letters and 
that's only for the field. Just imagine with the variable name before 
and a couple of indentation.

This is where comment are helpful, a developer can easily look for the 
structure/field to see a description of the field. So let's not make the 
code more horrible because you would have to split the line.

Cheers,
Manish Jaggi Jan. 16, 2018, 12:40 p.m. UTC | #3 
Hi Julien,


On 01/16/2018 02:11 AM, Julien Grall wrote:
>
>
> On 01/03/2018 05:34 AM, Manish Jaggi wrote:
>> Hi Sameer,
>
> Hi Manish,
>
>>> +    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 ?
>
> How about a much more verbose name...? This name is 21 letters and 
> that's only for the field. Just imagine with the variable name before 
> and a couple of indentation.
How about io_context? anything which makes it more verbose is ok with me.
>
> This is where comment are helpful, a developer can easily look for the 
> structure/field to see a description of the field. So let's not make 
> the code more horrible because you would have to split the line.
>
> Cheers,
>
Julien Grall Jan. 16, 2018, 1:14 p.m. UTC | #4 
On 16/01/18 12:40, Manish Jaggi wrote:
> Hi Julien,

Hi,

> On 01/16/2018 02:11 AM, Julien Grall wrote:
>>
>>
>> On 01/03/2018 05:34 AM, Manish Jaggi wrote:
>>> Hi Sameer,
>>
>> Hi Manish,
>>
>>>> +    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 ?
>>
>> How about a much more verbose name...? This name is 21 letters and 
>> that's only for the field. Just imagine with the variable name before 
>> and a couple of indentation.
> How about io_context? anything which makes it more verbose is ok with me.

I much prefer to stick with "contexts".

Cheers,
Manish Jaggi Jan. 16, 2018, 1:27 p.m. UTC | #5 
On 01/16/2018 06:44 PM, Julien Grall wrote:
>
>
> On 16/01/18 12:40, Manish Jaggi wrote:
>> Hi Julien,
>
> Hi,
>
>> On 01/16/2018 02:11 AM, Julien Grall wrote:
>>>
>>>
>>> On 01/03/2018 05:34 AM, Manish Jaggi wrote:
>>>> Hi Sameer,
>>>
>>> Hi Manish,
>>>
>>>>> +    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 ?
>>>
>>> How about a much more verbose name...? This name is 21 letters and 
>>> that's only for the field. Just imagine with the variable name 
>>> before and a couple of indentation.
>> How about io_context? anything which makes it more verbose is ok with 
>> me.
>
> I much prefer to stick with "contexts".
I am not able to understand why to use contexts for a list which has 
iommu_domain pointers.
If you are ok with this logic, we can rename all iommu_domain pointer 
variables in this file as context.
It is not intuitive to use context for iommu_domain in the same file it 
is confusing.

smmu code in xen is not easy to read.
There are other places which confuse...

|struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv; What 
is the first impression from this variable xen_domain, does it refer to 
a VM ? It is confusing. |

>
> Cheers,
>
Julien Grall Jan. 16, 2018, 1:40 p.m. UTC | #6 
Hi Manish,

On 16/01/18 13:27, Manish Jaggi wrote:
> 
> 
> On 01/16/2018 06:44 PM, Julien Grall wrote:
>>
>>
>> On 16/01/18 12:40, Manish Jaggi wrote:
>>> Hi Julien,
>>
>> Hi,
>>
>>> On 01/16/2018 02:11 AM, Julien Grall wrote:
>>>>
>>>>
>>>> On 01/03/2018 05:34 AM, Manish Jaggi wrote:
>>>>> Hi Sameer,
>>>>
>>>> Hi Manish,
>>>>
>>>>>> +    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 ?
>>>>
>>>> How about a much more verbose name...? This name is 21 letters and 
>>>> that's only for the field. Just imagine with the variable name 
>>>> before and a couple of indentation.
>>> How about io_context? anything which makes it more verbose is ok with 
>>> me.
>>
>> I much prefer to stick with "contexts".
> I am not able to understand why to use contexts for a list which has 
> iommu_domain pointers.

Because the comment should have been read "iommu_domain context". As it 
is in other description.

> If you are ok with this logic, we can rename all iommu_domain pointer 
> variables in this file as context.

Why do you keep asking renaming when I clearly said multiple time that 
we *should not* rename any code coming from Linux. This is breaking the 
idea of keeping Xen and Linux SMMU driver close.

If you don't want to get SMMUv3 close to Linux. Then fine, but it means 
you have to use Xen coding style and dropping all necessary code.

But I am afraid this is not the solution I (and AFAIK Stefano) prefer 
because it adds burden on maintenance on Xen community.

Cheers,
Manish Jaggi Jan. 17, 2018, 6:37 a.m. UTC | #7 
On 01/16/2018 07:10 PM, Julien Grall wrote:
> Hi Manish,
>
> On 16/01/18 13:27, Manish Jaggi wrote:
>>
>>
>> On 01/16/2018 06:44 PM, Julien Grall wrote:
>>>
>>>
>>> On 16/01/18 12:40, Manish Jaggi wrote:
>>>> Hi Julien,
>>>
>>> Hi,
>>>
>>>> On 01/16/2018 02:11 AM, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 01/03/2018 05:34 AM, Manish Jaggi wrote:
>>>>>> Hi Sameer,
>>>>>
>>>>> Hi Manish,
>>>>>
>>>>>>> +    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 ?
>>>>>
>>>>> How about a much more verbose name...? This name is 21 letters and 
>>>>> that's only for the field. Just imagine with the variable name 
>>>>> before and a couple of indentation.
>>>> How about io_context? anything which makes it more verbose is ok 
>>>> with me.
>>>
>>> I much prefer to stick with "contexts".
>> I am not able to understand why to use contexts for a list which has 
>> iommu_domain pointers.
>
> Because the comment should have been read "iommu_domain context". As 
> it is in other description.
>
>> If you are ok with this logic, we can rename all iommu_domain pointer 
>> variables in this file as context.
>
> Why do you keep asking renaming when I clearly said multiple time that 
> we *should not* rename any code coming from Linux. This is breaking 
> the idea of keeping Xen and Linux SMMU driver close.
>
> If you don't want to get SMMUv3 close to Linux. Then fine, but it 
> means you have to use Xen coding style and dropping all necessary code.
>
> But I am afraid this is not the solution I (and AFAIK Stefano) prefer 
> because it adds burden on maintenance on Xen community.
>
I agree with the point that code imported from linux should be touched 
minimally, but If you look closely the problem is in naming of xen 
specific code that is added later.
Not in code imported from linux. So why we cant fix xen specific code 
naming?

Below are the few examples
a. static struct iommu_domain *arm_smmu_get_domain(struct domain *d,...)
This function use of domain is confusing.
This is not linux function,

a function named arm_smmu_get_domain takes a parameter of domain which 
represents a virtual machine and returns an iommu_domain pointer.
while the file still has a structure arm_smmu_domain.
In what way this function naming explains itself?

If you want to take a cue from linux see this code below
structarm_smmu_domain *smmu_domain = to_smmu_domain(domain); => The 
naming is quite clear and not confusing.
Also If you see there are few functions in xen specifc smmu code which 
are named correctly
Look at arm_smmu_iommu_domain_init(struct domain *d)

But arm_smmu_iommu_domain_init(struct domain *d) and arm_smmu_get_domain 
naming dont match,  they are both referring to iommu_domain

So my _two cents_ on the current use of structures / functions and 
variable names which use domain in xen specific code.

b. Similarly contexts in arm_smmu_xen_domain structure is not coming 
from linux code
c. arm_smmu_assign_dev is a xen specific function so it can be changed
d. Why id arm_smmu_xen_domain named the way it is. Does linux code 
include _linux_ in any structure name?

> Cheers,
>
Julien Grall Jan. 23, 2018, 3:38 p.m. UTC | #8 
Hi Sameer,

On 19/12/17 03:17, Sameer Goel wrote:
> Pull common defines for SMMU drivers in a local header.

I was expected to see this patch before SMMUv3 is added. But I am ok 
with that too.

However, if you want to do some renaming this should be done separately. 
So it will be easier to know this patch is only consolidation.

But why do you need to do renaming at first in the SMMUv3? The code is new.

Cheers,
diff mbox series

Patch

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;
+    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 /* __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
  *