Re: [PATCH 4/4] KVM: MMU: Split out the main body of walk_addr_generic()

[Ingo Molnar <mingo@elte.hu>]

* Takuya Yoshikawa <takuya.yoshikawa@gmail.com> 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