> -----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
