Re: [RFC PATCH v2 06/22] xen/arm: its: Port ITS driver to xen

[Julien Grall <julien.grall@linaro.org>]

Hello Vijay,

On 19/03/2015 14:37, vijay.kilari@gmail.com wrote:
>   static LIST_HEAD(its_nodes);
>   static DEFINE_SPINLOCK(its_lock);
> -static struct device_node *gic_root_node;
> -static struct rdists *gic_rdists;
> +static struct dt_device_node *gic_root_node;
> +static struct rdist_prop  *gic_rdists;
>
> -#define gic_data_rdist()		(raw_cpu_ptr(gic_rdists->rdist))
> -#define gic_data_rdist_rd_base()	(gic_data_rdist()->rd_base)
> +#define gic_data_rdist()		(per_cpu(rdist, smp_processor_id()))

Again why didn't you return a pointer here? It would have been avoid 
some confusing changes (s/->/./) in the code.

#define gic_data_rdist(&per_cpu(rdist, smp_processor_id))

> +#define gic_data_rdist_rd_base()	(per_cpu(rdist, smp_processor_id()).rbase)

That would avoid this change too.

>
>   /*
>    * ITS command descriptors - parameters to be encoded in a command
> @@ -228,10 +243,10 @@ static struct its_collection *its_build_mapd_cmd(struct its_cmd_block *cmd,
>   						 struct its_cmd_desc *desc)
>   {
>   	unsigned long itt_addr;
> -	u8 size = ilog2(desc->its_mapd_cmd.dev->nr_ites);
> +	u8 size = max(fls(desc->its_mapd_cmd.dev->nr_ites) - 1, 1);

ilog2 on an uint32_t is defined as fls(val) - 1. Where does the max come 
from?

IHMO, I would define ilog2 in Xen, that would be easier.

>
> -	itt_addr = virt_to_phys(desc->its_mapd_cmd.dev->itt);
> -	itt_addr = ALIGN(itt_addr, ITS_ITT_ALIGN);
> +	itt_addr = __pa(desc->its_mapd_cmd.dev->itt);
> +        itt_addr = ROUNDUP(itt_addr, ITS_ITT_ALIGN);

This file use the Linux coding style. Please use hard tab.

>
>   	its_encode_cmd(cmd, GITS_CMD_MAPD);
>   	its_encode_devid(cmd, desc->its_mapd_cmd.dev->device_id);
> @@ -348,7 +363,7 @@ static struct its_cmd_block *its_allocate_entry(struct its_node *its)
>   	while (its_queue_full(its)) {
>   		count--;
>   		if (!count) {
> -			pr_err_ratelimited("ITS queue not draining\n");
> +			its_err("ITS queue not draining\n");

its_err and pr_err_ratelimited are not the same things. The former is 
not ratelimited.

AFAICT this function will be accessible in someway from the guest. It 
would be possible to DOS Xen when sending a command.

>   			return NULL;
>   		}
>   		cpu_relax();
> @@ -380,7 +395,7 @@ static void its_flush_cmd(struct its_node *its, struct its_cmd_block *cmd)
>   	 * the ITS.
>   	 */
>   	if (its->flags & ITS_FLAGS_CMDQ_NEEDS_FLUSHING)
> -		__flush_dcache_area(cmd, sizeof(*cmd));
> +		clean_and_invalidate_dcache_va_range(cmd, sizeof(*cmd));
>   	else
>   		dsb(ishst);
>   }
> @@ -402,7 +417,7 @@ static void its_wait_for_range_completion(struct its_node *its,
>
>   		count--;
>   		if (!count) {
> -			pr_err_ratelimited("ITS queue timeout\n");
> +			its_err("ITS queue timeout\n");

Ditto

[..]

> -static void its_send_inv(struct its_device *dev, u32 event_id)
> +/* TODO: Remove static for the sake of compilation */
> +void its_send_inv(struct its_device *dev, u32 event_id)

Rather than changing the prototype. Would it be possible to #if 0 the 
function? It would be easier to keep track change.

>   {
>   	struct its_cmd_desc desc;
>
> @@ -479,7 +495,8 @@ static void its_send_mapc(struct its_node *its, struct its_collection *col,
>   	its_send_single_command(its, its_build_mapc_cmd, &desc);
>   }
>
> -static void its_send_mapvi(struct its_device *dev, u32 irq_id, u32 id)
> +/* TODO: Remove static for the sake of compilation */
> +void its_send_mapvi(struct its_device *dev, u32 irq_id, u32 id)

Ditto and same for all those kind of changes.

[..]

> -static unsigned long *its_lpi_alloc_chunks(int nr_irqs, int *base, int *nr_ids)
> +static unsigned long *its_lpi_alloc_chunks(int nirqs, int *base, int *nr_ids)

This is because nr_irqs is a define on ARM, rigth? If so, I would prefer 
to define nr_irqs as a variable.

[..]

>   /*
> @@ -745,31 +769,31 @@ static void its_lpi_free(unsigned long *bitmap, int base, int nr_ids)
>   /*
>    * This is how many bits of ID we need, including the useless ones.
>    */
> -#define LPI_NRBITS		ilog2(LPI_PROPBASE_SZ + SZ_8K)
> +#define LPI_NRBITS		fls(LPI_PROPBASE_SZ + SZ_8K) - 1

Missing parenthesis.

>
>   #define LPI_PROP_DEFAULT_PRIO	0xa0

I would either move LPI_PROP_DEFAULT_PRIO in asm-arm/gic.h or define it 
using GIC_PRI_IRQ.

This would allow us to change the priority later without having issue 
with LPI.

>
>   static int __init its_alloc_lpi_tables(void)
>   {
> -	phys_addr_t paddr;
> +	paddr_t paddr;
>
> -	gic_rdists->prop_page = alloc_pages(GFP_NOWAIT,
> -					   get_order(LPI_PROPBASE_SZ));
> +	gic_rdists->prop_page = alloc_xenheap_pages(get_order_from_bytes(LPI_PROPBASE_SZ), 0);
>   	if (!gic_rdists->prop_page) {
> -		pr_err("Failed to allocate PROPBASE\n");
> +		its_err("Failed to allocate PROPBASE\n");
>   		return -ENOMEM;
>   	}
>
> -	paddr = page_to_phys(gic_rdists->prop_page);
> -	pr_info("GIC: using LPI property table @%pa\n", &paddr);
> +	paddr = __pa(gic_rdists->prop_page);
> +	its_info("GIC: using LPI property table @%pa\n", &paddr);

IIRC, %pa doesn't exist on Xen.

>
>   	/* Priority 0xa0, Group-1, disabled */
> -	memset(page_address(gic_rdists->prop_page),
> +	memset(gic_rdists->prop_page,
>   	       LPI_PROP_DEFAULT_PRIO | LPI_PROP_GROUP1,
>   	       LPI_PROPBASE_SZ);
>
>   	/* Make sure the GIC will observe the written configuration */
> -	__flush_dcache_area(page_address(gic_rdists->prop_page), LPI_PROPBASE_SZ);
> +	clean_and_invalidate_dcache_va_range(gic_rdists->prop_page,
> +	                                     LPI_PROPBASE_SZ);
>
>   	return 0;
>   }
> @@ -790,7 +814,7 @@ static void its_free_tables(struct its_node *its)
>
>   	for (i = 0; i < GITS_BASER_NR_REGS; i++) {
>   		if (its->tables[i]) {
> -			free_page((unsigned long)its->tables[i]);
> +			xfree(its->tables[i]);

The memory for the table is allocated via alloc_xenheap_pages. So 
freeing the memory should be done via free_xenheap_pages.

>   			its->tables[i] = NULL;
>   		}
>   	}
> @@ -807,7 +831,7 @@ static int its_alloc_tables(struct its_node *its)
>   		u64 val = readq_relaxed(its->base + GITS_BASER + i * 8);
>   		u64 type = GITS_BASER_TYPE(val);
>   		u64 entry_size = GITS_BASER_ENTRY_SIZE(val);
> -		int order = get_order(psz);
> +		int order = get_order_from_bytes(psz);

I saw multiple change from get_order to get_order_from_bytes.

I think get_order is very handy and more compact. I would add a macro 
for it.

>   		int alloc_size;
>   		u64 tmp;
>   		void *base;
> @@ -827,25 +851,25 @@ static int its_alloc_tables(struct its_node *its)
>   			u64 typer = readq_relaxed(its->base + GITS_TYPER);
>   			u32 ids = GITS_TYPER_DEVBITS(typer);
>
> -			order = get_order((1UL << ids) * entry_size);
> +			order = get_order_from_bytes((1UL << ids) * entry_size);
>   			if (order >= MAX_ORDER) {
>   				order = MAX_ORDER - 1;
> -				pr_warn("%s: Device Table too large, reduce its page order to %u\n",
> -					its->msi_chip.of_node->full_name, order);
> +				its_warn("Device Table too large, reduce its page order to %u\n",
> +					 order);
>   			}
>   		}
>
>   		alloc_size = (1 << order) * PAGE_SIZE;
> -		base = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, order);
> +		base = alloc_xenheap_pages(order, 0);
>   		if (!base) {
>   			err = -ENOMEM;
>   			goto out_free;
>   		}
> -

This change is not necessary.

> +		memset(base, 0, alloc_size);
>   		its->tables[i] = base;

[..]

>   	/* If we didn't allocate the pending table yet, do it now */
> -	pend_page = gic_data_rdist()->pend_page;
> +	pend_page = gic_data_rdist().pend_page;
>   	if (!pend_page) {
> -		phys_addr_t paddr;
> +		paddr_t paddr;
>   		/*
>   		 * The pending pages have to be at least 64kB aligned,
>   		 * hence the 'max(LPI_PENDBASE_SZ, SZ_64K)' below.
>   		 */
> -		pend_page = alloc_pages(GFP_NOWAIT | __GFP_ZERO,
> -					get_order(max(LPI_PENDBASE_SZ, SZ_64K)));
> +		pend_page = alloc_xenheap_pages(get_order_from_bytes(max(LPI_PENDBASE_SZ, SZ_64K)), 0);

This line is too long.

>   		if (!pend_page) {
> -			pr_err("Failed to allocate PENDBASE for CPU%d\n",
> +			its_err("Failed to allocate PENDBASE for CPU%d\n",
>   			       smp_processor_id());

indentation

>   			return;
>   		}
> -

Spurious change

> +		memset(pend_page, 0, max(LPI_PENDBASE_SZ, SZ_64K));
>   		/* Make sure the GIC will observe the zero-ed page */
> -		__flush_dcache_area(page_address(pend_page), LPI_PENDBASE_SZ);
> +		clean_and_invalidate_dcache_va_range(pend_page, LPI_PENDBASE_SZ);
>
> -		paddr = page_to_phys(pend_page);
> -		pr_info("CPU%d: using LPI pending table @%pa\n",
> +		paddr = __pa(pend_page);
> +		its_info("CPU%d: using LPI pending table @%pa\n",
>   			smp_processor_id(), &paddr);
> -		gic_data_rdist()->pend_page = pend_page;
> +		gic_data_rdist().pend_page = pend_page;
>   	}
>
>   	/* Disable LPIs */
> @@ -971,7 +993,7 @@ static void its_cpu_init_lpis(void)
>   	dsb(sy);
>
>   	/* set PROPBASE */
> -	val = (page_to_phys(gic_rdists->prop_page) |
> +	val = (__pa(gic_rdists->prop_page)   |
>   	       GICR_PROPBASER_InnerShareable |
>   	       GICR_PROPBASER_WaWb |
>   	       ((LPI_NRBITS - 1) & GICR_PROPBASER_IDBITS_MASK));
> @@ -980,12 +1002,12 @@ static void its_cpu_init_lpis(void)
>   	tmp = readq_relaxed(rbase + GICR_PROPBASER);
>
>   	if ((tmp ^ val) & GICR_PROPBASER_SHAREABILITY_MASK) {
> -		pr_info_once("GIC: using cache flushing for LPI property table\n");
> +		its_info("GIC: using cache flushing for LPI property table\n");

Not really the same.

[..]

> -static int its_alloc_device_irq(struct its_device *dev, irq_hw_number_t *hwirq)
> +/* TODO: Remove static for the sake of compilation */
> +int its_alloc_device_irq(struct its_device *dev, int *hwirq)
>   {
>   	int idx;
>
> @@ -1139,6 +1169,8 @@ static int its_alloc_device_irq(struct its_device *dev, irq_hw_number_t *hwirq)
>   	return 0;
>   }
>
> +/* pci and msi handling no more required here */

Hmmm why?

> +#if 0
>   struct its_pci_alias {
>   	struct pci_dev	*pdev;
>   	u32		dev_id;
> @@ -1218,6 +1250,9 @@ static struct msi_domain_info its_pci_msi_domain_info = {
>   	.chip	= &its_msi_irq_chip,
>   };
>
> +#endif
> +/* IRQ domain management is not required */
> +#if 0
>   static int its_irq_gic_domain_alloc(struct irq_domain *domain,
>   				    unsigned int virq,
>   				    irq_hw_number_t hwirq)
> @@ -1319,6 +1354,7 @@ static const struct irq_domain_ops its_domain_ops = {
>   	.activate		= its_irq_domain_activate,
>   	.deactivate		= its_irq_domain_deactivate,
>   };
> +#endif
>
>   static int its_force_quiescent(void __iomem *base)
>   {
> @@ -1348,58 +1384,57 @@ static int its_force_quiescent(void __iomem *base)
>   	}
>   }
>
> -static int its_probe(struct device_node *node, struct irq_domain *parent)
> +static int its_probe(struct dt_device_node *node)

[..]

>   	err = its_force_quiescent(its_base);
>   	if (err) {
> -		pr_warn("%s: failed to quiesce, giving up\n",
> +		its_warn("%s: failed to quiesce, giving up\n",
>   			node->full_name);

Indentation.

[..]

>   	if ((tmp ^ baser) & GITS_BASER_SHAREABILITY_MASK) {
> -		pr_info("ITS: using cache flushing for cmd queue\n");
> +		its_info("ITS: using cache flushing for cmd queue\n");
>   		its->flags |= ITS_FLAGS_CMDQ_NEEDS_FLUSHING;
>   	}
> -

Spurious change

> +#if 0
>   	if (of_property_read_bool(its->msi_chip.of_node, "msi-controller")) {
>   		its->domain = irq_domain_add_tree(NULL, &its_domain_ops, its);
>   		if (!its->domain) {
> @@ -1451,27 +1486,28 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
>   		if (err)
>   			goto out_free_domains;
>   	}
> -

Ditto

> +#endif
>   	spin_lock(&its_lock);
>   	list_add(&its->entry, &its_nodes);
>   	spin_unlock(&its_lock);
>
>   	return 0;
> -

Ditto

> +#if 0
>   out_free_domains:

[..]

> -static struct of_device_id its_device_id[] = {
> -	{	.compatible	= "arm,gic-v3-its",	},
> -	{},
> -};
> -
> -int its_init(struct device_node *node, struct rdists *rdists,
> -	     struct irq_domain *parent_domain)
> +int its_init(struct dt_device_node *node, struct rdist_prop *rdists)
>   {
> -	struct device_node *np;
> +	struct dt_device_node *np = NULL;
> +
> +	static const struct dt_device_match its_device_ids[] __initconst =
> +	{
> +		DT_MATCH_COMPATIBLE("arm,gic-v3-its"),
> +		{ /* sentinel */ },
> +	};
> +

Already said on V1: of_device_id and dt_device_match are compatible. If 
you change the name it will work too...

> +	while ((np = dt_find_matching_node(np, its_device_ids)))
> +	{
> +		if (!dt_find_property(np, "msi-controller", NULL))
> +		continue;

In your cover letter, you said you support multiple ITS node but this 
piece of code show that it's not the case...

> +	}
>
> -	for (np = of_find_matching_node(node, its_device_id); np;
> -	     np = of_find_matching_node(np, its_device_id)) {
> -		its_probe(np, parent_domain);

The for loop was perfect, why did you drop it?

> +	if (np) {
> +		its_probe(np);
>   	}
>
>   	if (list_empty(&its_nodes)) {
> -		pr_warn("ITS: No ITS available, not enabling LPIs\n");
> +		its_warn("ITS: No ITS available, not enabling LPIs\n");
>   		return -ENXIO;
>   	}
>
> diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h
> index 4e64b56..f8bac52 100644
> --- a/xen/include/asm-arm/gic_v3_defs.h
> +++ b/xen/include/asm-arm/gic_v3_defs.h
> @@ -59,11 +59,12 @@
>   #define GICR_WAKER_ProcessorSleep    (1U << 1)
>   #define GICR_WAKER_ChildrenAsleep    (1U << 2)
>
> -#define GICD_PIDR2_ARCH_REV_MASK     (0xf0)
> +#define GIC_PIDR2_ARCH_REV_MASK      (0xf0)
> +#define GICD_PIDR2_ARCH_REV_MASK     GIC_PIDR2_ARCH_REV_MASK

Why do you define GIC_PIDR2_ARCH_REV_MASK? It's not consistent with the 
other part of the code.

>   #define GICD_PIDR2_ARCH_REV_SHIFT    (0x4)
>   #define GICD_PIDR2_ARCH_GICV3        (0x3)
>
> -#define GICR_PIDR2_ARCH_REV_MASK     GICD_PIDR2_ARCH_REV_MASK
> +#define GICR_PIDR2_ARCH_REV_MASK     GIC_PIDR2_ARCH_REV_MASK

Why this change? GICD_PIDR2_ARCH_REV_MASK still exists...

Regards,

-- 
Julien Grall