Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220605064342.309219-1-jiangshanlai@gmail.com>
 <20220605064342.309219-6-jiangshanlai@gmail.com> <YtcLiNskPb8z/2Qc@google.com>
In-Reply-To: <YtcLiNskPb8z/2Qc@google.com>
From:   Lai Jiangshan <jiangshanlai@gmail.com>
Date:   Thu, 21 Jul 2022 17:32:53 +0800
Message-ID: <CAJhGHyAoM+6cOh7XQUvavgJcUts53FW6BnjM_wqMD6fkoYoB3w@mail.gmail.com>
Subject: Re: [PATCH 05/12] KVM: X86/MMU: Clear unsync bit directly in __mmu_unsync_walk()
To:     Sean Christopherson <seanjc@google.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        "open list:KERNEL VIRTUAL MACHINE FOR MIPS (KVM/mips)" 
        <kvm@vger.kernel.org>, Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Maxim Levitsky <mlevitsk@redhat.com>,
        Lai Jiangshan <jiangshan.ljs@antgroup.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jul 20, 2022 at 3:52 AM Sean Christopherson <seanjc@google.com> wrote:

> > ---
> >  arch/x86/kvm/mmu/mmu.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index f35fd5c59c38..2446ede0b7b9 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -1794,19 +1794,23 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp,
> >                               return -ENOSPC;
> >
> >                       ret = __mmu_unsync_walk(child, pvec);
> > -                     if (!ret) {
> > -                             clear_unsync_child_bit(sp, i);
> > -                             continue;
> > -                     } else if (ret > 0) {
> > -                             nr_unsync_leaf += ret;
> > -                     } else
> > +                     if (ret < 0)
> >                               return ret;
> > -             } else if (child->unsync) {
> > +                     nr_unsync_leaf += ret;
> > +             }
> > +
> > +             /*
> > +              * Clear unsync bit for @child directly if @child is fully
> > +              * walked and all the unsync shadow pages descended from
> > +              * @child (including itself) are added into @pvec, the caller
> > +              * must sync or zap all the unsync shadow pages in @pvec.
> > +              */
> > +             clear_unsync_child_bit(sp, i);
> > +             if (child->unsync) {
> >                       nr_unsync_leaf++;
> >                       if (mmu_pages_add(pvec, child, i))
>
> This ordering is wrong, no?  If the child itself is unsync and can't be added to
> @pvec, i.e. fails here, then clearing its bit in unsync_child_bitmap is wrong.

mmu_pages_add() can always successfully add the page to @pvec and
the caller needs to guarantee there is enough room to do so.

When it returns true, it means it will fail if you keep adding pages.

>
> I also dislike that that this patch obfuscates that a shadow page can't be unsync
> itself _and_ have unsync children (because only PG_LEVEL_4K can be unsync).  In
> other words, keep the
>
>         if (child->unsync_children) {
>
>         } else if (child->unsync) {
>
>         }
>

The code was not streamlined like this just because
I need to add some comments on clear_unsync_child_bit().

Duplicated clear_unsync_child_bit() would require
duplicated comments.  I will use "See above" instead.