Re: [RFC PATCH v6 037/104] KVM: x86/mmu: Allow non-zero value for non-present SPTE

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Aug 4, 2022 at 5:04 PM Huang, Kai <kai.huang@xxxxxxxxx> 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.



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux