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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dropped register updates for non-protected guests due to missing PKVM_HOST_STATE_DIRTY flag.
- [High] Missing HCR_EL2 sync on guest exit causes an infinite loop of virtual SError injections.
--

commit b6999f3ffe5ca70d542b029d1a2d4a9e5e9d8276
Author: Fuad Tabba <tabba@google.com>

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

This commit defers copying a non-protected guest's register context between
the host and hypervisor. It introduces a PKVM_HOST_STATE_DIRTY flag to only
sync the state when modified 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
[ ... ]
> @@ -489,6 +503,22 @@ int handle_exit(struct kvm_vcpu *vcpu, int exception_index)
>  /* For exit types that need handling before we can be preempted */
>  void handle_exit_early(struct kvm_vcpu *vcpu, int exception_index)
>  {
> +	bool inject_serror = ARM_SERROR_PENDING(exception_index) ||
> +		ARM_EXCEPTION_CODE(exception_index) == ARM_EXCEPTION_EL1_SERROR;
> +
> +	/*
> +	 * An SError injected below writes the host ctxt; for a non-protected
> +	 * guest, sync from the hyp vCPU and keep it dirty so it isn't dropped.
> +	 */
> +	if (is_protected_kvm_enabled()) {
> +		vcpu_clear_flag(vcpu, PKVM_HOST_STATE_DIRTY);

[Severity: High]
Does unconditionally clearing this flag on every exit drop register updates
for non-trap exits?

For non-trap exits like ARM_EXCEPTION_IRQ or ARM_EXCEPTION_IL,
handle_trap_exceptions() will not be called to set the flag again.

If an asynchronous request like KVM_REQ_VCPU_RESET (triggered by PSCI CPU_ON)
modifies the host vcpu->arch.ctxt in the kvm_arch_vcpu_ioctl_run() loop, or
if an undefined exception is injected via handle_exit(), flush_hyp_vcpu()
will skip copying these updates because the dirty flag is clear.

Could this cause the guest to resume execution from its old pc with stale
state?

> 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,13 @@ 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 the hcr_el2 sync cause an infinite loop of virtual SError
injections?

To inject a virtual SError, the host sets HCR_VSE in host_vcpu->arch.hcr_el2.
When the guest takes the SError, the hardware clears the VSE bit, which is
captured in hyp_vcpu->vcpu.arch.hcr_el2 on exit.

Since this patch removes the copy-back of hcr_el2 to the host for both
protected and non-protected guests, the host's HCR_VSE bit remains set
permanently.

Will this cause flush_hyp_vcpu() to endlessly re-inject the SError on every
subsequent entry?

> +	if (pkvm_hyp_vcpu_is_protected(hyp_vcpu)) {
> +		host_vcpu->arch.ctxt = hyp_vcpu->vcpu.arch.ctxt;
> +	} else {
> +		/* Keep PC (tracepoint) and PSTATE (vcpu_mode_is_bad_32bit) current. */
> +		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;
> +	}
>  
>  	host_vcpu->arch.fault		= hyp_vcpu->vcpu.arch.fault;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619070719.812227-1-tabba@google.com?part=8