Hardware would directly write x2APIC ICR register instead of software
emulation in some circumstances, e.g when Intel IPI virtualization is
enabled. This behavior requires normal reserved bits checking to ensure
them input as zero, otherwise it will cause #GP. So we need mask out
those reserved bits from the data written to vICR register.

Remove Delivery Status bit emulation in test case as this flag
is invalid and not needed in x2APIC mode. KVM may ignore clearing
it during interrupt dispatch which will lead to fake test failure.

Opportunstically correct vector number for test sending IPI to
non-existent vCPUs.

Signed-off-by: Zeng Guang <guang.zeng@intel.com>
---
 .../selftests/kvm/x86_64/xapic_state_test.c   | 20 ++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/xapic_state_test.c b/tools/testing/selftests/kvm/x86_64/xapic_state_test.c
index 0792334ba243..df916c6f53f9 100644
--- a/tools/testing/selftests/kvm/x86_64/xapic_state_test.c
+++ b/tools/testing/selftests/kvm/x86_64/xapic_state_test.c
@@ -70,13 +70,27 @@ static void ____test_icr(struct kvm_vm *vm, struct kvm_vcpu *vcpu, uint64_t val)
 	vcpu_ioctl(vm, vcpu->id, KVM_GET_LAPIC, &xapic);
 	icr = (u64)(*((u32 *)&xapic.regs[APIC_ICR])) |
 	      (u64)(*((u32 *)&xapic.regs[APIC_ICR2])) << 32;
-	if (!vcpu->is_x2apic)
+	if (!vcpu->is_x2apic) {
 		val &= (-1u | (0xffull << (32 + 24)));
-	ASSERT_EQ(icr, val & ~APIC_ICR_BUSY);
+		ASSERT_EQ(icr, val & ~APIC_ICR_BUSY);
+	} else {
+		ASSERT_EQ(icr & ~APIC_ICR_BUSY, val & ~APIC_ICR_BUSY);
+	}
 }
 
+#define X2APIC_RSVED_BITS_MASK  (GENMASK_ULL(31,20) | \
+				 GENMASK_ULL(17,16) | \
+				 GENMASK_ULL(13,13))
+
 static void __test_icr(struct kvm_vm *vm, struct kvm_vcpu *vcpu, uint64_t val)
 {
+	if (vcpu->is_x2apic) {
+		/* Hardware writing vICR register requires reserved bits 31:20,
+		 * 17:16 and 13 kept as zero to avoid #GP exception. Data value
+		 * written to vICR should mask out those bits above.
+		 */
+		val &= ~X2APIC_RSVED_BITS_MASK;
+	}
 	____test_icr(vm, vcpu, val | APIC_ICR_BUSY);
 	____test_icr(vm, vcpu, val & ~(u64)APIC_ICR_BUSY);
 }
@@ -100,7 +114,7 @@ static void test_icr(struct kvm_vm *vm, struct kvm_vcpu *vcpu)
 	icr = APIC_INT_ASSERT | 0xff;
 	for (i = vcpu->id + 1; i < 0xff; i++) {
 		for (j = 0; j < 8; j++)
-			__test_icr(vm, vcpu, i << (32 + 24) | APIC_INT_ASSERT | (j << 8));
+			__test_icr(vm, vcpu, i << (32 + 24) | icr | (j << 8));
 	}
 
 	/* And again with a shorthand destination for all types of IPIs. */
-- 
2.27.0

Queued, thanks.

Paolo

On Thu, Jun 23, 2022 at 05:45:11PM +0800, Zeng Guang wrote:
>Hardware would directly write x2APIC ICR register instead of software
>emulation in some circumstances, e.g when Intel IPI virtualization is
>enabled. This behavior requires normal reserved bits checking to ensure
>them input as zero, otherwise it will cause #GP. So we need mask out
>those reserved bits from the data written to vICR register.

OK. One open is:

Current KVM doesn't emulate this #GP. Is there any historical reason?
if no, we will fix KVM and add some tests to verify this #GP is
correctly emulated.

>
>Remove Delivery Status bit emulation in test case as this flag
>is invalid and not needed in x2APIC mode. KVM may ignore clearing
>it during interrupt dispatch which will lead to fake test failure.
>
>Opportunstically correct vector number for test sending IPI to
>non-existent vCPUs.
>
>Signed-off-by: Zeng Guang <guang.zeng@intel.com>
>---
> .../selftests/kvm/x86_64/xapic_state_test.c   | 20 ++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
>diff --git a/tools/testing/selftests/kvm/x86_64/xapic_state_test.c b/tools/testing/selftests/kvm/x86_64/xapic_state_test.c
>index 0792334ba243..df916c6f53f9 100644
>--- a/tools/testing/selftests/kvm/x86_64/xapic_state_test.c
>+++ b/tools/testing/selftests/kvm/x86_64/xapic_state_test.c
>@@ -70,13 +70,27 @@ static void ____test_icr(struct kvm_vm *vm, struct kvm_vcpu *vcpu, uint64_t val)
> 	vcpu_ioctl(vm, vcpu->id, KVM_GET_LAPIC, &xapic);
> 	icr = (u64)(*((u32 *)&xapic.regs[APIC_ICR])) |
> 	      (u64)(*((u32 *)&xapic.regs[APIC_ICR2])) << 32;
>-	if (!vcpu->is_x2apic)
>+	if (!vcpu->is_x2apic) {
> 		val &= (-1u | (0xffull << (32 + 24)));
>-	ASSERT_EQ(icr, val & ~APIC_ICR_BUSY);
>+		ASSERT_EQ(icr, val & ~APIC_ICR_BUSY);
>+	} else {

>+		ASSERT_EQ(icr & ~APIC_ICR_BUSY, val & ~APIC_ICR_BUSY);

Probably add a comment for it would be better. E.g.,

APIC_ICR_BUSY is removed and not used when CPU is in x2APIC mode.
It is undefined whether write 1 to this bit will be preserved. So,
even KVM keeps this bit cleared in some cases even in x2apic mode,
no guarantee that hardware (specifically, CPU ucode when Intel IPI
virtualization enabled) will clear the bit. So, skip checking this
bit.

On 6/23/2022 6:33 PM, Gao, Chao wrote:
> On Thu, Jun 23, 2022 at 05:45:11PM +0800, Zeng Guang wrote:
>
>> +		ASSERT_EQ(icr & ~APIC_ICR_BUSY, val & ~APIC_ICR_BUSY);
> Probably add a comment for it would be better. E.g.,
>
> APIC_ICR_BUSY is removed and not used when CPU is in x2APIC mode.
> It is undefined whether write 1 to this bit will be preserved. So,
> even KVM keeps this bit cleared in some cases even in x2apic mode,
> no guarantee that hardware (specifically, CPU ucode when Intel IPI
> virtualization enabled) will clear the bit. So, skip checking this
> bit.
Hardware won't touch APIC_ICR_BUSY in x2apic mode. It totally depends on 
KVM to
clear it or not if set for test purpose. While in Intel IPI 
virtualization case,
KVM doesn't take care of this bit in vICR writes. So how about the 
comments as
below:

APIC_ICR_BUSY is removed and not used when CPU is in x2APIC mode.
KVM doesn't guarantee to clear this bit in some cases e.g. When
Intel IPI virtualization enabled, if it's set for test purpose.
So, skip checking this bit.

Thanks.
Zeng Guang

On Fri, Jun 24, 2022 at 12:28:38PM +0800, Zeng Guang wrote:
>
>On 6/23/2022 6:33 PM, Gao, Chao wrote:
>> On Thu, Jun 23, 2022 at 05:45:11PM +0800, Zeng Guang wrote:
>> 
>> > +		ASSERT_EQ(icr & ~APIC_ICR_BUSY, val & ~APIC_ICR_BUSY);
>> Probably add a comment for it would be better. E.g.,
>> 
>> APIC_ICR_BUSY is removed and not used when CPU is in x2APIC mode.
>> It is undefined whether write 1 to this bit will be preserved. So,
>> even KVM keeps this bit cleared in some cases even in x2apic mode,
>> no guarantee that hardware (specifically, CPU ucode when Intel IPI
>> virtualization enabled) will clear the bit. So, skip checking this
>> bit.
>Hardware won't touch APIC_ICR_BUSY in x2apic mode.

IMO, SDM doesn't say how the processor deals with this bit in x2apic
mode. Even if SPR behaves like this, the behavior isn't architectural.
Otherwise, KVM shouldn't touch this bit and we can add a test to verify
that the bit won't be changed by CPU (or KVM) in x2apic mode.

>It totally depends on KVM to clear it or not if set for test purpose.
>While in Intel IPI virtualization case, KVM doesn't take care of this
>bit in vICR writes.

I don't think KVM behavior is the key problem here. If an IPI is
virtualized by ucode, KVM isn't involved in processing the IPI.
It means KVM has no chance to clear the APIC_ICR_BUSY bit.

+Venkatesh

On Thu, Jun 23, 2022, Chao Gao wrote:
> On Thu, Jun 23, 2022 at 05:45:11PM +0800, Zeng Guang wrote:
> >Hardware would directly write x2APIC ICR register instead of software
> >emulation in some circumstances, e.g when Intel IPI virtualization is
> >enabled. This behavior requires normal reserved bits checking to ensure
> >them input as zero, otherwise it will cause #GP. So we need mask out
> >those reserved bits from the data written to vICR register.
> 
> OK. One open is:
> 
> Current KVM doesn't emulate this #GP. Is there any historical reason?
> if no, we will fix KVM and add some tests to verify this #GP is
> correctly emulated.

It's a bug.  There are patches posted[*], but they need to be refreshed to fix a
rebase goof.

Venkatesh, are you planning on sending a v3 soonish?

[*] https://lore.kernel.org/all/20220525173933.1611076-1-venkateshs@chromium.org

On 6/24/2022 4:34 AM, Sean Christopherson wrote:
> +Venkatesh
>
> On Thu, Jun 23, 2022, Chao Gao wrote:
>> On Thu, Jun 23, 2022 at 05:45:11PM +0800, Zeng Guang wrote:
>>> Hardware would directly write x2APIC ICR register instead of software
>>> emulation in some circumstances, e.g when Intel IPI virtualization is
>>> enabled. This behavior requires normal reserved bits checking to ensure
>>> them input as zero, otherwise it will cause #GP. So we need mask out
>>> those reserved bits from the data written to vICR register.
>> OK. One open is:
>>
>> Current KVM doesn't emulate this #GP. Is there any historical reason?
>> if no, we will fix KVM and add some tests to verify this #GP is
>> correctly emulated.
> It's a bug.  There are patches posted[*], but they need to be refreshed to fix a
> rebase goof.
>
> Venkatesh, are you planning on sending a v3 soonish?
>
> [*] https://lore.kernel.org/all/20220525173933.1611076-1-venkateshs@chromium.org

This patch set doesn't emulate hardware behavior precisely . Actually 
#GP will
happen only if any of reserved bit ( bit[31:20],bit[17:16],bit[13]) is 
1-setting
in x2apic mode. Other bits including bit[12] won't have any impact. For 
xapic
mode, it doesn't have this restriction.