Message-ID: <20160604100930.39d7be05@arm.com>
Date:	Sat, 4 Jun 2016 10:09:30 +0100
From:	Marc Zyngier <marc.zyngier@....com>
To:	Shanker Donthineni <shankerd@...eaurora.org>
Cc:	<linux-arm-kernel@...ts.infradead.org>,
	<linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Jason Cooper <jason@...edaemon.net>,
	Vikram Sethi <vikrams@...eaurora.org>,
	Philip Elcan <pelcan@...eaurora.org>
Subject: Re: [PATCH V3 2/2] irqchip/gicv3-its: Implement two-level(indirect)
 device table support

On Mon, 9 May 2016 15:58:26 -0500
Shanker Donthineni <shankerd@...eaurora.org> wrote:

Hi Shanker,

> Since device IDs are extremely sparse, the single, a.k.a flat table is
> not sufficient for the following two reasons.
> 
> 1) According to ARM-GIC spec, ITS hw can access maximum of 256(pages)*
>    64K(pageszie) bytes. In the best case, it supports upto DEVid=21
         pagesize
>    sparse with minimum device table entry size 8bytes.
> 
> 2) The maximum memory size that is possible without memblock depends on
>    MAX_ORDER. 4MB on 4K page size kernel with default MAX_ORDER, so it
>    supports DEVid range 19bits.
> 
> The two-level device table feature brings us two advantages, the first
> is a very high possibility of supporting upto 32bit sparse, and the
> second one is the best utilization of memory allocation.
> 
> The feature is enabled automatically during driver probe if a single
> ITS page is not adequate for flat table and the hardware is capable
> of two-level table walk.
> 
> Signed-off-by: Shanker Donthineni <shankerd@...eaurora.org>
> ---
> 
> Changes since v2:
>   Fixed a porting bug device 'id' validation check in its_alloc_device_table()
> 
> Changes since v1:
>   Most of this patch has been rewritten after refactoring its_alloc_tables().
>   Always enable device two-level if the memory requirement is more than PAGE_SIZE.
>   Fixed the coding bug that breaks on the BE machine.
>   Edited the commit text.
> 
>  drivers/irqchip/irq-gic-v3-its.c | 97 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 80 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index b23e00c..60a1060 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -938,6 +938,18 @@ retry_baser:
>  	return 0;
>  }
>  
> +/**
> + * Find out whether an implemented baser register supports a single, flat table
> + * or a two-level table by reading bit offset at '62' after writing '1' to it.
> + */
> +static u64 its_baser_check_indirect(struct its_baser *baser)
> +{
> +	u64 val = GITS_BASER_InnerShareable | GITS_BASER_WaWb;
> +
> +	writeq_relaxed(val | GITS_BASER_INDIRECT, baser->hwreg);
> +	return (readq_relaxed(baser->hwreg) & GITS_BASER_INDIRECT); 

That's a bit ugly. You're returning a mask for the indirect bit, and
treat it either as a boolean or a mask. I'd rather you return a
boolean, treat as such in most of this code, and only turn it into a
mask when you compute the GITS_BASER value.

> +}
> +
>  static int its_alloc_tables(const char *node_name, struct its_node *its)
>  {
>  	u64 typer = readq_relaxed(its->base + GITS_TYPER);
> @@ -964,6 +976,7 @@ static int its_alloc_tables(const char *node_name, struct its_node *its)
>  		u64 entry_size = GITS_BASER_ENTRY_SIZE(val);
>  		int order = get_order(psz);
>  		struct its_baser *baser = its->tables + i;
> +		u64 indirect = 0;

The scope of this flag is confusingly wide. Once an indirect table has
been created, all the following tables are indirect too, which is
definitely not what we want (only the device table should be
indirected).

>  
>  		if (type == GITS_BASER_TYPE_NONE)
>  			continue;
> @@ -977,17 +990,27 @@ static int its_alloc_tables(const char *node_name, struct its_node *its)
>  		 * Allocate as many entries as required to fit the
>  		 * range of device IDs that the ITS can grok... The ID
>  		 * space being incredibly sparse, this results in a
> -		 * massive waste of memory.
> +		 * massive waste of memory if two-level device table
> +		 * feature is not supported by hardware.
>  		 *
>  		 * For other tables, only allocate a single page.
>  		 */
>  		if (type == GITS_BASER_TYPE_DEVICE) {
> -			/*
> -			 * 'order' was initialized earlier to the default page
> -			 * granule of the the ITS.  We can't have an allocation
> -			 * smaller than that.  If the requested allocation
> -			 * is smaller, round up to the default page granule.
> -			 */
> +			if ((entry_size << ids) > psz)
> +				indirect = its_baser_check_indirect(baser);
> +
> +			if (indirect) {
> +				/*
> +				 * The size of the lvl2 table is equal to ITS
> +				 * page size which is 'psz'. For computing lvl1
> +				 * table size, subtract ID bits that sparse
> +				 * lvl2 table from 'ids' which is reported by
> +				 * ITS hardware times lvl1 table entry size.
> +				 */
> +				ids -= ilog2(psz / entry_size);
> +				entry_size = GITS_LVL1_ENTRY_SIZE;
> +			}
> +
>  			order = max(get_order(entry_size << ids), order);
>  			if (order >= MAX_ORDER) {
>  				order = MAX_ORDER - 1;

This needs some splitting as well. Given that we're giving the
Device table a special treatment, I think it'd make sense to give it
its own function that would return the order of the the allocation and
the indirect flag.

> @@ -997,7 +1020,7 @@ static int its_alloc_tables(const char *node_name, struct its_node *its)
>  			}
>  		}
>  
> -		err = its_baser_setup(its, baser, order, 0);
> +		err = its_baser_setup(its, baser, order, indirect);
>  		if (err < 0) {
>  			its_free_tables(its);
>  			return err;
> @@ -1187,10 +1210,57 @@ static struct its_baser *its_get_baser(struct its_node *its, u32 type)
>  	return NULL;
>  }
>  
> +static bool its_alloc_device_table(struct its_node *its, u32 dev_id)
> +{
> +	struct its_baser *baser;
> +	struct page *page;
> +	u32 esz, idx;
> +	u64 *table;
> +
> +	baser = its_get_baser(its, GITS_BASER_TYPE_DEVICE);
> +
> +	/* Don't allow device id that exceeds ITS hardware limit */
> +	if (!baser)
> +		return (ilog2(dev_id) < its->device_ids);
> +
> +	/* Don't allow device id that exceeds single, flat table limit */
> +	esz = GITS_BASER_ENTRY_SIZE(baser->val);
> +	if (!(baser->val & GITS_BASER_INDIRECT))
> +		return (dev_id < (PAGE_ORDER_TO_SIZE(baser->order) / esz));
> +
> +	/* Compute 1st level table index & check if that exceeds table limit */
> +	idx = dev_id >> ilog2(baser->psz / esz);
> +	if (idx >= (PAGE_ORDER_TO_SIZE(baser->order) / GITS_LVL1_ENTRY_SIZE))
> +		return false;
> +
> +	table = baser->base;
> +
> +	/* Allocate memory for 2nd level table */
> +	if (!table[idx]) {
> +		page = alloc_pages(GFP_KERNEL | __GFP_ZERO, get_order(baser->psz));
> +		if (!page)
> +			return false;
> +
> +		/* Flush memory to PoC if hardware doesn't support coherency */
> +		if (!(baser->val & GITS_BASER_SHAREABILITY_MASK))
> +			__flush_dcache_area(page_address(page), baser->psz);
> +
> +		table[idx] = cpu_to_le64(page_to_phys(page) | GITS_BASER_VALID);
> +
> +		/* Flush memory to PoC if hardware doesn't support coherency */

Please don't use the same comment twice, this is a bit misleading.
Explain that the first clean/invalidate pushes out the data page, and
that the second pushes out the pointer to that page.

> +		if (!(baser->val & GITS_BASER_SHAREABILITY_MASK))
> +			__flush_dcache_area(table + idx, GITS_LVL1_ENTRY_SIZE);
> +
> +		/* Ensure updated table contents are visible to ITS hardware */
> +		dsb(sy);
> +	}
> +
> +	return true;
> +}
> +
>  static struct its_device *its_create_device(struct its_node *its, u32 dev_id,
>  					    int nvecs)
>  {
> -	struct its_baser *baser;
>  	struct its_device *dev;
>  	unsigned long *lpi_map;
>  	unsigned long flags;
> @@ -1201,14 +1271,7 @@ static struct its_device *its_create_device(struct its_node *its, u32 dev_id,
>  	int nr_ites;
>  	int sz;
>  
> -	baser = its_get_baser(its, GITS_BASER_TYPE_DEVICE);
> -
> -	/* Don't allow 'dev_id' that exceeds single, flat table limit */
> -	if (baser) {
> -		if (dev_id >= (PAGE_ORDER_TO_SIZE(baser->order) /
> -			      GITS_BASER_ENTRY_SIZE(baser->val)))
> -			return NULL;
> -	} else if (ilog2(dev_id) >= its->device_ids)
> +	if (!its_alloc_device_table(its, dev_id))
>  		return NULL;
>  
>  	dev = kzalloc(sizeof(*dev), GFP_KERNEL);


Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Powered by blists - more mailing lists