Re: [PATCH 15/60] kvm: Move VCPU scheduling state to struct kvm_vcpu_common

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The active vCPU pointer `common->current_vcpu` is not updated during `vcpu_load()`, causing preemption handlers to save and restore the physical state of the wrong VCPU plane.
- [Low] Mixing `goto`-based error handling and `__free()` cleanup declarations in the same function.
--

commit aa5b05dc8fcf8ce2c82b92d710cc33efdf5543d2
Author: Joerg Roedel <joerg.roedel@amd.com>

kvm: Move VCPU scheduling state to struct kvm_vcpu_common

The scheduling state of the KVM VCPU is shared between all per-plane
VCPU objects. Move it to struct kvm_vcpu_common.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -166,7 +166,7 @@ void vcpu_load(struct kvm_vcpu *vcpu)
>  	int cpu = get_cpu();
>  
>  	__this_cpu_write(kvm_running_vcpu, vcpu->common);
> -	preempt_notifier_register(&vcpu->preempt_notifier);
> +	preempt_notifier_register(&vcpu->common->preempt_notifier);
>  	kvm_arch_vcpu_load(vcpu, cpu);
>  	put_cpu();
>  }

[Severity: High]
Does this code need to dynamically record the actively loaded vCPU in
common->current_vcpu?

When an ioctl is invoked on a secondary plane's file descriptor, this will
load the physical state of the secondary plane onto the current CPU. 

If common->current_vcpu is not updated here, could preemption cause the wrong
state to be saved?

[ ... ]

> @@ -468,6 +468,12 @@ static int kvm_vcpu_init_common(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned
>  
>  	common->kvm = kvm;
>  	common->current_vcpu = vcpu;
> +
> +	common->wants_to_run = false;
> +	common->preempted = false;
> +	common->ready = false;
> +	preempt_notifier_init(&common->preempt_notifier, &kvm_preempt_ops);
> +
>  	vcpu->common = no_free_ptr(common);
>  
>  	return 0;

[Severity: Low]
This isn't a bug, but does kvm_vcpu_init_common() mix goto-based error
handling and __free() cleanup declarations in the same function?

The cleanup guidelines suggest that for a given routine, all resources should
be converted to scope-based cleanup, or none of them, to avoid confusing
ownership semantics.

[ ... ]

> @@ -6487,36 +6490,36 @@ static void kvm_init_debug(void)
>  }
>  
>  static inline
> -struct kvm_vcpu *preempt_notifier_to_vcpu(struct preempt_notifier *pn)
> +struct kvm_vcpu_common *preempt_notifier_to_vcpu_common(struct preempt_notifier *pn)
>  {
> -	return container_of(pn, struct kvm_vcpu, preempt_notifier);
> +	return container_of(pn, struct kvm_vcpu_common, preempt_notifier);
>  }

[ ... ]

>  static void kvm_sched_out(struct preempt_notifier *pn,
>  			  struct task_struct *next)
>  {
> -	struct kvm_vcpu *vcpu = preempt_notifier_to_vcpu(pn);
> +	struct kvm_vcpu_common *common = preempt_notifier_to_vcpu_common(pn);
>  
> -	WRITE_ONCE(vcpu->scheduled_out, true);
> +	WRITE_ONCE(common->scheduled_out, true);
>  
> -	if (task_is_runnable(current) && kvm_vcpu_wants_to_run(vcpu)) {
> -		WRITE_ONCE(vcpu->preempted, true);
> -		WRITE_ONCE(vcpu->ready, true);
>  	}
> -	kvm_arch_vcpu_put(vcpu);
> +	kvm_arch_vcpu_put(common->current_vcpu);
>  	__this_cpu_write(kvm_running_vcpu, NULL);
>  }

[Severity: High]
If common->current_vcpu still points to the primary plane because it wasn't
updated in vcpu_load(), will this save the active physical state of the
secondary plane directly into the primary plane's structure?

It looks like upon resuming, kvm_sched_in() would load the primary plane's
corrupted state back into the physical CPU, which might cause deterministic
architectural guest state corruption between planes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608144252.351443-1-joro@8bytes.org?part=15