On Tue, 2008-08-19 at 12:36 +0200, ehrhardt@xxxxxxxxxxxxxxxxxx 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(¤t->mm->mmap_sem);
> + pvmem_page = gfn_to_page(vcpu->kvm,
> + vcpu->arch.pvmem_gpaddr >> KVM_PPCPV_MAGIC_PAGE_SHIFT);
> + up_read(¤t->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
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
