On Tue, Jan 4, 2022 at 11:55 PM Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> On Mon, Dec 13, 2021 at 10:59:14PM +0000, David Matlack wrote:
> > When dirty logging is enabled without initially-all-set, attempt to
> > split all huge pages in the memslot down to 4KB pages so that vCPUs
> > do not have to take expensive write-protection faults to split huge
> > pages.
> >
> > Huge page splitting is best-effort only. This commit only adds the
> > support for the TDP MMU, and even there splitting may fail due to out
> > of memory conditions. Failures to split a huge page is fine from a
> > correctness standpoint because we still always follow it up by write-
> > protecting any remaining huge pages.
> >
> > Signed-off-by: David Matlack <dmatlack@xxxxxxxxxx>
>
> Thanks for adding the knob.
>
> Reviewed-by: Peter Xu <peterx@xxxxxxxxxx>
>
> One trivial nitpick below:
>
> > +u64 make_huge_page_split_spte(u64 huge_spte, int huge_level, int index, unsigned int access)
> > +{
> > + u64 child_spte;
> > + int child_level;
> > +
> > + if (WARN_ON(is_mmio_spte(huge_spte)))
> > + return 0;
> > +
> > + if (WARN_ON(!is_shadow_present_pte(huge_spte)))
> > + return 0;
> > +
> > + if (WARN_ON(!is_large_pte(huge_spte)))
> > + return 0;
> > +
> > + child_spte = huge_spte;
> > + child_level = huge_level - 1;
> > +
> > + /*
> > + * The child_spte already has the base address of the huge page being
> > + * split. So we just have to OR in the offset to the page at the next
> > + * lower level for the given index.
> > + */
> > + child_spte |= (index * KVM_PAGES_PER_HPAGE(child_level)) << PAGE_SHIFT;
> > +
> > + if (child_level == PG_LEVEL_4K) {
> > + child_spte &= ~PT_PAGE_SIZE_MASK;
> > +
> > + /* Allow execution for 4K pages if it was disabled for NX HugePages. */
> > + if (is_nx_huge_page_enabled() && access & ACC_EXEC_MASK)
>
> IMHO clearer to use brackets ("A && (B & C)").
Agreed.
>
> I don't even see anywhere that the tdp mmu disables the EXEC bit for 4K.. if
> that's true then perhaps we can even drop "access" and this check? But I could
> have missed something.
TDP MMU always passes ACC_ALL so the access check could be omitted
from this patch. But it will be needed to support eager splitting for
the shadow MMU, which does not always allow execution.
>
> > + child_spte = mark_spte_executable(child_spte);
> > + }
> > +
> > + return child_spte;
> > +}
>
> --
> Peter Xu
>
