| Message ID | 20230125163826.496739-4-mark.rutland@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: pseudo-nmi: elide code when CONFIG_ARM64_PSEUDO_NMI=n | expand |
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
On Wed, Jan 25, 2023 at 06:05:09PM +0000, Alexandru Elisei wrote: > 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. Ugh; yes, that should have been: arm64: make ARM64_HAS_GIC_PRIO_MASKING depend on ARM64_HAS_GIC_CPUIF_SYSREGS I've fixed that up locally, but I'll avoid sending a v3 unless there's something else that needs fixed too. Thanks, Mark. > > 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
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
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(-)