Re: [kvm-unit-tests PATCH v3 11/18] arm64: timer: Write to ICENABLER to disable timer IRQ

[Andre Przywara <andre.przywara@arm.com>]

On Tue, 31 Dec 2019 16:09:42 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

> According the Generic Interrupt Controller versions 2, 3 and 4 architecture
> specifications, a write of 0 to the GIC{D,R}_ISENABLER{,0} registers is
> ignored; this is also how KVM emulates the corresponding register. Write
> instead to the ICENABLER register when disabling the timer interrupt.
> 
> Note that fortunately for us, the timer test was still working as intended
> because KVM does the sensible thing and all interrupts are disabled by
> default when creating a VM.

Indeed, good catch!

> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  lib/arm/asm/gic-v3.h |  1 +
>  lib/arm/asm/gic.h    |  1 +
>  arm/timer.c          | 22 +++++++++++-----------
>  3 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index 347be2f9da17..0dc838b3ab2d 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -31,6 +31,7 @@
>  /* Re-Distributor registers, offsets from SGI_base */
>  #define GICR_IGROUPR0			GICD_IGROUPR
>  #define GICR_ISENABLER0			GICD_ISENABLER
> +#define GICR_ICENABLER0			GICD_ICENABLER
>  #define GICR_IPRIORITYR0		GICD_IPRIORITYR
>  
>  #define ICC_SGI1R_AFFINITY_1_SHIFT	16
> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> index 1fc10a096259..09826fd5bc29 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
> diff --git a/arm/timer.c b/arm/timer.c
> index b30fd6b6d90b..f390e8e65d31 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -17,6 +17,9 @@
>  #define ARCH_TIMER_CTL_ISTATUS (1 << 2)
>  
>  static void *gic_ispendr;
> +static void *gic_isenabler;
> +static void *gic_icenabler;
> +
>  static bool ptimer_unsupported;
>  
>  static void ptimer_unsupported_handler(struct pt_regs *regs, unsigned int esr)
> @@ -132,19 +135,12 @@ static struct timer_info ptimer_info = {
>  
>  static void set_timer_irq_enabled(struct timer_info *info, bool enabled)
>  {
> -	u32 val = 0;
> +	u32 val = 1 << PPI(info->irq);
>  
>  	if (enabled)
> -		val = 1 << PPI(info->irq);
> -
> -	switch (gic_version()) {
> -	case 2:
> -		writel(val, gicv2_dist_base() + GICD_ISENABLER + 0);
> -		break;
> -	case 3:
> -		writel(val, gicv3_sgi_base() + GICR_ISENABLER0);
> -		break;
> -	}
> +		writel(val, gic_isenabler);
> +	else
> +		writel(val, gic_icenabler);
>  }
>  
>  static void irq_handler(struct pt_regs *regs)
> @@ -306,9 +302,13 @@ static void test_init(void)
>  	switch (gic_version()) {
>  	case 2:
>  		gic_ispendr = gicv2_dist_base() + GICD_ISPENDR;
> +		gic_isenabler = gicv2_dist_base() + GICD_ISENABLER;
> +		gic_icenabler = gicv2_dist_base() + GICD_ICENABLER;
>  		break;
>  	case 3:
>  		gic_ispendr = gicv3_sgi_base() + GICD_ISPENDR;
> +		gic_isenabler = gicv3_sgi_base() + GICR_ISENABLER0;
> +		gic_icenabler = gicv3_sgi_base() + GICR_ICENABLER0;
>  		break;
>  	}
>