[PATCH v2 3/4] KVM: PPC: Alow kvmppc_get_last_inst() to fail
mihai.caraman at freescale.com
mihai.caraman at freescale.com
Wed May 7 05:06:43 EST 2014
> -----Original Message-----
> From: Alexander Graf [mailto:agraf at suse.de]
> Sent: Friday, May 02, 2014 12:55 PM
> To: Caraman Mihai Claudiu-B02008
> Cc: kvm-ppc at vger.kernel.org; kvm at vger.kernel.org; linuxppc-
> dev at lists.ozlabs.org
> 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
More information about the Linuxppc-dev
mailing list