Re: [PATCH 13/22] KVM: X86: Add kvm_read_guest_page_tdp function

Joerg Roedel
2010-04-27T13:11:06Z

On Tue, Apr 27, 2010 at 03:42:27PM +0300, Avi Kivity wrote:
> On 04/27/2010 01:38 PM, Joerg Roedel wrote:
> >This patch adds a function which can read from the guests
> >physical memory or from the guest's guest physical memory.
> >This will be used in the two-dimensional page table walker.
> >
> >Signed-off-by: Joerg Roedel<joerg.roedel@amd.com>
> >---
> > arch/x86/include/asm/kvm_host.h | 3 +++
> > arch/x86/kvm/x86.c | 24 ++++++++++++++++++++++++
> > 2 files changed, 27 insertions(+), 0 deletions(-)
> >
> >diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> >index 3cbfb51..7851bbc 100644
> >--- a/arch/x86/include/asm/kvm_host.h
> >+++ b/arch/x86/include/asm/kvm_host.h
> >@@ -635,6 +635,9 @@ void kvm_requeue_exception(struct kvm_vcpu *vcpu, unsigned nr);
> > void kvm_requeue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code);
> > void kvm_inject_page_fault(struct kvm_vcpu *vcpu, unsigned long cr2,
> > u32 error_code);
> >+int kvm_read_guest_page_tdp(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> >+ gfn_t gfn, void *data, int offset, int len,
> >+ u32 *error);
> > bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
> >
> > int kvm_pic_set_irq(void *opaque, int irq, int level);
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index 6b2ce1d..558d995 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -356,6 +356,30 @@ bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl)
> > EXPORT_SYMBOL_GPL(kvm_require_cpl);
> >
> > /*
> >+ * This function will be used to read from the physical memory of the currently
> >+ * running guest. The difference to kvm_read_guest_page ist that this function
> >+ * can read from guest physical or from the guest's guest physical memory.
> >+ */
>
> s/ist/is/

This is a common typo error I make ;) I'll fix it.

> >+int kvm_read_guest_page_tdp(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> >+ gfn_t gfn, void *data, int offset, int len,
> >+ u32 *error)
>
> Naming: I see 'tdp' as a host property, and this is valid whether
> tdp is enabled or not. Suggest calling it
> kvm_read_nested_guest_page().
>
> Following mmu.txt, the parameter would be ngfn, not gfn.
>
> >+{
> >+ gfn_t real_gfn;
> >+ gpa_t gpa;
> >+
> >+ *error = 0;
> >+ gpa = gfn<< PAGE_SHIFT;
> >+ real_gfn = mmu->translate_gpa(vcpu, gpa, error);
>
> Overflow: sizeof(gpa) > sizeof(gfn).
>
> >+ if (real_gfn == UNMAPPED_GVA)
> >+ return -EFAULT;
> >+
> >+ real_gfn>>= PAGE_SHIFT;
>
> gpa_to_gfn().

I'll fix the overflow errors and send an updated patch.

Thanks,
Joerg