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
Date: Fri, 10 Dec 2021 17:15:48 +0000 [thread overview]
Message-ID: <YbOLRLEdfpl51QLS@google.com> (raw)
In-Reply-To: <8ab8833f-2a89-71ff-98da-2cfbb251736f@redhat.com>
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.
next prev parent reply other threads:[~2021-12-10 17:15 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-09 6:05 [PATCH 0/7] KVM: x86/mmu: Obsolete root shadow page fix Sean Christopherson
2021-12-09 6:05 ` [PATCH 1/7] KVM: x86: Retry page fault if MMU reload is pending and root has no sp Sean Christopherson
2021-12-09 11:19 ` Paolo Bonzini
2021-12-10 12:41 ` Paolo Bonzini
2021-12-10 16:01 ` Sean Christopherson
2021-12-10 16:13 ` Paolo Bonzini
2021-12-10 17:15 ` Sean Christopherson [this message]
2021-12-15 18:53 ` Sean Christopherson
2021-12-19 18:41 ` Paolo Bonzini
2021-12-09 6:05 ` [PATCH 2/7] KVM: x86: Invoke kvm_mmu_unload() directly on CR4.PCIDE change Sean Christopherson
2021-12-09 6:05 ` [PATCH 3/7] KVM: Drop kvm_reload_remote_mmus(), open code request in x86 users Sean Christopherson
2021-12-09 6:05 ` [PATCH 4/7] KVM: x86/mmu: Zap only obsolete roots if a root shadow page is zapped Sean Christopherson
2021-12-09 6:05 ` [PATCH 5/7] KVM: s390: Replace KVM_REQ_MMU_RELOAD usage with arch specific request Sean Christopherson
2021-12-09 9:14 ` Claudio Imbrenda
2021-12-09 10:52 ` Janosch Frank
2021-12-09 6:05 ` [PATCH 6/7] KVM: Drop KVM_REQ_MMU_RELOAD and update vcpu-requests.rst documentation Sean Christopherson
2021-12-09 8:17 ` Claudio Imbrenda
2021-12-09 6:05 ` [PATCH 7/7] KVM: WARN if is_unsync_root() is called on a root without a shadow page Sean Christopherson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YbOLRLEdfpl51QLS@google.com \
--to=seanjc@google.com \
--cc=bgardon@google.com \
--cc=borntraeger@linux.ibm.com \
--cc=david@redhat.com \
--cc=frankja@linux.ibm.com \
--cc=imbrenda@linux.ibm.com \
--cc=jiangshanlai@gmail.com \
--cc=jmattson@google.com \
--cc=joro@8bytes.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mlevitsk@redhat.com \
--cc=pbonzini@redhat.com \
--cc=vkuznets@redhat.com \
--cc=wanpengli@tencent.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox