linux-kernel - Re: [RFC PATCH v2 2/3] KVM: x86: move ()_in

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YdyKlIHgf3b1K57O@google.com>
Date:   Mon, 10 Jan 2022 19:35:48 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Kechen Lu <kechenl@...dia.com>
Cc:     kvm@...r.kernel.org, pbonzini@...hat.com, wanpengli@...cent.com,
        vkuznets@...hat.com, mst@...hat.com, somduttar@...dia.com,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2 2/3] KVM: x86: move ()_in_guest checking to vCPU
 scope

The shortlog is weird, I get what you're going for with the "()", but it honestly
looks like a typo :-)  And add "power management" so that there's a bit more context
in the shortlog?  Maybe this?

  KVM: x86: Move *_in_guest power management flags to vCPU scope

On Tue, Dec 21, 2021, Kechen Lu wrote:
> For futher extensions on finer-grained control on per-vCPU exits
> disable control, and because VM-scoped restricted to set before
> vCPUs creation, runtime disabled exits flag check could be purely
> vCPU scope.

State what the patch does, not what it "could" do.  E.g.

Make the runtime disabled mwait/hlt/pause/cstate exits flags vCPU scope
to allow finer-grained, per-vCPU control.  The VM-scoped control is only
allowed before vCPUs are created, thus preserving the existing behavior
is a simple matter of snapshotting the flags at vCPU creation.

A few nits below that aren't even from this path, but otherwise,

Reviewed-by: Sean Christopherson <seanjc@...gle.com>

> Suggested-by: Sean Christopherson <seanjc@...gle.com>
> Signed-off-by: Kechen Lu <kechenl@...dia.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  5 +++++
>  arch/x86/kvm/cpuid.c            |  2 +-
>  arch/x86/kvm/lapic.c            |  2 +-
>  arch/x86/kvm/svm/svm.c          | 10 +++++-----
>  arch/x86/kvm/vmx/vmx.c          | 16 ++++++++--------
>  arch/x86/kvm/x86.c              |  6 +++++-
>  arch/x86/kvm/x86.h              | 16 ++++++++--------
>  7 files changed, 33 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 2164b9f4c7b0..edc5fca4d8c8 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -908,6 +908,11 @@ struct kvm_vcpu_arch {
>  #if IS_ENABLED(CONFIG_HYPERV)
>  	hpa_t hv_root_tdp;
>  #endif
> +
> +	bool mwait_in_guest;
> +	bool hlt_in_guest;
> +	bool pause_in_guest;
> +	bool cstate_in_guest;
>  };
>  
>  struct kvm_lpage_info {
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 07e9215e911d..6291e15710ba 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -177,7 +177,7 @@ void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
>  		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
>  
>  	best = kvm_find_kvm_cpuid_features(vcpu);
> -	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
> +	if (kvm_hlt_in_guest(vcpu) && best &&
>  		(best->eax & (1 << KVM_FEATURE_PV_UNHALT)))

Can you (or Paolo?) opportunistically align this?  And maybe even shuffle the
check on "best" to pair the !NULL check with the functional check?  E.g.

	if (kvm_hlt_in_guest(vcpu) &&
	    best && (best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
		best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);

>  		best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
>  
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index f206fc35deff..effb994e6642 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -119,7 +119,7 @@ static bool kvm_can_post_timer_interrupt(struct kvm_vcpu *vcpu)
>  bool kvm_can_use_hv_timer(struct kvm_vcpu *vcpu)
>  {
>  	return kvm_x86_ops.set_hv_timer
> -	       && !(kvm_mwait_in_guest(vcpu->kvm) ||
> +	       && !(kvm_mwait_in_guest(vcpu) ||

And another opportunistic tweak?  I'm been itching for an excuse to "fix" this
particular helper for quite some time :-)

	return kvm_x86_ops.set_hv_timer &&
	       !(kvm_mwait_in_guest(vcpu) || kvm_can_post_timer_interrupt(vcpu));

That overruns the 80 char soft limit, but IMO it's worth it in this case as the
resulting code is more readable.


>  		    kvm_can_post_timer_interrupt(vcpu));
>  }
>  EXPORT_SYMBOL_GPL(kvm_can_use_hv_timer);