Re: [PATCH v2 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 advertises HLE/RTM on cold boot even when host TSX is off
- [Low] i386: x-tsx-compat keeps HLE/RTM on Haswell hosts without checking microcode
--

commit dbdfc9450553c67e1ff2b80314cb5e7632457006
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 on, hle and rtm
requested by an Intel CPU model are not filtered against host support under
KVM. The property is enabled through pc_vz_9_2_compat for vz9.2.0 and older
vz machine types, so that a guest that booted with TSX can still be migrated
to a host where TSX is disabled.

> 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 default, it applies to cold boot as well as
to incoming migration. Can this make a freshly started guest see hle and
rtm in CPUID on a host where TSX is off?

Where the host disables TSX through TSX_CTRL (tsx=off or the TAA default),
XBEGIN raises #UD, as the commit message says. A guest that trusts CPUID,
for example through lock elision or a TSX-aware library, could then take
SIGILL or a guest kernel fault.

Previously a fresh guest on such a host booted with these bits filtered.
For a cold boot this looks like new exposure. The statement that "the cold
boot adds no new failure mode" only seems to hold for guests that already
ran with the bits on another node.

Would it be possible to limit the behaviour to incoming migration? Or could
the code check how the host disabled TSX?

> 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)
> +{

[ ... ]

> +    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 only conditions here are kvm_enabled(), tsx_compat and IS_INTEL_CPU().
Nothing checks for incoming migration or for the way the host disabled TSX.

x86_cpu_filter_features() subtracts this result from unavailable_features,
so hle and rtm stay in env->features[FEAT_7_0_EBX]. They are not recorded
through mark_unavailable_features(), so check and enforce stay silent.

The existing masking in kvm_arch_get_supported_cpuid() only runs when host
TSX is broken (the host_tsx_broken() branch that clears RTM and HLE). It
affects the supported-CPUID query and not the guest CPUID. Is anything in
kvm.c expected to mask these bits again before they reach KVM_SET_CPUID2?

I could not confirm how the kernel handles these bits in KVM_SET_CPUID2.

[Severity: Low]
There is a second question about Haswell hosts with the TSX erratum. In
kvm_arch_get_supported_cpuid(), host_tsx_broken() hides RTM and HLE. It
looks only at family, model and stepping, and it reads no microcode
revision.

With x-tsx-compat on, x86_cpu_compat_kept_features() returns HLE and RTM
for any Intel guest, regardless of host state. How does the code tell
"erratum microcode has disabled TSX" apart from "erratum part with TSX still
active"?

The commit message says the "erratum microcode disables TSX" on these parts.
Is there code or documentation that backs up that assumption? Without it, a
guest could see TSX enabled on silicon with the known erratum, and no
check/enforce warning would be printed.

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