[PATCH v2 3/4] KVM: PPC: Alow kvmppc_get_last_inst() to fail
Alexander Graf
agraf at suse.de
Fri May 2 19:54:57 EST 2014
On 05/01/2014 02:45 AM, Mihai Caraman wrote:
> On book3e, guest last instruction was read on the exist path using load
> external pid (lwepx) dedicated instruction. lwepx failures have to be
> handled by KVM and this would require additional checks in DO_KVM hooks
> (beside MSR[GS] = 1). However extra checks on host fast path are commonly
> considered too intrusive.
>
> This patch lay down the path for an altenative solution, by allowing
> kvmppc_get_lat_inst() function to fail. Booke architectures may choose
> to read guest instruction from kvmppc_ld_inst() and to instruct the
> emulation layer either to fail or to emulate again.
>
> emulation_result enum defintion is not accesible from book3 asm headers.
> Move kvmppc_get_last_inst() definitions that require emulation_result
> to source files.
>
> Signed-off-by: Mihai Caraman <mihai.caraman at freescale.com>
> ---
> v2:
> - integrated kvmppc_get_last_inst() in book3s code and checked build
> - addressed cosmetic feedback
> - please validate book3s functionality!
>
> arch/powerpc/include/asm/kvm_book3s.h | 26 --------------------
> arch/powerpc/include/asm/kvm_booke.h | 5 ----
> arch/powerpc/include/asm/kvm_ppc.h | 2 ++
> arch/powerpc/kvm/book3s.c | 32 ++++++++++++++++++++++++
> arch/powerpc/kvm/book3s.h | 1 +
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 16 +++---------
> arch/powerpc/kvm/book3s_paired_singles.c | 42 ++++++++++++++++++++------------
> arch/powerpc/kvm/book3s_pr.c | 36 +++++++++++++++++----------
> arch/powerpc/kvm/booke.c | 14 +++++++++++
> arch/powerpc/kvm/booke.h | 3 +++
> arch/powerpc/kvm/e500_mmu_host.c | 7 ++++++
> arch/powerpc/kvm/emulate.c | 18 +++++++++-----
> arch/powerpc/kvm/powerpc.c | 10 ++++++--
> 13 files changed, 132 insertions(+), 80 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
> index bb1e38a..e2a89f3 100644
> --- a/arch/powerpc/include/asm/kvm_book3s.h
> +++ b/arch/powerpc/include/asm/kvm_book3s.h
> @@ -273,32 +273,6 @@ static inline bool kvmppc_need_byteswap(struct kvm_vcpu *vcpu)
> return (vcpu->arch.shared->msr & MSR_LE) != (MSR_KERNEL & MSR_LE);
> }
>
> -static inline u32 kvmppc_get_last_inst_internal(struct kvm_vcpu *vcpu, ulong pc)
> -{
> - /* Load the instruction manually if it failed to do so in the
> - * exit path */
> - if (vcpu->arch.last_inst == KVM_INST_FETCH_FAILED)
> - kvmppc_ld(vcpu, &pc, sizeof(u32), &vcpu->arch.last_inst, false);
> -
> - return kvmppc_need_byteswap(vcpu) ? swab32(vcpu->arch.last_inst) :
> - vcpu->arch.last_inst;
> -}
> -
> -static inline u32 kvmppc_get_last_inst(struct kvm_vcpu *vcpu)
> -{
> - return kvmppc_get_last_inst_internal(vcpu, kvmppc_get_pc(vcpu));
> -}
> -
> -/*
> - * Like kvmppc_get_last_inst(), but for fetching a sc instruction.
> - * Because the sc instruction sets SRR0 to point to the following
> - * instruction, we have to fetch from pc - 4.
> - */
> -static inline u32 kvmppc_get_last_sc(struct kvm_vcpu *vcpu)
> -{
> - return kvmppc_get_last_inst_internal(vcpu, kvmppc_get_pc(vcpu) - 4);
> -}
> -
> static inline ulong kvmppc_get_fault_dar(struct kvm_vcpu *vcpu)
> {
> return vcpu->arch.fault_dar;
> diff --git a/arch/powerpc/include/asm/kvm_booke.h b/arch/powerpc/include/asm/kvm_booke.h
> index 80d46b5..6db1ca0 100644
> --- a/arch/powerpc/include/asm/kvm_booke.h
> +++ b/arch/powerpc/include/asm/kvm_booke.h
> @@ -69,11 +69,6 @@ static inline bool kvmppc_need_byteswap(struct kvm_vcpu *vcpu)
> return false;
> }
>
> -static inline u32 kvmppc_get_last_inst(struct kvm_vcpu *vcpu)
> -{
> - return vcpu->arch.last_inst;
> -}
> -
> static inline void kvmppc_set_ctr(struct kvm_vcpu *vcpu, ulong val)
> {
> vcpu->arch.ctr = val;
> 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().
> +
> /* Core-specific hooks */
>
> extern void kvmppc_mmu_map(struct kvm_vcpu *vcpu, u64 gvaddr, gpa_t gpaddr,
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index 94e597e..3553735 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -463,6 +463,38 @@ mmio:
> }
> EXPORT_SYMBOL_GPL(kvmppc_ld);
>
> +int kvmppc_get_last_inst_internal(struct kvm_vcpu *vcpu, ulong pc, u32 *inst)
> +{
> + int ret = EMULATE_DONE;
> +
> + /* Load the instruction manually if it failed to do so in the
> + * exit path */
> + if (vcpu->arch.last_inst == KVM_INST_FETCH_FAILED)
> + ret = kvmppc_ld(vcpu, &pc, sizeof(u32), &vcpu->arch.last_inst,
> + false);
> +
> + *inst = kvmppc_need_byteswap(vcpu) ? swab32(vcpu->arch.last_inst) :
> + vcpu->arch.last_inst;
> +
> + return ret;
> +}
> +
> +int kvmppc_get_last_inst(struct kvm_vcpu *vcpu, u32 *inst)
> +{
> + return kvmppc_get_last_inst_internal(vcpu, kvmppc_get_pc(vcpu), inst);
> +}
> +
> +/*
> + * Like kvmppc_get_last_inst(), but for fetching a sc instruction.
> + * Because the sc instruction sets SRR0 to point to the following
> + * instruction, we have to fetch from pc - 4.
> + */
> +int kvmppc_get_last_sc(struct kvm_vcpu *vcpu, u32 *inst)
> +{
> + return kvmppc_get_last_inst_internal(vcpu, kvmppc_get_pc(vcpu) - 4,
> + inst);
> +}
> +
> int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
> {
> return 0;
> diff --git a/arch/powerpc/kvm/book3s.h b/arch/powerpc/kvm/book3s.h
> index 4bf956c..d85839d 100644
> --- a/arch/powerpc/kvm/book3s.h
> +++ b/arch/powerpc/kvm/book3s.h
> @@ -30,5 +30,6 @@ extern int kvmppc_core_emulate_mfspr_pr(struct kvm_vcpu *vcpu,
> int sprn, ulong *spr_val);
> extern int kvmppc_book3s_init_pr(void);
> extern void kvmppc_book3s_exit_pr(void);
> +extern int kvmppc_get_last_sc(struct kvm_vcpu *vcpu, u32 *inst);
>
> #endif
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index fb25ebc..7ffcc24 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -541,21 +541,13 @@ static int instruction_is_store(unsigned int instr)
> static int kvmppc_hv_emulate_mmio(struct kvm_run *run, struct kvm_vcpu *vcpu,
> unsigned long gpa, gva_t ea, int is_store)
> {
> - int ret;
> u32 last_inst;
> - unsigned long srr0 = kvmppc_get_pc(vcpu);
>
> - /* We try to load the last instruction. We don't let
> - * emulate_instruction do it as it doesn't check what
> - * kvmppc_ld returns.
> + /*
> * If we fail, we just return to the guest and try executing it again.
> */
> - if (vcpu->arch.last_inst == KVM_INST_FETCH_FAILED) {
> - ret = kvmppc_ld(vcpu, &srr0, sizeof(u32), &last_inst, false);
> - if (ret != EMULATE_DONE || last_inst == KVM_INST_FETCH_FAILED)
> - return RESUME_GUEST;
> - vcpu->arch.last_inst = last_inst;
> - }
> + if (kvmppc_get_last_inst(vcpu, &last_inst) != EMULATE_DONE)
> + return RESUME_GUEST;
>
> /*
> * WARNING: We do not know for sure whether the instruction we just
> @@ -569,7 +561,7 @@ static int kvmppc_hv_emulate_mmio(struct kvm_run *run, struct kvm_vcpu *vcpu,
> * we just return and retry the instruction.
> */
>
> - if (instruction_is_store(kvmppc_get_last_inst(vcpu)) != !!is_store)
> + if (instruction_is_store(last_inst) != !!is_store)
> return RESUME_GUEST;
>
> /*
> diff --git a/arch/powerpc/kvm/book3s_paired_singles.c b/arch/powerpc/kvm/book3s_paired_singles.c
> index c1abd95..9a6e565 100644
> --- a/arch/powerpc/kvm/book3s_paired_singles.c
> +++ b/arch/powerpc/kvm/book3s_paired_singles.c
> @@ -637,26 +637,36 @@ static int kvmppc_ps_one_in(struct kvm_vcpu *vcpu, bool rc,
>
> int kvmppc_emulate_paired_single(struct kvm_run *run, struct kvm_vcpu *vcpu)
> {
> - u32 inst = kvmppc_get_last_inst(vcpu);
> - enum emulation_result emulated = EMULATE_DONE;
> -
> - int ax_rd = inst_get_field(inst, 6, 10);
> - int ax_ra = inst_get_field(inst, 11, 15);
> - int ax_rb = inst_get_field(inst, 16, 20);
> - int ax_rc = inst_get_field(inst, 21, 25);
> - short full_d = inst_get_field(inst, 16, 31);
> -
> - u64 *fpr_d = &VCPU_FPR(vcpu, ax_rd);
> - u64 *fpr_a = &VCPU_FPR(vcpu, ax_ra);
> - u64 *fpr_b = &VCPU_FPR(vcpu, ax_rb);
> - u64 *fpr_c = &VCPU_FPR(vcpu, ax_rc);
> -
> - bool rcomp = (inst & 1) ? true : false;
> - u32 cr = kvmppc_get_cr(vcpu);
> + u32 inst;
> + enum emulation_result emulated;
> + int ax_rd, ax_ra, ax_rb, ax_rc;
> + short full_d;
> + u64 *fpr_d, *fpr_a, *fpr_b, *fpr_c;
> +
> + bool rcomp;
> + u32 cr;
> #ifdef DEBUG
> int i;
> #endif
>
> + emulated = kvmppc_get_last_inst(vcpu, &inst);
> + if (emulated != EMULATE_DONE)
> + return emulated;
> +
> + ax_rd = inst_get_field(inst, 6, 10);
> + ax_ra = inst_get_field(inst, 11, 15);
> + ax_rb = inst_get_field(inst, 16, 20);
> + ax_rc = inst_get_field(inst, 21, 25);
> + full_d = inst_get_field(inst, 16, 31);
> +
> + fpr_d = &VCPU_FPR(vcpu, ax_rd);
> + fpr_a = &VCPU_FPR(vcpu, ax_ra);
> + fpr_b = &VCPU_FPR(vcpu, ax_rb);
> + fpr_c = &VCPU_FPR(vcpu, ax_rc);
> +
> + rcomp = (inst & 1) ? true : false;
> + cr = kvmppc_get_cr(vcpu);
> +
> if (!kvmppc_inst_is_paired_single(vcpu, inst))
> return EMULATE_FAIL;
>
> 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?
> 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?
>
> 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?
> 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.
> + 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.
Alex
More information about the Linuxppc-dev
mailing list