* Takuya Yoshikawa <takuya.yoshikawa@xxxxxxxxx> wrote:
> +/*
> + * do_walk() returns one of these.
> + *
> + * WALK_NEXT: Continue the walk loop.
> + * WALK_DONE: Break from the walk loop.
> + * WALK_RETRY: Retry walk.
> + * WALK_NOT_PRESENT: Set PFERR_PRESENT_MASK and goto error.
> + * WALK_RSVD_FAULT: Set PFERR_RSVD_MASK and goto error.
> + * WALK_ERROR: Goto error.
> + * WALK_ABORT: Return immediately.
hm, this iterator turned out to be more complex than i thought it
would become. Avi, are you still happy with that?
> + if (!*eperm && unlikely(!(*pte & PT_ACCESSED_MASK))) {
> + int ret;
> +
> + trace_kvm_mmu_set_accessed_bit(table_gfn, index, sizeof(*pte));
> + ret = FNAME(cmpxchg_gpte)(vcpu, mmu, *ptep_user, index,
> + *pte, *pte|PT_ACCESSED_MASK);
> + if (unlikely(ret < 0))
> + return WALK_NOT_PRESENT;
> + else if (ret)
> + return WALK_RETRY;
> +
> + mark_page_dirty(vcpu->kvm, table_gfn);
> + *pte |= PT_ACCESSED_MASK;
> + }
This wants to move into a set-accessed-bit helper inline.
> + if ((walker->level == PT_PAGE_TABLE_LEVEL) ||
> + ((walker->level == PT_DIRECTORY_LEVEL) && is_large_pte(*pte) &&
> + (PTTYPE == 64 || is_pse(vcpu))) ||
> + ((walker->level == PT_PDPE_LEVEL) && is_large_pte(*pte) &&
> + (mmu->root_level == PT64_ROOT_LEVEL))) {
This condition wants to move into a is-pte-large inline function.
> + gpa_t real_gpa;
> + gfn_t gfn;
> + u32 ac;
> +
> + gfn = gpte_to_gfn_lvl(*pte, walker->level);
> + gfn += (addr & PT_LVL_OFFSET_MASK(walker->level)) >> PAGE_SHIFT;
> +
> + if (PTTYPE == 32 && (walker->level == PT_DIRECTORY_LEVEL) &&
> + is_cpuid_PSE36())
> + gfn += pse36_gfn_delta(*pte);
> +
> + ac = write_fault | fetch_fault | user_fault;
> +
> + real_gpa = mmu->translate_gpa(vcpu, gfn_to_gpa(gfn), ac);
> + if (real_gpa == UNMAPPED_GVA)
> + return WALK_ABORT;
> +
> + walker->gfn = real_gpa >> PAGE_SHIFT;
> +
> + return WALK_DONE;
And this would look cleaner if it was in a handle-large-pte inline function?
> + ret = FNAME(do_walk)(walker, vcpu, mmu, addr, access,
> + &eperm, &pte, &ptep_user);
> + switch (ret) {
> + case WALK_NEXT:
> + break;
> + case WALK_DONE:
> + goto walk_done;
> + case WALK_RETRY:
> + goto walk_retry;
> + case WALK_NOT_PRESENT:
> errcode |= PFERR_PRESENT_MASK;
> goto error;
> + case WALK_RSVD_FAULT:
> errcode |= PFERR_RSVD_MASK;
> goto error;
> + case WALK_ERROR:
> + goto error;
> + case WALK_ABORT:
> + return 0;
Btw., there's a stylistic trick you could use here to make the
iteration logic even clearer:
switch (ret) {
case WALK_NEXT: break;
case WALK_DONE: goto walk_done;
case WALK_RETRY: goto walk_retry;
case WALK_NOT_PRESENT: errcode |= PFERR_PRESENT_MASK; goto error;
case WALK_RSVD_FAULT: errcode |= PFERR_RSVD_MASK; goto error;
case WALK_ERROR: goto error;
case WALK_ABORT: return 0;
}
But it's a pure matter of taste - it might not really fit into KVM
code. Avi's call :-)
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
