Re: [PATCH v7 8/8] KVM: arm64: Implement lazy vCPU state sync for non-protected guests

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing HCR_EL2 state synchronization breaks virtual SError injection, causing an infinite exception loop.
- [High] Lazy state sync fails to flush context after KVM_REQ_VCPU_RESET, causing vCPU resets (e.g., PSCI CPU_ON) to be ignored.
--

--- Patch [8]: [PATCH v7 8/8] KVM: arm64: Implement lazy vCPU state sync for non-protected guests ---
commit 80df5c4ca2cca0a1726813fa129b353353482892
Author: Fuad Tabba <[email protected]>

KVM: arm64: Implement lazy vCPU state sync for non-protected guests

This commit optimizes world switches for non-protected guests by deferring
the copy of the vCPU register context between the host and hypervisor.
State is now only synced on entry if marked dirty, and on exit only when
explicitly needed by the host.

> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -486,9 +486,32 @@ int handle_exit(struct kvm_vcpu *vcpu, int exception_index)
>  	}
>  }
>  
> +static void handle_exit_pkvm_state(struct kvm_vcpu *vcpu, int exception_index)
> +{
> +	int exception_code = ARM_EXCEPTION_CODE(exception_index);
> +
> +	if (!is_protected_kvm_enabled() || kvm_vm_is_protected(vcpu->kvm))
> +		return;
> +
> +	/*
> +	 * Sync the context back when the host will read (trap) or write
> +	 * (SError) it. Preempt-off here, so the loaded hyp vCPU is stable.
> +	 */
> +	if (exception_code == ARM_EXCEPTION_TRAP ||
> +	    exception_code == ARM_EXCEPTION_EL1_SERROR ||
> +	    ARM_SERROR_PENDING(exception_index)) {
> +		kvm_call_hyp_nvhe(__pkvm_vcpu_sync_state);
> +		vcpu_set_flag(vcpu, PKVM_HOST_STATE_DIRTY);
> +	} else {
> +		vcpu_clear_flag(vcpu, PKVM_HOST_STATE_DIRTY);

[Severity: High]
Is it possible for KVM_REQ_VCPU_RESET updates to be ignored due to this
cleared flag?

If a secondary vCPU is powered on (for example, via PSCI CPU_ON), it
receives KVM_REQ_VCPU_RESET and gets kicked, leading to an exit with an
IRQ. Since that exit takes this else branch and clears the dirty flag,
and the reset request is processed afterward in kvm_vcpu_check_requests()
which modifies the host-side vCPU context, wouldn't flush_hyp_vcpu() skip
syncing this updated reset state because the flag remains cleared?

This could result in the target vCPU resuming with its pre-IRQ stale
context, eventually overwriting the new reset state on the next exit.

> +	}
> +}
> +

[ ... ]

> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> @@ -203,9 +255,16 @@ static void sync_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
>  	fpsimd_sve_sync(&hyp_vcpu->vcpu);
>  	sync_debug_state(hyp_vcpu);
>  
> -	host_vcpu->arch.ctxt		= hyp_vcpu->vcpu.arch.ctxt;
> -
> -	host_vcpu->arch.hcr_el2		= hyp_vcpu->vcpu.arch.hcr_el2;

[Severity: High]
Does removing this HCR_EL2 assignment break virtual SError injection and
cause an infinite exception loop?

When the host injects a virtual SError into a non-protected guest under
pKVM by setting HCR_VSE in the host-side vcpu->arch.hcr_el2, the hardware
automatically clears the VSE bit in the physical HCR_EL2 register once the
guest takes the exception.

Without syncing this hardware-cleared state back to the host here, the
host's HCR_VSE bit remains set. On the next entry, flush_hyp_vcpu() will
repeatedly re-inject the stale HCR_VSE bit from the host into the hyp vCPU,
locking up the guest.

> +	if (pkvm_hyp_vcpu_is_protected(hyp_vcpu)) {
> +		host_vcpu->arch.ctxt = hyp_vcpu->vcpu.arch.ctxt;
> +	} else {
> +		/*
> +		 * PC feeds trace_kvm_exit(), PSTATE.SS the host software-step
> +		 * machine, and both run before the next on-demand ctxt sync.
> +		 */
> +		host_vcpu->arch.ctxt.regs.pc = hyp_vcpu->vcpu.arch.ctxt.regs.pc;
> +		host_vcpu->arch.ctxt.regs.pstate = hyp_vcpu->vcpu.arch.ctxt.regs.pstate;
> +	}

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