Re: [Devel] [PATCH hci-8.0/hci-8.1 2/6] i386: keep hle and rtm on Virtuozzo machine types up to vz9.2.0 #PSBM-149402

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] i386: x-tsx-compat bypasses host_tsx_broken() HLE/RTM filter on erratum CPUs
- [Medium] i386: x-tsx-compat is per machine type, so fresh guests also get HLE/RTM unfiltered
--

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

i386: keep hle and rtm on Virtuozzo machine types up to vz9.2.0 #PSBM-149402

This commit adds an x-tsx-compat CPU property. When it is set, HLE and RTM requested by an Intel CPU model, or on the command line, are not filtered out under KVM. The pc_vz_9_2_compat global properties turn the property on for vz9.2.0 and older vz machine types.

> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c

[ ... ]

> +GlobalProperty pc_vz_9_2_compat[] = {
> +    { TYPE_X86_CPU, "x-tsx-compat", "on" },
> +};
> +const size_t pc_vz_9_2_compat_len = G_N_ELEMENTS(pc_vz_9_2_compat);

[Severity: Medium]
This property is applied to every CPU on the affected machine types. That includes freshly booted guests, not only guests that are migrated, restored or reverted to a snapshot.

On a host with TSX disabled, a fresh guest on a vz9.2.0 or older machine type gets HLE and RTM in its CPUID. This happens when it uses an Intel CPU model, or hle/rtm=on.

If TSX_CTRL disables TSX on that host, XBEGIN raises #UD. A guest that probes CPUID and then uses RTM could fail. The commit message acknowledges this #UD behaviour.

Is there a way to limit the compat behaviour to incoming migration or restore? Alternatively, could the commit message explain why applying it to fresh boots is acceptable?

The commit message says vz9.2.0 and older are "the only ones used by guests started before release 8.0". Nothing in the repository shows this. Could this claim be backed up?

[ ... ]

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

[ ... ]

> +static uint64_t x86_cpu_compat_kept_features(X86CPU *cpu, FeatureWord w)
> +{

[ ... ]

> +    case FEAT_7_0_EBX:
> +        if (cpu->tsx_compat && IS_INTEL_CPU(env)) {
> +            kept = CPUID_7_0_EBX_HLE | CPUID_7_0_EBX_RTM;
> +        }
> +        break;

[Severity: Medium]
Can this bypass the host_tsx_broken() filter in kvm_arch_get_supported_cpuid()?

That filter clears RTM and HLE on Intel family 6 hosts that have the TSX erratum. Those are model 63 with stepping below 4, and models 60, 69 and 70:

target/i386/kvm/kvm.c:kvm_arch_get_supported_cpuid() {
    ...
        if (host_tsx_broken()) {
            ret &= ~(CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_HLE);
        }
    ...
}

On such a host, with tsx_compat set and an Intel CPU model that has hle/rtm, x86_cpu_compat_kept_features() returns HLE and RTM unconditionally. They are then removed from the unavailable set in x86_cpu_filter_features(), so they stay in env->features[FEAT_7_0_EBX].

kvm_arch_init_vcpu() builds the CPUID entries from cpu_x86_cpuid() and passes them to KVM_SET_CPUID2. It does not mask them against the KVM-supported set again, so the kept bits appear to reach the guest. The guest would then see TSX on silicon where TSX is known to misbehave.

The commit message only covers TAA microcode and tsx=off. Should the kept bits be limited to cases where host_tsx_broken() is false, or should keeping them on erratum parts be explicitly justified?

> @@ -9126,7 +9149,8 @@ static bool x86_cpu_filter_features(X86CPU *cpu, bool verbose)
>          uint64_t host_feat =
>              x86_cpu_get_supported_feature_word(NULL, w);
>          uint64_t requested_features = env->features[w];
> -        uint64_t unavailable_features = requested_features & ~host_feat;
> +        uint64_t unavailable_features = requested_features & ~host_feat &
> +            ~x86_cpu_compat_kept_features(cpu, w);
>          mark_unavailable_features(cpu, w, unavailable_features, prefix);

[Severity: Medium]
The kept bits are removed from unavailable_features before mark_unavailable_features() is called. They therefore never reach filtered_features.

As a result, the verbose warnings and the enforce check stay silent for HLE and RTM. This applies to fresh boots as well as migrations.

x86_cpu_filter_features() is also called with verbose=false from the CPU model and definition reporting path. Could that path report a model as runnable on a host without TSX?

[ ... ]

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

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