Re: [PATCH v3 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 1 potential issue(s) to consider:
- [Medium] i386: x-tsx-compat keeps HLE/RTM without checking host TSX support
--

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

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

This patch adds an x-tsx-compat CPU property. When it is set, HLE and RTM
requested by an Intel CPU model are not filtered out under KVM. The property
is turned on by pc_vz_9_2_compat for vz9.2.0 and all older vz machine types.
The goal is that a guest started with TSX can still be migrated, restored or
reverted on a host where microcode or tsx=off has disabled TSX.

> 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]
Because this is a machine type property, it is also on for a cold boot of
every vz type up to vz9.2.0, not only for incoming migration.

Can a cold-booted guest end up with HLE and RTM in its CPUID on a host that
has no TSX silicon, for example an AMD host running an Intel-named CPU
model, or an Intel part that never had TSX?

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

[ ... ]

> +/* Requested features that a vz compat property keeps despite the host */
> +static uint64_t x86_cpu_compat_kept_features(X86CPU *cpu, FeatureWord w)
> +{
> +    CPUX86State *env = &cpu->env;
> +    uint64_t kept = 0;
> +
> +    if (!kvm_enabled()) {
> +        return 0;
> +    }
> +
> +    switch (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]
The guard in x86_cpu_compat_kept_features() checks only kvm_enabled(),
cpu->tsx_compat and IS_INTEL_CPU(env). IS_INTEL_CPU(env) tests the vendor of
the guest CPU model, not of the host.

Nothing here checks whether the host implements TSX, as opposed to having it
disabled by microcode through TSX_CTRL or RTM_ALWAYS_ABORT. The commit
message justifies the bypass only for the second case, where XBEGIN aborts.

On silicon without TSX, XBEGIN raises #UD instead of aborting. Could a guest
that trusts CPUID and uses RTM (glibc lock elision, libitm) then take an
invalid-opcode fault?

Would it make sense to restrict the kept bits to hosts where TSX exists but
is disabled?

This also bypasses the existing safeguard in kvm_arch_get_supported_cpuid():

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);
        }
    ...
}

The commit message says this is intended for Haswell parts with the erratum.
Is it also intended for hosts with no TSX at all?

[ ... ]

> @@ -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]
In x86_cpu_filter_features(), the kept bits are removed from
unavailable_features before mark_unavailable_features() is called.

The bits therefore stay in env->features and go into the guest CPUID. They
are also never added to cpu->filtered_features, so the check and enforce
warnings and errors stay silent for them.

Is it intended that enforce can no longer reject HLE or RTM on a host that
lacks TSX entirely?

I did not trace the KVM_SET_CPUID2 path in kvm_arch_init_vcpu(). The commit
message says KVM ignores unsupported bits, but the kernel does not validate
userspace-supplied guest CPUID against host capability.

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