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

Joerg Roedel
2010-04-27T15:40:43Z

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