linux-kernel - Re: [RFC PATCH v1 1/2] KVM: arm64: Use kvm_has_feat() to check if FEAT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZvUL9SrVo4hn3aR0@linux.dev>
Date: Thu, 26 Sep 2024 09:23:33 +0200
From: Oliver Upton <oliver.upton@...ux.dev>
To: Shaoqin Huang <shahuang@...hat.com>
Cc: Marc Zyngier <maz@...nel.org>, kvmarm@...ts.linux.dev,
	Eric Auger <eauger@...hat.com>, Sebastian Ott <sebott@...hat.com>,
	Cornelia Huck <cohuck@...hat.com>,
	James Morse <james.morse@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Zenghui Yu <yuzenghui@...wei.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, Fuad Tabba <tabba@...gle.com>,
	Mark Brown <broonie@...nel.org>, Joey Gouly <joey.gouly@....com>,
	Kristina Martsenko <kristina.martsenko@....com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v1 1/2] KVM: arm64: Use kvm_has_feat() to check if
 FEAT_RAS is advertised to the guest

On Wed, Sep 25, 2024 at 11:22:39PM -0400, Shaoqin Huang wrote:
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index d7c2990e7c9e..99f256629ead 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -405,7 +405,7 @@ int handle_exit(struct kvm_vcpu *vcpu, int exception_index)
>  void handle_exit_early(struct kvm_vcpu *vcpu, int exception_index)
>  {
>  	if (ARM_SERROR_PENDING(exception_index)) {
> -		if (this_cpu_has_cap(ARM64_HAS_RAS_EXTN)) {
> +		if (kvm_has_feat(vcpu->kvm, ID_AA64PFR0_EL1, RAS, IMP)) {
>  			u64 disr = kvm_vcpu_get_disr(vcpu);
>  
>  			kvm_handle_guest_serror(vcpu, disr_to_esr(disr));

This is wrong; this is about handling *physical* SErrors, not virtual
ones.

So it really ought to be keyed off of the host cpucap.

> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 37ff87d782b6..bf176a3cc594 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -272,7 +272,7 @@ static inline void ___activate_traps(struct kvm_vcpu *vcpu, u64 hcr)
>  
>  	write_sysreg(hcr, hcr_el2);
>  
> -	if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN) && (hcr & HCR_VSE))
> +	if (kvm_has_feat(vcpu->kvm, ID_AA64PFR0_EL1, RAS, IMP) && (hcr & HCR_VSE))
>  		write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2);
>  }

I don't think this should be conditioned on guest visibility either. If
FEAT_RAS is implemented in hardware, ESR_EL1 is set to the value of
VSESR_EL2 when the vSError is taken, no matter what.

> diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> index 4c0fdabaf8ae..98526556d4e5 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> @@ -105,6 +105,8 @@ static inline void __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
>  
>  static inline void __sysreg_save_el2_return_state(struct kvm_cpu_context *ctxt)
>  {
> +	struct kvm_vcpu *vcpu = ctxt_to_vcpu(ctxt);
> +
>  	ctxt->regs.pc			= read_sysreg_el2(SYS_ELR);
>  	/*
>  	 * Guest PSTATE gets saved at guest fixup time in all
> @@ -113,7 +115,7 @@ static inline void __sysreg_save_el2_return_state(struct kvm_cpu_context *ctxt)
>  	if (!has_vhe() && ctxt->__hyp_running_vcpu)
>  		ctxt->regs.pstate	= read_sysreg_el2(SYS_SPSR);
>  
> -	if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN))
> +	if (kvm_has_feat(vcpu->kvm, ID_AA64PFR0_EL1, RAS, IMP))
>  		ctxt_sys_reg(ctxt, DISR_EL1) = read_sysreg_s(SYS_VDISR_EL2);
>  }
>  
> @@ -220,6 +222,7 @@ static inline void __sysreg_restore_el2_return_state(struct kvm_cpu_context *ctx
>  {
>  	u64 pstate = to_hw_pstate(ctxt);
>  	u64 mode = pstate & PSR_AA32_MODE_MASK;
> +	struct kvm_vcpu *vcpu = ctxt_to_vcpu(ctxt);
>  
>  	/*
>  	 * Safety check to ensure we're setting the CPU up to enter the guest
> @@ -238,7 +241,7 @@ static inline void __sysreg_restore_el2_return_state(struct kvm_cpu_context *ctx
>  	write_sysreg_el2(ctxt->regs.pc,			SYS_ELR);
>  	write_sysreg_el2(pstate,			SYS_SPSR);
>  
> -	if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN))
> +	if (kvm_has_feat(vcpu->kvm, ID_AA64PFR0_EL1, RAS, IMP))
>  		write_sysreg_s(ctxt_sys_reg(ctxt, DISR_EL1), SYS_VDISR_EL2);
>  }

These registers are still stateful no matter what, we cannot prevent an
ESB instruction inside the VM from consuming a pending vSError.

Keep in mind the ESB instruction is a NOP without FEAT_RAS, so it is
still a legal instruction for a VM w/o FEAT_RAS.

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 31e49da867ff..b09f8ba3525b 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -4513,7 +4513,7 @@ static void vcpu_set_hcr(struct kvm_vcpu *vcpu)
>  
>  	if (has_vhe() || has_hvhe())
>  		vcpu->arch.hcr_el2 |= HCR_E2H;
> -	if (cpus_have_final_cap(ARM64_HAS_RAS_EXTN)) {
> +	if (kvm_has_feat(kvm, ID_AA64PFR0_EL1, RAS, IMP)) {
>  		/* route synchronous external abort exceptions to EL2 */
>  		vcpu->arch.hcr_el2 |= HCR_TEA;
>  		/* trap error record accesses */

No, we want external aborts to be taken to EL2. Wouldn't this also have
the interesting property of allowing a VM w/o FEAT_RAS to access the
error record registers?

-- 
Thanks,
Oliver