linux-kernel - Re: [PATCH v2] KVM: selftest: Enhance handling WRMSR ICR register in x2APIC mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220623103314.GA14006@gao-cwp>
Date:   Thu, 23 Jun 2022 18:33:19 +0800
From:   Chao Gao <chao.gao@...el.com>
To:     Zeng Guang <guang.zeng@...el.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Shuah Khan <shuah@...nel.org>, linux-kselftest@...r.kernel.org,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] KVM: selftest: Enhance handling WRMSR ICR register in
 x2APIC mode

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@...el.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.