Re: [PATCH hci-8.0/hci-8.1] i386: hide la57 at fresh start on vz machine types up to vz10.0.0 #VSTOR-152405

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] i386: x86_cpu_hide_la57() hides la57 again on -loadvm restore
- [Low] i386: x86_cpu_hide_la57() uses runstate_check() outside !CONFIG_USER_ONLY
- [Low] i386: x86_cpu_hide_la57() also hides la57 for -cpu max, not only -cpu host
--

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

i386: hide la57 at fresh start on vz machine types up to vz10.0.0 #VSTOR-152405

This adds the x-no-la57-on-start CPU property and enables it in
pc_vz_10_0_compat for vz10.0.0 and older machine types. On a fresh
start under KVM, a "-cpu host" guest loses la57 unless la57=on is given.
An incoming migration keeps la57, and a hotplugged vCPU follows the boot
vCPUs.

> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -40,6 +40,7 @@
>  #ifndef CONFIG_USER_ONLY
>  #include "confidential-guest.h"
>  #include "system/reset.h"
> +#include "system/runstate.h"
>  #include "qapi/qapi-commands-machine.h"
>  #include "system/address-spaces.h"
>  #include "hw/boards.h"

[ ... ]

> +/*
> + * Reported as filtered, so that libvirt records la57=off in the live
> + * definition and passes it on to migration, save and snapshots.
> + */
> +static bool x86_cpu_hide_la57(X86CPU *cpu)
> +{
> +    CPUX86State *env = &cpu->env;
> +
> +    if (!cpu->no_la57_on_start || !X86_CPU_GET_CLASS(cpu)->max_features ||

[Severity: Low]
The commit message and the cpu.h comment describe this as applying to
"-cpu host". Is that the intended scope?

X86_CPU_GET_CLASS(cpu)->max_features is also set for the "max" CPU
class, not only the host class. x86_cpu_expand_features() uses the same
predicate to auto-enable host-supported features for both.

On vz10.0.0 and older machine types under KVM, "-cpu max" guests would
therefore also lose la57. Should the commit message and the comment
mention this, or should the check be narrowed to the host model?

> +        !kvm_enabled() ||
> +        !(env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_LA57) ||
> +        (env->user_features[FEAT_7_0_ECX] & CPUID_7_0_ECX_LA57)) {
> +        return false;
> +    }
> +    if (DEVICE(cpu)->hotplugged) {
> +        return first_cpu && !(X86_CPU(first_cpu)->env.features[FEAT_7_0_ECX] &
> +                              CPUID_7_0_ECX_LA57);
> +    }
> +    return !runstate_check(RUN_STATE_INMIGRATE);

[Severity: Low]
Will this build for linux-user and bsd-user i386 and x86_64 targets?

The only declaration of runstate_check() is in system/runstate.h, and
this patch adds that include inside the #ifndef CONFIG_USER_ONLY block
near the top of target/i386/cpu.c:

#ifndef CONFIG_USER_ONLY
#include "confidential-guest.h"
#include "system/reset.h"
#include "system/runstate.h"

But x86_cpu_hide_la57() is defined outside any CONFIG_USER_ONLY guard,
and x86_cpu_realizefn() calls it unconditionally. kvm_enabled() is
(0) in user-only builds, but that does not remove the runstate_check()
call from compilation, so it would be an implicit declaration there.

Could the helper and its call site be wrapped in #ifndef
CONFIG_USER_ONLY, or could a user-only stub be provided?

[Severity: Medium]
Is the la57 bit hidden again when a guest is restored with -loadvm?

Here only RUN_STATE_INMIGRATE keeps la57. With -loadvm, the vCPUs are
realized during machine init and load_snapshot() runs afterwards from
qemu_init():

    if (loadvm) {
        ...
        load_snapshot(loadvm, NULL, false, NULL, &error_fatal);

-incoming and -loadvm are mutually exclusive, so the runstate during
realize is PRELAUNCH and x86_cpu_hide_la57() returns true when
x-no-la57-on-start is set and la57=on was not given.

If the snapshot was taken from a guest started by an older QEMU, or
without la57=off, the guest may already run with CR4.LA57 set. After
-loadvm the vCPU CPUID lacks la57, so I would expect CR4 validation in
KVM_SET_SREGS to fail or the guest to misbehave. I did not confirm the
KVM behaviour in this tree.

Is there any check that detects an la57 mismatch between the saved
state and the restored CPU? As far as I can tell, nothing in QEMU
enforces consistency, and migration depends on the management layer
passing la57=off to the destination. The commit message notes that this
libvirt step was not tested.

> +}
> +
>  /* Requested features that a vz compat property keeps despite the host */
>  static uint64_t x86_cpu_compat_kept_features(X86CPU *cpu, FeatureWord w)
>  {

[ ... ]

> @@ -9404,6 +9426,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      }
>  
> +    if (x86_cpu_hide_la57(cpu)) {
> +        mark_unavailable_features(cpu, FEAT_7_0_ECX, CPUID_7_0_ECX_LA57, NULL);
> +    }
> +

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