Re: [PATCH v5 7/8] KVM: VMX: Update PID-pointer table entry when APIC ID is changed

[Maxim Levitsky <mlevitsk@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

On Fri, 2022-01-14 at 10:58 +0800, Chao Gao wrote:
> On Thu, Jan 13, 2022 at 10:19:21PM +0000, Sean Christopherson wrote:
> > On Tue, Jan 11, 2022, Maxim Levitsky wrote:
> > > Both Intel and AMD's PRM also state that changing APIC ID is implementation
> > > dependent.
> > >  
> > > I vote to forbid changing apic id, at least in the case any APIC acceleration
> > > is used, be that APICv or AVIC.
> > 
> > That has my vote as well.  For IPIv in particular there's not much concern with
> > backwards compability, i.e. we can tie the behavior to enable_ipiv.
Great!
> 
> Hi Sean and Levitsky,
> 
> Let's align on the implementation.
> 
> To disable changes for xAPIC ID when IPIv/AVIC is enabled:
> 
> 1. introduce a variable (forbid_apicid_change) for this behavior in kvm.ko
> and export it so that kvm-intel, kvm-amd can set it when IPIv/AVIC is
> enabled. To reduce complexity, this variable is a module level setting.
> 
> 2. when guest attempts to change xAPIC ID but it is forbidden, KVM prints
> a warning on host and injects a #GP to guest.
> 
> 3. remove AVIC code that deals with changes to xAPIC ID.
> 

I have a patch for both, I attached them.
I haven't tested either of these patches that much other than a smoke test,
but I did test all of the guests I  have and none broke in regard to boot.

I will send those patches as part of larger patch series that implements
nesting for AVIC. I hope to do this next week.

Best regards,
	Maxim Levitsky

[-- Attachment #2: 0001-KVM-x86-lapic-don-t-allow-to-change-APIC-ID-when-api.patch --]
[-- Type: text/x-patch, Size: 1241 bytes --]

From 4a70416b98c4725dc28608152b66ec42a233b2e8 Mon Sep 17 00:00:00 2001
From: Maxim Levitsky <mlevitsk@redhat.com>
Date: Sun, 9 Jan 2022 18:09:08 +0200
Subject: [PATCH 1/8] KVM: x86: lapic: don't allow to change APIC ID when apic
 acceleration is enabled

No sane guest would change physical APIC IDs, and allowing this introduces bugs
into APIC acceleration code.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/lapic.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 6e1fbbf4c508b..56bc494cadd3e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2007,10 +2007,16 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 
 	switch (reg) {
 	case APIC_ID:		/* Local APIC ID */
-		if (!apic_x2apic_mode(apic))
-			kvm_apic_set_xapic_id(apic, val >> 24);
-		else
+		if (!apic_x2apic_mode(apic) ||
+		    /*
+		     * Don't allow setting APIC ID with any APIC acceleration
+		     * enabled to avoid unexpected issues
+		     */
+		    (enable_apicv && ((val >> 24) != apic->vcpu->vcpu_id))) {
 			ret = 1;
+			break;
+		}
+		kvm_apic_set_xapic_id(apic, val >> 24);
 		break;
 
 	case APIC_TASKPRI:
-- 
2.26.3


[-- Attachment #3: 0002-KVM-x86-AVIC-remove-broken-code-that-updated-APIC-ID.patch --]
[-- Type: text/x-patch, Size: 2144 bytes --]

From 3200924ed056efe58b3d1d12675c194bea98c0fc Mon Sep 17 00:00:00 2001
From: Maxim Levitsky <mlevitsk@redhat.com>
Date: Sun, 9 Jan 2022 18:14:12 +0200
Subject: [PATCH 2/8] KVM: x86: AVIC: remove broken code that updated APIC ID

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/svm/avic.c | 37 ++++---------------------------------
 1 file changed, 4 insertions(+), 33 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index f3ab00f407d5b..8655b35043134 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -480,35 +480,6 @@ static int avic_handle_ldr_update(struct kvm_vcpu *vcpu)
 	return ret;
 }
 
-static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
-{
-	u64 *old, *new;
-	struct vcpu_svm *svm = to_svm(vcpu);
-	u32 id = kvm_xapic_id(vcpu->arch.apic);
-
-	if (vcpu->vcpu_id == id)
-		return 0;
-
-	old = avic_get_physical_id_entry(vcpu, vcpu->vcpu_id);
-	new = avic_get_physical_id_entry(vcpu, id);
-	if (!new || !old)
-		return 1;
-
-	/* We need to move physical_id_entry to new offset */
-	*new = *old;
-	*old = 0ULL;
-	to_svm(vcpu)->avic_physical_id_cache = new;
-
-	/*
-	 * Also update the guest physical APIC ID in the logical
-	 * APIC ID table entry if already setup the LDR.
-	 */
-	if (svm->ldr_reg)
-		avic_handle_ldr_update(vcpu);
-
-	return 0;
-}
-
 static void avic_handle_dfr_update(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
@@ -529,8 +500,10 @@ static int avic_unaccel_trap_write(struct vcpu_svm *svm)
 
 	switch (offset) {
 	case APIC_ID:
-		if (avic_handle_apic_id_update(&svm->vcpu))
-			return 0;
+		/* restore the value that we had, we don't support APIC ID
+		 * changes, but due to trap VM exit, the value was
+		 * already written*/
+		kvm_lapic_reg_write(apic, offset, svm->vcpu.vcpu_id << 24);
 		break;
 	case APIC_LDR:
 		if (avic_handle_ldr_update(&svm->vcpu))
@@ -624,8 +597,6 @@ int avic_init_vcpu(struct vcpu_svm *svm)
 
 void avic_post_state_restore(struct kvm_vcpu *vcpu)
 {
-	if (avic_handle_apic_id_update(vcpu) != 0)
-		return;
 	avic_handle_dfr_update(vcpu);
 	avic_handle_ldr_update(vcpu);
 }
-- 
2.26.3