Messages in this thread | | | Subject | Re: [PATCH 09/10 RFC] KVM: x86: MMU: Move parent_pte handling from kvm_mmu_get_page() to link_shadow_page() | From | Paolo Bonzini <> | Date | Fri, 13 Nov 2015 11:51:06 +0100 |
| |
On 13/11/2015 03:15, Takuya Yoshikawa wrote: > Actually, I don't understand why this is named kvm_mmu_put_page() for > just removing parent_pte pointer from the sp->parent_ptes pointer chain.
Because it undoes kvm_mmu_get_page, I guess. :)
> >> On to kvm_mmu_get_page... >> >> if (!direct) { >> if (rmap_write_protect(vcpu, gfn)) >> kvm_flush_remote_tlbs(vcpu->kvm); >> if (level > PT_PAGE_TABLE_LEVEL && need_sync) >> kvm_sync_pages(vcpu, gfn); >> >> This seems fishy. >> >> need_sync is set if sp->unsync, but then the parents have not been >> unsynced yet. > > Reaching here means that kvm_mmu_get_page() could not return sp > from inside the for_each_gfn_sp() loop above, so even without > this patch, mark_unsync() has not been called.
You're right.
> Here, sp holds the new page allocated by kvm_mmu_alloc_page(). > One confusing thing is that hlist_add_head() right before this > "if (!direct)" line has already added the new sp to the hash > list, so it will be found by for_each_gfn_indirect_valid_sp() > in kvm_sync_pages(). > > Because this sp is new and sp->unsync is not set, kvm_sync_pages() > will just skip it and look for other sp's whose ->unsync were found > to be set in the for_each_gfn_sp() loop. > > I'm not 100% sure if the existence of the parent_pte pointer in the > newly created sp->parent_ptes chain alone makes any difference:
No, I don't think so. Nothing needs the parent_ptes at this point:
- kvm_mmu_mark_parents_unsync, even in the existing code, it's called before the new SPTE is created.
- as you said, kvm_mmu_prepare_zap_page can be called by kvm_sync_pages but it will not operate on this page because its ->unsync is zero.
> So, "bool accessed" needs to be passed to kvm_mmu_get_page().
The "bool accessed" parameter is not necessary, I think. It is only false in the nested EPT case, and there's no reason not to set the accessed bit *in the shadow page* if the host supports EPT accessed/dirty bits. I'll test and send a patch to remove the argument.
> But any way, we need to understand if mmu_page_add_parent_pte() > really needs to be placed before the "if (!direct)" block.
No, I don't think so anymore.
I think these patches are fine as a starting point for further cleanups, I'll push them to kvm/queue very soon.
Paolo
| |