Re: [PATCH 2/3] arm64/cpufeature: Add cpucap for HCR_EL2.E2H RES1 (!FEAT_E2H0)

[Marc Zyngier <maz@kernel.org>]

On Sat, 29 Mar 2025 08:41:09 +0000,
Yicong Yang <yangyicong@huawei.com> wrote:
> 
> On 2025/3/29 16:12, Marc Zyngier wrote:
> > On Sat, 29 Mar 2025 03:44:08 +0000,
> > Yicong Yang <yangyicong@huawei.com> wrote:
> >>
> >> From: Yicong Yang <yangyicong@hisilicon.com>
> >>
> >> Arm introduced a "new" feature FEAT_E2H0 indicates that HCR_EL2.E2H can
> >> be programmed to the value 0 for legacy hardwares supported VHE. The
> >> feature is indicated by ID_AA64MMFR4_EL1.E2H0 == 0. It is needed to
> >> detect this feature for KVM mode initialization. Instead of bothering
> >> the existed hardwares, introduce a new cpucap HAS_E2H_RES1 to indicate
> >> FEAT_E2H0 is not supported. Make this a ARM64_CPUCAP_SYSTEM_FEATURE
> >> just like VHE.
> >>
> >> Introduce cpu_has_e2h_res1() for checking the feature's support
> >> which can be used in the early boot stage where CPU capabilities
> >> are not initialized.
> >>
> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> >> ---
> >>  arch/arm64/include/asm/cpufeature.h | 23 +++++++++++++++++++++++
> >>  arch/arm64/kernel/cpufeature.c      | 12 ++++++++++++
> >>  arch/arm64/tools/cpucaps            |  1 +
> >>  3 files changed, 36 insertions(+)
> >>
> >> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> >> index c4326f1cb917..b35d393da28d 100644
> >> --- a/arch/arm64/include/asm/cpufeature.h
> >> +++ b/arch/arm64/include/asm/cpufeature.h
> >> @@ -889,6 +889,29 @@ static inline bool cpu_has_hw_af(void)
> >>  						ID_AA64MMFR1_EL1_HAFDBS_SHIFT);
> >>  }
> >>  
> >> +/*
> >> + * Check whether FEAT_E2H0 is not supported, in which case HCR_EL2.E2H
> >> + * is implemented as RES1.
> >> + */
> >> +static __always_inline bool cpu_has_e2h_res1(void)
> >> +{
> >> +	u64 mmfr4;
> >> +	u32 val;
> >> +
> >> +	/*
> >> +	 * It's also used for checking the kvm mode cfg in early_param()
> >> +	 * where boot capabilities is not initialized. In such case read
> >> +	 * mmfr4 directly. This works same after boot stage since
> >> +	 * ARM64_HAS_E2H_RES1 is a system feature, the cached sanitised
> >> +	 * value keeps same with every single CPU.
> >> +	 */
> >> +	mmfr4 = read_sysreg_s(SYS_ID_AA64MMFR4_EL1);
> > 
> > This will result in traps to EL2 with nested. Are you expecting this
> > to be used on any form of hot paths?
> > 
> 
> No. If any use required in the hotpath, check ARM64_HAS_E2H_RES1 by
> alternative_has_cap* instead. We cannot check the capabilites in
> the early_param() (in early_kvm_mode_cfg()) since they are not initialized,
> so we can only rely on the registers directly.

Then I think an explicit comment would help clarifying what is
expected to be used. I also don't think the __always_inline is
mandated here. Specially if the helper needs to account for the broken
Apple stuff.

> 
> >> +	val = cpuid_feature_extract_signed_field(mmfr4,
> >> +						 ID_AA64MMFR4_EL1_E2H0_SHIFT);
> >> +
> >> +	return val != ID_AA64MMFR4_EL1_E2H0_IMP;
> > 
> > This is going to break badly on Apple HW, which predate the
> > "!FEAT_E2H0" relaxation and yet have HCR_EL2.E2H RAO/WI and
> > ID_AA64MMFR4_EL1.E2H0==0.
> 
> currently maybe only a wrong declaration of HCR_EL2.E2H RES1 and we can have
> a workaround for the apple? considering the only use case of this is in the
> early_kvm_mode_cfg() described below.

Indeed, I think you would need to handle the Apple behaviour here.

> 
> > 
> > The curent code was carefully designed to *avoid* doing this, because
> > the kernel doesn't really need to know anything about FEAT_E2H0 apart
> > from the very early boot.
> > 
> > What do we gain with this?
> > 
> 
> Only one usecase introduced in patch 3/3, avoid triggering the warning on
> !E2H0 platforms when booting with "kvm-arm.mode=nvhe". In such case "nvhe"
> is inavailable but user has no hint on this, booting with "kvm-arm.mode=nvhe"
> will trigger the warn [1]. Need to give some hint if user try to boot with
> "nvhe" mode on !FEAT_E2H0 platforms. But we need a ARM64_CPUCAP_SYSTEM_FEATURE
> capability to make this feature system wide consistent.
> 
> [1] https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kvm/arm.c#n2917

But *why* do we need an extra helper for this only functionnality? If
we reach the point where early_kvm_mode_cfg() gets called, that we are
still at EL2, and that the requested mode is "nvhe", then we know, by
construction, that we couldn't switch to E2H==0.

That's because idreg-override.c defines this:

 { "kvm_arm.mode=nvhe",		"arm64_sw.hvhe=0 id_aa64mmfr1.vh=0" },

and "id_aa64mmfr1.vh=0" gets filtered out by mmfr1_vh_filter().

Or am I missing something obvious?

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.