Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <8ab8833f-2a89-71ff-98da-2cfbb251736f@redhat.com>
Date:   Fri, 10 Dec 2021 17:13:29 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.0
Subject: Re: [PATCH 1/7] KVM: x86: Retry page fault if MMU reload is pending
 and root has no sp
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.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>
References: <20211209060552.2956723-1-seanjc@google.com>
 <20211209060552.2956723-2-seanjc@google.com>
 <c94b3aec-981e-8557-ba29-0094b075b8e4@redhat.com>
 <YbN58FS67bEBOZZu@google.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
In-Reply-To: <YbN58FS67bEBOZZu@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.  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 2) a case that has been handled in the 
inefficient way forever.

Paolo

> The other way to check for an invalid special root would be to treat
> it as obsolete if any of its children in entries 0-3 are present and
> obsolete.  That would be more precise, but it provides no benefit
> given KVM's current implementation.
> 
> I'm not completely opposed to doing nothing, but I do think it's
> silly to continue on knowing that the work done by the page fault is
> all but gauranteed to be useless.
>