Re: [patch 09/10] KVM: MMU: out of sync shadow core v2

Avi Kivity
2008-09-20T01:22:52Z

Marcelo Tosatti wrote:
> static struct kmem_cache *rmap_desc_cache;
> @@ -942,6 +943,39 @@ static void nonpaging_invlpg(struct kvm_
> {
> }
>
> +static int mmu_unsync_walk(struct kvm_mmu_page *parent, mmu_unsync_fn fn,
> + void *priv)
>

Instead of private, have an object contain both callback and private
data, and use container_of(). Reduces the chance of type errors.

> +{
> + int i, ret;
> + struct kvm_mmu_page *sp = parent;
> +
> + while (parent->unsync_children) {
> + for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
> + u64 ent = sp->spt[i];
> +
> + if (is_shadow_present_pte(ent)) {
> + struct kvm_mmu_page *child;
> + child = page_header(ent & PT64_BASE_ADDR_MASK);
> +
> + if (child->unsync_children) {
> + sp = child;
> + break;
> + }
> + if (child->unsync) {
> + ret = fn(child, priv);
> + if (ret)
> + return ret;
> + }
> + }
> + }
> + if (i == PT64_ENT_PER_PAGE) {
> + sp->unsync_children = 0;
> + sp = parent;
> + }
> + }
> + return 0;
> +}
>

What does this do?

> +static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
> +{
> + if (sp->role.glevels != vcpu->arch.mmu.root_level) {
> + kvm_mmu_zap_page(vcpu->kvm, sp);
> + return 1;
> + }
>

Suppose we switch to real mode, touch a pte, switch back. Is this handled?

> @@ -991,8 +1066,18 @@ static struct kvm_mmu_page *kvm_mmu_get_
> gfn, role.word);
> index = kvm_page_table_hashfn(gfn);
> bucket = &vcpu->kvm->arch.mmu_page_hash[index];
> - hlist_for_each_entry(sp, node, bucket, hash_link)
> - if (sp->gfn == gfn && sp->role.word == role.word) {
> + hlist_for_each_entry_safe(sp, node, tmp, bucket, hash_link)
> + if (sp->gfn == gfn) {
> + if (sp->unsync)
> + if (kvm_sync_page(vcpu, sp))
> + continue;
> +
> + if (sp->role.word != role.word)
> + continue;
> +
> + if (sp->unsync_children)
> + vcpu->arch.mmu.need_root_sync = 1;
>

mmu_reload() maybe?

> static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp)
> {
> + int ret;
> ++kvm->stat.mmu_shadow_zapped;
> + ret = mmu_zap_unsync_children(kvm, sp);
> kvm_mmu_page_unlink_children(kvm, sp);
> kvm_mmu_unlink_parents(kvm, sp);
> kvm_flush_remote_tlbs(kvm);
> if (!sp->role.invalid && !sp->role.metaphysical)
> unaccount_shadowed(kvm, sp->gfn);
> + if (sp->unsync)
> + kvm_unlink_unsync_page(kvm, sp);
> if (!sp->root_count) {
> hlist_del(&sp->hash_link);
> kvm_mmu_free_page(kvm, sp);
> @@ -1129,7 +1245,7 @@ static int kvm_mmu_zap_page(struct kvm *
> kvm_reload_remote_mmus(kvm);
> }
> kvm_mmu_reset_last_pte_updated(kvm);
> - return 0;
> + return ret;
> }
>

Why does the caller care if zap also zapped some other random pages? To
restart walking the list?

>
> +
> +static int kvm_unsync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
> +{
> + unsigned index;
> + struct hlist_head *bucket;
> + struct kvm_mmu_page *s;
> + struct hlist_node *node, *n;
> +
> + index = kvm_page_table_hashfn(sp->gfn);
> + bucket = &vcpu->kvm->arch.mmu_page_hash[index];
> + /* don't unsync if pagetable is shadowed with multiple roles */
> + hlist_for_each_entry_safe(s, node, n, bucket, hash_link) {
> + if (s->gfn != sp->gfn || s->role.metaphysical)
> + continue;
> + if (s->role.word != sp->role.word)
> + return 1;
> + }
>

This will happen for nonpae paging. But why not allow it? Zap all
unsynced pages on mode switch.

Oh, if a page is both a page directory and page table, yes. So to allow
nonpae oos, check the level instead.

--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.