Re: [Devel] [PATCH 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 2 potential issue(s) to consider:
- [Low] i386: svm-features-compat keeps FEAT_SVM non-zero and can raise CPUID xlevel
- [Low] i386: svm_features_compat dependency skip is not gated on KVM, so TCG warns or fails
--

commit 82ed7aadbf4e0e2f6288562589f8de9162210a0c
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. With the property set, requested SVM leaf features (leaf 0x8000000a) are kept when svm is off. They survive both the feature dependency on svm and the host feature filter.

> 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,12 @@ 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 && d->to.index == FEAT_SVM) {
> +            continue;
> +        }
> +

[Severity: Low]
Can this skip leave FEAT_SVM non-zero with svm off and change the guest
visible xlevel?

Later in x86_cpu_expand_features(), under full_cpuid_auto_level (which
defaults to true), this call is not guarded by the SVM bit:

    x86_cpu_adjust_feat_level(cpu, FEAT_SVM);

x86_cpu_adjust_feat_level() returns early only when env->features[w] is
zero.  With the dependency clear skipped, FEAT_SVM is now non-zero, so the
call ends up in:

    x86_cpu_adjust_level(cpu, &env->cpuid_min_xlevel, 0x8000000A);

This differs from the explicit block that is guarded:

    /* SVM requires CPUID[0x8000000A] */
    if (env->features[FEAT_8000_0001_ECX] & CPUID_EXT3_SVM) {

cpuid_xlevel is then taken from cpuid_min_xlevel, and cpu_x86_cpuid()
returns it in EAX for leaf 0x80000000.

For a CPU model whose xlevel is below 0x8000000A (kvm64 or the Intel
models at 0x80000008, for example) with svm=off and an SVM leaf feature
such as npt=on or nrip-save=on, xlevel would change from 0x80000008 to
0x8000000A.

Does this contradict the commit message statement "The guest sees no
difference, because cpu_x86_cpuid() reports leaf 0x8000000a only with
svm"?

Should the x86_cpu_adjust_feat_level(cpu, FEAT_SVM) call be made
conditional on the SVM bit when svm_features_compat is set, or should the
commit message be corrected?

[Severity: Low]
Is it intended that this dependency skip applies to every accelerator?

The skip has no kvm_enabled() check, but the matching host filter bypass
in x86_cpu_compat_kept_features() returns early for non-KVM:

    if (!kvm_enabled()) {
        return 0;
    }

Under TCG, x86_cpu_filter_features() then computes:

    uint64_t unavailable_features = requested_features & ~host_feat &
        ~x86_cpu_compat_kept_features(cpu, w);

Here host_feat is the TCG feature set for FEAT_SVM, TCG_SVM_FEATURES.  In
system emulation CPUID_SVM_KERNEL_FEATURES is 0, so NRIPSAVE and VNMI are
not in it.

A model such as EPYC with svm=off on a vz-9.2 machine type would keep
NRIPSAVE after x86_cpu_expand_features().  It would then be reported by
mark_unavailable_features() as unsupported.  Before this patch the
dependency cleared the bit silently.

x86_cpu_realizefn() calls x86_cpu_filter_features(cpu, check_cpuid ||
enforce_cpuid).  check defaults to true, so a warning is printed.  With
enforce=on, realization fails with "TCG doesn't support requested
features".

Should the dependency skip be gated on kvm_enabled() as well, so it
matches the host filter bypass?  The commit message statement that the
features stay "against the host" is only true for KVM.

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

[ ... ]

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

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel