[RFC PATCH 0/9] Split kvm_update_cpuid_runtime()

[RFC PATCH 0/9] Split kvm_update_cpuid_runtime()

[Robert Hoo]

kvm_update_cpuid_runtime() is currently called by various functions for the
purpose of updating vCPU's cpuid entries, due to specific runtime changes, e.g.
CR4 bits changes, XCR0 bits changes, etc. Each of them actually just needs to
update 1 ~ 2 CPUID entries. But current kvm_update_cpuid_runtime() packages all.
Given finding a target CPUID entry need to go through all CPUID entries, calling
kvm_update_cpuid_runtime() is a waste for each cause.

This patch set splits kvm_update_cpuid_runtime() into pieces according to
different updating causes.
Then let various callers call their specific necessary kvm_xxx_update_cpuid().

This not only significantly saves each caller's time, but also eliminates
unnecessary couplings.

Robert Hoo (9):
  KVM:x86: Abstract sub functions from kvm_update_cpuid_runtime() and   
     kvm_vcpu_after_set_cpuid()
  KVM:x86: Substitute kvm_update_cpuid_runtime() with
    kvm_apic_base_update_cpuid() in     kvm_lapic_set_base()
  KVM:x86: Substitute kvm_update_cpuid_runtime() with
    kvm_xcr0_update_cpuid() in     __kvm_set_xcr()
  KVM:x86: Substitute kvm_update_cpuid_runtime() with
    kvm_{osxsave,pke}_update_cpuid() in     kvm_set_cr4()
  KVM:x86: Substitute kvm_update_cpuid_runtime() with
    kvm_mwait_update_cpuid() in     kvm_set_msr_common()
  KVM:x86: Substitute kvm_update_cpuid_runtime() with
    kvm_{osxsave,pke}_update_cpuid() in     enter_smm()
  KVM:x86: Substitute kvm_update_cpuid_runtime() with
    kvm_{osxsave,pke}_update_cpuid() in     __set_sregs()
  KVM:x86: Substitute kvm_vcpu_after_set_cpuid() with abstracted
    functions
  KVM:x86: Remove kvm_update_cpuid_runtime()

 arch/x86/kvm/cpuid.c | 118 ++++++++++++++++++++++++++++++++++-----------------
 arch/x86/kvm/cpuid.h |   7 ++-
 arch/x86/kvm/lapic.c |   2 +-
 arch/x86/kvm/x86.c   |  29 ++++++++-----
 4 files changed, 103 insertions(+), 53 deletions(-)

-- 
1.8.3.1

[RFC PATCH 1/9] KVM:x86: Abstract sub functions from kvm_update_cpuid_runtime() and kvm_vcpu_after_set_cpuid()

[Robert Hoo]

Add below functions, whose aggregation equals kvm_update_cpuid_runtime() and
kvm_vcpu_after_set_cpuid().

void kvm_osxsave_update_cpuid(struct kvm_vcpu *vcpu, bool set)
void kvm_pke_update_cpuid(struct kvm_vcpu *vcpu, bool set)
void kvm_apic_base_update_cpuid(struct kvm_vcpu *vcpu, bool set)
void kvm_mwait_update_cpuid(struct kvm_vcpu *vcpu, bool set)
void kvm_xcr0_update_cpuid(struct kvm_vcpu *vcpu)
static void kvm_pv_unhalt_update_cpuid(struct kvm_vcpu *vcpu)
static void kvm_update_maxphyaddr(struct kvm_vcpu *vcpu)
static void kvm_update_lapic_timer_mode(struct kvm_vcpu *vcpu)

And, for some among the above, avoid double check set or clear inside function
body, but provided by caller.

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 arch/x86/kvm/cpuid.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kvm/cpuid.h |  6 ++++
 2 files changed, 105 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 7d92854..efa7182 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -73,6 +73,105 @@ static int kvm_check_cpuid(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
+void kvm_osxsave_update_cpuid(struct kvm_vcpu *vcpu, bool set)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry(vcpu, 1, 0);
+
+	if (best && boot_cpu_has(X86_FEATURE_XSAVE)) {
+		cpuid_entry_change(best, X86_FEATURE_OSXSAVE, set);
+	}
+}
+
+void kvm_pke_update_cpuid(struct kvm_vcpu *vcpu, bool set)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry(vcpu, 7, 0);
+
+	if (best && boot_cpu_has(X86_FEATURE_PKU)) {
+		cpuid_entry_change(best, X86_FEATURE_OSPKE, set);
+	}
+}
+
+void kvm_xcr0_update_cpuid(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
+	if (!best) {
+		vcpu->arch.guest_supported_xcr0 = 0;
+	} else {
+		vcpu->arch.guest_supported_xcr0 =
+			(best->eax | ((u64)best->edx << 32)) & supported_xcr0;
+		best->ebx = xstate_required_size(vcpu->arch.xcr0, false);
+	}
+
+	best = kvm_find_cpuid_entry(vcpu, 0xD, 1);
+	if (!best)
+		return;
+	if (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
+				cpuid_entry_has(best, X86_FEATURE_XSAVEC))
+		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
+}
+
+static void kvm_pv_unhalt_update_cpuid(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry(vcpu, KVM_CPUID_FEATURES, 0);
+	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
+					(best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
+		best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
+}
+
+void kvm_mwait_update_cpuid(struct kvm_vcpu *vcpu, bool set)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry(vcpu, 0x1, 0);
+	if (best)
+		cpuid_entry_change(best, X86_FEATURE_MWAIT, set);
+}
+
+static void kvm_update_maxphyaddr(struct kvm_vcpu *vcpu)
+{
+
+	/* Note, maxphyaddr must be updated before tdp_level. */
+	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
+	vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu);
+	kvm_mmu_reset_context(vcpu);
+}
+
+void kvm_apic_base_update_cpuid(struct kvm_vcpu *vcpu, bool set)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry(vcpu, 1, 0);
+	if (!best)
+		return;
+
+	cpuid_entry_change(best, X86_FEATURE_APIC, set);
+}
+
+static void kvm_update_lapic_timer_mode(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpuid_entry2 *best;
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	best = kvm_find_cpuid_entry(vcpu, 1, 0);
+	if (!best)
+		return;
+
+	if (apic) {
+		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
+			apic->lapic_timer.timer_mode_mask = 3 << 17;
+		else
+			apic->lapic_timer.timer_mode_mask = 1 << 17;
+	}
+}
+
 void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 3a923ae..7eabb44 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -9,6 +9,12 @@
 extern u32 kvm_cpu_caps[NCAPINTS] __read_mostly;
 void kvm_set_cpu_caps(void);
 
+void kvm_osxsave_update_cpuid(struct kvm_vcpu *vcpu, bool set);
+void kvm_pke_update_cpuid(struct kvm_vcpu *vcpu, bool set);
+void kvm_apic_base_update_cpuid(struct kvm_vcpu *vcpu, bool set);
+void kvm_mwait_update_cpuid(struct kvm_vcpu *vcpu, bool set);
+void kvm_xcr0_update_cpuid(struct kvm_vcpu *vcpu);
+
 void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu);
 struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
 					      u32 function, u32 index);
-- 
1.8.3.1

Re: [RFC PATCH 1/9] KVM:x86: Abstract sub functions from kvm_update_cpuid_runtime() and kvm_vcpu_after_set_cpuid()

[Sean Christopherson]

I think you want "extract", not "abstract".


On Fri, Jul 31, 2020 at 10:42:19AM +0800, Robert Hoo wrote:
> Add below functions, whose aggregation equals kvm_update_cpuid_runtime() and
> kvm_vcpu_after_set_cpuid().
> 
> void kvm_osxsave_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> void kvm_pke_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> void kvm_apic_base_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> void kvm_mwait_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> void kvm_xcr0_update_cpuid(struct kvm_vcpu *vcpu)
> static void kvm_pv_unhalt_update_cpuid(struct kvm_vcpu *vcpu)
> static void kvm_update_maxphyaddr(struct kvm_vcpu *vcpu)
> static void kvm_update_lapic_timer_mode(struct kvm_vcpu *vcpu)
> 
> And, for some among the above, avoid double check set or clear inside function
> body, but provided by caller.
> 
> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> ---
>  arch/x86/kvm/cpuid.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  arch/x86/kvm/cpuid.h |  6 ++++
>  2 files changed, 105 insertions(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 7d92854..efa7182 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -73,6 +73,105 @@ static int kvm_check_cpuid(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>  
> +void kvm_osxsave_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> +{
> +	struct kvm_cpuid_entry2 *best;

I vote that we opportunistically move away from the "best" terminology.  Either
there's a matching entry or there's not.  Using "e" would probably shave a few
lines of code.

> +
> +	best = kvm_find_cpuid_entry(vcpu, 1, 0);
> +
> +	if (best && boot_cpu_has(X86_FEATURE_XSAVE)) {

Braces not needed. We should also check boot_cpu_has() first, it's constant
time and maybe even in the cache, whereas finding CPUID entries is linear
time and outright slow.

Actually, can you add a helper to handle this?  With tht boot_cpu_has() check
outside of the helper?  That'll allow the helper to be used for more features,
and will force checking boot_cpu_has() first.  Hmm, and not all of the callers
will need the boot_cpu_has() check, e.g. toggling PKE from kvm_set_cr4()
doesn't need to be guarded because KVM disallows setting PKE if it's not
supported by the host.

static inline void guest_cpuid_change(struct kvm_vcpu *vcpu, u32 function,
				      u32 index, unsigned int feature, bool set)
{
	struct kvm_cpuid_entry2 *e =  kvm_find_cpuid_entry(vcpu, function, index);

	if (e)
		cpuid_entry_change(best, X86_FEATURE_OSXSAVE, set);
}

> +		cpuid_entry_change(best, X86_FEATURE_OSXSAVE, set);
> +	}
> +}
> +
> +void kvm_pke_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> +{
> +	struct kvm_cpuid_entry2 *best;
> +
> +	best = kvm_find_cpuid_entry(vcpu, 7, 0);
> +
> +	if (best && boot_cpu_has(X86_FEATURE_PKU)) {
> +		cpuid_entry_change(best, X86_FEATURE_OSPKE, set);
> +	}
> +}
> +
> +void kvm_xcr0_update_cpuid(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_cpuid_entry2 *best;a
> +
> +	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
> +	if (!best) {
> +		vcpu->arch.guest_supported_xcr0 = 0;
> +	} else {
> +		vcpu->arch.guest_supported_xcr0 =
> +			(best->eax | ((u64)best->edx << 32)) & supported_xcr0;
> +		best->ebx = xstate_required_size(vcpu->arch.xcr0, false);
> +	}
> +
> +	best = kvm_find_cpuid_entry(vcpu, 0xD, 1);
> +	if (!best)
> +		return;
> +	if (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
> +				cpuid_entry_has(best, X86_FEATURE_XSAVEC))

Indentation should be aligned, e.g.

	if (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
	    cpuid_entry_has(best, X86_FEATURE_XSAVEC))


> +		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
> +}
> +
> +static void kvm_pv_unhalt_update_cpuid(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_cpuid_entry2 *best;
> +
> +	best = kvm_find_cpuid_entry(vcpu, KVM_CPUID_FEATURES, 0);
> +	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
> +					(best->eax & (1 << KVM_FEATURE_PV_UNHALT)))
> +		best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
> +}
> +
> +void kvm_mwait_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> +{
> +	struct kvm_cpuid_entry2 *best;
> +
> +	best = kvm_find_cpuid_entry(vcpu, 0x1, 0);
> +	if (best)
> +		cpuid_entry_change(best, X86_FEATURE_MWAIT, set);
> +}
> +
> +static void kvm_update_maxphyaddr(struct kvm_vcpu *vcpu)
> +{
> +
> +	/* Note, maxphyaddr must be updated before tdp_level. */
> +	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
> +	vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu);
> +	kvm_mmu_reset_context(vcpu);
> +}
> +
> +void kvm_apic_base_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> +{
> +	struct kvm_cpuid_entry2 *best;
> +
> +	best = kvm_find_cpuid_entry(vcpu, 1, 0);
> +	if (!best)
> +		return;
> +
> +	cpuid_entry_change(best, X86_FEATURE_APIC, set);
> +}
> +
> +static void kvm_update_lapic_timer_mode(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_cpuid_entry2 *best;
> +	struct kvm_lapic *apic = vcpu->arch.apic;
> +
> +	best = kvm_find_cpuid_entry(vcpu, 1, 0);
> +	if (!best)
> +		return;
> +
> +	if (apic) {

Check apic before the lookup.

> +		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
> +			apic->lapic_timer.timer_mode_mask = 3 << 17;
> +		else
> +			apic->lapic_timer.timer_mode_mask = 1 << 17;
> +	}
> +}
> +
>  void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_cpuid_entry2 *best;
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index 3a923ae..7eabb44 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -9,6 +9,12 @@
>  extern u32 kvm_cpu_caps[NCAPINTS] __read_mostly;
>  void kvm_set_cpu_caps(void);
>  
> +void kvm_osxsave_update_cpuid(struct kvm_vcpu *vcpu, bool set);
> +void kvm_pke_update_cpuid(struct kvm_vcpu *vcpu, bool set);
> +void kvm_apic_base_update_cpuid(struct kvm_vcpu *vcpu, bool set);
> +void kvm_mwait_update_cpuid(struct kvm_vcpu *vcpu, bool set);
> +void kvm_xcr0_update_cpuid(struct kvm_vcpu *vcpu);
> +
>  void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu);
>  struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
>  					      u32 function, u32 index);
> -- 
> 1.8.3.1
> 

Re: [RFC PATCH 1/9] KVM:x86: Abstract sub functions from kvm_update_cpuid_runtime() and kvm_vcpu_after_set_cpuid()

[Robert Hoo]

On Mon, 2020-09-28 at 21:56 -0700, Sean Christopherson wrote:
> I think you want "extract", not "abstract".
> 
> 
> On Fri, Jul 31, 2020 at 10:42:19AM +0800, Robert Hoo wrote:
> > Add below functions, whose aggregation equals
> > kvm_update_cpuid_runtime() and
> > kvm_vcpu_after_set_cpuid().
> > 
> > void kvm_osxsave_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> > void kvm_pke_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> > void kvm_apic_base_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> > void kvm_mwait_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> > void kvm_xcr0_update_cpuid(struct kvm_vcpu *vcpu)
> > static void kvm_pv_unhalt_update_cpuid(struct kvm_vcpu *vcpu)
> > static void kvm_update_maxphyaddr(struct kvm_vcpu *vcpu)
> > static void kvm_update_lapic_timer_mode(struct kvm_vcpu *vcpu)
> > 
> > And, for some among the above, avoid double check set or clear
> > inside function
> > body, but provided by caller.
> > 
> > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 99
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  arch/x86/kvm/cpuid.h |  6 ++++
> >  2 files changed, 105 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 7d92854..efa7182 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -73,6 +73,105 @@ static int kvm_check_cpuid(struct kvm_vcpu
> > *vcpu)
> >  	return 0;
> >  }
> >  
> > +void kvm_osxsave_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> > +{
> > +	struct kvm_cpuid_entry2 *best;
> 
> I vote that we opportunistically move away from the "best"
> terminology.  Either
> there's a matching entry or there's not.  Using "e" would probably
> shave a few
> lines of code.
> 
> > +
> > +	best = kvm_find_cpuid_entry(vcpu, 1, 0);
> > +
> > +	if (best && boot_cpu_has(X86_FEATURE_XSAVE)) {
> 
> Braces not needed. We should also check boot_cpu_has() first, it's
> constant
> time and maybe even in the cache, whereas finding CPUID entries is
> linear
> time and outright slow.
> 
> Actually, can you add a helper to handle this?  With tht
> boot_cpu_has() check
> outside of the helper?  That'll allow the helper to be used for more
> features,
> and will force checking boot_cpu_has() first.  Hmm, and not all of
> the callers
> will need the boot_cpu_has() check, e.g. toggling PKE from
> kvm_set_cr4()
> doesn't need to be guarded because KVM disallows setting PKE if it's
> not
> supported by the host.

Do you mean because in kvm_set_cr4(), it has kvm_valid_cr4(vcpu, cr4)
check first?

Then how about the other 2 callers of kvm_pke_update_cpuid()?
enter_smm() -- I think it can ommit boot_cpu_has() check as well.
because it unconditionally cleared all CR4 bit before calls
kvm_set_cr4().
__set_sregs() -- looks like it doesn't valid host PKE status before
call kvm_pke_update_cpuid(). Can I ommit boot_cpu_has() as well?

So, I don't think kvm_pke_update_cpuid() can leverage the helper. Am I
right?

> 
> static inline void guest_cpuid_change(struct kvm_vcpu *vcpu, u32
> function,
> 				      u32 index, unsigned int feature,
> bool set)
> {
> 	struct kvm_cpuid_entry2 *e =  kvm_find_cpuid_entry(vcpu,
> function, index);
> 
> 	if (e)
> 		cpuid_entry_change(best, X86_FEATURE_OSXSAVE, set);
> }

Thanks Sean, I'm going to have this helper in v2 and have your signed-
off-by.
> 
> > +		cpuid_entry_change(best, X86_FEATURE_OSXSAVE, set);
> > +	}
> > +}
> > +
> > +void kvm_pke_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> > +{
> > +	struct kvm_cpuid_entry2 *best;
> > +
> > +	best = kvm_find_cpuid_entry(vcpu, 7, 0);
> > +
> > +	if (best && boot_cpu_has(X86_FEATURE_PKU)) {
> > +		cpuid_entry_change(best, X86_FEATURE_OSPKE, set);
> > +	}
> > +}
> > +
> > +void kvm_xcr0_update_cpuid(struct kvm_vcpu *vcpu)
> > +{
> > +	struct kvm_cpuid_entry2 *best;a
> > +
> > +	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
> > +	if (!best) {
> > +		vcpu->arch.guest_supported_xcr0 = 0;
> > +	} else {
> > +		vcpu->arch.guest_supported_xcr0 =
> > +			(best->eax | ((u64)best->edx << 32)) &
> > supported_xcr0;
> > +		best->ebx = xstate_required_size(vcpu->arch.xcr0,
> > false);
> > +	}
> > +
> > +	best = kvm_find_cpuid_entry(vcpu, 0xD, 1);
> > +	if (!best)
> > +		return;
> > +	if (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
> > +				cpuid_entry_has(best,
> > X86_FEATURE_XSAVEC))
> 
> Indentation should be aligned, e.g.
> 
> 	if (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
> 	    cpuid_entry_has(best, X86_FEATURE_XSAVEC))
> 
> 
> > +		best->ebx = xstate_required_size(vcpu->arch.xcr0,
> > true);
> > +}
> > +
> > +static void kvm_pv_unhalt_update_cpuid(struct kvm_vcpu *vcpu)
> > +{
> > +	struct kvm_cpuid_entry2 *best;
> > +
> > +	best = kvm_find_cpuid_entry(vcpu, KVM_CPUID_FEATURES, 0);
> > +	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
> > +					(best->eax & (1 <<
> > KVM_FEATURE_PV_UNHALT)))
> > +		best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
> > +}
> > +
> > +void kvm_mwait_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> > +{
> > +	struct kvm_cpuid_entry2 *best;
> > +
> > +	best = kvm_find_cpuid_entry(vcpu, 0x1, 0);
> > +	if (best)
> > +		cpuid_entry_change(best, X86_FEATURE_MWAIT, set);
> > +}
> > +
> > +static void kvm_update_maxphyaddr(struct kvm_vcpu *vcpu)
> > +{
> > +
> > +	/* Note, maxphyaddr must be updated before tdp_level. */
> > +	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
> > +	vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu);
> > +	kvm_mmu_reset_context(vcpu);
> > +}
> > +
> > +void kvm_apic_base_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> > +{
> > +	struct kvm_cpuid_entry2 *best;
> > +
> > +	best = kvm_find_cpuid_entry(vcpu, 1, 0);
> > +	if (!best)
> > +		return;
> > +
> > +	cpuid_entry_change(best, X86_FEATURE_APIC, set);
> > +}
> > +
> > +static void kvm_update_lapic_timer_mode(struct kvm_vcpu *vcpu)
> > +{
> > +	struct kvm_cpuid_entry2 *best;
> > +	struct kvm_lapic *apic = vcpu->arch.apic;
> > +
> > +	best = kvm_find_cpuid_entry(vcpu, 1, 0);
> > +	if (!best)
> > +		return;
> > +
> > +	if (apic) {
> 
> Check apic before the lookup.
> 
> > +		if (cpuid_entry_has(best,
> > X86_FEATURE_TSC_DEADLINE_TIMER))
> > +			apic->lapic_timer.timer_mode_mask = 3 << 17;
> > +		else
> > +			apic->lapic_timer.timer_mode_mask = 1 << 17;
> > +	}
> > +}
> > +
> >  void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
> >  {

...
> > -- 
> > 1.8.3.1
> > 

Re: [RFC PATCH 1/9] KVM:x86: Abstract sub functions from kvm_update_cpuid_runtime() and kvm_vcpu_after_set_cpuid()

[Sean Christopherson]

On Tue, Oct 20, 2020 at 01:38:32PM +0800, Robert Hoo wrote:
> On Mon, 2020-09-28 at 21:56 -0700, Sean Christopherson wrote:
> > > +
> > > +	best = kvm_find_cpuid_entry(vcpu, 1, 0);
> > > +
> > > +	if (best && boot_cpu_has(X86_FEATURE_XSAVE)) {
> > 
> > Braces not needed. We should also check boot_cpu_has() first, it's constant
> > time and maybe even in the cache, whereas finding CPUID entries is linear
> > time and outright slow.
> > 
> > Actually, can you add a helper to handle this?  With tht boot_cpu_has()
> > check outside of the helper?  That'll allow the helper to be used for more
> > features, and will force checking boot_cpu_has() first.  Hmm, and not all
> > of the callers will need the boot_cpu_has() check, e.g. toggling PKE from
> > kvm_set_cr4() doesn't need to be guarded because KVM disallows setting PKE
> > if it's not supported by the host.
> 
> Do you mean because in kvm_set_cr4(), it has kvm_valid_cr4(vcpu, cr4)
> check first?

Yes.

> Then how about the other 2 callers of kvm_pke_update_cpuid()?
> enter_smm() -- I think it can ommit boot_cpu_has() check as well.
> because it unconditionally cleared all CR4 bit before calls
> kvm_set_cr4().
> __set_sregs() -- looks like it doesn't valid host PKE status before
> call kvm_pke_update_cpuid(). Can I ommit boot_cpu_has() as well?
> 
> So, I don't think kvm_pke_update_cpuid() can leverage the helper. Am I
> right?

It can leverage the guest_cpuid_change() helper below, no?
 
> > static inline void guest_cpuid_change(struct kvm_vcpu *vcpu, u32
> > function,
> > 				      u32 index, unsigned int feature,
> > bool set)
> > {
> > 	struct kvm_cpuid_entry2 *e =  kvm_find_cpuid_entry(vcpu,
> > function, index);
> > 
> > 	if (e)
> > 		cpuid_entry_change(best, X86_FEATURE_OSXSAVE, set);
> > }
> 
> Thanks Sean, I'm going to have this helper in v2 and have your signed-
> off-by.

Eh, no need to give me credit, it's just a snippet in feedback.  It's
not even correct :-)  E.g. s/X86_FEATURE_OSXSAVE/feature below.

> > 
> > > +		cpuid_entry_change(best, X86_FEATURE_OSXSAVE, set);
> > > +	}
> > > +}

Re: [RFC PATCH 1/9] KVM:x86: Abstract sub functions from kvm_update_cpuid_runtime() and kvm_vcpu_after_set_cpuid()

[Sean Christopherson]

On Fri, Jul 31, 2020 at 10:42:19AM +0800, Robert Hoo wrote:
> Add below functions, whose aggregation equals kvm_update_cpuid_runtime() and
> kvm_vcpu_after_set_cpuid().
> 
> void kvm_osxsave_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> void kvm_pke_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> void kvm_apic_base_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> void kvm_mwait_update_cpuid(struct kvm_vcpu *vcpu, bool set)
> void kvm_xcr0_update_cpuid(struct kvm_vcpu *vcpu)
> static void kvm_pv_unhalt_update_cpuid(struct kvm_vcpu *vcpu)
> static void kvm_update_maxphyaddr(struct kvm_vcpu *vcpu)
> static void kvm_update_lapic_timer_mode(struct kvm_vcpu *vcpu)

Ugh, I just reread this, you're adding functions with no callers.  This patch
should replace the existing code so that there are callers, and more
importantly so that we can verify old==new.

[RFC PATCH 2/9] KVM:x86: Substitute kvm_update_cpuid_runtime() with kvm_apic_base_update_cpuid() in kvm_lapic_set_base()

[Robert Hoo]

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 arch/x86/kvm/lapic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index d5fb2ea..1e3d03c 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2231,7 +2231,7 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
 	vcpu->arch.apic_base = value;
 
 	if ((old_value ^ value) & MSR_IA32_APICBASE_ENABLE)
-		kvm_update_cpuid_runtime(vcpu);
+		kvm_apic_base_update_cpuid(vcpu, !!(value & MSR_IA32_APICBASE_ENABLE));
 
 	if (!apic)
 		return;
-- 
1.8.3.1

[RFC PATCH 3/9] KVM:x86: Substitute kvm_update_cpuid_runtime() with kvm_xcr0_update_cpuid() in __kvm_set_xcr()

[Robert Hoo]

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 arch/x86/kvm/x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 95ef629..263ba47 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -943,7 +943,7 @@ static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
 	vcpu->arch.xcr0 = xcr0;
 
 	if ((xcr0 ^ old_xcr0) & XFEATURE_MASK_EXTEND)
-		kvm_update_cpuid_runtime(vcpu);
+		kvm_xcr0_update_cpuid(vcpu);
 	return 0;
 }
 
-- 
1.8.3.1

[RFC PATCH 4/9] KVM:x86: Substitute kvm_update_cpuid_runtime() with kvm_{osxsave,pke}_update_cpuid() in kvm_set_cr4()

[Robert Hoo]

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 arch/x86/kvm/x86.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 263ba47..e31dba2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1006,8 +1006,10 @@ int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 	    (!(cr4 & X86_CR4_PCIDE) && (old_cr4 & X86_CR4_PCIDE)))
 		kvm_mmu_reset_context(vcpu);
 
-	if ((cr4 ^ old_cr4) & (X86_CR4_OSXSAVE | X86_CR4_PKE))
-		kvm_update_cpuid_runtime(vcpu);
+	if ((cr4 ^ old_cr4) & X86_CR4_OSXSAVE)
+		kvm_osxsave_update_cpuid(vcpu, !!(cr4 & X86_CR4_OSXSAVE));
+	if ((cr4 ^ old_cr4) & X86_CR4_PKE)
+		kvm_pke_update_cpuid(vcpu, !!(cr4 & X86_CR4_PKE));
 
 	return 0;
 }
-- 
1.8.3.1

[RFC PATCH 5/9] KVM:x86: Substitute kvm_update_cpuid_runtime() with kvm_mwait_update_cpuid() in kvm_set_msr_common()

[Robert Hoo]

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 arch/x86/kvm/x86.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e31dba2..d269670 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2934,8 +2934,9 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		    ((vcpu->arch.ia32_misc_enable_msr ^ data) & MSR_IA32_MISC_ENABLE_MWAIT)) {
 			if (!guest_cpuid_has(vcpu, X86_FEATURE_XMM3))
 				return 1;
+			kvm_mwait_update_cpuid(vcpu, !!(data & MSR_IA32_MISC_ENABLE_MWAIT));
+
 			vcpu->arch.ia32_misc_enable_msr = data;
-			kvm_update_cpuid_runtime(vcpu);
 		} else {
 			vcpu->arch.ia32_misc_enable_msr = data;
 		}
-- 
1.8.3.1

[RFC PATCH 6/9] KVM:x86: Substitute kvm_update_cpuid_runtime() with kvm_{osxsave,pke}_update_cpuid() in enter_smm()

[Robert Hoo]

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 arch/x86/kvm/x86.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d269670..13a2915 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8162,6 +8162,8 @@ static void enter_smm(struct kvm_vcpu *vcpu)
 	vcpu->arch.cr0 = cr0;
 
 	kvm_x86_ops.set_cr4(vcpu, 0);
+	kvm_osxsave_update_cpuid(vcpu, false);
+	kvm_pke_update_cpuid(vcpu, false);
 
 	/* Undocumented: IDT limit is set to zero on entry to SMM.  */
 	dt.address = dt.size = 0;
@@ -8199,7 +8201,6 @@ static void enter_smm(struct kvm_vcpu *vcpu)
 		kvm_x86_ops.set_efer(vcpu, 0);
 #endif
 
-	kvm_update_cpuid_runtime(vcpu);
 	kvm_mmu_reset_context(vcpu);
 }
 
-- 
1.8.3.1

[RFC PATCH 7/9] KVM:x86: Substitute kvm_update_cpuid_runtime() with kvm_{osxsave,pke}_update_cpuid() in __set_sregs()

[Robert Hoo]

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 arch/x86/kvm/x86.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 13a2915..35cb3d9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9183,7 +9183,7 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
 {
 	struct msr_data apic_base_msr;
 	int mmu_reset_needed = 0;
-	int cpuid_update_needed = 0;
+	ulong old_cr4 = 0;
 	int pending_vec, max_bits, idx;
 	struct desc_ptr dt;
 	int ret = -EINVAL;
@@ -9217,12 +9217,15 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
 	kvm_x86_ops.set_cr0(vcpu, sregs->cr0);
 	vcpu->arch.cr0 = sregs->cr0;
 
-	mmu_reset_needed |= kvm_read_cr4(vcpu) != sregs->cr4;
-	cpuid_update_needed |= ((kvm_read_cr4(vcpu) ^ sregs->cr4) &
-				(X86_CR4_OSXSAVE | X86_CR4_PKE));
+	old_cr4 = kvm_read_cr4(vcpu);
+	mmu_reset_needed |= old_cr4 != sregs->cr4;
+
 	kvm_x86_ops.set_cr4(vcpu, sregs->cr4);
-	if (cpuid_update_needed)
-		kvm_update_cpuid_runtime(vcpu);
+
+	if ((old_cr4 ^ sregs->cr4) & X86_CR4_OSXSAVE)
+		kvm_osxsave_update_cpuid(vcpu, !!(sregs->cr4 & X86_CR4_OSXSAVE));
+	if ((old_cr4 ^ sregs->cr4) & X86_CR4_PKE)
+		kvm_pke_update_cpuid(vcpu, !!(sregs->cr4 & X86_CR4_PKE));
 
 	idx = srcu_read_lock(&vcpu->kvm->srcu);
 	if (is_pae_paging(vcpu)) {
-- 
1.8.3.1

[RFC PATCH 8/9] KVM:x86: Substitute kvm_vcpu_after_set_cpuid() with abstracted functions

[Robert Hoo]

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 arch/x86/kvm/cpuid.c | 26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index efa7182..1d206aa 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -217,32 +217,16 @@ void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
 
 static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 {
-	struct kvm_lapic *apic = vcpu->arch.apic;
-	struct kvm_cpuid_entry2 *best;
-
 	kvm_x86_ops.vcpu_after_set_cpuid(vcpu);
 
-	best = kvm_find_cpuid_entry(vcpu, 1, 0);
-	if (best && apic) {
-		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
-			apic->lapic_timer.timer_mode_mask = 3 << 17;
-		else
-			apic->lapic_timer.timer_mode_mask = 1 << 17;
+	kvm_update_lapic_timer_mode(vcpu);
+	kvm_apic_set_version(vcpu);
 
-		kvm_apic_set_version(vcpu);
-	}
+	kvm_xcr0_update_cpuid(vcpu);
 
-	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
-	if (!best)
-		vcpu->arch.guest_supported_xcr0 = 0;
-	else
-		vcpu->arch.guest_supported_xcr0 =
-			(best->eax | ((u64)best->edx << 32)) & supported_xcr0;
+	kvm_update_maxphyaddr(vcpu);
 
-	/* Note, maxphyaddr must be updated before tdp_level. */
-	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
-	vcpu->arch.tdp_level = kvm_x86_ops.get_tdp_level(vcpu);
-	kvm_mmu_reset_context(vcpu);
+	kvm_pv_unhalt_update_cpuid(vcpu);
 
 	kvm_pmu_refresh(vcpu);
 	vcpu->arch.cr4_guest_rsvd_bits =
-- 
1.8.3.1

[RFC PATCH 9/9] KVM:x86: Remove kvm_update_cpuid_runtime()

[Robert Hoo]

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 arch/x86/kvm/cpuid.c | 45 ---------------------------------------------
 arch/x86/kvm/cpuid.h |  1 -
 2 files changed, 46 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 1d206aa..d7f5a6d 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -172,49 +172,6 @@ static void kvm_update_lapic_timer_mode(struct kvm_vcpu *vcpu)
 	}
 }
 
-void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
-{
-	struct kvm_cpuid_entry2 *best;
-
-	best = kvm_find_cpuid_entry(vcpu, 1, 0);
-	if (best) {
-		/* Update OSXSAVE bit */
-		if (boot_cpu_has(X86_FEATURE_XSAVE))
-			cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
-				   kvm_read_cr4_bits(vcpu, X86_CR4_OSXSAVE));
-
-		cpuid_entry_change(best, X86_FEATURE_APIC,
-			   vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
-	}
-
-	best = kvm_find_cpuid_entry(vcpu, 7, 0);
-	if (best && boot_cpu_has(X86_FEATURE_PKU) && best->function == 0x7)
-		cpuid_entry_change(best, X86_FEATURE_OSPKE,
-				   kvm_read_cr4_bits(vcpu, X86_CR4_PKE));
-
-	best = kvm_find_cpuid_entry(vcpu, 0xD, 0);
-	if (best)
-		best->ebx = xstate_required_size(vcpu->arch.xcr0, false);
-
-	best = kvm_find_cpuid_entry(vcpu, 0xD, 1);
-	if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
-		     cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
-		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
-
-	best = kvm_find_cpuid_entry(vcpu, KVM_CPUID_FEATURES, 0);
-	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 = kvm_find_cpuid_entry(vcpu, 0x1, 0);
-		if (best)
-			cpuid_entry_change(best, X86_FEATURE_MWAIT,
-					   vcpu->arch.ia32_misc_enable_msr &
-					   MSR_IA32_MISC_ENABLE_MWAIT);
-	}
-}
-
 static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 {
 	kvm_x86_ops.vcpu_after_set_cpuid(vcpu);
@@ -314,7 +271,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 	}
 
 	cpuid_fix_nx_cap(vcpu);
-	kvm_update_cpuid_runtime(vcpu);
 	kvm_vcpu_after_set_cpuid(vcpu);
 
 	kvfree(cpuid_entries);
@@ -342,7 +298,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
 		goto out;
 	}
 
-	kvm_update_cpuid_runtime(vcpu);
 	kvm_vcpu_after_set_cpuid(vcpu);
 out:
 	return r;
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 7eabb44..7ef46bc 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -15,7 +15,6 @@
 void kvm_mwait_update_cpuid(struct kvm_vcpu *vcpu, bool set);
 void kvm_xcr0_update_cpuid(struct kvm_vcpu *vcpu);
 
-void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu);
 struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
 					      u32 function, u32 index);
 int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
-- 
1.8.3.1