Re: [PATCH v4 21/29] KVM: arm64: Add NVHCR_EL2 handling to the sysreg array

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Inverted context logic for NVHCR_EL2 in locate_register() incorrectly maps the register to an unmanaged hardware CPU register.
- [High] Missing explicit Context Synchronization Event (ISB) after writing to the architectural control-plane system register NVHCR_EL2.
--

--- Patch [21]: [PATCH v4 21/29] KVM: arm64: Add NVHCR_EL2 handling to the sysreg array ---
commit aea2461c165a3a2d9e5915b4946e1ec44773006d
Author: Marc Zyngier <[email protected]>
Subject: KVM: arm64: Add NVHCR_EL2 handling to the sysreg array

This commit exposes NVHCR_EL2 to userspace and treats direct access as UNDEF 
to catch exception routing bugs. It also updates generic accessors to handle
the register's location.

> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 9439c5b2b1fe8..0aeb2e736fde3 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -221,6 +221,20 @@ static void locate_register(const struct kvm_vcpu *vcpu, enum vcpu_sysreg reg,
>  		if (is_hyp_ctxt(vcpu) && vcpu_el2_e2h_is_set(vcpu))
>  			loc->loc = SR_LOC_SPECIAL;
>  		break;
> +	case NVHCR_EL2:
> +		/*
> +		 * Yes, NVHCR_EL2 maps to itself when loaded in nested
> +		 * context. If you feel like the architecture is double
> +		 * backing on itself upside down, you're not alone.
> +		 */
> +		WARN_ON_ONCE(!kvm_has_nv3(vcpu->kvm));
> +		if (is_hyp_ctxt(vcpu)) {
> +			loc->loc = SR_LOC_MEMORY;
> +		} else {
> +			loc->loc = SR_LOC_LOADED | SR_LOC_MAPPED;
> +			loc->map_reg = NVHCR_EL2;
> +		}
> +		break;

[Severity: High]
Does this invert the context logic for NVHCR_EL2? 

If !is_hyp_ctxt(vcpu) (meaning the vCPU is running a nested guest), this maps 
the register to an unmanaged hardware CPU register rather than the guest's 
VNCR memory array. 

Since KVM doesn't save or restore the physical SYS_NVHCR_EL2 register on 
context switches, could this cause KVM_SET_ONE_REG to lose the guest's 
NVHCR_EL2 state, and KVM_GET_ONE_REG to leak uninitialized host physical 
register state?

[ ... ]

> @@ -298,6 +313,7 @@ static void write_sr_to_cpu(enum vcpu_sysreg reg, u64 val)
>  	case DACR32_EL2:	write_sysreg_s(val, SYS_DACR32_EL2);	break;
>  	case IFSR32_EL2:	write_sysreg_s(val, SYS_IFSR32_EL2);	break;
>  	case DBGVCR32_EL2:	write_sysreg_s(val, SYS_DBGVCR32_EL2);	break;
> +	case NVHCR_EL2:		write_sysreg_s(val, SYS_NVHCR_EL2);	break;

[Severity: High]
Is an explicit context synchronization event (isb) required here after writing 
to NVHCR_EL2? 

The write_sysreg_s macro modifies a control-plane system register but is 
followed immediately by a branch (break), skipping the architectural 
requirement of having an isb as the very next instruction. 

Could this leave the CPU pipeline operating in an architecturally undefined 
state when userspace calls KVM_SET_ONE_REG?

>  	default:		WARN_ON_ONCE(1);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=21