linux-kernel - Re: [RFC v2] KVM: x86/vmx: Suppress posted interrupt notification when CPU is in host

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] [day] [month] [year] [list]

Message-ID: <YzQvWhr9nAeesRKy@gao-cwp>
Date:   Wed, 28 Sep 2022 19:26:18 +0800
From:   Chao Gao <chao.gao@...el.com>
To:     Sean Christopherson <seanjc@...gle.com>
CC:     <kvm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <jon@...anix.com>, <kevin.tian@...el.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [RFC v2] KVM: x86/vmx: Suppress posted interrupt notification
 when CPU is in host

On Tue, Sep 27, 2022 at 09:43:08PM +0000, Sean Christopherson wrote:
>The benefits I see are:
>
>  1. The code is very explicit.  When clearing SN, check PIR and set ON=1 to
>     ensure IRQs aren't ignored.
>
>  2. pi_enable_wakeup_handler() is only responsible for changing the vector,
>     i.e. there's no clearing of SN "hidden" in the CMPXCHG loop.
>
>  3. Same for vmx_vcpu_pi_load(), it's only responsible for updating the pCPU
>     and the vector, it doesn't touch SN.
>
>  4. The logic is common to all paths that clear SN, i.e. the same helper can
>     be used for both VM-Enter and vCPU blocking.
>
>E.g. the VMX hook for both VM-Enter and vCPU blocking could be:
>
>  static void vmx_no_idea_what_to_call_this(struct kvm_vcpu *vcpu)
>  {
>	pi_clear_sn(&vmx->pi_desc);
>
>	/*
>	 * Clear SN before reading the bitmap.  The VT-d firmware writes the
>	 * bitmap and reads SN atomically (5.2.3 in the spec), so it doesn't
>	 * really have a memory barrier that pairs with this, but we cannot do
>	 * that and we need one.
>	 */
>	smp_mb__after_atomic();
>
>	/* blah blah blah */
>	if (!pi_test_on(&vmx->pi_desc) && !pi_is_pir_empty(&vmx->pi_desc))
>		pi_set_on(&vmx->pi_desc);
>  }
>
>One related thought thing I want to capture:
>
>The proposed approach is having vmx_sync_pir_to_irr() check PIR if ON=1 || SN=1
>is effectively _required_ to avoid breaking halt-polling.  I was thinking we could
>keep that path optimized to check only ON=1, but with that approach, KVM won't detect
>the pending IRQ until it actually starts to block the vCPU, i.e. until it clears SN
>and manually checks the PIR.  Clearing SN in kvm_arch_vcpu_pending() would be better
>than waiting until the vCPU is "put", but even that is too late from a halt-polling
>perspective.
>
>A comment in vmx_sync_pir_to_irr() is likely needed to capture that.

All your comments make sense to me. I just posted v3 with all issues of v2
addressed.