linux-kernel - Re: [PATCH 5/6] KVM: x86: never clear irr_pending in kvm_apic_update

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: <be325dd1fbbf9023fb6ce3ceebf0418f631d43c1.camel@redhat.com>
Date:   Thu, 09 Dec 2021 17:03:48 +0200
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org
Cc:     "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" 
        <linux-kernel@...r.kernel.org>, Wanpeng Li <wanpengli@...cent.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Joerg Roedel <joro@...tes.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Borislav Petkov <bp@...en8.de>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jim Mattson <jmattson@...gle.com>,
        Sean Christopherson <seanjc@...gle.com>
Subject: Re: [PATCH 5/6] KVM: x86: never clear irr_pending in
 kvm_apic_update_apicv

On Thu, 2021-12-09 at 15:12 +0100, Paolo Bonzini wrote:
> On 12/9/21 12:54, Maxim Levitsky wrote:
> > Also reorder call to kvm_apic_update_apicv to be after
> > .refresh_apicv_exec_ctrl, although that doesn't guarantee
> > that it will see up to date IRR bits.
> 
> Can you spell out why do that?

Here is what I seen happening during kvm_vcpu_update_apicv when we about to disable AVIC:

1. we call kvm_apic_update_apicv which sets irr_pending == false,
because there is nothing in IRR yet.

2. we call kvm_x86_refresh_apicv_exec_ctrl which disables AVIC

If IPI arrives in between 1 and 2, the IRR bits are set, and legit there
is no VMexit happening so no chance of irr_pending to be set to true.

This is why I reordered those calls and added a memory barrier between them
(but I didn't post it in the series)

However I then found out that even with incomplete IPI handler setting irr_pending,
I can here observe irr_pending = true but no bits in IRR so the kvm_apic_update_apicv
would reset it. I expected VM exit to be write barrier but it seems that it isn't.

However I ended up fixing the incomplete IPI handler to just always 
	- set irr_pending
	- raise KVM_REQ_EVENT
	- kick the vcpu

Because kicking a sleeping vCPU is just waking it up,
and otherwise vcpu kick only sends IPI when the target vCPU
is in guest mode anyway.

That I think ensures for good that interrupt will be processed by
this vCPU regardless of order of these calls, and barrier between them.

The only thing I kept is that make kvm_apic_update_apicv never clear
irr_pending to make sure it doesn't reset it if it sees the writes out of order.

Later the KVM_REQ_EVENT should see writes in order because kvm_make_request
includes a write barrier, and the kick should ensure that the vCPU will
process that request.

So in summary this reorder is not needed anymore but it seems more logical
to scan IRR after we disable AVIC.
Or on the second though I think we should drop the IRR scan from here at all,
now that the callers do vcpu kicks.

Best regards,
	Maxim Levitsky

> 
> Paolo
>