Date:   Wed, 15 Dec 2021 18:53:16 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Paolo Bonzini <pbonzini@redhat.com>
Cc:     Christian Borntraeger <borntraeger@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        David Hildenbrand <david@redhat.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Maxim Levitsky <mlevitsk@redhat.com>,
        Ben Gardon <bgardon@google.com>,
        Lai Jiangshan <jiangshanlai@gmail.com>
Subject: Re: [PATCH 1/7] KVM: x86: Retry page fault if MMU reload is pending
 and root has no sp
Message-ID: <Ybo5nOu7/bVPhzCK@google.com>
References: <20211209060552.2956723-1-seanjc@google.com>
 <20211209060552.2956723-2-seanjc@google.com>
 <c94b3aec-981e-8557-ba29-0094b075b8e4@redhat.com>
 <YbN58FS67bEBOZZu@google.com>
 <8ab8833f-2a89-71ff-98da-2cfbb251736f@redhat.com>
 <YbOLRLEdfpl51QLS@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YbOLRLEdfpl51QLS@google.com>
Precedence: bulk

On Fri, Dec 10, 2021, Sean Christopherson wrote:
> On Fri, Dec 10, 2021, Paolo Bonzini wrote:
> > On 12/10/21 17:01, Sean Christopherson wrote:
> > > > KVM_REQ_MMU_RELOAD is raised after kvm->arch.mmu_valid_gen is fixed (of
> > > > course, otherwise the other CPU might just not see any obsoleted page
> > > > from the legacy MMU), therefore any check on KVM_REQ_MMU_RELOAD is just
> > > > advisory.
> > > 
> > > I disagree.  IMO, KVM should not be installing SPTEs into obsolete shadow pages,
> > > which is what continuing on allows.  I don't _think_ it's problematic, but I do
> > > think it's wrong.
> > > 
> > > [...] Eh, for all intents and purposes, KVM_REQ_MMU_RELOAD very much says
> > > special roots are obsolete.  The root will be unloaded, i.e. will no
> > > longer be used, i.e. is obsolete.
> > 
> > I understand that---but it takes some unspoken details to understand that.
> 
> Eh, it takes just as many unspoken details to understand why it's safe-ish to
> install SPTEs into an obsolete shadow page.
> 
> > In particular that both kvm_reload_remote_mmus and is_page_fault_stale are
> > called under mmu_lock write-lock, and that there's no unlock between
> > updating mmu_valid_gen and calling kvm_reload_remote_mmus.
> > 
> > (This also suggests, for the other six patches, keeping
> > kvm_reload_remote_mmus and just moving it to arch/x86/kvm/mmu/mmu.c, with an
> > assertion that the MMU lock is held for write).
> > 
> > But since we have a way forward for having no special roots to worry about,
> > it seems an unnecessary overload for 1) a patch that will last one or two
> > releasees at most 
> 
> Yeah, I don't disagree, which is why I'm not totally opposed to punting this and
> naturally fixing it by allocating shadow pages for the special roots.  But this
> code needs to be modified by Jiangshan's series either way, so it's not like we're
> saving anything meaningful.
> 
> > 2) a case that has been handled in the inefficient way forever.
> 
> I don't care about inefficiency, I'm worried about correctness.  It's extremely
> unlikely this fixes a true bug in the legacy MMU, but there's also no real
> downside to adding the check.
> 
> Anyways, either way is fine.

Ping, in case this dropped off your radar.  Regardless of how we fix this goof,
it needs to get fixed in 5.16.