Re: [PATCH v2 hci-8.0/hci-8.1 6/6] i386: keep SVM leaf features without svm on vz types up to vz9.2.0 #PSBM-144630

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] i386: svm-features-compat keeps all FEAT_SVM bits, not only requested ones
--

commit 9fd87512847e5481366616a7120c63227cf34e0f
Author: Denis V. Lunev <den@openvz.org>

i386: keep SVM leaf features without svm on vz types up to vz9.2.0 #PSBM-144630

This commit adds the x-svm-features-compat CPU property and enables it in
pc_vz_9_2_compat. Under KVM, SVM leaf features (CPUID 0x8000000a) are kept
when svm is off, both against the svm dependency and against the host.

> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c

[ ... ]

> @@ -8996,6 +8996,13 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>  
>      for (i = 0; i < ARRAY_SIZE(feature_dependencies); i++) {
>          FeatureDep *d = &feature_dependencies[i];
> +
> +        /* CPUID leaf 0x8000000a stays empty without svm anyway */
> +        if (cpu->svm_features_compat && kvm_enabled() &&
> +            d->to.index == FEAT_SVM) {
> +            continue;
> +        }
> +

[Severity: Low]
The commit message says that "requested SVM leaf features stay when svm is
off". Does this skip go further than that?

This continue applies to every FEAT_SVM dependency entry. In
x86_cpu_expand_features(), when xcc->max_features is set (-cpu host or
-cpu max), the earlier loop ORs x86_cpu_get_supported_feature_word() into
env->features[FEAT_SVM] for bits the user did not set.

With svm=off on an AMD KVM host with nested SVM, those host-derived bits are
no longer cleared by the svm dependency.

The skip also bypasses mark_unavailable_features() for explicitly requested
SVM leaf features.

>          if (!(env->features[d->from.index] & d->from.mask)) {
>              uint64_t unavailable_features = env->features[d->to.index] & d->to.mask;

[ ... ]

> @@ -9138,6 +9145,10 @@ static uint64_t x86_cpu_compat_kept_features(X86CPU *cpu, FeatureWord w)
>          if (cpu->v_vmsave_vmload_compat && IS_AMD_CPU(env)) {
>              kept = CPUID_SVM_V_VMSAVE_VMLOAD;
>          }
> +        if (cpu->svm_features_compat &&
> +            !(env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM)) {
> +            kept = ~0ULL;
> +        }
>          break;

Is it intended that kept = ~0ULL keeps every FEAT_SVM bit against the host?

The result is masked only with env->features[w]. Nothing limits it to
explicitly requested bits, and there is no IS_AMD_CPU() check. The
neighbouring compat cases use IS_AMD_CPU() or IS_INTEL_CPU().

As a result, x86_cpu_adjust_feat_level(cpu, FEAT_SVM) can see a non-zero
env->features[FEAT_SVM] and raise cpuid_min_xlevel to 0x8000000a. The kept
bits can also show up in the QOM feature properties and in
query-cpu-model-expansion.

Guest CPUID leaf 0x8000000a itself stays zero, since cpu_x86_cpuid() returns
zeros without svm. Would it make sense to restrict the kept mask to
env->user_features[FEAT_SVM] and to add a vendor check, so the behaviour
matches the commit message?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261029113436.93521-1-den@openvz.org?part=6