RE: [PATCH v2 3/4] KVM: PPC: Alow kvmppc_get_last_inst() to fail

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Alexander Graf [mailto:agraf@xxxxxxx]
> Sent: Friday, May 02, 2014 12:55 PM
> To: Caraman Mihai Claudiu-B02008
> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; linuxppc-
> dev@xxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 3/4] KVM: PPC: Alow kvmppc_get_last_inst() to fail
> 
> On 05/01/2014 02:45 AM, Mihai Caraman wrote:
...
> > diff --git a/arch/powerpc/include/asm/kvm_ppc.h
> b/arch/powerpc/include/asm/kvm_ppc.h
> > index 4096f16..6e7c358 100644
> > --- a/arch/powerpc/include/asm/kvm_ppc.h
> > +++ b/arch/powerpc/include/asm/kvm_ppc.h
> > @@ -72,6 +72,8 @@ extern int kvmppc_sanity_check(struct kvm_vcpu
> *vcpu);
> >   extern int kvmppc_subarch_vcpu_init(struct kvm_vcpu *vcpu);
> >   extern void kvmppc_subarch_vcpu_uninit(struct kvm_vcpu *vcpu);
> >
> > +extern int kvmppc_get_last_inst(struct kvm_vcpu *vcpu, u32 *inst);
> 
> Phew. Moving this into a separate function sure has some performance
> implications. Was there no way to keep it in a header?
> 
> You could just move it into its own .h file which we include after
> kvm_ppc.h. That way everything's available. That would also help me a
> lot with the little endian port where I'm also struggling with header
> file inclusion order and kvmppc_need_byteswap().

Great, I will do this.

> > diff --git a/arch/powerpc/kvm/book3s_pr.c
> b/arch/powerpc/kvm/book3s_pr.c
> > index c5c052a..b7fffd1 100644
> > --- a/arch/powerpc/kvm/book3s_pr.c
> > +++ b/arch/powerpc/kvm/book3s_pr.c
> > @@ -608,12 +608,9 @@ void kvmppc_giveup_ext(struct kvm_vcpu *vcpu,
> ulong msr)
> >
> >   static int kvmppc_read_inst(struct kvm_vcpu *vcpu)
> >   {
> > -	ulong srr0 = kvmppc_get_pc(vcpu);
> > -	u32 last_inst = kvmppc_get_last_inst(vcpu);
> > -	int ret;
> > +	u32 last_inst;
> >
> > -	ret = kvmppc_ld(vcpu, &srr0, sizeof(u32), &last_inst, false);
> > -	if (ret == -ENOENT) {
> > +	if (kvmppc_get_last_inst(vcpu, &last_inst) == -ENOENT) {
> 
> ENOENT?

You have to tell us :) Why does kvmppc_ld() mix emulation_result
enumeration with generic errors? Do you want to change that and
use EMULATE_FAIL instead?

> 
> >   		ulong msr = vcpu->arch.shared->msr;
> >
> >   		msr = kvmppc_set_field(msr, 33, 33, 1);
> > @@ -867,15 +864,18 @@ int kvmppc_handle_exit_pr(struct kvm_run *run,
> struct kvm_vcpu *vcpu,
> >   	{
> >   		enum emulation_result er;
> >   		ulong flags;
> > +		u32 last_inst;
> >
> >   program_interrupt:
> >   		flags = vcpu->arch.shadow_srr1 & 0x1f0000ull;
> > +		kvmppc_get_last_inst(vcpu, &last_inst);
> 
> No check for the return value?

Should we queue a program exception and resume guest?

> 
> >
> >   		if (vcpu->arch.shared->msr & MSR_PR) {
> >   #ifdef EXIT_DEBUG
> > -			printk(KERN_INFO "Userspace triggered 0x700 exception
> at 0x%lx (0x%x)\n", kvmppc_get_pc(vcpu), kvmppc_get_last_inst(vcpu));
> > +			pr_info("Userspace triggered 0x700 exception at\n"
> > +			    "0x%lx (0x%x)\n", kvmppc_get_pc(vcpu), last_inst);
> >   #endif
> > -			if ((kvmppc_get_last_inst(vcpu) & 0xff0007ff) !=
> > +			if ((last_inst & 0xff0007ff) !=
> >   			    (INS_DCBZ & 0xfffffff7)) {
> >   				kvmppc_core_queue_program(vcpu, flags);
> >   				r = RESUME_GUEST;
> > @@ -894,7 +894,7 @@ program_interrupt:
> >   			break;
> >   		case EMULATE_FAIL:
> >   			printk(KERN_CRIT "%s: emulation at %lx failed
> (%08x)\n",
> > -			       __func__, kvmppc_get_pc(vcpu),
> kvmppc_get_last_inst(vcpu));
> > +			       __func__, kvmppc_get_pc(vcpu), last_inst);
> >   			kvmppc_core_queue_program(vcpu, flags);
> >   			r = RESUME_GUEST;
> >   			break;
> > @@ -911,8 +911,12 @@ program_interrupt:
> >   		break;
> >   	}
> >   	case BOOK3S_INTERRUPT_SYSCALL:
> > +	{
> > +		u32 last_sc;
> > +
> > +		kvmppc_get_last_sc(vcpu, &last_sc);
> 
> No check for the return value?

The existing code does not handle KVM_INST_FETCH_FAILED. 
How should we continue if papr is enabled and last_sc fails?

> 
> >   		if (vcpu->arch.papr_enabled &&
> > -		    (kvmppc_get_last_sc(vcpu) == 0x44000022) &&
> > +		    (last_sc == 0x44000022) &&
> >   		    !(vcpu->arch.shared->msr & MSR_PR)) {
> >   			/* SC 1 papr hypercalls */
> >   			ulong cmd = kvmppc_get_gpr(vcpu, 3);
> > @@ -957,6 +961,7 @@ program_interrupt:
> >   			r = RESUME_GUEST;
> >   		}
> >   		break;
> > +	}
> >   	case BOOK3S_INTERRUPT_FP_UNAVAIL:
> >   	case BOOK3S_INTERRUPT_ALTIVEC:
> >   	case BOOK3S_INTERRUPT_VSX:
> > @@ -985,15 +990,20 @@ program_interrupt:
> >   		break;
> >   	}
> >   	case BOOK3S_INTERRUPT_ALIGNMENT:
> > +	{
> > +		u32 last_inst;
> > +
> >   		if (kvmppc_read_inst(vcpu) == EMULATE_DONE) {
> > -			vcpu->arch.shared->dsisr = kvmppc_alignment_dsisr(vcpu,
> > -				kvmppc_get_last_inst(vcpu));
> > -			vcpu->arch.shared->dar = kvmppc_alignment_dar(vcpu,
> > -				kvmppc_get_last_inst(vcpu));
> > +			kvmppc_get_last_inst(vcpu, &last_inst);
> 
> I think with an error returning kvmppc_get_last_inst we can just use
> completely get rid of kvmppc_read_inst() and only use
> kvmppc_get_last_inst() instead.

The only purpose of kvmppc_read_inst() function is to generate an
instruction storage interrupt in case of load failure. It is also called
from kvmppc_check_ext(). Do you suggest to get rid of this logic? Or to
duplicate it in order to avoid kvmppc_get_last_inst() redundant call?

> 
> > +			vcpu->arch.shared->dsisr =
> > +				kvmppc_alignment_dsisr(vcpu, last_inst);
> > +			vcpu->arch.shared->dar =
> > +				kvmppc_alignment_dar(vcpu, last_inst);
> >   			kvmppc_book3s_queue_irqprio(vcpu, exit_nr);
> >   		}
> >   		r = RESUME_GUEST;
> >   		break;
> > +	}
> >   	case BOOK3S_INTERRUPT_MACHINE_CHECK:
> >   	case BOOK3S_INTERRUPT_TRACE:
> >   		kvmppc_book3s_queue_irqprio(vcpu, exit_nr);
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > index ab62109..df25db0 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -752,6 +752,9 @@ static int emulation_exit(struct kvm_run *run,
> struct kvm_vcpu *vcpu)
> >   		 * they were actually modified by emulation. */
> >   		return RESUME_GUEST_NV;
> >
> > +	case EMULATE_AGAIN:
> > +		return RESUME_GUEST;
> > +
> >   	case EMULATE_DO_DCR:
> >   		run->exit_reason = KVM_EXIT_DCR;
> >   		return RESUME_HOST;
> > @@ -1911,6 +1914,17 @@ void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu)
> >   	vcpu->kvm->arch.kvm_ops->vcpu_put(vcpu);
> >   }
> >
> > +int kvmppc_get_last_inst(struct kvm_vcpu *vcpu, u32 *inst)
> > +{
> > +	int result = EMULATE_DONE;
> > +
> > +	if (vcpu->arch.last_inst == KVM_INST_FETCH_FAILED)
> > +		result = kvmppc_ld_inst(vcpu, &vcpu->arch.last_inst);
> > +	*inst = vcpu->arch.last_inst;
> 
> This function looks almost identical to the book3s one. Can we share the
> same code path here? Just always return false for
> kvmppc_needs_byteswap() on booke.

Sounds good.

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




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux