On Thu, Apr 28, 2022, Sagi Shahar wrote:
> > @@ -468,23 +503,49 @@ static void __handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> >
> > if (was_leaf && is_dirty_spte(old_spte) &&
> > (!is_present || !is_dirty_spte(new_spte) || pfn_changed))
> > - kvm_set_pfn_dirty(spte_to_pfn(old_spte));
> > + kvm_set_pfn_dirty(old_pfn);
> > +
> > + /*
> > + * Special handling for the private mapping. We are either
> > + * setting up new mapping at middle level page table, or leaf,
> > + * or tearing down existing mapping.
> > + */
> > + if (private_spte) {
> > + void *sept_page = NULL;
> > +
> > + if (is_present && !is_leaf) {
> > + struct kvm_mmu_page *sp = to_shadow_page(pfn_to_hpa(new_pfn));
> > +
> > + sept_page = kvm_mmu_private_sp(sp);
> > + WARN_ON(!sept_page);
> > + WARN_ON(sp->role.level + 1 != level);
> > + WARN_ON(sp->gfn != gfn);
> > + }
> > +
> > + static_call(kvm_x86_handle_changed_private_spte)(
> > + kvm, gfn, level,
> > + old_pfn, was_present, was_leaf,
> > + new_pfn, is_present, is_leaf, sept_page);
> > + }
> >
> > /*
> > * Recursively handle child PTs if the change removed a subtree from
> > * the paging structure.
> > */
> > - if (was_present && !was_leaf && (pfn_changed || !is_present))
> > + if (was_present && !was_leaf && (pfn_changed || !is_present)) {
> > + WARN_ON(private_spte !=
> > + is_private_spte(spte_to_child_pt(old_spte, level)));
This sanity check is pointless. The private flag comes from the parent shadow
page role, and that's not changing.
> > @@ -1015,6 +1137,12 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
> > is_large_pte(iter.old_spte)) {
> > if (!tdp_mmu_zap_spte_atomic(vcpu->kvm, &iter))
> > break;
> > + /*
> > + * TODO: large page support.
> > + * Doesn't support large page for TDX now
> > + */
> > + WARN_ON(is_private_spte(&iter.old_spte));
>
> The above line is causing a null ptr dereferencing when running the
> KVM unit tests.
> It should be is_private_spte(iter.sptep) instead of
> is_private_spte(&iter.old_spte)
> While old_spte holds a snapshot of the value pointed to by sptep,
> &old_spte is not equivalent to sptep.
Bug aside, the name is really, really bad. All of the existing helpers with an
"is_blah_spte()" name take an SPTE value, not a pointer to an SPTE.
is_private_sptep() is the obvious choice. That makes me a bit nervous too, and
I don't love having to go back to the parent to query private vs shared.
That said, I think it's worth waiting to see the next version of this series before
going behind the bikeshed, I suspect many/most of the calls will go away, i.e. we
might find a better option presents itself.
