+Kechen
On Thu, Mar 30, 2023, Hou Wenlong wrote:
> __kvm_update_cpuid_runtime() may be called during vCPU running and KVM
> PV feature CPUID is updated too. But the cached KVM PV feature bitmap is
> not updated. Actually, KVM PV feature CPUID shouldn't be updated,
> otherwise, KVM PV feature would be broken in guest. Currently, only
> KVM_FEATURE_PV_UNHALT is updated, and it's impossible after disallow
> disable HLT exits. However, KVM PV feature CPUID should be updated only
> in KVM_SET_CPUID{,2} ioctl.
>
> Signed-off-by: Hou Wenlong <houwenlong.hwl@xxxxxxxxxxxx>
> ---
> arch/x86/kvm/cpuid.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 6972e0be60fa..af92d3422c79 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -222,6 +222,17 @@ static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcp
> vcpu->arch.cpuid_nent);
> }
>
> +static void kvm_update_pv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries,
> + int nent)
> +{
> + struct kvm_cpuid_entry2 *best;
> +
> + best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
> + if (kvm_hlt_in_guest(vcpu->kvm) && best &&
> + (best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
> + best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
> +}
> +
> void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
> {
> struct kvm_cpuid_entry2 *best = kvm_find_kvm_cpuid_features(vcpu);
> @@ -280,11 +291,6 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
> cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
> best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
>
> - best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
> - if (kvm_hlt_in_guest(vcpu->kvm) && best &&
> - (best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
> - best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
> -
> if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT)) {
> best = cpuid_entry2_find(entries, nent, 0x1, KVM_CPUID_INDEX_NOT_SIGNIFICANT);
> if (best)
> @@ -402,6 +408,7 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
> int r;
>
> __kvm_update_cpuid_runtime(vcpu, e2, nent);
> + kvm_update_pv_cpuid(vcpu, e2, nent);
Hrm, this will silently conflict with the proposed per-vCPU controls[*]. Though
arguably that patch is buggy and "needs" to toggle PV_UNHALT when userspace
messes with HLT passthrough. But that doesn't really make sense either because
no guest will react kindly to KVM_FEATURE_PV_UNHALT disappearing.
I really wish this code didn't exist, i.e. that KVM let/forced userspace deal
with correctly defining guest CPUID.
Kechen, is it feasible for your userspace to clear PV_UNHALT when it (might) use
the per-vCPU control? I.e. can KVM do as this series proposes and update guest
CPUID only on KVM_SET_CPUID{2}? Dropping the behavior for the per-VM control
is probably not an option as I gotta assume that'd break userspace, but I would
really like to avoid carrying that over to the per-vCPU control, which would get
quite messy and probably can't work anyways.
[*] https://lkml.kernel.org/r/20230121020738.2973-6-kechenl%40nvidia.com
