On 21.07.14 11:59, mihai.caraman@freescale.com wrote:

quoted

-----Original Message-----
From: Linuxppc-dev [mailto:linuxppc-dev-
bounces+mihai.caraman=freescale.com@lists.ozlabs.org] On Behalf Of
mihai.caraman@freescale.com
Sent: Friday, July 18, 2014 12:06 PM
To: Alexander Graf; kvm-ppc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org; kvm@vger.kernel.org
Subject: RE: [PATCH v5 4/5] KVM: PPC: Alow kvmppc_get_last_inst() to fail

quoted

-----Original Message-----
From: Alexander Graf [mailto:agraf@suse.de]
Sent: Thursday, July 17, 2014 5:21 PM
To: Caraman Mihai Claudiu-B02008; kvm-ppc@vger.kernel.org
Cc: kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v5 4/5] KVM: PPC: Alow kvmppc_get_last_inst() to

fail

quoted

On 17.07.14 13:22, Mihai Caraman wrote:

quoted

On book3e, guest last instruction is read on the exit path using load
external pid (lwepx) dedicated instruction. This load operation may

fail

quoted

due to TLB eviction and execute-but-not-read entries.

This patch lay down the path for an alternative solution to read the

guest

quoted

last instruction, by allowing kvmppc_get_lat_inst() function to fail.
Architecture specific implmentations of kvmppc_load_last_inst() may

read

quoted

last guest instruction and instruct the emulation layer to re-execute

the

quoted

guest in case of failure.

Make kvmppc_get_last_inst() definition common between architectures.

Signed-off-by: Mihai Caraman <redacted>
---

...

quoted

quoted

diff --git a/arch/powerpc/include/asm/kvm_ppc.h

b/arch/powerpc/include/asm/kvm_ppc.h

quoted

index e2fd5a1..7f9c634 100644

--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h

@@ -47,6 +47,11 @@ enum emulation_result {
   	EMULATE_EXIT_USER,    /* emulation requires exit to user-

space */

quoted

quoted

   };

+enum instruction_type {
+	INST_GENERIC,
+	INST_SC,		/* system call */
+};
+
   extern int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu

*vcpu);

quoted

   extern int __kvmppc_vcpu_run(struct kvm_run *kvm_run, struct

kvm_vcpu

quoted

*vcpu);

quoted

   extern void kvmppc_handler_highmem(void);

@@ -62,6 +67,9 @@ extern int kvmppc_handle_store(struct kvm_run *run,

struct kvm_vcpu *vcpu,

quoted

   			       u64 val, unsigned int bytes,
   			       int is_default_endian);

+extern int kvmppc_load_last_inst(struct kvm_vcpu *vcpu,
+				 enum instruction_type type, u32 *inst);
+
   extern int kvmppc_emulate_instruction(struct kvm_run *run,
                                         struct kvm_vcpu *vcpu);
   extern int kvmppc_emulate_mmio(struct kvm_run *run, struct kvm_vcpu

*vcpu);

quoted

@@ -234,6 +242,23 @@ struct kvmppc_ops {
   extern struct kvmppc_ops *kvmppc_hv_ops;
   extern struct kvmppc_ops *kvmppc_pr_ops;

+static inline int kvmppc_get_last_inst(struct kvm_vcpu *vcpu,
+					enum instruction_type type, 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_load_last_inst(vcpu, type, &vcpu-

arch.last_inst);
+
+
+	*inst = (ret == EMULATE_DONE && kvmppc_need_byteswap(vcpu)) ?
+		swab32(vcpu->arch.last_inst) : vcpu->arch.last_inst;

This makes even less sense than the previous version. Either you treat
inst as "definitely overwritten" or as "preserves previous data on
failure".

Both v4 and v5 versions treat inst as "definitely overwritten".

quoted

So either you unconditionally swap like you did before

If we make abstraction of its symmetry, KVM_INST_FETCH_FAILED is operated
in host endianness, so it doesn't need byte swap.

I agree with your reasoning if last_inst is initialized and compared with
data in guest endianess, which is not the case yet for
KVM_INST_FETCH_FAILED.

Alex, are you relying on the fact that KVM_INST_FETCH_FAILED value is symmetrical?
With a non symmetrical value like 0xDEADBEEF, and considering a little-endian guest
on a big-endian host, we need to fix kvm logic to initialize and compare last_inst
with 0xEFBEADDE swaped value.

Your suggestion to unconditionally swap makes sense only with the above fix, otherwise
inst may end up with 0xEFBEADDE swaped value with is wrong.

Only for *inst which we would treat as "undefined" after the function 
returned EMULATE_AGAIN. last_inst stays  unmodified.


Alex