Messages in this thread | | | Date | Tue, 27 Apr 2010 16:35:06 +0300 | From | Avi Kivity <> | Subject | Re: [PATCH 15/22] KVM: MMU: Introduce kvm_read_guest_page_x86() |
| |
On 04/27/2010 04:20 PM, Joerg Roedel wrote: > On Tue, Apr 27, 2010 at 03:52:37PM +0300, Avi Kivity wrote: > >> On 04/27/2010 01:38 PM, Joerg Roedel wrote: >> >>> This patch introduces the kvm_read_guest_page_x86 function >>> which reads from the physical memory of the guest. If the >>> guest is running in guest-mode itself with nested paging >>> enabled it will read from the guest's guest physical memory >>> instead. >>> The patch also changes changes the code to use this function >>> where it is necessary. >>> >>> >>> >>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h >>> index 7851bbc..d9dfc8c 100644 >>> --- a/arch/x86/include/asm/kvm_host.h >>> +++ b/arch/x86/include/asm/kvm_host.h >>> @@ -254,6 +254,13 @@ struct kvm_mmu { >>> union kvm_mmu_page_role base_role; >>> bool direct_map; >>> >>> + /* >>> + * If true the mmu runs in two-level mode. >>> + * vcpu->arch.nested_mmu needs to contain meaningful values in >>> + * this case. >>> + */ >>> + bool nested; >>> + >>> >> struct mmu_context *active_mmu? (in vcpu->arch) >> > 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.
> The arch.nested_mmu contains the l2 paging mode and is only used for > nested gva_to_gpa translations (thats the reason it is only partially > initialized). > > >>> u64 *pae_root; >>> u64 rsvd_bits_mask[2][4]; >>> }; >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>> index 558d995..317ad26 100644 >>> --- a/arch/x86/kvm/x86.c >>> +++ b/arch/x86/kvm/x86.c >>> @@ -379,6 +379,20 @@ int kvm_read_guest_page_tdp(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu, >>> } >>> EXPORT_SYMBOL_GPL(kvm_read_guest_page_tdp); >>> >>> +int kvm_read_guest_page_x86(struct kvm_vcpu *vcpu, gfn_t gfn, >>> + void *data, int offset, int len, u32 *error) >>> +{ >>> + struct kvm_mmu *mmu; >>> + >>> + if (vcpu->arch.mmu.nested) >>> + mmu =&vcpu->arch.nested_mmu; >>> + else >>> + mmu =&vcpu->arch.mmu; >>> + >>> + return kvm_read_guest_page_tdp(vcpu, mmu, gfn, data, offset, len, >>> + error); >>> +} >>> >> This is really not x86 specific (though the implementation certainly >> is). s390 will have exactly the same need when it gets nested virt. >> I think this can be folded into >> kvm_read_guest_page_tdp()/kvm_read_nested_guest_page(). >> > For the generic walk_addr I need a version of that function that takes > an mmu_context parameter. Thats the reason I made two functions. > The function (or at least its semantic) is useful for !x86 too, thats > right. But it currently can't be made generic because the MMU > implementation is architecture specific. Do you suggest to give it a > more generic name so we can move it later? >
kvm_read_guest_page_mmu() for the internal one, kvm_read_nested_guest_page() for the generic one?
-- error compiling committee.c: too many arguments to function
| |