Re: [PATCH 2/2] arm64: cpufeature: Add GCS to cpucap_is_possible()

[Mark Rutland <mark.rutland@arm.com>]

On Thu, Dec 05, 2024 at 03:04:11PM +0000, Mark Brown wrote:
> On Thu, Dec 05, 2024 at 01:48:10PM +0000, Robin Murphy wrote:
> >  static inline bool system_supports_gcs(void)
> >  {
> > -	return IS_ENABLED(CONFIG_ARM64_GCS) &&
> > -		alternative_has_cap_unlikely(ARM64_HAS_GCS);
> > +	return alternative_has_cap_unlikely(ARM64_HAS_GCS);
> >  }
> 
> Ah, this is bitrot since the series was on the list for so long.  As
> well as HAFT which Rutland mentioned it looks like _bti_kernel(),
> _irq_prio_masking() and possibly others have the same thing.

In <asm/cpufeature.h>, only system_supports_gcs() and
system_supports_haft() don't use cpucap_is_possible(); all the other
IS_ENABLED() checks are for additional dependent properties, and rely on
the base case being handled in cpucap_is_possible().

In <asm/cpucaps.h> we have:

	static __always_inline bool
	cpucap_is_possible(const unsigned int cap)
	{
		...
		switch (cap) {
		...
		case ARM64_HAS_PAN:
			return IS_ENABLED(CONFIG_ARM64_PAN);
		...
		case ARM64_BTI:
			return IS_ENABLED(CONFIG_ARM64_BTI);
		...
		case ARM64_HAS_GIC_PRIO_MASKING:
			return IS_ENABLED(CONFIG_ARM64_PSEUDO_NMI);
		...
		}
	}

In <asm/cpufeature.h> we have:

	// base case
	static inline bool system_uses_hw_pan(void)
	{
		return alternative_has_cap_unlikely(ARM64_HAS_PAN);
	}

	// additional dependent property
	static inline bool system_uses_ttbr0_pan(void)
	{
		return IS_ENABLED(CONFIG_ARM64_SW_TTBR0_PAN) &&
		       !system_uses_hw_pan();
	}

	...

	// base case
	static __always_inline bool system_uses_irq_prio_masking(void)
	{
		return alternative_has_cap_unlikely(ARM64_HAS_GIC_PRIO_MASKING);
	}

	// additional dependent property
	static inline bool system_has_prio_mask_debugging(void)
	{
		return IS_ENABLED(CONFIG_ARM64_DEBUG_PRIORITY_MASKING) &&
		       system_uses_irq_prio_masking();
	}

	...

	// base case
	static inline bool system_supports_bti(void)
	{
		return cpus_have_final_cap(ARM64_BTI);
	}

	// additional dependent property
	static inline bool system_supports_bti_kernel(void)
	{
		return IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) &&
			cpus_have_final_boot_cap(ARM64_BTI);
	}

> Ideally we'd have no uses of IS_ENABLED() in these functions so it's a
> bit more obvious.

For the three cases above we can't do that without additional cpucaps,
and I think that'd be worse. I don't see anything we can practically do
other than add comments, and I suspect that's not going to help either.

Mark.