Re: [PATCH v5 4/5] KVM: PPC: Alow kvmppc_get_last_inst() to fail

Am 23.07.2014 um 10:24 schrieb "mihai.caraman@freescale.com" <mihai.caraman@=
freescale.com>:

quoted

-----Original Message-----
From: kvm-ppc-owner@vger.kernel.org [mailto:kvm-ppc-
owner@vger.kernel.org] On Behalf Of Alexander Graf
Sent: Wednesday, July 23, 2014 12:21 AM
To: Caraman Mihai Claudiu-B02008
Cc: kvm-ppc@vger.kernel.org; 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

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

quoted

quoted

-----Original Message-----
From: Linuxppc-dev [mailto:linuxppc-dev-
bounces+mihai.caraman=3Dfreescale.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

quoted

=20

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

=20

quoted

On 17.07.14 13:22, Mihai Caraman wrote:
On book3e, guest last instruction is read on the exit path using

load

quoted

quoted

quoted

quoted

external pid (lwepx) dedicated instruction. This load operation may

fail

quoted

due to TLB eviction and execute-but-not-read entries.
=20
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.

quoted

quoted

quoted

quoted

Architecture specific implmentations of kvmppc_load_last_inst() may

read

quoted

last guest instruction and instruct the emulation layer to re-

execute

quoted

quoted

quoted

the

quoted

guest in case of failure.
=20
Make kvmppc_get_last_inst() definition common between architectures.
=20
Signed-off-by: Mihai Caraman <redacted>
---

...
=20

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

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

kvm_vcpu

quoted

quoted

quoted

*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,

quoted

quoted

quoted

struct kvm_vcpu *vcpu,

quoted

                     u64 val, unsigned int bytes,
                     int is_default_endian);
=20
+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

quoted

quoted

quoted

*vcpu);

quoted

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

=20
+static inline int kvmppc_get_last_inst(struct kvm_vcpu *vcpu,
+                    enum instruction_type type, u32

*inst)

quoted

quoted

quoted

quoted

+{
+    int ret =3D EMULATE_DONE;
+
+    /* Load the instruction manually if it failed to do so in the
+     * exit path */
+    if (vcpu->arch.last_inst =3D=3D KVM_INST_FETCH_FAILED)
+        ret =3D kvmppc_load_last_inst(vcpu, type, &vcpu-
arch.last_inst);
+
+
+    *inst =3D (ret =3D=3D EMULATE_DONE && kvmppc_need_byteswap(vcpu)=

) ?

quoted

quoted

quoted

quoted

quoted

+        swab32(vcpu->arch.last_inst) : vcpu->arch.last_inst;

This makes even less sense than the previous version. Either you

treat

quoted

quoted

quoted

inst as "definitely overwritten" or as "preserves previous data on
failure".

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

quoted

So either you unconditionally swap like you did before

If we make abstraction of its symmetry, KVM_INST_FETCH_FAILED is

operated

quoted

quoted

in host endianness, so it doesn't need byte swap.
=20
I agree with your reasoning if last_inst is initialized and compared

with

quoted

quoted

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?

quoted

With a non symmetrical value like 0xDEADBEEF, and considering a little-

endian guest

quoted

on a big-endian host, we need to fix kvm logic to initialize and

compare last_inst

quoted

with 0xEFBEADDE swaped value.
=20
Your suggestion to unconditionally swap makes sense only with the above

fix, otherwise

quoted

inst may end up with 0xEFBEADDE swaped value with is wrong.

=20
Only for *inst which we would treat as "undefined" after the function
returned EMULATE_AGAIN.

=20
Right. With this do you acknowledge that v5 (definitely overwritten approa=

ch)

is ok?

I think I'm starting to understand your logic of v5. You write fetch_failed i=
nto *inst unswapped if the fetch failed.

I think that's ok, but I definitely do not like the code flow - it's too har=
d to understand at a glimpse. Just rewrite it to swab at local variable leve=
l, preferably with if()s and comments what this is about and have a single u=
nconditional *inst =3D fetched_inst; at the end of the function.

Alex