linux-kernel - Re: [PATCH v5 5/8] KVM: x86: Support interrupt dispatch in x2APIC mode with APIC-write VM exit

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 for Android: free password hash cracker in your pocket

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

Message-ID: <be6cb4b3-541c-8a8b-8d49-c53f84ac8a9c@intel.com>
Date:   Wed, 19 Jan 2022 10:48:46 +0800
From:   Zeng Guang <guang.zeng@...el.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "Luck, Tony" <tony.luck@...el.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Kim Phillips <kim.phillips@....com>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        Jethro Beekman <jethro@...tanix.com>,
        "Huang, Kai" <kai.huang@...el.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Hu, Robert" <robert.hu@...el.com>,
        "Gao, Chao" <chao.gao@...el.com>
Subject: Re: [PATCH v5 5/8] KVM: x86: Support interrupt dispatch in x2APIC
 mode with APIC-write VM exit

On 1/19/2022 2:17 AM, Sean Christopherson wrote:
> On Sat, Jan 15, 2022, Zeng Guang wrote:
>>> What about tweaking my prep patch from before to the below?  That would yield:
>>>
>>> 	if (apic_x2apic_mode(apic)) {
>>> 		if (WARN_ON_ONCE(offset != APIC_ICR))
>>> 			return 1;
>>>
>>> 		kvm_lapic_msr_read(apic, offset, &val);
>> I think it's problematic to use kvm_lapic_msr_read() in this case. It
>> premises the high 32bit value already valid at APIC_ICR2, while in handling
>> "nodecode" x2APIC writes we need get continuous 64bit data from offset 300H
>> first and prepare emulation of APIC_ICR2 write.
> Ah, I read this part of the spec:
>
>    All 64 bits of the ICR are written by using WRMSR to access the MSR with index 830H.
>    If ECX = 830H, WRMSR writes the 64-bit value in EDX:EAX to the ICR, causing the APIC
>    to send an IPI. If any of bits 13, 17:16, or 31:20 are set in EAX, WRMSR detects a
>    reserved-bit violation and causes a general-protection exception (#GP).
>
> but not the part down below that explicit says
>
>    VICR refers the 64-bit field at offset 300H on the virtual-APIC page. When the
>    “virtualize x2APIC mode” VM-execution control is 1 (indicating virtualization of
>    x2APIC mode), this field is used to virtualize the entire ICR.
>
> But that's indicative of an existing KVM problem.  KVM's emulation of x2APIC is
> broken.  The SDM, in section 10.12.9 ICR Operation in x2APIC Mode, clearly states
> that the ICR is extended to 64-bits.  ICR2 does not exist in x2APIC mode, full stop.
> KVM botched things by effectively aliasing ICR[63:32] to ICR2.
>
> We can and should fix that issue before merging IPIv suport, that way we don't
> further propagate KVM's incorrect behavior.  KVM will need to manipulate the APIC
> state in KVM_{G,S}ET_LAPIC so as not to "break" migration, "break" in quotes because
> I highly doubt any kernel reads ICR[63:32] for anything but debug purposes.  But
> we'd need to do that anyways for IPIv, otherwise migration from an IPIv host to
> a non-IPIv host would suffer the same migration bug.
>
> I'll post a series this week, in theory we should be able to reduce the patch for
> IPIv support to just having to only touching kvm_apic_write_nodecode().
OK, I'll adapt patch after your fix is ready. Suppose 
kvm_lapic_msr_{write,read} needn't emulate ICR2 write/read anymore.