| Message ID | 20220623094511.26066-1-guang.zeng@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] KVM: selftest: Enhance handling WRMSR ICR register in x2APIC mode | expand |
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.
+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.
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.
Queued, thanks. Paolo
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. */
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(-)