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.
