Re: [PATCH v2 3/5] arm64: make ARM64_HAS_GIC_PRIO_MASKING depend on ARM64_HAS_GIC_PRIO_MASKING

From: Alexandru Elisei <alexandru.elisei@arm.com>
To: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org, broonie@kernel.org,
	catalin.marinas@arm.com, maz@kernel.org, will@kernel.org
Subject: Re: [PATCH v2 3/5] arm64: make ARM64_HAS_GIC_PRIO_MASKING depend on ARM64_HAS_GIC_PRIO_MASKING
Date: Wed, 25 Jan 2023 18:05:09 +0000	[thread overview]
Message-ID: <Y9FvKuI3JmL66NYL@monolith.localdoman> (raw)
In-Reply-To: <20230125163826.496739-4-mark.rutland@arm.com>

Hi,

Very low hanging fruit here, but I think you wanted the commit subject to
say that ARM64_HAS_GIC_PRIO_MASKING should depend on
ARM64_HAS_GIC_CPUIF_SYSREGS instead of depending on itself.

Thanks,
Alex

On Wed, Jan 25, 2023 at 04:38:24PM +0000, Mark Rutland wrote:
> Currently the arm64_cpu_capabilities structure for
> ARM64_HAS_GIC_PRIO_MASKING open-codes the same CPU field definitions as
> the arm64_cpu_capabilities structure for ARM64_HAS_GIC_CPUIF_SYSREGS, so
> that can_use_gic_priorities() can use has_useable_gicv3_cpuif().
> 
> This duplication isn't ideal for the legibility of the code, and sets a
> bad example for any ARM64_HAS_GIC_* definitions added by subsequent
> patches.
> 
> Instead, have ARM64_HAS_GIC_PRIO_MASKING check for the
> ARM64_HAS_GIC_CPUIF_SYSREGS cpucap, and add a comment explaining why
> this is safe. Subsequent patches will use the same pattern where one
> cpucap depends upon another.
> 
> There should be no functional change as a result of this patch.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/cpufeature.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index afd547a5309c7..515975f42d037 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -2046,7 +2046,15 @@ early_param("irqchip.gicv3_pseudo_nmi", early_enable_pseudo_nmi);
>  static bool can_use_gic_priorities(const struct arm64_cpu_capabilities *entry,
>  				   int scope)
>  {
> -	return enable_pseudo_nmi && has_useable_gicv3_cpuif(entry, scope);
> +	/*
> +	 * ARM64_HAS_GIC_CPUIF_SYSREGS has a lower index, and is a boot CPU
> +	 * feature, so will be detected earlier.
> +	 */
> +	BUILD_BUG_ON(ARM64_HAS_GIC_PRIO_MASKING <= ARM64_HAS_GIC_CPUIF_SYSREGS);
> +	if (!cpus_have_cap(ARM64_HAS_GIC_CPUIF_SYSREGS))
> +		return false;
> +
> +	return enable_pseudo_nmi;
>  }
>  #endif
>  
> @@ -2537,11 +2545,6 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>  		.capability = ARM64_HAS_GIC_PRIO_MASKING,
>  		.type = ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE,
>  		.matches = can_use_gic_priorities,
> -		.sys_reg = SYS_ID_AA64PFR0_EL1,
> -		.field_pos = ID_AA64PFR0_EL1_GIC_SHIFT,
> -		.field_width = 4,
> -		.sign = FTR_UNSIGNED,
> -		.min_field_value = 1,
>  	},
>  #endif
>  #ifdef CONFIG_ARM64_E0PD
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel