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

Avi Kivity
2010-04-27T13:35:10Z

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