Re: [PATCH v2 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 3 potential issue(s) to consider:
- [Medium] i386: spec-ctrl compat checks host AMD bits, not the guest's own AMD bits
- [Low] i386: spec_ctrl_compat field sits under the TSX-only comment with no doc
- [Low] i386: spec-ctrl compat keyed on guest vendor, not covering Intel models on AMD hosts
--

commit 345ccab3ac1f9c86d5966e00d0e21318a3f7dff4
Author: Denis V. Lunev <den@openvz.org>

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

This patch adds the x-spec-ctrl-compat CPU property and enables it in
pc_vz_9_2_compat. When it is set, an AMD guest keeps a requested spec-ctrl
if the host reports amd-ibpb and one of amd-ibrs, amd-stibp or amd-ssbd.
This avoids spec-ctrl being filtered on hosts where KVM no longer offers it
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]
Can spec-ctrl be kept for a guest that does not itself carry the AMD
bits? The condition in x86_cpu_compat_kept_features() reads only the
host-supported bits from kvm_arch_get_supported_cpuid() for 0x80000008
EBX. It never looks at env->features[FEAT_8000_0008_EBX].

In x86_cpu_filter_features() the guest's FEAT_8000_0008_EBX is filtered
independently, and the kept bit bypasses the host-support filter:

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

A guest with x-spec-ctrl-compat=on and spec-ctrl requested, but lacking
amd-ibpb or all of amd-ibrs, amd-stibp and amd-ssbd, could keep spec-ctrl
in CPUID. KVM might then leave MSR_IA32_SPEC_CTRL or MSR_IA32_PRED_CMD
unavailable to that guest, so a guest wrmsr would take #GP.

This relies on the KVM behaviour described in the cited kernel commits,
which I could not verify in this tree. Should the check also test the
guest's own FEAT_8000_0008_EBX bits? The later patches in the series do
not change this case.

> +            }
> +        }
> +        break;

[Severity: Low]
Is the IS_AMD_CPU(env) gate too narrow? It tests the guest CPUID vendor,
not the host vendor. For an Intel-vendor guest model on an AMD host,
kept stays 0 in x86_cpu_compat_kept_features(), so
x86_cpu_filter_features() still marks spec-ctrl unavailable:

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

The existing Intel models set CPUID_7_0_EDX_SPEC_CTRL, and nothing in
this tree forces the guest vendor to match the host vendor. Such a guest
would hit the same migration, restore and snapshot revert failure
described in the commit message. Is it intended that Hygon guests and
Intel-vendor models on AMD hosts are left out? The series-final state
keeps the same IS_AMD_CPU(env) gate.

[ ... ]

> 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 is placed directly
under the comment that describes tsx_compat, so that comment appears to
cover it as well. Could it get its own comment describing the AMD
spec-ctrl behaviour?

>  
>      /* Number of physical address bits supported */
>      uint32_t phys_bits;

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