Re: [PATCH v9 19/26] irqchip/gic-v3: Detect if GIC can support pseudo-NMIs

[Marc Zyngier <marc.zyngier@arm.com>]

On Mon, 21 Jan 2019 15:33:38 +0000,
Julien Thierry <julien.thierry@arm.com> wrote:
> 
> The values non secure EL1 needs to use for PMR and RPR registers depends on
> the value of SCR_EL3.FIQ.
> 
> The values non secure EL1 sees from the distributor and redistributor
> depend on whether security is enabled for the GIC or not.
> 
> To avoid having to deal with two sets of values for PMR
> masking/unmasking, only enable pseudo-NMIs when GIC has non-secure view
> of priorities.
> 
> Also, add firmware requirements related to SCR_EL3.
> 
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  Documentation/arm64/booting.txt |  5 ++++
>  drivers/irqchip/irq-gic-v3.c    | 58 ++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 57 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
> index 8df9f46..fbab7e2 100644
> --- a/Documentation/arm64/booting.txt
> +++ b/Documentation/arm64/booting.txt
> @@ -188,6 +188,11 @@ Before jumping into the kernel, the following conditions must be met:
>    the kernel image will be entered must be initialised by software at a
>    higher exception level to prevent execution in an UNKNOWN state.
>  
> +  - SCR_EL3.FIQ must have the same value across all CPUs the kernel is
> +    executing on.
> +  - The value of SCR_EL3.FIQ must be the same as the one present at boot
> +    time whenever the kernel is executing.
> +
>    For systems with a GICv3 interrupt controller to be used in v3 mode:
>    - If EL3 is present:
>      ICC_SRE_EL3.Enable (bit 3) must be initialiased to 0b1.
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 5a703ae..5374b43 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -66,6 +66,31 @@ struct gic_chip_data {
>  static struct gic_chip_data gic_data __read_mostly;
>  static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key);
>  
> +/*
> + * The behaviours of RPR and PMR registers differ depending on the value of
> + * SCR_EL3.FIQ, and the behaviour of non-secure priority registers of the
> + * distributor and redistributors depends on whether security is enabled in the
> + * GIC.
> + *
> + * When security is enabled, non-secure priority values from the (re)distributor
> + * are presented to the GIC CPUIF as follow:
> + *     (GIC_(R)DIST_PRI[irq] >> 1) | 0x80;
> + *
> + * If SCR_EL3.FIQ == 1, the values writen to/read from PMR and RPR at non-secure
> + * EL1 are subject to a similar operation thus matching the priorities presented
> + * from the (re)distributor when security is enabled.
> + *
> + * see GICv3/GICv4 Architecture Specification (IHI0069D):
> + * - section 4.8.1 Non-secure accesses to register fields for Secure interrupt
> + *   priorities.
> + * - Figure 4-7 Secure read of the priority field for a Non-secure Group 1
> + *   interrupt.
> + *
> + * For now, we only support pseudo-NMIs if we have non-secure view of
> + * priorities.
> + */
> +static DEFINE_STATIC_KEY_FALSE(supports_pseudo_nmis);
> +
>  static struct gic_kvm_info gic_v3_kvm_info;
>  static DEFINE_PER_CPU(bool, has_rss);
>  
> @@ -232,6 +257,12 @@ static void gic_unmask_irq(struct irq_data *d)
>  	gic_poke_irq(d, GICD_ISENABLER);
>  }
>  
> +static inline bool gic_supports_nmi(void)
> +{
> +	return IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI) &&
> +	       static_branch_likely(&supports_pseudo_nmis);
> +}
> +
>  static int gic_irq_set_irqchip_state(struct irq_data *d,
>  				     enum irqchip_irq_state which, bool val)
>  {
> @@ -573,6 +604,12 @@ static void gic_update_vlpi_properties(void)
>  		!gic_data.rdists.has_direct_lpi ? "no " : "");
>  }
>  
> +/* Check whether it's single security state view */
> +static inline bool gic_dist_security_disabled(void)
> +{
> +	return readl_relaxed(gic_data.dist_base + GICD_CTLR) & GICD_CTLR_DS;
> +}
> +
>  static void gic_cpu_sys_reg_init(void)
>  {
>  	int i, cpu = smp_processor_id();
> @@ -598,6 +635,9 @@ static void gic_cpu_sys_reg_init(void)
>  	/* Set priority mask register */
>  	if (!gic_prio_masking_enabled())
>  		write_gicreg(DEFAULT_PMR_VALUE, ICC_PMR_EL1);
> +	else if (gic_supports_nmi() && group0)
> +		/* Mismatch configuration with boot CPU */
> +		WARN_ON(!gic_dist_security_disabled());

You can probably write this as a single line:

	WARN_ON(gic_supports_nmi() && group0 && !gic_dist_security_disabled());

Maybe even add a comment saying that in this case, the system is
likely to be dead, as the masking of interrupt will not work
correctly.

>  
>  	/*
>  	 * Some firmwares hand over to the kernel with the BPR changed from
> @@ -852,12 +892,6 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
>  #endif
>  
>  #ifdef CONFIG_CPU_PM
> -/* Check whether it's single security state view */
> -static bool gic_dist_security_disabled(void)
> -{
> -	return readl_relaxed(gic_data.dist_base + GICD_CTLR) & GICD_CTLR_DS;
> -}
> -
>  static int gic_cpu_pm_notifier(struct notifier_block *self,
>  			       unsigned long cmd, void *v)
>  {
> @@ -1110,6 +1144,11 @@ static bool gic_enable_quirk_msm8996(void *data)
>  	return true;
>  }
>  
> +static void gic_enable_nmi_support(void)
> +{
> +	static_branch_enable(&supports_pseudo_nmis);
> +}
> +
>  static int __init gic_init_bases(void __iomem *dist_base,
>  				 struct redist_region *rdist_regs,
>  				 u32 nr_redist_regions,
> @@ -1179,6 +1218,13 @@ static int __init gic_init_bases(void __iomem *dist_base,
>  		its_cpu_init();
>  	}
>  
> +	if (gic_prio_masking_enabled()) {
> +		if (!gic_has_group0() || gic_dist_security_disabled())
> +			gic_enable_nmi_support();
> +		else
> +			pr_warn("SCR_EL3.FIQ is cleared, cannot enable use of pseudo-NMIs\n");
> +	}
> +
>  	return 0;
>  
>  out_free:
> -- 
> 1.9.1
> 

Otherwise:

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.

-- 
Jazz is not dead, it just smell funny.