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: <cover.1651774250.git.isaku.yamahata@intel.com>
 <bfa4f7415a1d059bd3a4c6d14105f2baf2d03ba6.1651774250.git.isaku.yamahata@intel.com>
 <YuxOHPpkhKnnstqw@google.com> <CALzav=cf_2dz8vMD+D_Xo1zBJZndJmtMBxbnYpQKP_mci1np=A@mail.gmail.com>
 <BL1PR11MB5978DB988E482B8329339881F79E9@BL1PR11MB5978.namprd11.prod.outlook.com>
In-Reply-To: <BL1PR11MB5978DB988E482B8329339881F79E9@BL1PR11MB5978.namprd11.prod.outlook.com>
From:   David Matlack <dmatlack@google.com>
Date:   Fri, 5 Aug 2022 09:46:06 -0700
Message-ID: <CALzav=cJ_Bp2Vg1n=aHv4ewH0U-rDGG5Nni=0CdizG-64GtpLA@mail.gmail.com>
Subject: Re: [RFC PATCH v6 037/104] KVM: x86/mmu: Allow non-zero value for
 non-present SPTE
To:     "Huang, Kai" <kai.huang@intel.com>
Cc:     "Yamahata, Isaku" <isaku.yamahata@intel.com>,
        kvm list <kvm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Isaku Yamahata <isaku.yamahata@gmail.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        "Aktas, Erdem" <erdemaktas@google.com>,
        "Christopherson,, Sean" <seanjc@google.com>,
        "Shahar, Sagi" <sagis@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Aug 4, 2022 at 5:04 PM Huang, Kai <kai.huang@intel.com> wrote:
>
> > > In addition to the suggestions above, I'd suggest breaking this patch
> > > up, since it is doing multiple things:
> > >
> > > 1. Patch initialize shadow page tables to EMPTY_SPTE (0) and
> > >    replace TDP MMU hard-coded 0 with EMPTY_SPTE.
> > > 2. Patch to change FNAME(sync_page) to not assume EMPTY_SPTE is 0.
> > > 3. Patch to set bit 63 in EMPTY_SPTE.
> > > 4. Patch to set bit 63 in REMOVED_SPTE.
>
> I think 1/2 can be separate patches, but 3/4 should be done together.
>
> Patch 3 alone is broken as when TDP MMU zaps SPTE and replaces it with REMOVED_SPTE, it loses bit 63.  This is not what we want.  We always want bit 63 set if it is supposed to be  set to a non-present SPTE.

How is patch 3 alone be broken? The TDX support that depends on bit 63
does not exist at this point in the series, i.e. setting bit 63 is
entirely optional and only done in preparation for future patches.

>
> But I also don't see splitting to 3  patches is absolutely worth to do as doing above in one patch is also fine to me.

Splitting patches up into logically independent changes makes it a lot
easier to review, and therefore reduces the chances of bugs.

Smaller changes also makes it easier for patches to get through the
review process, because reviewers can sign-off on specific patches
with Reviewed-by tags while discussion continues on patches that still
need more work. If the patches are too large, it makes it more
difficult to collect Reviewed-by tags because the entire patch has to
be correct.

Case in point, the above patch description has 9 paragraphs because
the patch is doing so many different things. It's difficult to keep
track of all of the different changes this patch aims to accomplish
when reviewing the code.