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

[Alexander Graf <agraf@suse.de>]

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