[kvm-unit-tests,03/17] arm: gic: Provide per-IRQ helper functions
diff mbox series

Message ID 20191108144240.204202-4-andre.przywara@arm.com
State New
Headers show
Series
  • arm: gic: Test SPIs and interrupt groups
Related show

Commit Message

Andre Przywara Nov. 8, 2019, 2:42 p.m. UTC
A common theme when accessing per-IRQ parameters in the GIC distributor
is to set fields of a certain bit width in a range of MMIO registers.
Examples are the enabled status (one bit per IRQ), the level/edge
configuration (2 bits per IRQ) or the priority (8 bits per IRQ).

Add a generic helper function which is able to mask and set the
respective number of bits, given the IRQ number and the MMIO offset.
Provide wrappers using this function to easily allow configuring an IRQ.

For now assume that private IRQ numbers always refer to the current CPU.
In a GICv2 accessing the "other" private IRQs is not easily doable (the
registers are banked per CPU on the same MMIO address), so we impose the
same limitation on GICv3, even though those registers are not banked
there anymore.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 lib/arm/asm/gic-v3.h |  1 +
 lib/arm/asm/gic.h    |  9 +++++
 lib/arm/gic.c        | 90 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+)

Comments

Alexandru Elisei Nov. 12, 2019, 12:51 p.m. UTC | #1
Hi,

On 11/8/19 2:42 PM, Andre Przywara wrote:
> A common theme when accessing per-IRQ parameters in the GIC distributor
> is to set fields of a certain bit width in a range of MMIO registers.
> Examples are the enabled status (one bit per IRQ), the level/edge
> configuration (2 bits per IRQ) or the priority (8 bits per IRQ).
>
> Add a generic helper function which is able to mask and set the
> respective number of bits, given the IRQ number and the MMIO offset.
> Provide wrappers using this function to easily allow configuring an IRQ.
>
> For now assume that private IRQ numbers always refer to the current CPU.
> In a GICv2 accessing the "other" private IRQs is not easily doable (the
> registers are banked per CPU on the same MMIO address), so we impose the
> same limitation on GICv3, even though those registers are not banked
> there anymore.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  lib/arm/asm/gic-v3.h |  1 +
>  lib/arm/asm/gic.h    |  9 +++++
>  lib/arm/gic.c        | 90 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 100 insertions(+)
>
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index ed6a5ad..8cfaed1 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -23,6 +23,7 @@
>  #define GICD_CTLR_ENABLE_G1A		(1U << 1)
>  #define GICD_CTLR_ENABLE_G1		(1U << 0)
>  
> +#define GICD_IROUTER			0x6000
>  #define GICD_PIDR2			0xffe8
>  
>  /* Re-Distributor registers, offsets from RD_base */
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index 1fc10a0..21cdb58 100644
> --- a/lib/arm/asm/gic.h
> +++ b/lib/arm/asm/gic.h
> @@ -15,6 +15,7 @@
>  #define GICD_IIDR			0x0008
>  #define GICD_IGROUPR			0x0080
>  #define GICD_ISENABLER			0x0100
> +#define GICD_ICENABLER			0x0180
>  #define GICD_ISPENDR			0x0200
>  #define GICD_ICPENDR			0x0280
>  #define GICD_ISACTIVER			0x0300
> @@ -73,5 +74,13 @@ extern void gic_write_eoir(u32 irqstat);
>  extern void gic_ipi_send_single(int irq, int cpu);
>  extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
>  
> +void gic_set_irq_bit(int irq, int offset);
> +void gic_enable_irq(int irq);
> +void gic_disable_irq(int irq);
> +void gic_set_irq_priority(int irq, u8 prio);
> +void gic_set_irq_target(int irq, int cpu);
> +void gic_set_irq_group(int irq, int group);
> +int gic_get_irq_group(int irq);
> +
>  #endif /* !__ASSEMBLY__ */
>  #endif /* _ASMARM_GIC_H_ */
> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> index 9430116..cf4e811 100644
> --- a/lib/arm/gic.c
> +++ b/lib/arm/gic.c
> @@ -146,3 +146,93 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
>  	assert(gic_common_ops && gic_common_ops->ipi_send_mask);
>  	gic_common_ops->ipi_send_mask(irq, dest);
>  }
> +
> +enum gic_bit_access {
> +	ACCESS_READ,
> +	ACCESS_SET,
> +	ACCESS_RMW
> +};
> +
> +static u8 gic_masked_irq_bits(int irq, int offset, int bits, u8 value,
> +			      enum gic_bit_access access)
> +{
> +	void *base;
> +	int split = 32 / bits;
> +	int shift = (irq % split) * bits;
> +	u32 reg, mask = ((1U << bits) - 1) << shift;
> +
> +	switch (gic_version()) {
> +	case 2:
> +		base = gicv2_dist_base();
> +		break;
> +	case 3:
> +		if (irq < 32)
> +			base = gicv3_sgi_base();
> +		else
> +			base = gicv3_dist_base();
> +		break;
> +	default:
> +		return 0;
> +	}
> +	base += offset + (irq / split) * 4;

This is probably not what you intended, if irq = 4 and split = 8, (irq / split) *
4 = 0. On the other hand, irq * 4 / split = 2.

> +
> +	switch (access) {
> +	case ACCESS_READ:
> +		return (readl(base) & mask) >> shift;
> +	case ACCESS_SET:
> +		reg = 0;
> +		break;
> +	case ACCESS_RMW:
> +		reg = readl(base) & ~mask;
> +		break;
> +	}
> +
> +	writel(reg | ((u32)value << shift), base);
> +
> +	return 0;
> +}
This function looks a bit out of place:
- the function name has a verb in the past tense ('masked'), which makes me think
it should return a bool, but the function actually performs an access to a GIC
register.
- the return value is an u8, but it returns an u32 on a read, because readl
returns an u32.
- the semantics of the function and the return value change based on the access
parameter; worse yet, the return value on a write is completely ignored by the
callers and the value parameter is ignored on reads.

You could split it into separate functions - see below.

> +
> +void gic_set_irq_bit(int irq, int offset)
> +{
> +	gic_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET);
> +}
> +
> +void gic_enable_irq(int irq)
> +{
> +	gic_set_irq_bit(irq, GICD_ISENABLER);
> +}
> +
> +void gic_disable_irq(int irq)
> +{
> +	gic_set_irq_bit(irq, GICD_ICENABLER);
> +}
> +
> +void gic_set_irq_priority(int irq, u8 prio)
> +{
> +	gic_masked_irq_bits(irq, GICD_IPRIORITYR, 8, prio, ACCESS_RMW);
> +}
> +
> +void gic_set_irq_target(int irq, int cpu)
> +{
> +	if (irq < 32)
> +		return;
> +
> +	if (gic_version() == 2) {
> +		gic_masked_irq_bits(irq, GICD_ITARGETSR, 8, 1U << cpu,
> +				    ACCESS_RMW);
> +
> +		return;
> +	}
> +
> +	writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
> +}
> +
> +void gic_set_irq_group(int irq, int group)
> +{
> +	gic_masked_irq_bits(irq, GICD_IGROUPR, 1, group, ACCESS_RMW);
> +}
> +
> +int gic_get_irq_group(int irq)
> +{
> +	return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ);
> +}

The pattern for the public functions in this file is to check that the GIC has
been initialized (assert(gic_common_ops)).

I propose we rewrite the functions like this (compile tested only):

diff --git a/lib/arm/gic.c b/lib/arm/gic.c
index 94301169215c..1f5aa7b48828 100644
--- a/lib/arm/gic.c
+++ b/lib/arm/gic.c
@@ -146,3 +146,89 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
        assert(gic_common_ops && gic_common_ops->ipi_send_mask);
        gic_common_ops->ipi_send_mask(irq, dest);
 }
+
+static void *gic_get_irq_reg(int irq, int offset, int width)
+{
+       void *base;
+
+       switch (gic_version()) {
+       case 2:
+               base = gicv2_dist_base();
+               break;
+       case 3:
+               if (irq < 32)
+                       base = gicv3_sgi_base();
+               else
+                       base = gicv3_dist_base();
+               break;
+       default:
+               return 0;
+       }
+
+       return base + offset + (irq * width / 32);
+}
+
+static void gic_set_irq_field(int irq, int offset, int width, u32 value)
+{
+       void *reg;
+       u32 val;
+       int shift = (irq * width) % 32;
+       u32 mask = ((1U << width) - 1) << shift;
+
+       reg = gic_get_irq_reg(irq, offset, width);
+       val = readl(reg);
+       val = (val & ~mask) | (value << shift);
+       writel(val, reg);
+}
+
+void gic_enable_irq(int irq)
+{
+       assert(gic_common_ops);
+       gic_set_irq_field(irq, GICD_ISENABLER, 1, 1);
+}
+
+void gic_disable_irq(int irq)
+{
+       assert(gic_common_ops);
+       gic_set_irq_field(irq, GICD_ICENABLER, 1, 1);
+}
+
+void gic_set_irq_priority(int irq, u8 prio)
+{
+       assert(gic_common_ops);
+       gic_set_irq_field(irq, GICD_IPRIORITYR, 8, prio);
+}
+
+void gic_set_irq_target(int irq, int cpu)
+{
+       assert(gic_common_ops);
+
+       if (irq < 32)
+               return;
+
+       if (gic_version() == 2) {
+               gic_set_irq_field(irq, GICD_ITARGETSR, 8, 1U << cpu);
+               return;
+       }
+
+       writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
+}
+
+void gic_set_irq_group(int irq, int group)
+{
+       assert(gic_common_ops);
+       gic_set_irq_field(irq, GICD_IGROUPR, 1, 1);
+}
+
+int gic_get_irq_group(int irq)
+{
+       void *reg;
+       u32 val;
+       int shift = irq % 32;
+
+       assert(gic_common_ops);
+       reg = gic_get_irq_reg(irq, GICD_IGROUPR, 1);
+       val = readl(reg);
+
+       return (val >> shift) & 0x1;
+}

A bit more lines of code, but to me more readable. What do you think?
Auger Eric Nov. 12, 2019, 1:49 p.m. UTC | #2
Hi Andre

On 11/8/19 3:42 PM, Andre Przywara wrote:
> A common theme when accessing per-IRQ parameters in the GIC distributor
> is to set fields of a certain bit width in a range of MMIO registers.
> Examples are the enabled status (one bit per IRQ), the level/edge
> configuration (2 bits per IRQ) or the priority (8 bits per IRQ).
> 
> Add a generic helper function which is able to mask and set the
> respective number of bits, given the IRQ number and the MMIO offset.
> Provide wrappers using this function to easily allow configuring an IRQ.
> 
> For now assume that private IRQ numbers always refer to the current CPU.
> In a GICv2 accessing the "other" private IRQs is not easily doable (the
> registers are banked per CPU on the same MMIO address), so we impose the
> same limitation on GICv3, even though those registers are not banked
> there anymore.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  lib/arm/asm/gic-v3.h |  1 +
>  lib/arm/asm/gic.h    |  9 +++++
>  lib/arm/gic.c        | 90 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 100 insertions(+)
> 
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index ed6a5ad..8cfaed1 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -23,6 +23,7 @@
>  #define GICD_CTLR_ENABLE_G1A		(1U << 1)
>  #define GICD_CTLR_ENABLE_G1		(1U << 0)
>  
> +#define GICD_IROUTER			0x6000
>  #define GICD_PIDR2			0xffe8
>  
>  /* Re-Distributor registers, offsets from RD_base */
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index 1fc10a0..21cdb58 100644
> --- a/lib/arm/asm/gic.h
> +++ b/lib/arm/asm/gic.h
> @@ -15,6 +15,7 @@
>  #define GICD_IIDR			0x0008
>  #define GICD_IGROUPR			0x0080
>  #define GICD_ISENABLER			0x0100
> +#define GICD_ICENABLER			0x0180
>  #define GICD_ISPENDR			0x0200
>  #define GICD_ICPENDR			0x0280
>  #define GICD_ISACTIVER			0x0300
> @@ -73,5 +74,13 @@ extern void gic_write_eoir(u32 irqstat);
>  extern void gic_ipi_send_single(int irq, int cpu);
>  extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
>  
> +void gic_set_irq_bit(int irq, int offset);
> +void gic_enable_irq(int irq);
> +void gic_disable_irq(int irq);
> +void gic_set_irq_priority(int irq, u8 prio);
> +void gic_set_irq_target(int irq, int cpu);
> +void gic_set_irq_group(int irq, int group);
> +int gic_get_irq_group(int irq);
> +
>  #endif /* !__ASSEMBLY__ */
>  #endif /* _ASMARM_GIC_H_ */
> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> index 9430116..cf4e811 100644
> --- a/lib/arm/gic.c
> +++ b/lib/arm/gic.c
> @@ -146,3 +146,93 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
>  	assert(gic_common_ops && gic_common_ops->ipi_send_mask);
>  	gic_common_ops->ipi_send_mask(irq, dest);
>  }
> +
> +enum gic_bit_access {
> +	ACCESS_READ,
> +	ACCESS_SET,
> +	ACCESS_RMW
> +};
> +
> +static u8 gic_masked_irq_bits(int irq, int offset, int bits, u8 value,
> +			      enum gic_bit_access access)
> +{
> +	void *base;
> +	int split = 32 / bits;
> +	int shift = (irq % split) * bits;
> +	u32 reg, mask = ((1U << bits) - 1) << shift;
> +
> +	switch (gic_version()) {
> +	case 2:
> +		base = gicv2_dist_base();
> +		break;
> +	case 3:
> +		if (irq < 32)
> +			base = gicv3_sgi_base();
> +		else
> +			base = gicv3_dist_base();
> +		break;
> +	default:
> +		return 0;
> +	}
> +	base += offset + (irq / split) * 4;
> +
> +	switch (access) {
> +	case ACCESS_READ:
> +		return (readl(base) & mask) >> shift;
> +	case ACCESS_SET:
> +		reg = 0;
> +		break;
> +	case ACCESS_RMW:
> +		reg = readl(base) & ~mask;
> +		break;
> +	}
> +
> +	writel(reg | ((u32)value << shift), base);
value & mask may be safer
> +
> +	return 0;
> +}
> +
> +void gic_set_irq_bit(int irq, int offset)
> +{
> +	gic_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET);
Why don't we use ACCESS_RMW?
> +}
> +
> +void gic_enable_irq(int irq)
> +{
> +	gic_set_irq_bit(irq, GICD_ISENABLER);
here we just want to touch one bit and not erase other bits in the word?
> +}
> +
> +void gic_disable_irq(int irq)
> +{
> +	gic_set_irq_bit(irq, GICD_ICENABLER);
> +}
> +
> +void gic_set_irq_priority(int irq, u8 prio)
> +{
> +	gic_masked_irq_bits(irq, GICD_IPRIORITYR, 8, prio, ACCESS_RMW);
> +}
> +
> +void gic_set_irq_target(int irq, int cpu)
> +{
> +	if (irq < 32)
> +		return;
> +
> +	if (gic_version() == 2) {
> +		gic_masked_irq_bits(irq, GICD_ITARGETSR, 8, 1U << cpu,
> +				    ACCESS_RMW);
> +
> +		return;
> +	}
> +
> +	writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
> +}
> +
> +void gic_set_irq_group(int irq, int group)
> +{
> +	gic_masked_irq_bits(irq, GICD_IGROUPR, 1, group, ACCESS_RMW);
> +}
> +
> +int gic_get_irq_group(int irq)
> +{
> +	return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ);
> +}
> 
Thanks

Eric
Auger Eric Nov. 12, 2019, 3:53 p.m. UTC | #3
Hi Alex,

On 11/12/19 1:51 PM, Alexandru Elisei wrote:
> Hi,
> 
> On 11/8/19 2:42 PM, Andre Przywara wrote:
>> A common theme when accessing per-IRQ parameters in the GIC distributor
>> is to set fields of a certain bit width in a range of MMIO registers.
>> Examples are the enabled status (one bit per IRQ), the level/edge
>> configuration (2 bits per IRQ) or the priority (8 bits per IRQ).
>>
>> Add a generic helper function which is able to mask and set the
>> respective number of bits, given the IRQ number and the MMIO offset.
>> Provide wrappers using this function to easily allow configuring an IRQ.
>>
>> For now assume that private IRQ numbers always refer to the current CPU.
>> In a GICv2 accessing the "other" private IRQs is not easily doable (the
>> registers are banked per CPU on the same MMIO address), so we impose the
>> same limitation on GICv3, even though those registers are not banked
>> there anymore.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  lib/arm/asm/gic-v3.h |  1 +
>>  lib/arm/asm/gic.h    |  9 +++++
>>  lib/arm/gic.c        | 90 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 100 insertions(+)
>>
>> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
>> index ed6a5ad..8cfaed1 100644
>> --- a/lib/arm/asm/gic-v3.h
>> +++ b/lib/arm/asm/gic-v3.h
>> @@ -23,6 +23,7 @@
>>  #define GICD_CTLR_ENABLE_G1A		(1U << 1)
>>  #define GICD_CTLR_ENABLE_G1		(1U << 0)
>>  
>> +#define GICD_IROUTER			0x6000
>>  #define GICD_PIDR2			0xffe8
>>  
>>  /* Re-Distributor registers, offsets from RD_base */
>> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
>> index 1fc10a0..21cdb58 100644
>> --- a/lib/arm/asm/gic.h
>> +++ b/lib/arm/asm/gic.h
>> @@ -15,6 +15,7 @@
>>  #define GICD_IIDR			0x0008
>>  #define GICD_IGROUPR			0x0080
>>  #define GICD_ISENABLER			0x0100
>> +#define GICD_ICENABLER			0x0180
>>  #define GICD_ISPENDR			0x0200
>>  #define GICD_ICPENDR			0x0280
>>  #define GICD_ISACTIVER			0x0300
>> @@ -73,5 +74,13 @@ extern void gic_write_eoir(u32 irqstat);
>>  extern void gic_ipi_send_single(int irq, int cpu);
>>  extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
>>  
>> +void gic_set_irq_bit(int irq, int offset);
>> +void gic_enable_irq(int irq);
>> +void gic_disable_irq(int irq);
>> +void gic_set_irq_priority(int irq, u8 prio);
>> +void gic_set_irq_target(int irq, int cpu);
>> +void gic_set_irq_group(int irq, int group);
>> +int gic_get_irq_group(int irq);
>> +
>>  #endif /* !__ASSEMBLY__ */
>>  #endif /* _ASMARM_GIC_H_ */
>> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
>> index 9430116..cf4e811 100644
>> --- a/lib/arm/gic.c
>> +++ b/lib/arm/gic.c
>> @@ -146,3 +146,93 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
>>  	assert(gic_common_ops && gic_common_ops->ipi_send_mask);
>>  	gic_common_ops->ipi_send_mask(irq, dest);
>>  }
>> +
>> +enum gic_bit_access {
>> +	ACCESS_READ,
>> +	ACCESS_SET,
>> +	ACCESS_RMW
>> +};
>> +
>> +static u8 gic_masked_irq_bits(int irq, int offset, int bits, u8 value,
>> +			      enum gic_bit_access access)
>> +{
>> +	void *base;
>> +	int split = 32 / bits;
>> +	int shift = (irq % split) * bits;
>> +	u32 reg, mask = ((1U << bits) - 1) << shift;
>> +
>> +	switch (gic_version()) {
>> +	case 2:
>> +		base = gicv2_dist_base();
>> +		break;
>> +	case 3:
>> +		if (irq < 32)
>> +			base = gicv3_sgi_base();
>> +		else
>> +			base = gicv3_dist_base();
>> +		break;
>> +	default:
>> +		return 0;
>> +	}
>> +	base += offset + (irq / split) * 4;
> 
> This is probably not what you intended, if irq = 4 and split = 8, (irq / split) *
> 4 = 0. On the other hand, irq * 4 / split = 2.

I think that's correct. if bits = 4 this means there are 8 of such
fields in a word and the field corresponding to irq=4 is indeed located
in word 0.

Thanks

Eric
> 
>> +
>> +	switch (access) {
>> +	case ACCESS_READ:
>> +		return (readl(base) & mask) >> shift;
>> +	case ACCESS_SET:
>> +		reg = 0;
>> +		break;
>> +	case ACCESS_RMW:
>> +		reg = readl(base) & ~mask;
>> +		break;
>> +	}
>> +
>> +	writel(reg | ((u32)value << shift), base);
>> +
>> +	return 0;
>> +}
> This function looks a bit out of place:
> - the function name has a verb in the past tense ('masked'), which makes me think
> it should return a bool, but the function actually performs an access to a GIC
> register.
> - the return value is an u8, but it returns an u32 on a read, because readl
> returns an u32.
> - the semantics of the function and the return value change based on the access
> parameter; worse yet, the return value on a write is completely ignored by the
> callers and the value parameter is ignored on reads.
> 
> You could split it into separate functions - see below.
> 
>> +
>> +void gic_set_irq_bit(int irq, int offset)
>> +{
>> +	gic_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET);
>> +}
>> +
>> +void gic_enable_irq(int irq)
>> +{
>> +	gic_set_irq_bit(irq, GICD_ISENABLER);
>> +}
>> +
>> +void gic_disable_irq(int irq)
>> +{
>> +	gic_set_irq_bit(irq, GICD_ICENABLER);
>> +}
>> +
>> +void gic_set_irq_priority(int irq, u8 prio)
>> +{
>> +	gic_masked_irq_bits(irq, GICD_IPRIORITYR, 8, prio, ACCESS_RMW);
>> +}
>> +
>> +void gic_set_irq_target(int irq, int cpu)
>> +{
>> +	if (irq < 32)
>> +		return;
>> +
>> +	if (gic_version() == 2) {
>> +		gic_masked_irq_bits(irq, GICD_ITARGETSR, 8, 1U << cpu,
>> +				    ACCESS_RMW);
>> +
>> +		return;
>> +	}
>> +
>> +	writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
>> +}
>> +
>> +void gic_set_irq_group(int irq, int group)
>> +{
>> +	gic_masked_irq_bits(irq, GICD_IGROUPR, 1, group, ACCESS_RMW);
>> +}
>> +
>> +int gic_get_irq_group(int irq)
>> +{
>> +	return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ);
>> +}
> 
> The pattern for the public functions in this file is to check that the GIC has
> been initialized (assert(gic_common_ops)).
> 
> I propose we rewrite the functions like this (compile tested only):
> 
> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> index 94301169215c..1f5aa7b48828 100644
> --- a/lib/arm/gic.c
> +++ b/lib/arm/gic.c
> @@ -146,3 +146,89 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
>         assert(gic_common_ops && gic_common_ops->ipi_send_mask);
>         gic_common_ops->ipi_send_mask(irq, dest);
>  }
> +
> +static void *gic_get_irq_reg(int irq, int offset, int width)
> +{
> +       void *base;
> +
> +       switch (gic_version()) {
> +       case 2:
> +               base = gicv2_dist_base();
> +               break;
> +       case 3:
> +               if (irq < 32)
> +                       base = gicv3_sgi_base();
> +               else
> +                       base = gicv3_dist_base();
> +               break;
> +       default:
> +               return 0;
> +       }
> +
> +       return base + offset + (irq * width / 32);
> +}
> +
> +static void gic_set_irq_field(int irq, int offset, int width, u32 value)
> +{
> +       void *reg;
> +       u32 val;
> +       int shift = (irq * width) % 32;
> +       u32 mask = ((1U << width) - 1) << shift;
> +
> +       reg = gic_get_irq_reg(irq, offset, width);
> +       val = readl(reg);
> +       val = (val & ~mask) | (value << shift);
> +       writel(val, reg);
> +}
> +
> +void gic_enable_irq(int irq)
> +{
> +       assert(gic_common_ops);
> +       gic_set_irq_field(irq, GICD_ISENABLER, 1, 1);
> +}
> +
> +void gic_disable_irq(int irq)
> +{
> +       assert(gic_common_ops);
> +       gic_set_irq_field(irq, GICD_ICENABLER, 1, 1);
> +}
> +
> +void gic_set_irq_priority(int irq, u8 prio)
> +{
> +       assert(gic_common_ops);
> +       gic_set_irq_field(irq, GICD_IPRIORITYR, 8, prio);
> +}
> +
> +void gic_set_irq_target(int irq, int cpu)
> +{
> +       assert(gic_common_ops);
> +
> +       if (irq < 32)
> +               return;
> +
> +       if (gic_version() == 2) {
> +               gic_set_irq_field(irq, GICD_ITARGETSR, 8, 1U << cpu);
> +               return;
> +       }
> +
> +       writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
> +}
> +
> +void gic_set_irq_group(int irq, int group)
> +{
> +       assert(gic_common_ops);
> +       gic_set_irq_field(irq, GICD_IGROUPR, 1, 1);
> +}
> +
> +int gic_get_irq_group(int irq)
> +{
> +       void *reg;
> +       u32 val;
> +       int shift = irq % 32;
> +
> +       assert(gic_common_ops);
> +       reg = gic_get_irq_reg(irq, GICD_IGROUPR, 1);
> +       val = readl(reg);
> +
> +       return (val >> shift) & 0x1;
> +}
> 
> A bit more lines of code, but to me more readable. What do you think?
> 
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>
Alexandru Elisei Nov. 12, 2019, 4:53 p.m. UTC | #4
Hi,

On 11/12/19 3:53 PM, Auger Eric wrote:
> Hi Alex,
>
> On 11/12/19 1:51 PM, Alexandru Elisei wrote:
>> Hi,
>>
>> On 11/8/19 2:42 PM, Andre Przywara wrote:
>>> A common theme when accessing per-IRQ parameters in the GIC distributor
>>> is to set fields of a certain bit width in a range of MMIO registers.
>>> Examples are the enabled status (one bit per IRQ), the level/edge
>>> configuration (2 bits per IRQ) or the priority (8 bits per IRQ).
>>>
>>> Add a generic helper function which is able to mask and set the
>>> respective number of bits, given the IRQ number and the MMIO offset.
>>> Provide wrappers using this function to easily allow configuring an IRQ.
>>>
>>> For now assume that private IRQ numbers always refer to the current CPU.
>>> In a GICv2 accessing the "other" private IRQs is not easily doable (the
>>> registers are banked per CPU on the same MMIO address), so we impose the
>>> same limitation on GICv3, even though those registers are not banked
>>> there anymore.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  lib/arm/asm/gic-v3.h |  1 +
>>>  lib/arm/asm/gic.h    |  9 +++++
>>>  lib/arm/gic.c        | 90 ++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 100 insertions(+)
>>>
>>> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
>>> index ed6a5ad..8cfaed1 100644
>>> --- a/lib/arm/asm/gic-v3.h
>>> +++ b/lib/arm/asm/gic-v3.h
>>> @@ -23,6 +23,7 @@
>>>  #define GICD_CTLR_ENABLE_G1A		(1U << 1)
>>>  #define GICD_CTLR_ENABLE_G1		(1U << 0)
>>>  
>>> +#define GICD_IROUTER			0x6000
>>>  #define GICD_PIDR2			0xffe8
>>>  
>>>  /* Re-Distributor registers, offsets from RD_base */
>>> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
>>> index 1fc10a0..21cdb58 100644
>>> --- a/lib/arm/asm/gic.h
>>> +++ b/lib/arm/asm/gic.h
>>> @@ -15,6 +15,7 @@
>>>  #define GICD_IIDR			0x0008
>>>  #define GICD_IGROUPR			0x0080
>>>  #define GICD_ISENABLER			0x0100
>>> +#define GICD_ICENABLER			0x0180
>>>  #define GICD_ISPENDR			0x0200
>>>  #define GICD_ICPENDR			0x0280
>>>  #define GICD_ISACTIVER			0x0300
>>> @@ -73,5 +74,13 @@ extern void gic_write_eoir(u32 irqstat);
>>>  extern void gic_ipi_send_single(int irq, int cpu);
>>>  extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
>>>  
>>> +void gic_set_irq_bit(int irq, int offset);
>>> +void gic_enable_irq(int irq);
>>> +void gic_disable_irq(int irq);
>>> +void gic_set_irq_priority(int irq, u8 prio);
>>> +void gic_set_irq_target(int irq, int cpu);
>>> +void gic_set_irq_group(int irq, int group);
>>> +int gic_get_irq_group(int irq);
>>> +
>>>  #endif /* !__ASSEMBLY__ */
>>>  #endif /* _ASMARM_GIC_H_ */
>>> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
>>> index 9430116..cf4e811 100644
>>> --- a/lib/arm/gic.c
>>> +++ b/lib/arm/gic.c
>>> @@ -146,3 +146,93 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
>>>  	assert(gic_common_ops && gic_common_ops->ipi_send_mask);
>>>  	gic_common_ops->ipi_send_mask(irq, dest);
>>>  }
>>> +
>>> +enum gic_bit_access {
>>> +	ACCESS_READ,
>>> +	ACCESS_SET,
>>> +	ACCESS_RMW
>>> +};
>>> +
>>> +static u8 gic_masked_irq_bits(int irq, int offset, int bits, u8 value,
>>> +			      enum gic_bit_access access)
>>> +{
>>> +	void *base;
>>> +	int split = 32 / bits;
>>> +	int shift = (irq % split) * bits;
>>> +	u32 reg, mask = ((1U << bits) - 1) << shift;
>>> +
>>> +	switch (gic_version()) {
>>> +	case 2:
>>> +		base = gicv2_dist_base();
>>> +		break;
>>> +	case 3:
>>> +		if (irq < 32)
>>> +			base = gicv3_sgi_base();
>>> +		else
>>> +			base = gicv3_dist_base();
>>> +		break;
>>> +	default:
>>> +		return 0;
>>> +	}
>>> +	base += offset + (irq / split) * 4;
>> This is probably not what you intended, if irq = 4 and split = 8, (irq / split) *
>> 4 = 0. On the other hand, irq * 4 / split = 2.
> I think that's correct. if bits = 4 this means there are 8 of such
> fields in a word and the field corresponding to irq=4 is indeed located
> in word 0.

You're right, I got confused about the 4. Now I realize that the 4 represents the
size of a register.

Thanks,
Alex
> Thanks
>
> Eric
>>> +
>>> +	switch (access) {
>>> +	case ACCESS_READ:
>>> +		return (readl(base) & mask) >> shift;
>>> +	case ACCESS_SET:
>>> +		reg = 0;
>>> +		break;
>>> +	case ACCESS_RMW:
>>> +		reg = readl(base) & ~mask;
>>> +		break;
>>> +	}
>>> +
>>> +	writel(reg | ((u32)value << shift), base);
>>> +
>>> +	return 0;
>>> +}
>> This function looks a bit out of place:
>> - the function name has a verb in the past tense ('masked'), which makes me think
>> it should return a bool, but the function actually performs an access to a GIC
>> register.
>> - the return value is an u8, but it returns an u32 on a read, because readl
>> returns an u32.
>> - the semantics of the function and the return value change based on the access
>> parameter; worse yet, the return value on a write is completely ignored by the
>> callers and the value parameter is ignored on reads.
>>
>> You could split it into separate functions - see below.
>>
>>> +
>>> +void gic_set_irq_bit(int irq, int offset)
>>> +{
>>> +	gic_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET);
>>> +}
>>> +
>>> +void gic_enable_irq(int irq)
>>> +{
>>> +	gic_set_irq_bit(irq, GICD_ISENABLER);
>>> +}
>>> +
>>> +void gic_disable_irq(int irq)
>>> +{
>>> +	gic_set_irq_bit(irq, GICD_ICENABLER);
>>> +}
>>> +
>>> +void gic_set_irq_priority(int irq, u8 prio)
>>> +{
>>> +	gic_masked_irq_bits(irq, GICD_IPRIORITYR, 8, prio, ACCESS_RMW);
>>> +}
>>> +
>>> +void gic_set_irq_target(int irq, int cpu)
>>> +{
>>> +	if (irq < 32)
>>> +		return;
>>> +
>>> +	if (gic_version() == 2) {
>>> +		gic_masked_irq_bits(irq, GICD_ITARGETSR, 8, 1U << cpu,
>>> +				    ACCESS_RMW);
>>> +
>>> +		return;
>>> +	}
>>> +
>>> +	writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
>>> +}
>>> +
>>> +void gic_set_irq_group(int irq, int group)
>>> +{
>>> +	gic_masked_irq_bits(irq, GICD_IGROUPR, 1, group, ACCESS_RMW);
>>> +}
>>> +
>>> +int gic_get_irq_group(int irq)
>>> +{
>>> +	return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ);
>>> +}
>> The pattern for the public functions in this file is to check that the GIC has
>> been initialized (assert(gic_common_ops)).
>>
>> I propose we rewrite the functions like this (compile tested only):
>>
>> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
>> index 94301169215c..1f5aa7b48828 100644
>> --- a/lib/arm/gic.c
>> +++ b/lib/arm/gic.c
>> @@ -146,3 +146,89 @@ void gic_ipi_send_mask(int irq, const cpumask_t *dest)
>>         assert(gic_common_ops && gic_common_ops->ipi_send_mask);
>>         gic_common_ops->ipi_send_mask(irq, dest);
>>  }
>> +
>> +static void *gic_get_irq_reg(int irq, int offset, int width)
>> +{
>> +       void *base;
>> +
>> +       switch (gic_version()) {
>> +       case 2:
>> +               base = gicv2_dist_base();
>> +               break;
>> +       case 3:
>> +               if (irq < 32)
>> +                       base = gicv3_sgi_base();
>> +               else
>> +                       base = gicv3_dist_base();
>> +               break;
>> +       default:
>> +               return 0;
>> +       }
>> +
>> +       return base + offset + (irq * width / 32);
>> +}
>> +
>> +static void gic_set_irq_field(int irq, int offset, int width, u32 value)
>> +{
>> +       void *reg;
>> +       u32 val;
>> +       int shift = (irq * width) % 32;
>> +       u32 mask = ((1U << width) - 1) << shift;
>> +
>> +       reg = gic_get_irq_reg(irq, offset, width);
>> +       val = readl(reg);
>> +       val = (val & ~mask) | (value << shift);
>> +       writel(val, reg);
>> +}
>> +
>> +void gic_enable_irq(int irq)
>> +{
>> +       assert(gic_common_ops);
>> +       gic_set_irq_field(irq, GICD_ISENABLER, 1, 1);
>> +}
>> +
>> +void gic_disable_irq(int irq)
>> +{
>> +       assert(gic_common_ops);
>> +       gic_set_irq_field(irq, GICD_ICENABLER, 1, 1);
>> +}
>> +
>> +void gic_set_irq_priority(int irq, u8 prio)
>> +{
>> +       assert(gic_common_ops);
>> +       gic_set_irq_field(irq, GICD_IPRIORITYR, 8, prio);
>> +}
>> +
>> +void gic_set_irq_target(int irq, int cpu)
>> +{
>> +       assert(gic_common_ops);
>> +
>> +       if (irq < 32)
>> +               return;
>> +
>> +       if (gic_version() == 2) {
>> +               gic_set_irq_field(irq, GICD_ITARGETSR, 8, 1U << cpu);
>> +               return;
>> +       }
>> +
>> +       writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
>> +}
>> +
>> +void gic_set_irq_group(int irq, int group)
>> +{
>> +       assert(gic_common_ops);
>> +       gic_set_irq_field(irq, GICD_IGROUPR, 1, 1);
>> +}
>> +
>> +int gic_get_irq_group(int irq)
>> +{
>> +       void *reg;
>> +       u32 val;
>> +       int shift = irq % 32;
>> +
>> +       assert(gic_common_ops);
>> +       reg = gic_get_irq_reg(irq, GICD_IGROUPR, 1);
>> +       val = readl(reg);
>> +
>> +       return (val >> shift) & 0x1;
>> +}
>>
>> A bit more lines of code, but to me more readable. What do you think?
>>
>>
>> _______________________________________________
>> kvmarm mailing list
>> kvmarm@lists.cs.columbia.edu
>> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>>

Patch
diff mbox series

diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
index ed6a5ad..8cfaed1 100644
--- a/lib/arm/asm/gic-v3.h
+++ b/lib/arm/asm/gic-v3.h
@@ -23,6 +23,7 @@ 
 #define GICD_CTLR_ENABLE_G1A		(1U << 1)
 #define GICD_CTLR_ENABLE_G1		(1U << 0)
 
+#define GICD_IROUTER			0x6000
 #define GICD_PIDR2			0xffe8
 
 /* Re-Distributor registers, offsets from RD_base */
diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
index 1fc10a0..21cdb58 100644
--- a/lib/arm/asm/gic.h
+++ b/lib/arm/asm/gic.h
@@ -15,6 +15,7 @@ 
 #define GICD_IIDR			0x0008
 #define GICD_IGROUPR			0x0080
 #define GICD_ISENABLER			0x0100
+#define GICD_ICENABLER			0x0180
 #define GICD_ISPENDR			0x0200
 #define GICD_ICPENDR			0x0280
 #define GICD_ISACTIVER			0x0300
@@ -73,5 +74,13 @@  extern void gic_write_eoir(u32 irqstat);
 extern void gic_ipi_send_single(int irq, int cpu);
 extern void gic_ipi_send_mask(int irq, const cpumask_t *dest);
 
+void gic_set_irq_bit(int irq, int offset);
+void gic_enable_irq(int irq);
+void gic_disable_irq(int irq);
+void gic_set_irq_priority(int irq, u8 prio);
+void gic_set_irq_target(int irq, int cpu);
+void gic_set_irq_group(int irq, int group);
+int gic_get_irq_group(int irq);
+
 #endif /* !__ASSEMBLY__ */
 #endif /* _ASMARM_GIC_H_ */
diff --git a/lib/arm/gic.c b/lib/arm/gic.c
index 9430116..cf4e811 100644
--- a/lib/arm/gic.c
+++ b/lib/arm/gic.c
@@ -146,3 +146,93 @@  void gic_ipi_send_mask(int irq, const cpumask_t *dest)
 	assert(gic_common_ops && gic_common_ops->ipi_send_mask);
 	gic_common_ops->ipi_send_mask(irq, dest);
 }
+
+enum gic_bit_access {
+	ACCESS_READ,
+	ACCESS_SET,
+	ACCESS_RMW
+};
+
+static u8 gic_masked_irq_bits(int irq, int offset, int bits, u8 value,
+			      enum gic_bit_access access)
+{
+	void *base;
+	int split = 32 / bits;
+	int shift = (irq % split) * bits;
+	u32 reg, mask = ((1U << bits) - 1) << shift;
+
+	switch (gic_version()) {
+	case 2:
+		base = gicv2_dist_base();
+		break;
+	case 3:
+		if (irq < 32)
+			base = gicv3_sgi_base();
+		else
+			base = gicv3_dist_base();
+		break;
+	default:
+		return 0;
+	}
+	base += offset + (irq / split) * 4;
+
+	switch (access) {
+	case ACCESS_READ:
+		return (readl(base) & mask) >> shift;
+	case ACCESS_SET:
+		reg = 0;
+		break;
+	case ACCESS_RMW:
+		reg = readl(base) & ~mask;
+		break;
+	}
+
+	writel(reg | ((u32)value << shift), base);
+
+	return 0;
+}
+
+void gic_set_irq_bit(int irq, int offset)
+{
+	gic_masked_irq_bits(irq, offset, 1, 1, ACCESS_SET);
+}
+
+void gic_enable_irq(int irq)
+{
+	gic_set_irq_bit(irq, GICD_ISENABLER);
+}
+
+void gic_disable_irq(int irq)
+{
+	gic_set_irq_bit(irq, GICD_ICENABLER);
+}
+
+void gic_set_irq_priority(int irq, u8 prio)
+{
+	gic_masked_irq_bits(irq, GICD_IPRIORITYR, 8, prio, ACCESS_RMW);
+}
+
+void gic_set_irq_target(int irq, int cpu)
+{
+	if (irq < 32)
+		return;
+
+	if (gic_version() == 2) {
+		gic_masked_irq_bits(irq, GICD_ITARGETSR, 8, 1U << cpu,
+				    ACCESS_RMW);
+
+		return;
+	}
+
+	writeq(cpus[cpu], gicv3_dist_base() + GICD_IROUTER + irq * 8);
+}
+
+void gic_set_irq_group(int irq, int group)
+{
+	gic_masked_irq_bits(irq, GICD_IGROUPR, 1, group, ACCESS_RMW);
+}
+
+int gic_get_irq_group(int irq)
+{
+	return gic_masked_irq_bits(irq, GICD_IGROUPR, 1, 0, ACCESS_READ);
+}