Re: [PATCH 05/35] irqchip/gic-v3: Add GICv4.1 VPEID size discovery

[Marc Zyngier]

On 24/09/2019 11:49, Andrew Murray wrote:
> On Mon, Sep 23, 2019 at 07:25:36PM +0100, Marc Zyngier wrote:
>> While GICv4.0 mandates 16 bit worth of VPEIDs, GICv4.1 allows smaller
> 
> s/VPEIDs/vPEIDs/
> 
>> implementations to be built. Add the required glue to dynamically
>> compute the limit.
>>
>> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
>> ---
>>  drivers/irqchip/irq-gic-v3-its.c   | 11 ++++++++++-
>>  drivers/irqchip/irq-gic-v3.c       |  3 +++
>>  include/linux/irqchip/arm-gic-v3.h |  5 +++++
>>  3 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index c94eb287393b..17b77a0b9d97 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -119,7 +119,16 @@ struct its_node {
>>  #define ITS_ITT_ALIGN		SZ_256
>>  
>>  /* The maximum number of VPEID bits supported by VLPI commands */
>> -#define ITS_MAX_VPEID_BITS	(16)
>> +#define ITS_MAX_VPEID_BITS						\
>> +	({								\
>> +		int nvpeid = 16;					\
>> +		if (gic_rdists->has_rvpeid &&				\
> 
> We use rvpeid as a way of determining if this is a GICv4.1, are there any
> other means of determining this? If we use it in this way, is there any
> benefit to having a has_gicv4_1 type of flag instead?

RVPEID *is* the way to discover a GICv4.1. To be clear, if we adopted
the ARM ARM nomenclature to describe extensions, GICv4.1 would be called
GIC-RVPEID, and that'd be it.

> Also for 'insane' configurations we set has_rvpeid to false, thus preventing
> this feature. Does it make sense to do that?

It makes perfect sense. RVPEID *and* VLPI are set to false, and we don't
do *any* direct injection, because it simply cannot work.

> GICD_TYPER2 is reserved in GICv4, however I understand this reads as RES0,
> can we just rely on that instead? (We read it below anyway).

Yes. In general for the GIC, any RESERVED register is RAZ/WI.

> 
>> +		    gic_rdists->gicd_typer2 & GICD_TYPER2_VIL)		\
>> +			nvpeid = 1 + (gic_rdists->gicd_typer2 &		\
>> +				      GICD_TYPER2_VID);			\
>> +									\
>> +		nvpeid;							\
>> +	})
>>  #define ITS_MAX_VPEID		(1 << (ITS_MAX_VPEID_BITS))
>>  
>>  /* Convert page order to size in bytes */
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index 0b545e2c3498..fb6360161d6c 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -1556,6 +1556,9 @@ static int __init gic_init_bases(void __iomem *dist_base,
>>  
>>  	pr_info("%d SPIs implemented\n", GIC_LINE_NR - 32);
>>  	pr_info("%d Extended SPIs implemented\n", GIC_ESPI_NR);
>> +
>> +	gic_data.rdists.gicd_typer2 = readl_relaxed(gic_data.dist_base + GICD_TYPER2);
>> +
>>  	gic_data.domain = irq_domain_create_tree(handle, &gic_irq_domain_ops,
>>  						 &gic_data);
>>  	irq_domain_update_bus_token(gic_data.domain, DOMAIN_BUS_WIRED);
>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
>> index b34e0c113697..71730b9def0c 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -13,6 +13,7 @@
>>  #define GICD_CTLR			0x0000
>>  #define GICD_TYPER			0x0004
>>  #define GICD_IIDR			0x0008
>> +#define GICD_TYPER2			0x000C
>>  #define GICD_STATUSR			0x0010
>>  #define GICD_SETSPI_NSR			0x0040
>>  #define GICD_CLRSPI_NSR			0x0048
>> @@ -89,6 +90,9 @@
>>  #define GICD_TYPER_ESPIS(typer)						\
>>  	(((typer) & GICD_TYPER_ESPI) ? GICD_TYPER_SPIS((typer) >> 27) : 0)
>>  
>> +#define GICD_TYPER2_VIL			(1U << 7)
>> +#define GICD_TYPER2_VID			GENMASK(4, 0)
> 
> Given that the 4th bit is reserved for future expansion and values greater
> than 0xF are reserved, is there value in changing this to GENMASK(3, 0)?

No, I'd rather leave the field to match the specification, and discard
values that go beyond 16 in the ITS_MAX_VPEID_BITS macro.

Thanks,
	
	M.
-- 
Jazz is not dead, it just smells funny...