Re: [PATCH 15/22] KVM: MMU: Introduce kvm_read_guest_page_x86()

[Joerg Roedel <joerg.roedel@amd.com>]

On Tue, Apr 27, 2010 at 04:35:06PM +0300, Avi Kivity wrote:
> On 04/27/2010 04:20 PM, Joerg Roedel wrote:

> >Hmm, difficult since both mmu's are active in the npt-npt case. The
> >arch.mmu struct contains mostly the l1 paging state initialized for
> >shadow paging and different set_cr3/get_cr3/inject_page_fault functions.
> >This keeps the changes to the mmu small and optimize for the common case
> >(a nested npt fault).
> 
> Well, it reduces the changes to the mmu, but it makes a 'struct
> kvm_mmu' incoherent since its meaning depends on whether it is
> nested or not.  For someone reading the code, it is hard to see when
> to use ->nested_mmu or ->mmu.
> 
> Perhaps have
> 
>    struct kvm_mmu base_mmu;
>    struct kvm_mmu nested_mmu;
>    struct kvm_mmu *mmu;
> 
> You could have one patch that mindlessly changes mmu. to mmu->.  The
> impact of the patchset increases, but I think the result is more
> readable.
> 
> It will be a pain adapting the patchset, but easier than updating
> mmu.txt to reflect the current situation.

Okay, I finally read most of mmu.txt :)
I agree that the implemented scheme for using arch.mmu and
arch.nested_mmu is non-obvious. The most complicated thing is that
.gva_to_gpa functions are exchanged between mmu and nested_mmu. This
means that nested_mmu.gva_to_gpa basically walks a page table in a mode
described by arch.mmu while mmu.gva_to_gpa walks the full
two-dimensional page table.
But I also don't yet see how a *mmu pointer would help here. From my
current understanding of the idea the only place to use it would be the
init_kvm_mmu path. But maybe we could also do this to make things more
clear:

	* Rename nested_mmu to l2_mmu and use it to save the current
	  paging mode of the l2 guest
	* Do not switch the the .gva_to_gpa function pointers between
	  mmu contexts and provide a wrapper function for all callers of
	  arch.mmu.gva_to_gpa which uses the right one for the current
	  context. The arch.nested flag is the indicator for which one
	  to use.

Btw. the purpose of the nested-flag is to prevent to reset arch.mmu when
the l2 guest changes his paging mode and we could remove this flag with
a pointer-solution.
Currently its a bit unclear when to use mmu or nested_mmu. With a
pointer it would be unclear to the code reader when to use the pointer
and when to select the mmu_contexts directly.

	Joerg