From mboxrd@z Thu Jan  1 00:00:00 1970
From: Hollis Blanchard <hollisb@us.ibm.com>
Date: Wed, 20 Aug 2008 17:52:34 +0000
Subject: Re: [PATCH 2/6] kvmppc: magic page hypercall - host part
Message-Id: <1219254754.14362.85.camel@localhost.localdomain>
List-Id: <kvm-ppc.vger.kernel.org>
References: <1219142205-12062-3-git-send-email-ehrhardt@linux.vnet.ibm.com>
In-Reply-To: <1219142205-12062-3-git-send-email-ehrhardt@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: kvm-ppc@vger.kernel.org

On Tue, 2008-08-19 at 12:36 +0200, ehrhardt@linux.vnet.ibm.com wrote:
> diff --git a/arch/powerpc/kvm/booke_guest.c b/arch/powerpc/kvm/booke_guest.c
> --- a/arch/powerpc/kvm/booke_guest.c
> +++ b/arch/powerpc/kvm/booke_guest.c
> @@ -21,6 +21,7 @@
>  #include <linux/errno.h>
>  #include <linux/err.h>
>  #include <linux/kvm_host.h>
> +#include <linux/kvm_para.h>
>  #include <linux/module.h>
>  #include <linux/vmalloc.h>
>  #include <linux/fs.h>
> @@ -43,6 +44,7 @@
>  	{ "itlb_v",     VCPU_STAT(itlb_virt_miss_exits) },
>  	{ "dtlb_r",     VCPU_STAT(dtlb_real_miss_exits) },
>  	{ "dtlb_v",     VCPU_STAT(dtlb_virt_miss_exits) },
> +	{ "dtlb_pv",    VCPU_STAT(dtlb_pvmem_miss_exits) },
>  	{ "sysc",       VCPU_STAT(syscall_exits) },
>  	{ "isi",        VCPU_STAT(isi_exits) },
>  	{ "dsi",        VCPU_STAT(dsi_exits) },
> @@ -337,6 +339,16 @@
>  		unsigned long eaddr = vcpu->arch.fault_dear;
>  		gfn_t gfn;
> 
> +
> +		if (vcpu->arch.pvmem && kvmppc_is_pvmem(vcpu, eaddr)) {
> +			kvmppc_mmu_map(vcpu, eaddr,
> +			 vcpu->arch.pvmem_gpaddr >> KVM_PPCPV_MAGIC_PAGE_SHIFT,
> +			 0, KVM_PPCPV_MAGIC_PAGE_FLAGS);
> +			vcpu->stat.dtlb_pvmem_miss_exits++;
> +			r = RESUME_GUEST;
> +			break;
> +		}
> +
>  		/* Check the guest TLB. */
>  		gtlbe = kvmppc_44x_dtlb_search(vcpu, eaddr);
>  		if (!gtlbe) {

By the way, when this code is running, what's the rate of this new
counter? How does it compare to the reduction in instruction emulation?

> @@ -488,6 +500,8 @@
> 
>  	vcpu->arch.shadow_pid = 1;
> 
> +	vcpu->arch.pvmem = NULL;

Isn't the whole structure initialized to 0? I don't think this is
needed.

>  	/* Eye-catching number so we know if the guest takes an interrupt
>  	 * before it's programmed its own IVPR. */
>  	vcpu->arch.ivpr = 0x55550000;
> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
> --- a/arch/powerpc/kvm/emulate.c
> +++ b/arch/powerpc/kvm/emulate.c
> @@ -21,6 +21,7 @@
>  #include <linux/timer.h>
>  #include <linux/types.h>
>  #include <linux/string.h>
> +#include <linux/highmem.h>
>  #include <linux/kvm_host.h>
>  #include <linux/kvm_para.h>
> 
> @@ -195,7 +196,7 @@
>  		          get_jiffies_64() + nr_jiffies);
>  	} else {
>  		del_timer(&vcpu->arch.dec_timer);
> -	}
> +}
>  }

This looks wrong.

>  static void kvmppc_emul_rfi(struct kvm_vcpu *vcpu)
> @@ -207,8 +208,18 @@
>  static int kvmppc_do_hypercall(struct kvm_vcpu *vcpu)
>  {
>  	int ret = 0;
> +	struct page *pvmem_page;
> 
>  	switch (vcpu->arch.gpr[0]) {
> +	case KVM_HCALL_RESERVE_MAGICPAGE:
> +		vcpu->arch.pvmem_gvaddr = vcpu->arch.gpr[3];
> +		vcpu->arch.pvmem_gpaddr = vcpu->arch.gpr[4];
> +		down_read(&current->mm->mmap_sem);
> +		pvmem_page = gfn_to_page(vcpu->kvm, 
> +			vcpu->arch.pvmem_gpaddr >> KVM_PPCPV_MAGIC_PAGE_SHIFT);
> +		up_read(&current->mm->mmap_sem);
> +		vcpu->arch.pvmem = kmap(pvmem_page);
> +		break;
>  	default:
>  		printk(KERN_ERR "unknown hypercall %d\n", vcpu->arch.gpr[0]);
>  		kvmppc_dump_vcpu(vcpu);

Where is vcpu->arch.pvmem unmapped?

What happens if the guest makes repeated KVM_HCALL_RESERVE_MAGICPAGE
hypercalls? Looks like a good way to leak host memory.

Also, if we migrate a guest which has a page registered, the new host
won't have vcpu->arch.pvmem set because the guest doesn't re-invoke the
KVM_HCALL_RESERVE_MAGICPAGE hypercall.

I think we need to put a little more thought into all the corner cases
here.

-- 
Hollis Blanchard
IBM Linux Technology Center