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

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

On 17.07.14 13:22, Mihai Caraman wrote:
On book3e, guest last instruction is read on the exit path using load
external pid (lwepx) dedicated instruction. This load operation may
fail
due to TLB eviction and execute-but-not-read entries.

This patch lay down the path for an alternative solution to read the
guest
last instruction, by allowing kvmppc_get_lat_inst() function to fail.
Architecture specific implmentations of kvmppc_load_last_inst() may
read
last guest instruction and instruct the emulation layer to re-execute
the
guest in case of failure.

Make kvmppc_get_last_inst() definition common between architectures.

Signed-off-by: Mihai Caraman <mihai.cara...@freescale.com>
---
...

diff --git a/arch/powerpc/include/asm/kvm_ppc.h
b/arch/powerpc/include/asm/kvm_ppc.h
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 */
   };

+enum instruction_type {
+       INST_GENERIC,
+       INST_SC,                /* system call */
+};
+
   extern int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu
*vcpu);
   extern int __kvmppc_vcpu_run(struct kvm_run *kvm_run, struct
kvm_vcpu
*vcpu);
   extern void kvmppc_handler_highmem(void);
@@ -62,6 +67,9 @@ extern int kvmppc_handle_store(struct kvm_run *run,
struct kvm_vcpu *vcpu,
                               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);
@@ -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".

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

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Reply via email to