KVM: x86: Fix a WARN in kvm_apic_send_ipi()

PATCH In review Tao Su 5 participants 14 messages Sep 4, 2023 – Sep 24, 2023

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Tao Su

author

2023-09-04 01:36 UTC #0 lore raw

cover KVM: x86: Fix a WARN in kvm_apic_send_ipi()

When IPI virtualization is enabled, a WARN is triggered if bit12 of ICR MSR is set after APIC-write VM-exit. The reason is kvm_apic_send_ipi() thinks the APIC_ICR_BUSY bit should be cleared because KVM has no delay, but kvm_apic_write_nodecode() doesn't clear the APIC_ICR_BUSY bit.

The APIC_ICR_BUSY bit is removed in x2APIC mode, and bit12 of ICR is changed to UNUSED bit, but kvm_x2apic_icr_write() still uses APIC_ICR_BUSY, which may cause ambiguity, so introducing X2APIC_ICR_UNUSED_12 instead.

When X2APIC_ICR_UNUSED_12 is set, how the hardware handles it determines how the WARN is fixed. However SDM has no detail about it, we tested on Intel CPU (SRF/GNR) with IPI virtualization and found X2APIC_ICR_UNUSED_12 was also cleared by hardware without #GP. Therefore, the clearing of bit12 should be still kept being consistent with the hardware behavior.

Tao Su (2):
x86/apic: Introduce X2APIC_ICR_UNUSED_12 for x2APIC mode
KVM: x86: Clear X2APIC_ICR_UNUSED_12 after APIC-write VM-exit

 arch/x86/include/asm/apicdef.h |  1 +
 arch/x86/kvm/lapic.c           | 27 ++++++++++++++++++++-------
 2 files changed, 21 insertions(+), 7 deletions(-)

base-commit: 708283abf896dd4853e673cc8cba70acaf9bf4ea

Signature · 1 line

2.34.1

Tao Su

author

2023-09-04 01:36 UTC #1 lore raw

PATCH 1/2 x86/apic: Introduce X2APIC_ICR_UNUSED_12 for x2APIC mode

According to SDM, bit12 of ICR is no longer BUSY bit but UNUSED bit in x2APIC mode, which is the only difference of lower ICR between xAPIC and x2APIC mode. To avoid ambiguity, introduce X2APIC_ICR_UNUSED_12 for x2APIC mode.

Signed-off-by Tao Su <[email protected]>

---
 arch/x86/include/asm/apicdef.h | 1 +
 1 file changed, 1 insertion(+)

arch/x86/include/asm/apicdef.h +1 -0

index 4b125e5b3187..ea2725738b81 100644
@@ -78,6 +78,7 @@
 #define		APIC_INT_LEVELTRIG	0x08000
 #define		APIC_INT_ASSERT		0x04000
 #define		APIC_ICR_BUSY		0x01000
+#define		X2APIC_ICR_UNUSED_12	0x01000
 #define		APIC_DEST_LOGICAL	0x00800
 #define		APIC_DEST_PHYSICAL	0x00000
 #define		APIC_DM_FIXED		0x00000

Signature · 1 line

2.34.1

Chao Gao

2023-09-04 02:58 UTC #2 lore raw

Re: [PATCH 1/2] x86/apic: Introduce X2APIC_ICR_UNUSED_12 for x2APIC mode

Quoted 6 lines · Tao Su

According to SDM, bit12 of ICR is no longer BUSY bit but UNUSED bit in x2APIC mode, which is the only difference of lower ICR between xAPIC and x2APIC mode. To avoid ambiguity, introduce X2APIC_ICR_UNUSED_12 for x2APIC mode.
Signed-off-by Tao Su <[email protected]>

Please use scripts/get_maintainer.pl to help create the Cc/To lists. I believe x86 maintainers should be copied for this patch.

Quoted 19 lines

---
arch/x86/include/asm/apicdef.h | 1 +

1 file changed, 1 insertion(+)

arch/x86/include/asm/apicdef.h +1 -0

index 4b125e5b3187..ea2725738b81 100644
@@ -78,6 +78,7 @@
 #define		APIC_INT_LEVELTRIG	0x08000
 #define		APIC_INT_ASSERT		0x04000
 #define		APIC_ICR_BUSY		0x01000
+#define		X2APIC_ICR_UNUSED_12	0x01000
 #define		APIC_DEST_LOGICAL	0x00800
 #define		APIC_DEST_PHYSICAL	0x00000
 #define		APIC_DM_FIXED		0x00000

Signature · 1 line

2.34.1

Tao Su

author

2023-09-04 03:06 UTC #3 lore raw

Re: [PATCH 1/2] x86/apic: Introduce X2APIC_ICR_UNUSED_12 for x2APIC mode

Quoted 10 lines · Chao Gao

Quoted 6 lines · Tao Su
According to SDM, bit12 of ICR is no longer BUSY bit but UNUSED bit in x2APIC mode, which is the only difference of lower ICR between xAPIC and x2APIC mode. To avoid ambiguity, introduce X2APIC_ICR_UNUSED_12 for x2APIC mode.
Signed-off-by Tao Su <[email protected]>
Please use scripts/get_maintainer.pl to help create the Cc/To lists. I believe x86 maintainers should be copied for this patch.

Ok, I will cc x86 maintainers if keep this patch in the next version.

Thanks,
Tao

Quoted 20 lines

---
arch/x86/include/asm/apicdef.h | 1 +

1 file changed, 1 insertion(+)

arch/x86/include/asm/apicdef.h +1 -0

index 4b125e5b3187..ea2725738b81 100644
@@ -78,6 +78,7 @@
 #define		APIC_INT_LEVELTRIG	0x08000
 #define		APIC_INT_ASSERT		0x04000
 #define		APIC_ICR_BUSY		0x01000
+#define		X2APIC_ICR_UNUSED_12	0x01000
 #define		APIC_DEST_LOGICAL	0x00800
 #define		APIC_DEST_PHYSICAL	0x00000
 #define		APIC_DM_FIXED		0x00000

Signature · 1 line

2.34.1

Tao Su

author

2023-09-04 01:36 UTC #4 lore raw

PATCH 2/2 KVM: x86: Clear X2APIC_ICR_UNUSED_12 after APIC-write VM-exit

In reply to Tao Su

Since bit12 of ICR is no longer BUSY bit but UNUSED bit in x2APIC mode, and SDM has no detail about how hardware will handle the UNUSED bit12 set, we tested on Intel CPU (SRF/GNR) with IPI virtualization and found the UNUSED bit12 was also cleared by hardware without #GP. Therefore, the clearing of bit12 should be still kept being consistent with the hardware behavior.

Fixes 5413bcba7ed5 ("KVM: x86: Add support for vICR APIC-write VM-Exits in x2APIC mode")

Signed-off-by Tao Su <[email protected]>

Tested-by Yi Lai <[email protected]>

---
 arch/x86/kvm/lapic.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

arch/x86/kvm/lapic.c +20 -7

index a983a16163b1..09a376aeb4a0 100644
@@ -1482,8 +1482,17 @@ void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high)
 {
 	struct kvm_lapic_irq irq;
 
-	/* KVM has no delay and should always clear the BUSY/PENDING flag. */
-	WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
+	/*
+	 * In non-x2apic mode, KVM has no delay and should always clear the
+	 * BUSY/PENDING flag. In x2apic mode, KVM should clear the unused bit12
+	 * of ICR since hardware will also clear this bit. Although
+	 * APIC_ICR_BUSY and X2APIC_ICR_UNUSED_12 are same, they mean different
+	 * things in different modes.
+	 */
+	if (!apic_x2apic_mode(apic))
+		WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
+	else
+		WARN_ON_ONCE(icr_low & X2APIC_ICR_UNUSED_12);
 
 	irq.vector = icr_low & APIC_VECTOR_MASK;
 	irq.delivery_mode = icr_low & APIC_MODE_MASK;

@@ -2429,13 +2438,12 @@ void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
 	 * ICR is a single 64-bit register when x2APIC is enabled.  For legacy
 	 * xAPIC, ICR writes need to go down the common (slightly slower) path
 	 * to get the upper half from ICR2.
+	 *
+	 * TODO: optimize to just emulate side effect w/o one more write
 	 */
 	if (apic_x2apic_mode(apic) && offset == APIC_ICR) {
-		val = kvm_lapic_get_reg64(apic, APIC_ICR);
-		kvm_apic_send_ipi(apic, (u32)val, (u32)(val >> 32));
-		trace_kvm_apic_write(APIC_ICR, val);
+		kvm_x2apic_icr_write(apic, val);
 	} else {
-		/* TODO: optimize to just emulate side effect w/o one more write */
 		val = kvm_lapic_get_reg(apic, offset);
 		kvm_lapic_reg_write(apic, offset, (u32)val);
 	}

@@ -3122,7 +3130,12 @@ int kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr)
 
 int kvm_x2apic_icr_write(struct kvm_lapic *apic, u64 data)
 {
-	data &= ~APIC_ICR_BUSY;
+	/*
+	 * The Delivery Status Bit(bit 12) is removed in x2apic mode, but this
+	 * bit is also cleared by hardware, so keep consistent with hardware
+	 * behavior to clear this bit.
+	 */
+	data &= ~X2APIC_ICR_UNUSED_12;
 
 	kvm_apic_send_ipi(apic, (u32)data, (u32)(data >> 32));
 	kvm_lapic_set_reg64(apic, APIC_ICR, data);

Signature · 1 line

2.34.1

Chao Gao

2023-09-04 02:46 UTC #5 lore raw

Re: [PATCH 2/2] KVM: x86: Clear X2APIC_ICR_UNUSED_12 after APIC-write VM-exit

Quoted 30 lines · Tao Su

When IPI virtualization is enabled, a WARN is triggered if bit12 of ICR MSR is set after APIC-write VM-exit. The reason is kvm_apic_send_ipi() thinks the APIC_ICR_BUSY bit should be cleared because KVM has no delay, but kvm_apic_write_nodecode() doesn't clear the APIC_ICR_BUSY bit.
Since bit12 of ICR is no longer BUSY bit but UNUSED bit in x2APIC mode, and SDM has no detail about how hardware will handle the UNUSED bit12 set, we tested on Intel CPU (SRF/GNR) with IPI virtualization and found the UNUSED bit12 was also cleared by hardware without #GP. Therefore, the clearing of bit12 should be still kept being consistent with the hardware behavior.
Fixes 5413bcba7ed5 ("KVM: x86: Add support for vICR APIC-write VM-Exits in x2APIC mode")
Signed-off-by Tao Su <[email protected]>
Tested-by Yi Lai <[email protected]>
---
arch/x86/kvm/lapic.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
arch/x86/kvm/lapic.c +1 -2
index a983a16163b1..09a376aeb4a0 100644
@@ -1482,8 +1482,17 @@ void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high)
 {
 	struct kvm_lapic_irq irq;
 
-	/* KVM has no delay and should always clear the BUSY/PENDING flag. */
-	WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
+	/*

arch/x86/kvm/lapic.c void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high) · line 1495

Quoted 10 lines

arch/x86/kvm/lapic.c +10 -0

+	 * In non-x2apic mode, KVM has no delay and should always clear the
+	 * BUSY/PENDING flag. In x2apic mode, KVM should clear the unused bit12
+	 * of ICR since hardware will also clear this bit. Although
+	 * APIC_ICR_BUSY and X2APIC_ICR_UNUSED_12 are same, they mean different
+	 * things in different modes.
+	 */
+	if (!apic_x2apic_mode(apic))
+		WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
+	else
+		WARN_ON_ONCE(icr_low & X2APIC_ICR_UNUSED_12);

Hi Tao,

Using the alias for APIC_ICR_BUSY is not strictly necessary for the bug fix. I think it is better to move this hunk into patch 1.

Quoted 21 lines

arch/x86/kvm/lapic.c +3 -4

 	irq.vector = icr_low & APIC_VECTOR_MASK;
 	irq.delivery_mode = icr_low & APIC_MODE_MASK;

@@ -2429,13 +2438,12 @@ void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
 	 * ICR is a single 64-bit register when x2APIC is enabled.  For legacy
 	 * xAPIC, ICR writes need to go down the common (slightly slower) path
 	 * to get the upper half from ICR2.
+	 *
+	 * TODO: optimize to just emulate side effect w/o one more write
 	 */
 	if (apic_x2apic_mode(apic) && offset == APIC_ICR) {
-		val = kvm_lapic_get_reg64(apic, APIC_ICR);
-		kvm_apic_send_ipi(apic, (u32)val, (u32)(val >> 32));
-		trace_kvm_apic_write(APIC_ICR, val);
+		kvm_x2apic_icr_write(apic, val);
 	} else {
-		/* TODO: optimize to just emulate side effect w/o one more write */
 		val = kvm_lapic_get_reg(apic, offset);
 		kvm_lapic_reg_write(apic, offset, (u32)val);
 	}

arch/x86/kvm/lapic.c int kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr) · line 3138

Quoted 10 lines

arch/x86/kvm/lapic.c +6 -1

@@ -3122,7 +3130,12 @@ int kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr)
 
 int kvm_x2apic_icr_write(struct kvm_lapic *apic, u64 data)
 {
-	data &= ~APIC_ICR_BUSY;
+	/*
+	 * The Delivery Status Bit(bit 12) is removed in x2apic mode, but this
+	 * bit is also cleared by hardware, so keep consistent with hardware
+	 * behavior to clear this bit.
+	 */
+	data &= ~X2APIC_ICR_UNUSED_12;

Ditto.

and can you also swap patch 1 with patch 2? Because patch 1 is an optional improvement, putting such a patch at the end of a patch set gives maintainers an easy choice about whether they want it or not.

Quoted 6 lines

arch/x86/kvm/lapic.c +0 -0

 	kvm_apic_send_ipi(apic, (u32)data, (u32)(data >> 32));
 	kvm_lapic_set_reg64(apic, APIC_ICR, data);

Signature · 1 line

2.34.1

Tao Su

author

2023-09-04 03:02 UTC #6 lore raw

Re: [PATCH 2/2] KVM: x86: Clear X2APIC_ICR_UNUSED_12 after APIC-write VM-exit

Quoted 82 lines · Chao Gao

Quoted 30 lines · Tao Su
When IPI virtualization is enabled, a WARN is triggered if bit12 of ICR MSR is set after APIC-write VM-exit. The reason is kvm_apic_send_ipi() thinks the APIC_ICR_BUSY bit should be cleared because KVM has no delay, but kvm_apic_write_nodecode() doesn't clear the APIC_ICR_BUSY bit.
Since bit12 of ICR is no longer BUSY bit but UNUSED bit in x2APIC mode, and SDM has no detail about how hardware will handle the UNUSED bit12 set, we tested on Intel CPU (SRF/GNR) with IPI virtualization and found the UNUSED bit12 was also cleared by hardware without #GP. Therefore, the clearing of bit12 should be still kept being consistent with the hardware behavior.
Fixes 5413bcba7ed5 ("KVM: x86: Add support for vICR APIC-write VM-Exits in x2APIC mode")
Signed-off-by Tao Su <[email protected]>
Tested-by Yi Lai <[email protected]>
---
arch/x86/kvm/lapic.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
arch/x86/kvm/lapic.c +1 -2
index a983a16163b1..09a376aeb4a0 100644
@@ -1482,8 +1482,17 @@ void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high)
 {
 	struct kvm_lapic_irq irq;
 
-	/* KVM has no delay and should always clear the BUSY/PENDING flag. */
-	WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
+	/*
arch/x86/kvm/lapic.c void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high) · line 1495
Quoted 10 lines
arch/x86/kvm/lapic.c +10 -0
+	 * In non-x2apic mode, KVM has no delay and should always clear the
+	 * BUSY/PENDING flag. In x2apic mode, KVM should clear the unused bit12
+	 * of ICR since hardware will also clear this bit. Although
+	 * APIC_ICR_BUSY and X2APIC_ICR_UNUSED_12 are same, they mean different
+	 * things in different modes.
+	 */
+	if (!apic_x2apic_mode(apic))
+		WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
+	else
+		WARN_ON_ONCE(icr_low & X2APIC_ICR_UNUSED_12);
Hi Tao,
Using the alias for APIC_ICR_BUSY is not strictly necessary for the bug fix. I think it is better to move this hunk into patch 1.
Quoted 21 lines
arch/x86/kvm/lapic.c +3 -4
 	irq.vector = icr_low & APIC_VECTOR_MASK;
 	irq.delivery_mode = icr_low & APIC_MODE_MASK;
@@ -2429,13 +2438,12 @@ void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
 	 * ICR is a single 64-bit register when x2APIC is enabled.  For legacy
 	 * xAPIC, ICR writes need to go down the common (slightly slower) path
 	 * to get the upper half from ICR2.
+	 *
+	 * TODO: optimize to just emulate side effect w/o one more write
 	 */
 	if (apic_x2apic_mode(apic) && offset == APIC_ICR) {
-		val = kvm_lapic_get_reg64(apic, APIC_ICR);
-		kvm_apic_send_ipi(apic, (u32)val, (u32)(val >> 32));
-		trace_kvm_apic_write(APIC_ICR, val);
+		kvm_x2apic_icr_write(apic, val);
 	} else {
-		/* TODO: optimize to just emulate side effect w/o one more write */
 		val = kvm_lapic_get_reg(apic, offset);
 		kvm_lapic_reg_write(apic, offset, (u32)val);
 	}
arch/x86/kvm/lapic.c int kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr) · line 3138
Quoted 10 lines
arch/x86/kvm/lapic.c +6 -1
@@ -3122,7 +3130,12 @@ int kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr)
 
 int kvm_x2apic_icr_write(struct kvm_lapic *apic, u64 data)
 {
-	data &= ~APIC_ICR_BUSY;
+	/*
+	 * The Delivery Status Bit(bit 12) is removed in x2apic mode, but this
+	 * bit is also cleared by hardware, so keep consistent with hardware
+	 * behavior to clear this bit.
+	 */
+	data &= ~X2APIC_ICR_UNUSED_12;
Ditto.

Quoted 3 lines

and can you also swap patch 1 with patch 2? Because patch 1 is an optional improvement, putting such a patch at the end of a patch set gives maintainers an easy choice about whether they want it or not.

Yes, I just think the alias will make the code clearer because x2APIC does remove bit12. If it is not necessary, I will directly drop this alias in version 2.

Thanks,
Tao

Quoted 7 lines

Quoted 7 lines
arch/x86/kvm/lapic.c +0 -0
 	kvm_apic_send_ipi(apic, (u32)data, (u32)(data >> 32));
 	kvm_lapic_set_reg64(apic, APIC_ICR, data);
Signature · 1 line
2.34.1

kernel test robot

build bot

2023-09-04 04:17 UTC #7 lore raw

Re: [PATCH 2/2] KVM: x86: Clear X2APIC_ICR_UNUSED_12 after APIC-write VM-exit

In reply to Tao Su

kernel test robot noticed the following build warnings:

Hi Tao,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 708283abf896dd4853e673cc8cba70acaf9bf4ea]

url:    https://github.com/intel-lab-lkp/linux/commits/Tao-Su/x86-apic-Introduce-X2APIC_ICR_UNUSED_12-for-x2APIC-mode/20230904-093801
base:   708283abf896dd4853e673cc8cba70acaf9bf4ea
patch link:    https://lore.kernel.org/r/20230904013555.725413-3-tao1.su%40linux.intel.com
patch subject: [PATCH 2/2] KVM: x86: Clear X2APIC_ICR_UNUSED_12 after APIC-write VM-exit
config: x86_64-randconfig-003-20230904 (https://download.01.org/0day-ci/archive/20230904/[email protected]/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230904/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

>> arch/x86/kvm/lapic.c:2445:30: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
                   kvm_x2apic_icr_write(apic, val);
                                              ^~~
   arch/x86/kvm/lapic.c:2435:9: note: initialize the variable 'val' to silence this warning
           u64 val;
                  ^
                   = 0

1 warning generated.

vim +/val +2445 arch/x86/kvm/lapic.c

2430
2431 /* emulate APIC access in a trap manner */
2432 void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
2433 {
2434 struct kvm_lapic *apic = vcpu->arch.apic;
2435 u64 val;
2436
2437 /*
2438 * ICR is a single 64-bit register when x2APIC is enabled. For legacy
2439 * xAPIC, ICR writes need to go down the common (slightly slower) path
2440 * to get the upper half from ICR2.
2441 *
2442 * TODO: optimize to just emulate side effect w/o one more write
2443 */
2444 if (apic_x2apic_mode(apic) && offset == APIC_ICR) {

> 2445			kvm_x2apic_icr_write(apic, val);

2446 } else {
2447 val = kvm_lapic_get_reg(apic, offset);
2448 kvm_lapic_reg_write(apic, offset, (u32)val);
2449 }
2450 }
2451 EXPORT_SYMBOL_GPL(kvm_apic_write_nodecode);
2452

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Tao Su

author

2023-09-04 05:05 UTC #8 lore raw

Re: [PATCH 2/2] KVM: x86: Clear X2APIC_ICR_UNUSED_12 after APIC-write VM-exit

In reply to Tao Su

Quoted 44 lines · Tao Su

When IPI virtualization is enabled, a WARN is triggered if bit12 of ICR MSR is set after APIC-write VM-exit. The reason is kvm_apic_send_ipi() thinks the APIC_ICR_BUSY bit should be cleared because KVM has no delay, but kvm_apic_write_nodecode() doesn't clear the APIC_ICR_BUSY bit.
Since bit12 of ICR is no longer BUSY bit but UNUSED bit in x2APIC mode, and SDM has no detail about how hardware will handle the UNUSED bit12 set, we tested on Intel CPU (SRF/GNR) with IPI virtualization and found the UNUSED bit12 was also cleared by hardware without #GP. Therefore, the clearing of bit12 should be still kept being consistent with the hardware behavior.
Fixes 5413bcba7ed5 ("KVM: x86: Add support for vICR APIC-write VM-Exits in x2APIC mode")
Signed-off-by Tao Su <[email protected]>
Tested-by Yi Lai <[email protected]>
---
 arch/x86/kvm/lapic.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)
arch/x86/kvm/lapic.c +11 -2
index a983a16163b1..09a376aeb4a0 100644
@@ -1482,8 +1482,17 @@ void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high)
 {
 	struct kvm_lapic_irq irq;
 
-	/* KVM has no delay and should always clear the BUSY/PENDING flag. */
-	WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
+	/*
+	 * In non-x2apic mode, KVM has no delay and should always clear the
+	 * BUSY/PENDING flag. In x2apic mode, KVM should clear the unused bit12
+	 * of ICR since hardware will also clear this bit. Although
+	 * APIC_ICR_BUSY and X2APIC_ICR_UNUSED_12 are same, they mean different
+	 * things in different modes.
+	 */
+	if (!apic_x2apic_mode(apic))
+		WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
+	else
+		WARN_ON_ONCE(icr_low & X2APIC_ICR_UNUSED_12);
 
 	irq.vector = icr_low & APIC_VECTOR_MASK;
 	irq.delivery_mode = icr_low & APIC_MODE_MASK;

arch/x86/kvm/lapic.c void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset) · line 2434

Quoted 8 lines

arch/x86/kvm/lapic.c +2 -1

@@ -2429,13 +2438,12 @@ void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
 	 * ICR is a single 64-bit register when x2APIC is enabled.  For legacy
 	 * xAPIC, ICR writes need to go down the common (slightly slower) path
 	 * to get the upper half from ICR2.
+	 *
+	 * TODO: optimize to just emulate side effect w/o one more write
 	 */
 	if (apic_x2apic_mode(apic) && offset == APIC_ICR) {
-		val = kvm_lapic_get_reg64(apic, APIC_ICR);

Sorry for the mistake, I notice this line is removed accidentally, I will add it back in the next version.

Quoted 25 lines

arch/x86/kvm/lapic.c +7 -4

-		kvm_apic_send_ipi(apic, (u32)val, (u32)(val >> 32));
-		trace_kvm_apic_write(APIC_ICR, val);
+		kvm_x2apic_icr_write(apic, val);
 	} else {
-		/* TODO: optimize to just emulate side effect w/o one more write */
 		val = kvm_lapic_get_reg(apic, offset);
 		kvm_lapic_reg_write(apic, offset, (u32)val);
 	}

@@ -3122,7 +3130,12 @@ int kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr)
 
 int kvm_x2apic_icr_write(struct kvm_lapic *apic, u64 data)
 {
-	data &= ~APIC_ICR_BUSY;
+	/*
+	 * The Delivery Status Bit(bit 12) is removed in x2apic mode, but this
+	 * bit is also cleared by hardware, so keep consistent with hardware
+	 * behavior to clear this bit.
+	 */
+	data &= ~X2APIC_ICR_UNUSED_12;
 
 	kvm_apic_send_ipi(apic, (u32)data, (u32)(data >> 32));
 	kvm_lapic_set_reg64(apic, APIC_ICR, data);

Signature · 1 line

2.34.1

Sean Christopherson

maintainer

2023-09-05 23:03 UTC #9 lore raw

Re: [PATCH 2/2] KVM: x86: Clear X2APIC_ICR_UNUSED_12 after APIC-write VM-exit

In reply to Tao Su

+Suravee

Quoted 11 lines · Tao Su

When IPI virtualization is enabled, a WARN is triggered if bit12 of ICR MSR is set after APIC-write VM-exit. The reason is kvm_apic_send_ipi() thinks the APIC_ICR_BUSY bit should be cleared because KVM has no delay, but kvm_apic_write_nodecode() doesn't clear the APIC_ICR_BUSY bit.
Since bit12 of ICR is no longer BUSY bit but UNUSED bit in x2APIC mode, and SDM has no detail about how hardware will handle the UNUSED bit12 set, we tested on Intel CPU (SRF/GNR) with IPI virtualization and found the UNUSED bit12 was also cleared by hardware without #GP. Therefore, the clearing of bit12 should be still kept being consistent with the hardware behavior.

I'm confused. If hardware clears the bit, then why is it set in the vAPIC page after a trap-like APIC-write VM-Exit? In other words, how is this not a ucode or hardware bug?

Suravee, can you confirm what happens on AMD with x2AVIC? Does hardware *always* clear the busy bit if it's set by the guest? If so, then we could "optimize" avic_incomplete_ipi_interception() to skip the busy check, e.g.

arch/x86/kvm/svm/avic.c +1 -1

index cfc8ab773025..4bf0bb250147 100644
@@ -513,7 +513,7 @@ int avic_incomplete_ipi_interception(struct kvm_vcpu *vcpu)
                 * in which case KVM needs to emulate the ICR write as well in
                 * order to clear the BUSY flag.
                 */
-               if (icrl & APIC_ICR_BUSY)
+               if (!apic_x2apic_mode(apic) && (icrl & APIC_ICR_BUSY))
                        kvm_apic_write_nodecode(vcpu, APIC_ICR);
                else
                        kvm_apic_send_ipi(apic, icrl, icrh);

Quoted 18 lines

Fixes 5413bcba7ed5 ("KVM: x86: Add support for vICR APIC-write VM-Exits in x2APIC mode")

Signed-off-by Tao Su <[email protected]>

Tested-by Yi Lai <[email protected]>

---
 arch/x86/kvm/lapic.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

arch/x86/kvm/lapic.c +1 -2

index a983a16163b1..09a376aeb4a0 100644
@@ -1482,8 +1482,17 @@ void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high)
 {
 	struct kvm_lapic_irq irq;
 
-	/* KVM has no delay and should always clear the BUSY/PENDING flag. */
-	WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
+	/*

arch/x86/kvm/lapic.c void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high) · line 1495

Quoted 10 lines

arch/x86/kvm/lapic.c +10 -0

+	 * In non-x2apic mode, KVM has no delay and should always clear the
+	 * BUSY/PENDING flag. In x2apic mode, KVM should clear the unused bit12
+	 * of ICR since hardware will also clear this bit. Although
+	 * APIC_ICR_BUSY and X2APIC_ICR_UNUSED_12 are same, they mean different
+	 * things in different modes.
+	 */
+	if (!apic_x2apic_mode(apic))
+		WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
+	else
+		WARN_ON_ONCE(icr_low & X2APIC_ICR_UNUSED_12);

NAK to the new name, KVM is absolutely not going to zero an arbitrary "unused" bit. If Intel wants to reclaim bit 12 for something useful in the future, then Intel can ship CPUs that don't touch the "reserved" bit, and deal with all the fun of finding and updating all software that unnecessarily sets the busy bit in x2apic mode.

If we really want to pretend that Intel has more than a snowball's chance in hell of doing something useful with bit 12, then the right thing to do in KVM is to ignore the bit entirely and let the guest keep the pieces, e.g.

arch/x86/kvm/lapic.c +7 -4

index 113ca9661ab2..36ec195a3339 100644
@@ -1473,8 +1473,13 @@ void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high)
 {
        struct kvm_lapic_irq irq;
 
-       /* KVM has no delay and should always clear the BUSY/PENDING flag. */
-       WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
+       /*
+        * KVM has no delay and should always clear the BUSY/PENDING flag.
+        * The flag doesn't exist in x2APIC mode; both the SDM and APM state
+        * that the flag "Must Be Zero", but neither Intel nor AMD enforces
+        * that (or any other reserved bits in ICR).
+        */
+       WARN_ON_ONCE(!apic_x2apic_mode(apic) && (icr_low & APIC_ICR_BUSY));
 
        irq.vector = icr_low & APIC_VECTOR_MASK;
        irq.delivery_mode = icr_low & APIC_MODE_MASK;

@@ -3113,8 +3118,6 @@ int kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr)
 
 int kvm_x2apic_icr_write(struct kvm_lapic *apic, u64 data)
 {
-       data &= ~APIC_ICR_BUSY;
-
        kvm_apic_send_ipi(apic, (u32)data, (u32)(data >> 32));
        kvm_lapic_set_reg64(apic, APIC_ICR, data);
        trace_kvm_apic_write(APIC_ICR, data);

arch/x86/kvm/svm/avic.c +1 -1

index cfc8ab773025..4bf0bb250147 100644
@@ -513,7 +513,7 @@ int avic_incomplete_ipi_interception(struct kvm_vcpu *vcpu)
                 * in which case KVM needs to emulate the ICR write as well in
                 * order to clear the BUSY flag.
                 */
-               if (icrl & APIC_ICR_BUSY)
+               if (!apic_x2apic_mode(apic) && (icrl & APIC_ICR_BUSY))
                        kvm_apic_write_nodecode(vcpu, APIC_ICR);
                else
                        kvm_apic_send_ipi(apic, icrl, icrh);

Tao Su

author

2023-09-06 05:10 UTC #10 lore raw

Re: [PATCH 2/2] KVM: x86: Clear X2APIC_ICR_UNUSED_12 after APIC-write VM-exit

Quoted 15 lines · Sean Christopherson

+Suravee
Quoted 11 lines · Tao Su
When IPI virtualization is enabled, a WARN is triggered if bit12 of ICR MSR is set after APIC-write VM-exit. The reason is kvm_apic_send_ipi() thinks the APIC_ICR_BUSY bit should be cleared because KVM has no delay, but kvm_apic_write_nodecode() doesn't clear the APIC_ICR_BUSY bit.
Since bit12 of ICR is no longer BUSY bit but UNUSED bit in x2APIC mode, and SDM has no detail about how hardware will handle the UNUSED bit12 set, we tested on Intel CPU (SRF/GNR) with IPI virtualization and found the UNUSED bit12 was also cleared by hardware without #GP. Therefore, the clearing of bit12 should be still kept being consistent with the hardware behavior.

Quoted 3 lines

I'm confused. If hardware clears the bit, then why is it set in the vAPIC page after a trap-like APIC-write VM-Exit? In other words, how is this not a ucode or hardware bug?

Sorry, I didn't describe it clearly.

On bare-metal, bit12 of ICR MSR will be cleared after setting this bit.

If bit12 is set in guest, the bit is not cleared in the vAPIC page after APIC-write VM-Exit. So whether to clear bit12 in vAPIC page needs to be considered.

Quoted 54 lines

Suravee, can you confirm what happens on AMD with x2AVIC? Does hardware *always* clear the busy bit if it's set by the guest? If so, then we could "optimize" avic_incomplete_ipi_interception() to skip the busy check, e.g.
arch/x86/kvm/svm/avic.c +1 -1
index cfc8ab773025..4bf0bb250147 100644
@@ -513,7 +513,7 @@ int avic_incomplete_ipi_interception(struct kvm_vcpu *vcpu)
                 * in which case KVM needs to emulate the ICR write as well in
                 * order to clear the BUSY flag.
                 */
-               if (icrl & APIC_ICR_BUSY)
+               if (!apic_x2apic_mode(apic) && (icrl & APIC_ICR_BUSY))
                        kvm_apic_write_nodecode(vcpu, APIC_ICR);
                else
                        kvm_apic_send_ipi(apic, icrl, icrh);
Quoted 19 lines
Fixes 5413bcba7ed5 ("KVM: x86: Add support for vICR APIC-write VM-Exits in x2APIC mode")
Signed-off-by Tao Su <[email protected]>
Tested-by Yi Lai <[email protected]>
---
 arch/x86/kvm/lapic.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)
arch/x86/kvm/lapic.c +1 -2
index a983a16163b1..09a376aeb4a0 100644
@@ -1482,8 +1482,17 @@ void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high)
 {
 	struct kvm_lapic_irq irq;
 
-	/* KVM has no delay and should always clear the BUSY/PENDING flag. */
-	WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
+	/*
arch/x86/kvm/lapic.c void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high) · line 1495
Quoted 10 lines
arch/x86/kvm/lapic.c +10 -0
+	 * In non-x2apic mode, KVM has no delay and should always clear the
+	 * BUSY/PENDING flag. In x2apic mode, KVM should clear the unused bit12
+	 * of ICR since hardware will also clear this bit. Although
+	 * APIC_ICR_BUSY and X2APIC_ICR_UNUSED_12 are same, they mean different
+	 * things in different modes.
+	 */
+	if (!apic_x2apic_mode(apic))
+		WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
+	else
+		WARN_ON_ONCE(icr_low & X2APIC_ICR_UNUSED_12);
NAK to the new name, KVM is absolutely not going to zero an arbitrary "unused" bit. If Intel wants to reclaim bit 12 for something useful in the future, then Intel can ship CPUs that don't touch the "reserved" bit, and deal with all the fun of finding and updating all software that unnecessarily sets the busy bit in x2apic mode.

Quoted 3 lines

If we really want to pretend that Intel has more than a snowball's chance in hell of doing something useful with bit 12, then the right thing to do in KVM is to ignore the bit entirely and let the guest keep the pieces, e.g.

Yes, agree. Currently bit12 is unused and cleared by hardware on bare-metal, clearing bit12 in guest is for keeping the same behavior as bare-metal. But KVM should not to zero the unused bit until SDM reclaims it for something, so ignoring the bit in KVM should be better.

Thanks,
Tao

Quoted 44 lines

arch/x86/kvm/lapic.c +7 -4

index 113ca9661ab2..36ec195a3339 100644
@@ -1473,8 +1473,13 @@ void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high)
 {
        struct kvm_lapic_irq irq;
 
-       /* KVM has no delay and should always clear the BUSY/PENDING flag. */
-       WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
+       /*
+        * KVM has no delay and should always clear the BUSY/PENDING flag.
+        * The flag doesn't exist in x2APIC mode; both the SDM and APM state
+        * that the flag "Must Be Zero", but neither Intel nor AMD enforces
+        * that (or any other reserved bits in ICR).
+        */
+       WARN_ON_ONCE(!apic_x2apic_mode(apic) && (icr_low & APIC_ICR_BUSY));
 
        irq.vector = icr_low & APIC_VECTOR_MASK;
        irq.delivery_mode = icr_low & APIC_MODE_MASK;

@@ -3113,8 +3118,6 @@ int kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr)
 
 int kvm_x2apic_icr_write(struct kvm_lapic *apic, u64 data)
 {
-       data &= ~APIC_ICR_BUSY;
-
        kvm_apic_send_ipi(apic, (u32)data, (u32)(data >> 32));
        kvm_lapic_set_reg64(apic, APIC_ICR, data);
        trace_kvm_apic_write(APIC_ICR, data);

arch/x86/kvm/svm/avic.c +1 -1

index cfc8ab773025..4bf0bb250147 100644
@@ -513,7 +513,7 @@ int avic_incomplete_ipi_interception(struct kvm_vcpu *vcpu)
                 * in which case KVM needs to emulate the ICR write as well in
                 * order to clear the BUSY flag.
                 */
-               if (icrl & APIC_ICR_BUSY)
+               if (!apic_x2apic_mode(apic) && (icrl & APIC_ICR_BUSY))
                        kvm_apic_write_nodecode(vcpu, APIC_ICR);
                else
                        kvm_apic_send_ipi(apic, icrl, icrh);

Sean Christopherson

maintainer

2023-09-06 22:17 UTC #11 lore raw

Re: [PATCH 2/2] KVM: x86: Clear X2APIC_ICR_UNUSED_12 after APIC-write VM-exit

Quoted 20 lines · Tao Su

Quoted 15 lines · Sean Christopherson
+Suravee
Quoted 11 lines · Tao Su
When IPI virtualization is enabled, a WARN is triggered if bit12 of ICR MSR is set after APIC-write VM-exit. The reason is kvm_apic_send_ipi() thinks the APIC_ICR_BUSY bit should be cleared because KVM has no delay, but kvm_apic_write_nodecode() doesn't clear the APIC_ICR_BUSY bit.
Since bit12 of ICR is no longer BUSY bit but UNUSED bit in x2APIC mode, and SDM has no detail about how hardware will handle the UNUSED bit12 set, we tested on Intel CPU (SRF/GNR) with IPI virtualization and found the UNUSED bit12 was also cleared by hardware without #GP. Therefore, the clearing of bit12 should be still kept being consistent with the hardware behavior.
Quoted 3 lines
I'm confused. If hardware clears the bit, then why is it set in the vAPIC page after a trap-like APIC-write VM-Exit? In other words, how is this not a ucode or hardware bug?

Quoted 6 lines

Sorry, I didn't describe it clearly.
On bare-metal, bit12 of ICR MSR will be cleared after setting this bit.
If bit12 is set in guest, the bit is not cleared in the vAPIC page after APIC-write VM-Exit. So whether to clear bit12 in vAPIC page needs to be considered.

I got that, the behavior just seems odd to me. And I'm grumpy that Intel punted the problem to software. But the SDM specifically calls out that this is the correct behavior :-/

Specifically, in the context of IPI virtualization:

If ECX contains 830H, a general-protection fault occurs if any of bits 31:20, 17:16, or 13 of EAX is non-zero.

and

If ECX contains 830H, the processor then checks the value of VICR to determine whether the following are all true:

Bits 19:18 (destination shorthand) are 00B (no shorthand). Bit 15 (trigger mode) is 0 (edge).
Bit 12 (unused) is 0.
Bit 11 (destination mode) is 0 (physical).
Bits 10:8 (delivery mode) are 000B (fixed).

If all of the items above are true, the processor performs IPI virtualization using the 8-bit vector in byte 0 of VICR and the 32-bit APIC ID in VICR[63:32] (see Section 30.1.6). Otherwise, the logical processor causes an APIC-write VM exit (see Section 30.4.3.3). If ECX contains 830H, the processor then checks the value of VICR to determine whether the following are all true:

I.e. the "unused" busy bit must be zero. The part that makes me grumpy is that hardware does check that other reserved bits are actually zero:

If special processing applies, no general-protection exception is produced due
to the fact that the local APIC is in xAPIC mode. However, WRMSR does perform
the normal reserved-bit checking:
- If ECX contains 808H or 83FH, a general-protection fault occurs if either EDX or EAX[31:8] is non-zero.
- If ECX contains 80BH, a general-protection fault occurs if either EDX or EAX is non-zero.
- If ECX contains 830H, a general-protection fault occurs if any of bits 31:20, 17:16, or 13 of EAX is non-zero.

Which implies that the hardware *does* enforce all the other reserved bits, but punted bit 12 to the hypervisor :-(

That said, I think we have an "out". Under the x2APIC section, regarding ICR, the SDM also says:

It remains readable only to aid in debugging; however, software should not assume the value returned by reading the ICR is the last written value.

I.e. KVM basically has free reign to do whatever it wants, so long as it doesn't confuse userspace or break KVM's ABI. As much as I want to say "do nothing", clearing bit 12 so that it reads back as '0' is the safer approach. Just please don't add a new #define for, it's far easier to understand what's going on if we just use APIC_ICR_BUSY, especially given that I highly doubt the bit will actually be repurposed for something new.

FWIW, I also suspect that hardware isn't clearing the busy bit per se, I suspect that hardware simply reads the bit as zero.

Side topic, unless I'm blind, KVM is missing the reserved bits #GP checks for ICR bits bits 31:20, 17:16, and 13.

Tao Su

author

2023-09-07 16:41 UTC #12 lore raw

Re: [PATCH 2/2] KVM: x86: Clear X2APIC_ICR_UNUSED_12 after APIC-write VM-exit

Quoted 73 lines · Sean Christopherson

Quoted 20 lines · Tao Su
Quoted 15 lines · Sean Christopherson
+Suravee
Quoted 11 lines · Tao Su
When IPI virtualization is enabled, a WARN is triggered if bit12 of ICR MSR is set after APIC-write VM-exit. The reason is kvm_apic_send_ipi() thinks the APIC_ICR_BUSY bit should be cleared because KVM has no delay, but kvm_apic_write_nodecode() doesn't clear the APIC_ICR_BUSY bit.
Since bit12 of ICR is no longer BUSY bit but UNUSED bit in x2APIC mode, and SDM has no detail about how hardware will handle the UNUSED bit12 set, we tested on Intel CPU (SRF/GNR) with IPI virtualization and found the UNUSED bit12 was also cleared by hardware without #GP. Therefore, the clearing of bit12 should be still kept being consistent with the hardware behavior.
Quoted 3 lines
I'm confused. If hardware clears the bit, then why is it set in the vAPIC page after a trap-like APIC-write VM-Exit? In other words, how is this not a ucode or hardware bug?
Quoted 6 lines
Sorry, I didn't describe it clearly.
On bare-metal, bit12 of ICR MSR will be cleared after setting this bit.
If bit12 is set in guest, the bit is not cleared in the vAPIC page after APIC-write VM-Exit. So whether to clear bit12 in vAPIC page needs to be considered.
I got that, the behavior just seems odd to me. And I'm grumpy that Intel punted the problem to software. But the SDM specifically calls out that this is the correct behavior :-/
Specifically, in the context of IPI virtualization:
If ECX contains 830H, a general-protection fault occurs if any of bits 31:20, 17:16, or 13 of EAX is non-zero.
and
If ECX contains 830H, the processor then checks the value of VICR to determine whether the following are all true:
Bits 19:18 (destination shorthand) are 00B (no shorthand). Bit 15 (trigger mode) is 0 (edge).
Bit 12 (unused) is 0.
Bit 11 (destination mode) is 0 (physical).
Bits 10:8 (delivery mode) are 000B (fixed).
If all of the items above are true, the processor performs IPI virtualization using the 8-bit vector in byte 0 of VICR and the 32-bit APIC ID in VICR[63:32] (see Section 30.1.6). Otherwise, the logical processor causes an APIC-write VM exit (see Section 30.4.3.3). If ECX contains 830H, the processor then checks the value of VICR to determine whether the following are all true:
I.e. the "unused" busy bit must be zero. The part that makes me grumpy is that hardware does check that other reserved bits are actually zero:
If special processing applies, no general-protection exception is produced due
to the fact that the local APIC is in xAPIC mode. However, WRMSR does perform
the normal reserved-bit checking:
- If ECX contains 808H or 83FH, a general-protection fault occurs if either EDX or EAX[31:8] is non-zero.
- If ECX contains 80BH, a general-protection fault occurs if either EDX or EAX is non-zero.
- If ECX contains 830H, a general-protection fault occurs if any of bits 31:20, 17:16, or 13 of EAX is non-zero.
Which implies that the hardware *does* enforce all the other reserved bits, but punted bit 12 to the hypervisor :-(
That said, I think we have an "out". Under the x2APIC section, regarding ICR, the SDM also says:
It remains readable only to aid in debugging; however, software should not assume the value returned by reading the ICR is the last written value.

Quoted 6 lines

I.e. KVM basically has free reign to do whatever it wants, so long as it doesn't confuse userspace or break KVM's ABI. As much as I want to say "do nothing", clearing bit 12 so that it reads back as '0' is the safer approach. Just please don't add a new #define for, it's far easier to understand what's going on if we just use APIC_ICR_BUSY, especially given that I highly doubt the bit will actually be repurposed for something new.

Thanks for such a detailed analysis. I will submit a patch to clear bit12 since it is a safer approach, and also drop the unnecessary alias.

Quoted 6 lines

FWIW, I also suspect that hardware isn't clearing the busy bit per se, I suspect that hardware simply reads the bit as zero.
Side topic, unless I'm blind, KVM is missing the reserved bits #GP checks for ICR bits bits 31:20, 17:16, and 13.

Yes, it is needed to check the reserved bits of ICR and inject #GP when IPI virtualization disabled (otherwise it is injected by hardware). The related kselftest is also necessary to verify the #GP is correctly emulated.

Thanks,
Tao

kernel test robot

build bot

2023-09-24 13:58 UTC #13 lore raw

Re: [PATCH 2/2] KVM: x86: Clear X2APIC_ICR_UNUSED_12 after APIC-write VM-exit

In reply to Tao Su

kernel test robot noticed "kernel-selftests.kvm.xapic_state_test.fail" on:

Hello,

kernel test robot noticed "kernel-selftests.kvm.xapic_state_test.fail" on:

commit: 6ccade077c151e719397b0a86f48554db9aa1c24 ("[PATCH 2/2] KVM: x86: Clear X2APIC_ICR_UNUSED_12 after APIC-write VM-exit")
url: https://github.com/intel-lab-lkp/linux/commits/Tao-Su/x86-apic-Introduce-X2APIC_ICR_UNUSED_12-for-x2APIC-mode/20230904-093801
patch link: https://lore.kernel.org/all/[email protected]/
patch subject: [PATCH 2/2] KVM: x86: Clear X2APIC_ICR_UNUSED_12 after APIC-write VM-exit

in testcase: kernel-selftests
version: kernel-selftests-x86_64-60acb023-1_20230329 with following parameters:

group: kvm

compiler: gcc-12
test machine: 224 threads 2 sockets Intel(R) Xeon(R) Platinum 8480+ (Sapphire Rapids) with 256G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)

besides, we also noticed kvm.smm_test.fail which keeps pass on parent.

83c02e23e21fec27 6ccade077c151e719397b0a86f4
---------------- ---------------------------
fail:runs %reproduction fail:runs
| | |
:6 100% 6:6 kernel-selftests.kvm.smm_test.fail
:6 100% 6:6 kernel-selftests.kvm.xapic_state_test.fail

# timeout set to 120
# selftests: kvm: smm_test
# ==== Test Assertion Failure ====
#   x86_64/smm_test.c:179: stage_reported == stage || stage_reported == SMRAM_STAGE
#   pid=6354 tid=6354 errno=4 - Interrupted system call
#      1	0x00000000004026fc: main at smm_test.c:179
#      2	0x00007f08310cb189: ?? ??:0
#      3	0x00007f08310cb244: ?? ??:0
#      4	0x0000000000402810: _start at ??:?
#   Unexpected stage: #3, got 4
not ok 25 selftests: kvm: smm_test # exit=254

...

# timeout set to 120
# selftests: kvm: xapic_state_test
# ==== Test Assertion Failure ====
#   x86_64/xapic_state_test.c:78: __a == __b
#   pid=7081 tid=7081 errno=4 - Interrupted system call
#      1	0x0000000000402a1f: ____test_icr at xapic_state_test.c:78
#      2	0x0000000000402c51: __test_icr at xapic_state_test.c:95 (discriminator 3)
#      3	 (inlined by) test_icr at xapic_state_test.c:106 (discriminator 3)
#      4	0x0000000000402433: main at xapic_state_test.c:197
#      5	0x00007f8a0c660189: ?? ??:0
#      6	0x00007f8a0c660244: ?? ??:0
#      7	0x0000000000402640: _start at ??:?
#   ASSERT_EQ(icr & ~APIC_ICR_BUSY, val & ~APIC_ICR_BUSY) failed.
# 	icr & ~APIC_ICR_BUSY is 0
# 	val & ~APIC_ICR_BUSY is 0x44000
not ok 47 selftests: kvm: xapic_state_test # exit=254

The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20230924/[email protected]

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki