[PATCH 0 of 5] PowerPC KVM fixes for 2.6.26

[PATCH 0 of 5] PowerPC KVM fixes for 2.6.26

[Hollis Blanchard]

Hi Avi, these bug fixes should be committed for 2.6.26. Most are pretty
harmless. Although the mmap_sem locking problem is fatal if you hit it, in
practice we simply shouldn't hit that error case anyways.

-Hollis

[PATCH 1 of 5] KVM: ppc: Remove duplicate function

[Hollis Blanchard]

This was left behind from some code movement.

Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>

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
@@ -225,39 +225,6 @@ void kvmppc_check_and_deliver_interrupts
 		                         BITS_PER_BYTE * sizeof(*pending),
 		                         priority + 1);
 	}
-}
-
-static int kvmppc_emulate_mmio(struct kvm_run *run, struct kvm_vcpu *vcpu)
-{
-	enum emulation_result er;
-	int r;
-
-	er = kvmppc_emulate_instruction(run, vcpu);
-	switch (er) {
-	case EMULATE_DONE:
-		/* Future optimization: only reload non-volatiles if they were
-		 * actually modified. */
-		r = RESUME_GUEST_NV;
-		break;
-	case EMULATE_DO_MMIO:
-		run->exit_reason = KVM_EXIT_MMIO;
-		/* We must reload nonvolatiles because "update" load/store
-		 * instructions modify register state. */
-		/* Future optimization: only reload non-volatiles if they were
-		 * actually modified. */
-		r = RESUME_HOST_NV;
-		break;
-	case EMULATE_FAIL:
-		/* XXX Deliver Program interrupt to guest. */
-		printk(KERN_EMERG "%s: emulation failed (%08x)\n", __func__,
-		       vcpu->arch.last_inst);
-		r = RESUME_HOST;
-		break;
-	default:
-		BUG();
-	}
-
-	return r;
 }
 
 /**
diff --git a/include/asm-powerpc/kvm_ppc.h b/include/asm-powerpc/kvm_ppc.h
--- a/include/asm-powerpc/kvm_ppc.h
+++ b/include/asm-powerpc/kvm_ppc.h
@@ -57,6 +57,7 @@ extern int kvmppc_handle_store(struct kv
 
 extern int kvmppc_emulate_instruction(struct kvm_run *run,
                                       struct kvm_vcpu *vcpu);
+extern int kvmppc_emulate_mmio(struct kvm_run *run, struct kvm_vcpu *vcpu);
 
 extern void kvmppc_mmu_map(struct kvm_vcpu *vcpu, u64 gvaddr, gfn_t gfn,
                            u64 asid, u32 flags);

[PATCH 2 of 5] KVM: ppc: add lwzx/stwz emulation

[Hollis Blanchard]

Somehow these load/store instructions got missed before, but weren't used by
the guest so didn't break anything.

Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
Signed-off-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>

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
@@ -246,6 +246,11 @@ int kvmppc_emulate_instruction(struct kv
 	case 31:
 		switch (get_xop(inst)) {
 
+		case 23:                                        /* lwzx */
+			rt = get_rt(inst);
+			emulated = kvmppc_handle_load(run, vcpu, rt, 4, 1);
+			break;
+
 		case 83:                                        /* mfmsr */
 			rt = get_rt(inst);
 			vcpu->arch.gpr[rt] = vcpu->arch.msr;
@@ -265,6 +270,13 @@ int kvmppc_emulate_instruction(struct kv
 		case 146:                                       /* mtmsr */
 			rs = get_rs(inst);
 			kvmppc_set_msr(vcpu, vcpu->arch.gpr[rs]);
+			break;
+
+		case 151:                                       /* stwx */
+			rs = get_rs(inst);
+			emulated = kvmppc_handle_store(run, vcpu,
+			                               vcpu->arch.gpr[rs],
+			                               4, 1);
 			break;
 
 		case 163:                                       /* wrteei */

[PATCH 3 of 5] KVM: ppc: Remove unmatched kunmap() call

[Hollis Blanchard]

We're not calling kmap() now, so we shouldn't call kunmap() either. This has no
practical effect in the non-highmem case, which is why it hasn't caused more
obvious problems.

Pointed out by Anthony Liguori.

Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>

diff --git a/arch/powerpc/kvm/44x_tlb.c b/arch/powerpc/kvm/44x_tlb.c
--- a/arch/powerpc/kvm/44x_tlb.c
+++ b/arch/powerpc/kvm/44x_tlb.c
@@ -116,8 +116,6 @@ static void kvmppc_44x_shadow_release(st
 	struct tlbe *stlbe = &vcpu->arch.shadow_tlb[index];
 	struct page *page = vcpu->arch.shadow_pages[index];
 
-	kunmap(vcpu->arch.shadow_pages[index]);
-
 	if (get_tlb_v(stlbe)) {
 		if (kvmppc_44x_tlbe_is_writable(stlbe))
 			kvm_release_page_dirty(page);

[PATCH 4 of 5] KVM: ppc: Use a read lock around MMU operations, and release it on error

[Hollis Blanchard]

gfn_to_page() and kvm_release_page_clean() are called from other contexts with
mmap_sem locked only for reading.

Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>

diff --git a/arch/powerpc/kvm/44x_tlb.c b/arch/powerpc/kvm/44x_tlb.c
--- a/arch/powerpc/kvm/44x_tlb.c
+++ b/arch/powerpc/kvm/44x_tlb.c
@@ -142,18 +142,19 @@ void kvmppc_mmu_map(struct kvm_vcpu *vcp
 	stlbe = &vcpu->arch.shadow_tlb[victim];
 
 	/* Get reference to new page. */
-	down_write(&current->mm->mmap_sem);
+	down_read(&current->mm->mmap_sem);
 	new_page = gfn_to_page(vcpu->kvm, gfn);
 	if (is_error_page(new_page)) {
 		printk(KERN_ERR "Couldn't get guest page!\n");
 		kvm_release_page_clean(new_page);
+		up_read(&current->mm->mmap_sem);
 		return;
 	}
 	hpaddr = page_to_phys(new_page);
 
 	/* Drop reference to old page. */
 	kvmppc_44x_shadow_release(vcpu, victim);
-	up_write(&current->mm->mmap_sem);
+	up_read(&current->mm->mmap_sem);
 
 	vcpu->arch.shadow_pages[victim] = new_page;
 

[PATCH 5 of 5] [kvm powerpc] Report bad GFNs

[Hollis Blanchard]

This code shouldn't be hit anyways, but when it is, it's useful to have a
little more information about the failure.

Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>

diff --git a/arch/powerpc/kvm/44x_tlb.c b/arch/powerpc/kvm/44x_tlb.c
--- a/arch/powerpc/kvm/44x_tlb.c
+++ b/arch/powerpc/kvm/44x_tlb.c
@@ -145,7 +145,7 @@ void kvmppc_mmu_map(struct kvm_vcpu *vcp
 	down_read(&current->mm->mmap_sem);
 	new_page = gfn_to_page(vcpu->kvm, gfn);
 	if (is_error_page(new_page)) {
-		printk(KERN_ERR "Couldn't get guest page!\n");
+		printk(KERN_ERR "Couldn't get guest page for gfn %lx!\n", gfn);
 		kvm_release_page_clean(new_page);
 		up_read(&current->mm->mmap_sem);
 		return;

Re: [PATCH 5 of 5] [kvm powerpc] Report bad GFNs

[Anthony Liguori]

Hollis Blanchard wrote:
> This code shouldn't be hit anyways, but when it is, it's useful to have a
> little more information about the failure.
>
> Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
>
> diff --git a/arch/powerpc/kvm/44x_tlb.c b/arch/powerpc/kvm/44x_tlb.c
> --- a/arch/powerpc/kvm/44x_tlb.c
> +++ b/arch/powerpc/kvm/44x_tlb.c
> @@ -145,7 +145,7 @@ void kvmppc_mmu_map(struct kvm_vcpu *vcp
>  	down_read(&current->mm->mmap_sem);
>  	new_page = gfn_to_page(vcpu->kvm, gfn);
>  	if (is_error_page(new_page)) {
> -		printk(KERN_ERR "Couldn't get guest page!\n");
> +		printk(KERN_ERR "Couldn't get guest page for gfn %lx!\n", gfn);
>  		kvm_release_page_clean(new_page);
>  		up_read(&current->mm->mmap_sem);
>  		return;
>   

FWIW, you should probably rate limit this sort of thing if this is what 
would get triggered if the guest tried to program the tlb with an 
invalid gfn.

Regards,

Anthony Liguori

> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>   

Re: [PATCH 5 of 5] [kvm powerpc] Report bad GFNs

[Hollis Blanchard]

On Wednesday 21 May 2008 21:47:40 Anthony Liguori wrote:
> Hollis Blanchard wrote:
> > This code shouldn't be hit anyways, but when it is, it's useful to have a
> > little more information about the failure.
> >
> > Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
> >
> > diff --git a/arch/powerpc/kvm/44x_tlb.c b/arch/powerpc/kvm/44x_tlb.c
> > --- a/arch/powerpc/kvm/44x_tlb.c
> > +++ b/arch/powerpc/kvm/44x_tlb.c
> > @@ -145,7 +145,7 @@ void kvmppc_mmu_map(struct kvm_vcpu *vcp
> >  	down_read(&current->mm->mmap_sem);
> >  	new_page = gfn_to_page(vcpu->kvm, gfn);
> >  	if (is_error_page(new_page)) {
> > -		printk(KERN_ERR "Couldn't get guest page!\n");
> > +		printk(KERN_ERR "Couldn't get guest page for gfn %lx!\n", gfn);
> >  		kvm_release_page_clean(new_page);
> >  		up_read(&current->mm->mmap_sem);
> >  		return;
> >   
> 
> FWIW, you should probably rate limit this sort of thing if this is what 
> would get triggered if the guest tried to program the tlb with an 
> invalid gfn.

That was my initial thought as well, but in general, kvmppc_mmu_map() is only 
called if a TLB entry is "safe" -- meaning kvm_is_visible_gfn(kvm, gfn) was 
true. So in the absence of bugs elsewhere, this should never happen... but we 
know how that goes. ;)

Anyways, that's why I'm not worried about rate limiting here.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH 0 of 5] PowerPC KVM fixes for 2.6.26

[Avi Kivity]

Hollis Blanchard wrote:
> Hi Avi, these bug fixes should be committed for 2.6.26. Most are pretty
> harmless. Although the mmap_sem locking problem is fatal if you hit it, in
> practice we simply shouldn't hit that error case anyways.
>   

Thanks; applied all.  AFAICS only the locking fix is necessary for 2.6.26?


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0 of 5] PowerPC KVM fixes for 2.6.26

[Hollis Blanchard]

On Sunday 25 May 2008 05:35:36 Avi Kivity wrote:
> Hollis Blanchard wrote:
> > Hi Avi, these bug fixes should be committed for 2.6.26. Most are pretty
> > harmless. Although the mmap_sem locking problem is fatal if you hit it, in
> > practice we simply shouldn't hit that error case anyways.
> >   
> 
> Thanks; applied all.  AFAICS only the locking fix is necessary for 2.6.26?

Yes, the locking fix is necessary, since it could crash the host. Technically 
speaking this is the only critical patch, but I think the chance of the 
others causing regressions is very low (especially since there was no KVM PPC 
support in 2.6.25, so by definition... ;) .

-- 
Hollis Blanchard
IBM Linux Technology Center