[PATCH 0/7] KVM: x86: Clean up MSR_IA32_APICBASE_BASE code

[PATCH 0/7] KVM: x86: Clean up MSR_IA32_APICBASE_BASE code

[Sean Christopherson]

Clean up code related to setting and getting MSR_IA32_APICBASE_BASE.

E.g. it's absurdly difficult to tease out that kvm_set_apic_base() exists
purely to avoid an extra call to kvm_recalculate_apic_map() (which may or
may not be worth the code, but whatever).

Simiarly, it's quite difficult to see that kvm_lapic_set_base() doesn't
do anything useful if the incoming MSR value is the same as the current
value.

Sean Christopherson (7):
  KVM: x86: Short-circuit all kvm_lapic_set_base() if MSR value isn't
    changing
  KVM: x86: Drop superfluous kvm_lapic_set_base() call when setting APIC
    state
  KVM: x86: Get vcpu->arch.apic_base directly and drop
    kvm_get_apic_base()
  KVM: x86: Inline kvm_get_apic_mode() in lapic.h
  KVM: x86: Move kvm_set_apic_base() implementation to lapic.c (from
    x86.c)
  KVM: x86: Rename APIC base setters to better capture their
    relationship
  KVM: x86: Make kvm_recalculate_apic_map() local to lapic.c

 arch/x86/kvm/lapic.c | 31 +++++++++++++++++++++++++++----
 arch/x86/kvm/lapic.h | 11 ++++++-----
 arch/x86/kvm/x86.c   | 42 +++++-------------------------------------
 3 files changed, 38 insertions(+), 46 deletions(-)


base-commit: 8cf0b93919e13d1e8d4466eb4080a4c4d9d66d7b
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 1/7] KVM: x86: Short-circuit all kvm_lapic_set_base() if MSR value isn't changing

[Sean Christopherson]

Do nothing in kvm_lapic_set_base() if the APIC base MSR value is the same
as the current value.  All flows except the handling of the base address
explicitly take effect if and only if relevant bits are changing.

For the base address, invoking kvm_lapic_set_base() before KVM initializes
the base to APIC_DEFAULT_PHYS_BASE during vCPU RESET would be a KVM bug,
i.e. KVM _must_ initialize apic->base_address before exposing the vCPU (to
userspace or KVM at-large).

Note, the inhibit is intended to be set if the base address is _changed_
from the default, i.e. is also covered by the RESET behavior.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 2098dc689088..ffccd6e7e5c1 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2582,6 +2582,9 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
 	u64 old_value = vcpu->arch.apic_base;
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
+	if (old_value == value)
+		return;
+
 	vcpu->arch.apic_base = value;
 
 	if ((old_value ^ value) & MSR_IA32_APICBASE_ENABLE)
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 2/7] KVM: x86: Drop superfluous kvm_lapic_set_base() call when setting APIC state

[Sean Christopherson]

Now that kvm_lapic_set_base() does nothing if the "new" APIC base MSR is
the same as the current value, drop the kvm_lapic_set_base() call in the
KVM_SET_LAPIC flow that passes in the current value, as it too does
nothing.

Note, the purpose of invoking kvm_lapic_set_base() was purely to set
apic->base_address (see commit 5dbc8f3fed0b ("KVM: use kvm_lapic_set_base()
to change apic_base")).  And there is no evidence that explicitly setting
apic->base_address in KVM_SET_LAPIC ever had any functional impact; even
in the original commit 96ad2cc61324 ("KVM: in-kernel LAPIC save and restore
support"), all flows that set apic_base also set apic->base_address to the
same address.  E.g. svm_create_vcpu() did open code a write to apic_base,

	svm->vcpu.apic_base = 0xfee00000 | MSR_IA32_APICBASE_ENABLE;

but it also called kvm_create_lapic() when irqchip_in_kernel() is true.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index ffccd6e7e5c1..fe30f465611f 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -3072,7 +3072,6 @@ int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s)
 
 	kvm_x86_call(apicv_pre_state_restore)(vcpu);
 
-	kvm_lapic_set_base(vcpu, vcpu->arch.apic_base);
 	/* set SPIV separately to get count of SW disabled APICs right */
 	apic_set_spiv(apic, *((u32 *)(s->regs + APIC_SPIV)));
 
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 3/7] KVM: x86: Get vcpu->arch.apic_base directly and drop kvm_get_apic_base()

[Sean Christopherson]

Access KVM's emulated APIC base MSR value directly instead of bouncing
through a helper, as there is no reason to add a layer of indirection, and
there are other MSRs with a "set" but no "get", e.g. EFER.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.h |  1 -
 arch/x86/kvm/x86.c   | 13 ++++---------
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 1b8ef9856422..441abc4f4afd 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -117,7 +117,6 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 		struct kvm_lapic_irq *irq, int *r, struct dest_map *dest_map);
 void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high);
 
-u64 kvm_get_apic_base(struct kvm_vcpu *vcpu);
 int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
 int kvm_apic_get_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
 int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 83fe0a78146f..046cb7513436 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -667,14 +667,9 @@ static void drop_user_return_notifiers(void)
 		kvm_on_user_return(&msrs->urn);
 }
 
-u64 kvm_get_apic_base(struct kvm_vcpu *vcpu)
-{
-	return vcpu->arch.apic_base;
-}
-
 enum lapic_mode kvm_get_apic_mode(struct kvm_vcpu *vcpu)
 {
-	return kvm_apic_mode(kvm_get_apic_base(vcpu));
+	return kvm_apic_mode(vcpu->arch.apic_base);
 }
 EXPORT_SYMBOL_GPL(kvm_get_apic_mode);
 
@@ -4314,7 +4309,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		msr_info->data = 1 << 24;
 		break;
 	case MSR_IA32_APICBASE:
-		msr_info->data = kvm_get_apic_base(vcpu);
+		msr_info->data = vcpu->arch.apic_base;
 		break;
 	case APIC_BASE_MSR ... APIC_BASE_MSR + 0xff:
 		return kvm_x2apic_msr_read(vcpu, msr_info->index, &msr_info->data);
@@ -10159,7 +10154,7 @@ static void post_kvm_run_save(struct kvm_vcpu *vcpu)
 
 	kvm_run->if_flag = kvm_x86_call(get_if_flag)(vcpu);
 	kvm_run->cr8 = kvm_get_cr8(vcpu);
-	kvm_run->apic_base = kvm_get_apic_base(vcpu);
+	kvm_run->apic_base = vcpu->arch.apic_base;
 
 	kvm_run->ready_for_interrupt_injection =
 		pic_in_kernel(vcpu->kvm) ||
@@ -11711,7 +11706,7 @@ static void __get_sregs_common(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
 	sregs->cr4 = kvm_read_cr4(vcpu);
 	sregs->cr8 = kvm_get_cr8(vcpu);
 	sregs->efer = vcpu->arch.efer;
-	sregs->apic_base = kvm_get_apic_base(vcpu);
+	sregs->apic_base = vcpu->arch.apic_base;
 }
 
 static void __get_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 4/7] KVM: x86: Inline kvm_get_apic_mode() in lapic.h

[Sean Christopherson]

Inline kvm_get_apic_mode() in lapic.h to avoid a CALL+RET as well as an
export.  The underlying kvm_apic_mode() helper is public information, i.e.
there is no state/information that needs to be hidden from vendor modules.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.h | 6 +++++-
 arch/x86/kvm/x86.c   | 6 ------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 441abc4f4afd..fc4bd36d44cf 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -120,7 +120,6 @@ void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high);
 int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
 int kvm_apic_get_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
 int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
-enum lapic_mode kvm_get_apic_mode(struct kvm_vcpu *vcpu);
 int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu);
 
 u64 kvm_get_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu);
@@ -270,6 +269,11 @@ static inline enum lapic_mode kvm_apic_mode(u64 apic_base)
 	return apic_base & (MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE);
 }
 
+static inline enum lapic_mode kvm_get_apic_mode(struct kvm_vcpu *vcpu)
+{
+	return kvm_apic_mode(vcpu->arch.apic_base);
+}
+
 static inline u8 kvm_xapic_id(struct kvm_lapic *apic)
 {
 	return kvm_lapic_get_reg(apic, APIC_ID) >> 24;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 046cb7513436..c70ee3b33b93 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -667,12 +667,6 @@ static void drop_user_return_notifiers(void)
 		kvm_on_user_return(&msrs->urn);
 }
 
-enum lapic_mode kvm_get_apic_mode(struct kvm_vcpu *vcpu)
-{
-	return kvm_apic_mode(vcpu->arch.apic_base);
-}
-EXPORT_SYMBOL_GPL(kvm_get_apic_mode);
-
 int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
 	enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 5/7] KVM: x86: Move kvm_set_apic_base() implementation to lapic.c (from x86.c)

[Sean Christopherson]

Move kvm_set_apic_base() to lapic.c so that the bulk of KVM's local APIC
code resides in lapic.c, regardless of whether or not KVM is emulating the
local APIC in-kernel.  This will also allow making various helpers visible
only to lapic.c.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c | 21 +++++++++++++++++++++
 arch/x86/kvm/x86.c   | 21 ---------------------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index fe30f465611f..6239cfd89aad 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2628,6 +2628,27 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
 	}
 }
 
+int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
+{
+	enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
+	enum lapic_mode new_mode = kvm_apic_mode(msr_info->data);
+	u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff |
+		(guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);
+
+	if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID)
+		return 1;
+	if (!msr_info->host_initiated) {
+		if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC)
+			return 1;
+		if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC)
+			return 1;
+	}
+
+	kvm_lapic_set_base(vcpu, msr_info->data);
+	kvm_recalculate_apic_map(vcpu->kvm);
+	return 0;
+}
+
 void kvm_apic_update_apicv(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c70ee3b33b93..a2a2a6126e67 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -667,27 +667,6 @@ static void drop_user_return_notifiers(void)
 		kvm_on_user_return(&msrs->urn);
 }
 
-int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
-{
-	enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
-	enum lapic_mode new_mode = kvm_apic_mode(msr_info->data);
-	u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff |
-		(guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);
-
-	if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID)
-		return 1;
-	if (!msr_info->host_initiated) {
-		if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC)
-			return 1;
-		if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC)
-			return 1;
-	}
-
-	kvm_lapic_set_base(vcpu, msr_info->data);
-	kvm_recalculate_apic_map(vcpu->kvm);
-	return 0;
-}
-
 /*
  * Handle a fault on a hardware virtualization (VMX or SVM) instruction.
  *
-- 
2.47.0.rc1.288.g06298d1525-goog

Re: [PATCH 5/7] KVM: x86: Move kvm_set_apic_base() implementation to lapic.c (from x86.c)

[Huang, Kai]

On Wed, 2024-10-09 at 11:17 -0700, Sean Christopherson wrote:
> Move kvm_set_apic_base() to lapic.c so that the bulk of KVM's local APIC
> code resides in lapic.c, regardless of whether or not KVM is emulating the
> local APIC in-kernel.  This will also allow making various helpers visible
> only to lapic.c.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/lapic.c | 21 +++++++++++++++++++++
>  arch/x86/kvm/x86.c   | 21 ---------------------
>  2 files changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index fe30f465611f..6239cfd89aad 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2628,6 +2628,27 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
>  	}
>  }
>  
> +int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> +{
> +	enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
> +	enum lapic_mode new_mode = kvm_apic_mode(msr_info->data);
> +	u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff |
> +		(guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);
> +
> +	if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID)
> +		return 1;
> +	if (!msr_info->host_initiated) {
> +		if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC)
> +			return 1;
> +		if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC)
> +			return 1;
> +	}
> +
> +	kvm_lapic_set_base(vcpu, msr_info->data);
> +	kvm_recalculate_apic_map(vcpu->kvm);
> +	return 0;
> +}

Nit:

It is a little bit weird to use 'struct msr_data *msr_info' as function
parameter if kvm_set_apic_base() is in lapic.c.  Maybe we can change to take
apic_base and host_initialized directly.

A side gain is we can get rid of using the 'struct msr_data apic_base_msr' local
variable in __set_sregs_common() when calling kvm_apic_set_base():

static int __set_sregs_common(...)
{
        struct msr_data apic_base_msr;
	...

        apic_base_msr.data = sregs->apic_base;
        apic_base_msr.host_initiated = true;
        if (kvm_set_apic_base(vcpu, &apic_base_msr))
                return -EINVAL;
	...
}

Re: [PATCH 5/7] KVM: x86: Move kvm_set_apic_base() implementation to lapic.c (from x86.c)

[Sean Christopherson]

On Mon, Oct 14, 2024, Kai Huang wrote:
> On Wed, 2024-10-09 at 11:17 -0700, Sean Christopherson wrote:
> > Move kvm_set_apic_base() to lapic.c so that the bulk of KVM's local APIC
> > code resides in lapic.c, regardless of whether or not KVM is emulating the
> > local APIC in-kernel.  This will also allow making various helpers visible
> > only to lapic.c.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  arch/x86/kvm/lapic.c | 21 +++++++++++++++++++++
> >  arch/x86/kvm/x86.c   | 21 ---------------------
> >  2 files changed, 21 insertions(+), 21 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index fe30f465611f..6239cfd89aad 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -2628,6 +2628,27 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
> >  	}
> >  }
> >  
> > +int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > +{
> > +	enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
> > +	enum lapic_mode new_mode = kvm_apic_mode(msr_info->data);
> > +	u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff |
> > +		(guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);
> > +
> > +	if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID)
> > +		return 1;
> > +	if (!msr_info->host_initiated) {
> > +		if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC)
> > +			return 1;
> > +		if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC)
> > +			return 1;
> > +	}
> > +
> > +	kvm_lapic_set_base(vcpu, msr_info->data);
> > +	kvm_recalculate_apic_map(vcpu->kvm);
> > +	return 0;
> > +}
> 
> Nit:
> 
> It is a little bit weird to use 'struct msr_data *msr_info' as function
> parameter if kvm_set_apic_base() is in lapic.c.  Maybe we can change to take
> apic_base and host_initialized directly.
> 
> A side gain is we can get rid of using the 'struct msr_data apic_base_msr' local
> variable in __set_sregs_common() when calling kvm_apic_set_base():

Ooh, nice.  I agree, it'd be better to pass in separate parameters.

Gah, and looking at this with fresh eyes reminded me why I even started poking at
this code in the first place.  Patch 1's changelog does a poor job of calling it
out, but the main impetus for this series was to avoid kvm_recalculate_apic_map()
when doing KVM_SET_SREGS without modifying APIC_BASE.  That's _mostly_ handled by
patch 1, but it doesn't completely fix things because if the map is already DIRTY,
then KVM will unnecessarily fall into kvm_recalculate_apic_map()'s slow path, even
though some other vCPU/task is responsible for refreshing the calculation.

I'll send a v2 with your suggested change, a better changelog for patch 1, and
another patch at the end to short-circuit kvm_apic_set_base() (not just the inner
helper) if the new value is the same as the old value.

Thanks Kai!

> static int __set_sregs_common(...)
> {
>         struct msr_data apic_base_msr;
> 	...
> 
>         apic_base_msr.data = sregs->apic_base;
>         apic_base_msr.host_initiated = true;
>         if (kvm_set_apic_base(vcpu, &apic_base_msr))
>                 return -EINVAL;
> 	...
> }
> 

Re: [PATCH 5/7] KVM: x86: Move kvm_set_apic_base() implementation to lapic.c (from x86.c)

[Sean Christopherson]

On Mon, Oct 14, 2024, Sean Christopherson wrote:
> On Mon, Oct 14, 2024, Kai Huang wrote:
> > On Wed, 2024-10-09 at 11:17 -0700, Sean Christopherson wrote:
> > > Move kvm_set_apic_base() to lapic.c so that the bulk of KVM's local APIC
> > > code resides in lapic.c, regardless of whether or not KVM is emulating the
> > > local APIC in-kernel.  This will also allow making various helpers visible
> > > only to lapic.c.
> > > 
> > > No functional change intended.
> > > 
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > ---
> > >  arch/x86/kvm/lapic.c | 21 +++++++++++++++++++++
> > >  arch/x86/kvm/x86.c   | 21 ---------------------
> > >  2 files changed, 21 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index fe30f465611f..6239cfd89aad 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -2628,6 +2628,27 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
> > >  	}
> > >  }
> > >  
> > > +int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > > +{
> > > +	enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
> > > +	enum lapic_mode new_mode = kvm_apic_mode(msr_info->data);
> > > +	u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff |
> > > +		(guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);
> > > +
> > > +	if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID)
> > > +		return 1;
> > > +	if (!msr_info->host_initiated) {
> > > +		if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC)
> > > +			return 1;
> > > +		if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC)
> > > +			return 1;
> > > +	}
> > > +
> > > +	kvm_lapic_set_base(vcpu, msr_info->data);
> > > +	kvm_recalculate_apic_map(vcpu->kvm);
> > > +	return 0;
> > > +}
> > 
> > Nit:
> > 
> > It is a little bit weird to use 'struct msr_data *msr_info' as function
> > parameter if kvm_set_apic_base() is in lapic.c.  Maybe we can change to take
> > apic_base and host_initialized directly.
> > 
> > A side gain is we can get rid of using the 'struct msr_data apic_base_msr' local
> > variable in __set_sregs_common() when calling kvm_apic_set_base():
> 
> Ooh, nice.  I agree, it'd be better to pass in separate parameters.
> 
> Gah, and looking at this with fresh eyes reminded me why I even started poking at
> this code in the first place.  Patch 1's changelog does a poor job of calling it
> out,

Duh, because patch 1 doesn't change any of that.  KVM already skips setting the
map DIRTY if neither MSR_IA32_APICBASE_ENABLE nor X2APIC_ENABLE is toggled.  So
it's really just the (IIRC, rare) collision with an already-dirty map that's nice
to avoid.

> but the main impetus for this series was to avoid kvm_recalculate_apic_map()
> when doing KVM_SET_SREGS without modifying APIC_BASE.  That's _mostly_ handled by
> patch 1, but it doesn't completely fix things because if the map is already DIRTY,
> then KVM will unnecessarily fall into kvm_recalculate_apic_map()'s slow path, even
> though some other vCPU/task is responsible for refreshing the calculation.
> 
> I'll send a v2 with your suggested change, a better changelog for patch 1, and
> another patch at the end to short-circuit kvm_apic_set_base() (not just the inner
> helper) if the new value is the same as the old value.
> 
> Thanks Kai!
> 
> > static int __set_sregs_common(...)
> > {
> >         struct msr_data apic_base_msr;
> > 	...
> > 
> >         apic_base_msr.data = sregs->apic_base;
> >         apic_base_msr.host_initiated = true;
> >         if (kvm_set_apic_base(vcpu, &apic_base_msr))
> >                 return -EINVAL;
> > 	...
> > }
> > 

Re: [PATCH 5/7] KVM: x86: Move kvm_set_apic_base() implementation to lapic.c (from x86.c)

[Huang, Kai]

On 15/10/2024 8:40 am, Sean Christopherson wrote:
> On Mon, Oct 14, 2024, Sean Christopherson wrote:
>> On Mon, Oct 14, 2024, Kai Huang wrote:
>>> On Wed, 2024-10-09 at 11:17 -0700, Sean Christopherson wrote:
>>>> Move kvm_set_apic_base() to lapic.c so that the bulk of KVM's local APIC
>>>> code resides in lapic.c, regardless of whether or not KVM is emulating the
>>>> local APIC in-kernel.  This will also allow making various helpers visible
>>>> only to lapic.c.
>>>>
>>>> No functional change intended.
>>>>
>>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>>> ---
>>>>   arch/x86/kvm/lapic.c | 21 +++++++++++++++++++++
>>>>   arch/x86/kvm/x86.c   | 21 ---------------------
>>>>   2 files changed, 21 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>>> index fe30f465611f..6239cfd89aad 100644
>>>> --- a/arch/x86/kvm/lapic.c
>>>> +++ b/arch/x86/kvm/lapic.c
>>>> @@ -2628,6 +2628,27 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
>>>>   	}
>>>>   }
>>>>   
>>>> +int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>>> +{
>>>> +	enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
>>>> +	enum lapic_mode new_mode = kvm_apic_mode(msr_info->data);
>>>> +	u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff |
>>>> +		(guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE);
>>>> +
>>>> +	if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID)
>>>> +		return 1;
>>>> +	if (!msr_info->host_initiated) {
>>>> +		if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC)
>>>> +			return 1;
>>>> +		if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC)
>>>> +			return 1;
>>>> +	}
>>>> +
>>>> +	kvm_lapic_set_base(vcpu, msr_info->data);
>>>> +	kvm_recalculate_apic_map(vcpu->kvm);
>>>> +	return 0;
>>>> +}
>>>
>>> Nit:
>>>
>>> It is a little bit weird to use 'struct msr_data *msr_info' as function
>>> parameter if kvm_set_apic_base() is in lapic.c.  Maybe we can change to take
>>> apic_base and host_initialized directly.
>>>
>>> A side gain is we can get rid of using the 'struct msr_data apic_base_msr' local
>>> variable in __set_sregs_common() when calling kvm_apic_set_base():
>>
>> Ooh, nice.  I agree, it'd be better to pass in separate parameters.
>>
>> Gah, and looking at this with fresh eyes reminded me why I even started poking at
>> this code in the first place.  Patch 1's changelog does a poor job of calling it
>> out,
> 
> Duh, because patch 1 doesn't change any of that.  KVM already skips setting the
> map DIRTY if neither MSR_IA32_APICBASE_ENABLE nor X2APIC_ENABLE is toggled.  So
> it's really just the (IIRC, rare) collision with an already-dirty map that's nice
> to avoid.
> 

I think if the map is already dirty, the other thread that makes the map 
dirty must be responsible for calling kvm_recalculate_apic_map() after that.

For updating 'apic_base' path, IIUC it makes sense anyway to avoid 
everything when 'apic_base' doesn't change.

Calling kvm_recalculate_apic_map() when 'apic_base' is not changed has 
no harm, but logically I don't think there's need to do that, even the 
map is already dirty.

[PATCH 6/7] KVM: x86: Rename APIC base setters to better capture their relationship

[Sean Christopherson]

Rename kvm_set_apic_base() and kvm_lapic_set_base() to kvm_apic_set_base()
and __kvm_apic_set_base() respectively to capture that the underscores
version is a "special" variant (it exists purely to avoid recalculating
the optimized map multiple times when stuffing the RESET value).

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c | 8 ++++----
 arch/x86/kvm/lapic.h | 3 +--
 arch/x86/kvm/x86.c   | 4 ++--
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 6239cfd89aad..0a73d9a09fe0 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2577,7 +2577,7 @@ u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu)
 	return (tpr & 0xf0) >> 4;
 }
 
-void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
+static void __kvm_apic_set_base(struct kvm_vcpu *vcpu, u64 value)
 {
 	u64 old_value = vcpu->arch.apic_base;
 	struct kvm_lapic *apic = vcpu->arch.apic;
@@ -2628,7 +2628,7 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
 	}
 }
 
-int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
+int kvm_apic_set_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
 	enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
 	enum lapic_mode new_mode = kvm_apic_mode(msr_info->data);
@@ -2644,7 +2644,7 @@ int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			return 1;
 	}
 
-	kvm_lapic_set_base(vcpu, msr_info->data);
+	__kvm_apic_set_base(vcpu, msr_info->data);
 	kvm_recalculate_apic_map(vcpu->kvm);
 	return 0;
 }
@@ -2752,7 +2752,7 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
 		msr_val = APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE;
 		if (kvm_vcpu_is_reset_bsp(vcpu))
 			msr_val |= MSR_IA32_APICBASE_BSP;
-		kvm_lapic_set_base(vcpu, msr_val);
+		__kvm_apic_set_base(vcpu, msr_val);
 	}
 
 	if (!apic)
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index fc4bd36d44cf..0dd5055852ad 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -95,7 +95,6 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);
 void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu);
-void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);
 void kvm_recalculate_apic_map(struct kvm *kvm);
 void kvm_apic_set_version(struct kvm_vcpu *vcpu);
 void kvm_apic_after_set_mcg_cap(struct kvm_vcpu *vcpu);
@@ -117,7 +116,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 		struct kvm_lapic_irq *irq, int *r, struct dest_map *dest_map);
 void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high);
 
-int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
+int kvm_apic_set_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
 int kvm_apic_get_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
 int kvm_apic_set_state(struct kvm_vcpu *vcpu, struct kvm_lapic_state *s);
 int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a2a2a6126e67..803db3e9ab8a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3858,7 +3858,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_MTRRdefType:
 		return kvm_mtrr_set_msr(vcpu, msr, data);
 	case MSR_IA32_APICBASE:
-		return kvm_set_apic_base(vcpu, msr_info);
+		return kvm_apic_set_base(vcpu, msr_info);
 	case APIC_BASE_MSR ... APIC_BASE_MSR + 0xff:
 		return kvm_x2apic_msr_write(vcpu, msr, data);
 	case MSR_IA32_TSC_DEADLINE:
@@ -11865,7 +11865,7 @@ static int __set_sregs_common(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs,
 
 	apic_base_msr.data = sregs->apic_base;
 	apic_base_msr.host_initiated = true;
-	if (kvm_set_apic_base(vcpu, &apic_base_msr))
+	if (kvm_apic_set_base(vcpu, &apic_base_msr))
 		return -EINVAL;
 
 	if (vcpu->arch.guest_state_protected)
-- 
2.47.0.rc1.288.g06298d1525-goog

Re: [PATCH 6/7] KVM: x86: Rename APIC base setters to better capture their relationship

[Paolo Bonzini]

On 10/9/24 20:17, Sean Christopherson wrote:
> Rename kvm_set_apic_base() and kvm_lapic_set_base() to kvm_apic_set_base()
> and __kvm_apic_set_base() respectively to capture that the underscores
> version is a "special" variant (it exists purely to avoid recalculating
> the optimized map multiple times when stuffing the RESET value).
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/lapic.c | 8 ++++----
>   arch/x86/kvm/lapic.h | 3 +--
>   arch/x86/kvm/x86.c   | 4 ++--
>   3 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 6239cfd89aad..0a73d9a09fe0 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2577,7 +2577,7 @@ u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu)
>   	return (tpr & 0xf0) >> 4;
>   }
>   
> -void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
> +static void __kvm_apic_set_base(struct kvm_vcpu *vcpu, u64 value)
>   {
>   	u64 old_value = vcpu->arch.apic_base;
>   	struct kvm_lapic *apic = vcpu->arch.apic;
> @@ -2628,7 +2628,7 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
>   	}
>   }
>   
> -int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> +int kvm_apic_set_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>   {
>   	enum lapic_mode old_mode = kvm_get_apic_mode(vcpu);
>   	enum lapic_mode new_mode = kvm_apic_mode(msr_info->data);
> @@ -2644,7 +2644,7 @@ int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>   			return 1;
>   	}
>   
> -	kvm_lapic_set_base(vcpu, msr_info->data);
> +	__kvm_apic_set_base(vcpu, msr_info->data);
>   	kvm_recalculate_apic_map(vcpu->kvm);
>   	return 0;
>   }
> @@ -2752,7 +2752,7 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
>   		msr_val = APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE;
>   		if (kvm_vcpu_is_reset_bsp(vcpu))
>   			msr_val |= MSR_IA32_APICBASE_BSP;
> -		kvm_lapic_set_base(vcpu, msr_val);
> +		__kvm_apic_set_base(vcpu, msr_val);

Might be worth a comment here, otherwise

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

for the entire series.

Paolo

[PATCH 7/7] KVM: x86: Make kvm_recalculate_apic_map() local to lapic.c

[Sean Christopherson]

Make kvm_recalculate_apic_map() local to lapic.c now that all external
callers are gone.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c | 2 +-
 arch/x86/kvm/lapic.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 0a73d9a09fe0..21fe50aad603 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -382,7 +382,7 @@ enum {
 	DIRTY
 };
 
-void kvm_recalculate_apic_map(struct kvm *kvm)
+static void kvm_recalculate_apic_map(struct kvm *kvm)
 {
 	struct kvm_apic_map *new, *old = NULL;
 	struct kvm_vcpu *vcpu;
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 0dd5055852ad..fdd6cf29a0be 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -95,7 +95,6 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event);
 u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
 void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);
 void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu);
-void kvm_recalculate_apic_map(struct kvm *kvm);
 void kvm_apic_set_version(struct kvm_vcpu *vcpu);
 void kvm_apic_after_set_mcg_cap(struct kvm_vcpu *vcpu);
 bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
-- 
2.47.0.rc1.288.g06298d1525-goog

Re: [PATCH 0/7] KVM: x86: Clean up MSR_IA32_APICBASE_BASE code

[Huang, Kai]

On Wed, 2024-10-09 at 11:17 -0700, Sean Christopherson wrote:
> Clean up code related to setting and getting MSR_IA32_APICBASE_BASE.
> 

For this series,

Reviewed-by: Kai Huang <kai.huang@intel.com>