Re: [PATCH 44/60] kvm: x86: Move cpu_caps to struct kvm_vcpu_arch_common

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Moving `cpu_caps` to the shared `kvm_vcpu_arch_common` structure causes per-plane dynamic capabilities (e.g., MWAIT, OSXSAVE, OSPKE) to overwrite each other, breaking CPUID emulation for multi-plane vCPUs.

Pre-existing issues:
- [High] The shared `cpuid_entries` and `cpuid_dynamic_bits_dirty` state causes the guest to observe the wrong plane's dynamic CPUID bits.
- [High] `kvm_set_cpuid` fails to initialize `guest_fpu` XFD features and Hyper-V state for all planes.
--

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

Now that CPUID state is shared across all planes, cpu_caps can be shared
as well.

This commit shares cpu_caps across all vCPU execution planes by moving the
array from kvm_vcpu_arch into the shared kvm_vcpu_arch_common structure.

> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -284,7 +284,7 @@ static __always_inline void kvm_update_feature_runtime(struct kvm_vcpu *vcpu,
>  						       bool has_feature)
>  {
>  	cpuid_entry_change(entry, x86_feature, has_feature);

[Severity: High]
This is a pre-existing issue, but does applying plane-specific dynamic
state to the shared cpuid_entries cause the guest to observe the wrong
plane's dynamic CPUID bits?

When kvm_set_cpuid() iterates through all planes, it calls
kvm_update_cpuid_runtime() for each plane. This updates the shared
cpuid_entries with the current plane's dynamic state and clears the
shared cpuid_dynamic_bits_dirty flag.

Since the dirty flag is cleared without leaving it dirty for the next
check, won't subsequent planes executing CPUID just read the shared
cpuid_entries populated by the final plane's state?

> -	guest_cpu_cap_change(vcpu, x86_feature, has_feature);
> +	guest_cpu_cap_change(vcpu->common, x86_feature, has_feature);

[Severity: High]
Does this code incorrectly evaluate per-plane architectural state and
commit the dynamic results (such as MWAIT, OSXSAVE, or OSPKE) into the
globally shared cpu_caps array?

If this is re-evaluated for each plane, won't the capabilities of the last
plane processed overwrite the state for all prior planes?

>  }
>  
[ ... ]
> @@ -382,7 +382,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  	bool allow_gbpages;
>  	int i;
>  
> -	memset(vcpu->arch.cpu_caps, 0, sizeof(vcpu->arch.cpu_caps));
> +	memset(common->arch.cpu_caps, 0, sizeof(common->arch.cpu_caps));

[Severity: High]
Since kvm_set_cpuid() iterates over all planes calling
kvm_vcpu_after_set_cpuid(), does this unconditional memset() destroy the
shared cpu_caps state on every iteration of the per-plane initialization
loop?

[ ... ]
> @@ -536,8 +536,8 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
>  	swap(common->arch.cpuid_entries, e2);
>  	swap(common->arch.cpuid_nent, nent);
>  
> -	memcpy(vcpu_caps, vcpu->arch.cpu_caps, sizeof(vcpu_caps));
> -	BUILD_BUG_ON(sizeof(vcpu_caps) != sizeof(vcpu->arch.cpu_caps));
> +	memcpy(vcpu_caps, common->arch.cpu_caps, sizeof(vcpu_caps));
> +	BUILD_BUG_ON(sizeof(vcpu_caps) != sizeof(common->arch.cpu_caps));

[Severity: High]
This is a pre-existing issue, but does kvm_set_cpuid() fail to initialize
guest_fpu XFD features and Hyper-V state for all execution planes?

Looking at kvm_set_cpuid(), it mistakenly passes the loop invariant vcpu
instead of the iteration variable v inside the loop, failing to
initialize Hyper-V state for other planes:

arch/x86/kvm/cpuid.c:kvm_set_cpuid() {
    ...
    if (kvm_cpuid_has_hyperv(vcpu)) {
        vcpu_for_each_plane(common, i, v) {
            r = kvm_hv_vcpu_init(vcpu);
    ...
}

Additionally, kvm_check_cpuid(vcpu) is called completely outside the loop:

arch/x86/kvm/cpuid.c:kvm_set_cpuid() {
    ...
    r = kvm_check_cpuid(vcpu);
    if (r)
        goto err;

    vcpu_for_each_plane(vcpu->common, i, v) {
    ...
}

Doesn't this leave other planes uninitialized for XFD features?

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