Re: [Devel] [PATCH hci-8.0/hci-8.1 3/6] i386: keep spec-ctrl for AMD guests on vz machine types up to vz9.2.0 #VSTOR-151770

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] i386: spec-ctrl compat check uses host bits only, not guest CPUID bits
- [Low] i386: spec_ctrl_compat field in struct ArchCPU has no comment
--

commit 155fa87fb8605b42ed9c5514241235cd8cfb7663
Author: Denis V. Lunev <den@openvz.org>

i386: keep spec-ctrl for AMD guests on vz machine types up to vz9.2.0 #VSTOR-151770

This commit adds an x-spec-ctrl-compat CPU property and enables it in
pc_vz_9_2_compat. When the property is set, an AMD guest keeps a requested
spec-ctrl bit if the host reports amd-ibpb plus one of amd-ibrs, amd-stibp
or amd-ssbd. This is meant to keep migration and snapshot revert working
after the KVM change that stopped offering spec-ctrl on AMD hosts without
IBPB_RET.

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

[ ... ]

> @@ -9116,6 +9116,19 @@ static uint64_t x86_cpu_compat_kept_features(X86CPU *cpu, FeatureWord w)
>              kept = CPUID_7_0_EBX_HLE | CPUID_7_0_EBX_RTM;
>          }
>          break;
> +    case FEAT_7_0_EDX:
> +        if (cpu->spec_ctrl_compat && IS_AMD_CPU(env)) {
> +            uint32_t ebx = kvm_arch_get_supported_cpuid(kvm_state, 0x80000008,
> +                                                        0, R_EBX);
> +
> +            if ((ebx & CPUID_8000_0008_EBX_IBPB) &&
> +                (ebx & (CPUID_8000_0008_EBX_IBRS |
> +                        CPUID_8000_0008_EBX_STIBP |
> +                        CPUID_8000_0008_EBX_AMD_SSBD))) {
> +                kept = CPUID_7_0_EDX_SPEC_CTRL;

[Severity: Medium]
Is it enough to test only the host-supported bits here? The condition in
x86_cpu_compat_kept_features() looks only at the value returned by
kvm_arch_get_supported_cpuid() for CPUID 0x80000008 EBX. It never looks at
the guest's own env->features[FEAT_8000_0008_EBX].

The commit message says KVM keeps MSR_IA32_SPEC_CTRL and
MSR_IA32_PRED_CMD available to the guest "through those bits". That holds
for the host bits. It is not checked for the guest's amd-ibpb, amd-ibrs,
amd-stibp and amd-ssbd bits.

Some AMD models set only a subset of those bits. For example, one EPYC
variant in builtin_x86_defs sets only:

    .features[FEAT_8000_0008_EBX] =
        CPUID_8000_0008_EBX_IBPB,

None of the AMD models set CPUID_7_0_EDX_SPEC_CTRL by default. So a guest
on a vz machine type up to vz9.2.0 that requests spec-ctrl explicitly, or
uses -cpu host or max, would keep spec-ctrl based on host bits alone.

Could the guest's own CPUID.80000008:EBX bits be checked as well, so that
spec-ctrl is kept only when the guest has the matching amd-* bits? I could
not check how KVM gates the two MSRs from guest CPUID, so this may turn out
to be a non-issue.

[ ... ]

> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -2337,6 +2337,7 @@ struct ArchCPU {
>       * older node keeps its CPUID.
>       */
>      bool tsx_compat;
> +    bool spec_ctrl_compat;

[Severity: Low]
This isn't a bug, but the new spec_ctrl_compat field in struct ArchCPU has
no comment of its own. It sits directly below the comment block describing
tsx_compat, so it can be read as sharing the tsx_compat semantics.

Could a short comment be added saying that it applies only to AMD guests
under KVM, and that it is enabled only through the vz machine type compat
properties?

[ ... ]

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

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