* [PATCH 0/3] KVM: PPC: Book3S: MMIO support for Little Endian guests
@ 2013-10-07 14:27 Cédric Le Goater
2013-10-07 14:27 ` [PATCH 1/3] KVM: PPC: Book3S: add helper routine to load guest instructions Cédric Le Goater
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Cédric Le Goater @ 2013-10-07 14:27 UTC (permalink / raw)
To: agraf, paulus; +Cc: kvm-ppc, kvm, Cédric Le Goater
MMIO emulation reads the last instruction executed by the guest
and then emulates. If the guest is running in Little Endian mode,
the instruction needs to be byte-swapped before being emulated.
The first patches add simple helper routines to load instructions from
the guest. It prepares ground for the byte-swapping of instructions
when reading memory from Little Endian guests. There might be room for
more changes in kvmppc_read_inst() : is the kvmppc_get_last_inst() call
actually useful ?
The last patch enables the MMIO support by byte-swapping the last
instruction if the guest is Little Endian.
This patchset is based on Alex Graf's kvm-ppc-queue branch. It has been
tested with anton's patchset for Big Endian and Little Endian HV guests
and Big Endian PR guests.
Thanks,
C.
Cédric Le Goater (3):
KVM: PPC: Book3S: add helper routine to load guest instructions
KVM: PPC: Book3S: add helper routines to detect endian order
KVM: PPC: Book3S: MMIO emulation support for little endian guests
arch/powerpc/include/asm/kvm_book3s.h | 33 +++++++++++++-
arch/powerpc/kvm/book3s_64_mmu_hv.c | 2 +-
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 12 ++++++
arch/powerpc/kvm/book3s_pr.c | 2 +-
arch/powerpc/kvm/book3s_segment.S | 11 +++++
arch/powerpc/kvm/emulate.c | 72 +++++++++++++++++--------------
6 files changed, 96 insertions(+), 36 deletions(-)
--
1.7.10.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] KVM: PPC: Book3S: add helper routine to load guest instructions
2013-10-07 14:27 [PATCH 0/3] KVM: PPC: Book3S: MMIO support for Little Endian guests Cédric Le Goater
@ 2013-10-07 14:27 ` Cédric Le Goater
2013-10-07 14:27 ` [PATCH 2/3] KVM: PPC: Book3S: add helper routines to detect endian order Cédric Le Goater
2013-10-07 14:27 ` [PATCH 3/3] KVM: PPC: Book3S: MMIO emulation support for little endian guests Cédric Le Goater
2 siblings, 0 replies; 6+ messages in thread
From: Cédric Le Goater @ 2013-10-07 14:27 UTC (permalink / raw)
To: agraf, paulus; +Cc: kvm-ppc, kvm, Cédric Le Goater
This patch adds an helper routine kvmppc_ld_inst() to load an
instruction form the guest. This routine will be modified in
the next patches to take into account the endian order of the
guest.
Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
arch/powerpc/include/asm/kvm_book3s.h | 16 ++++++++++++++--
arch/powerpc/kvm/book3s_64_mmu_hv.c | 2 +-
arch/powerpc/kvm/book3s_pr.c | 2 +-
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
index 0ec00f4..dfe8f11 100644
--- a/arch/powerpc/include/asm/kvm_book3s.h
+++ b/arch/powerpc/include/asm/kvm_book3s.h
@@ -270,6 +270,18 @@ static inline ulong kvmppc_get_pc(struct kvm_vcpu *vcpu)
return vcpu->arch.pc;
}
+static inline int kvmppc_ld32(struct kvm_vcpu *vcpu, ulong *eaddr,
+ u32 *ptr, bool data)
+{
+ return kvmppc_ld(vcpu, eaddr, sizeof(u32), ptr, data);
+}
+
+static inline int kvmppc_ld_inst(struct kvm_vcpu *vcpu, ulong *eaddr,
+ u32 *inst)
+{
+ return kvmppc_ld32(vcpu, eaddr, inst, false);
+}
+
static inline u32 kvmppc_get_last_inst(struct kvm_vcpu *vcpu)
{
ulong pc = kvmppc_get_pc(vcpu);
@@ -277,7 +289,7 @@ static inline u32 kvmppc_get_last_inst(struct kvm_vcpu *vcpu)
/* 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);
+ kvmppc_ld_inst(vcpu, &pc, &vcpu->arch.last_inst);
return vcpu->arch.last_inst;
}
@@ -294,7 +306,7 @@ static inline u32 kvmppc_get_last_sc(struct kvm_vcpu *vcpu)
/* 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);
+ kvmppc_ld_inst(vcpu, &pc, &vcpu->arch.last_inst);
return vcpu->arch.last_inst;
}
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 3a89b85..0083cd0 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -544,7 +544,7 @@ static int kvmppc_hv_emulate_mmio(struct kvm_run *run, struct kvm_vcpu *vcpu,
* 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);
+ ret = kvmppc_ld_inst(vcpu, &srr0, &last_inst);
if (ret != EMULATE_DONE || last_inst == KVM_INST_FETCH_FAILED)
return RESUME_GUEST;
vcpu->arch.last_inst = last_inst;
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index 6075dbd..a817ef6 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -600,7 +600,7 @@ static int kvmppc_read_inst(struct kvm_vcpu *vcpu)
u32 last_inst = kvmppc_get_last_inst(vcpu);
int ret;
- ret = kvmppc_ld(vcpu, &srr0, sizeof(u32), &last_inst, false);
+ ret = kvmppc_ld_inst(vcpu, &srr0, &last_inst);
if (ret == -ENOENT) {
ulong msr = vcpu->arch.shared->msr;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] KVM: PPC: Book3S: add helper routines to detect endian order
2013-10-07 14:27 [PATCH 0/3] KVM: PPC: Book3S: MMIO support for Little Endian guests Cédric Le Goater
2013-10-07 14:27 ` [PATCH 1/3] KVM: PPC: Book3S: add helper routine to load guest instructions Cédric Le Goater
@ 2013-10-07 14:27 ` Cédric Le Goater
2013-10-07 14:27 ` [PATCH 3/3] KVM: PPC: Book3S: MMIO emulation support for little endian guests Cédric Le Goater
2 siblings, 0 replies; 6+ messages in thread
From: Cédric Le Goater @ 2013-10-07 14:27 UTC (permalink / raw)
To: agraf, paulus; +Cc: kvm-ppc, kvm, Cédric Le Goater
They will be used to decide whether to byte-swap or not. When Little
Endian host kernels come, these routines will need to be changed
accordingly.
Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
arch/powerpc/include/asm/kvm_book3s.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
index dfe8f11..00c2061 100644
--- a/arch/powerpc/include/asm/kvm_book3s.h
+++ b/arch/powerpc/include/asm/kvm_book3s.h
@@ -270,6 +270,16 @@ static inline ulong kvmppc_get_pc(struct kvm_vcpu *vcpu)
return vcpu->arch.pc;
}
+static inline bool kvmppc_need_byteswap(struct kvm_vcpu *vcpu)
+{
+ return vcpu->arch.shared->msr & MSR_LE;
+}
+
+static inline bool kvmppc_is_bigendian(struct kvm_vcpu *vcpu)
+{
+ return !kvmppc_need_byteswap(vcpu);
+}
+
static inline int kvmppc_ld32(struct kvm_vcpu *vcpu, ulong *eaddr,
u32 *ptr, bool data)
{
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] KVM: PPC: Book3S: MMIO emulation support for little endian guests
2013-10-07 14:27 [PATCH 0/3] KVM: PPC: Book3S: MMIO support for Little Endian guests Cédric Le Goater
2013-10-07 14:27 ` [PATCH 1/3] KVM: PPC: Book3S: add helper routine to load guest instructions Cédric Le Goater
2013-10-07 14:27 ` [PATCH 2/3] KVM: PPC: Book3S: add helper routines to detect endian order Cédric Le Goater
@ 2013-10-07 14:27 ` Cédric Le Goater
2013-10-08 11:22 ` Paul Mackerras
2 siblings, 1 reply; 6+ messages in thread
From: Cédric Le Goater @ 2013-10-07 14:27 UTC (permalink / raw)
To: agraf, paulus; +Cc: kvm-ppc, kvm, Cédric Le Goater
MMIO emulation reads the last instruction executed by the guest
and then emulates. If the guest is running in Little Endian mode,
the instruction needs to be byte-swapped before being emulated.
This patch stores the last instruction in the endian order of the
host, primarily doing a byte-swap if needed. The common code
which fetches 'last_inst' uses a helper routine kvmppc_need_byteswap().
and the exit paths for the Book3S PV and HR guests use their own
version in assembly.
Finally, kvmppc_emulate_instruction() uses kvmppc_is_bigendian()
to define in which endian order the mmio needs to be done.
Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
arch/powerpc/include/asm/kvm_book3s.h | 9 +++-
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 12 ++++++
arch/powerpc/kvm/book3s_segment.S | 11 +++++
arch/powerpc/kvm/emulate.c | 72 +++++++++++++++++--------------
4 files changed, 71 insertions(+), 33 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
index 00c2061..9c2b865 100644
--- a/arch/powerpc/include/asm/kvm_book3s.h
+++ b/arch/powerpc/include/asm/kvm_book3s.h
@@ -289,7 +289,14 @@ static inline int kvmppc_ld32(struct kvm_vcpu *vcpu, ulong *eaddr,
static inline int kvmppc_ld_inst(struct kvm_vcpu *vcpu, ulong *eaddr,
u32 *inst)
{
- return kvmppc_ld32(vcpu, eaddr, inst, false);
+ int ret;
+
+ ret = kvmppc_ld32(vcpu, eaddr, inst, false);
+
+ if (kvmppc_need_byteswap(vcpu))
+ *inst = swab32(*inst);
+
+ return ret;
}
static inline u32 kvmppc_get_last_inst(struct kvm_vcpu *vcpu)
diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index 77f1baa..7c9978a 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -1404,10 +1404,22 @@ fast_interrupt_c_return:
lwz r8, 0(r10)
mtmsrd r3
+ ld r0, VCPU_MSR(r9)
+
+ /* r10 = vcpu->arch.msr & MSR_LE */
+ rldicl. r10, r0, 0, 63
+
/* Store the result */
stw r8, VCPU_LAST_INST(r9)
+ beq after_inst_store
+
+ /* Swap and store the result */
+ addi r11, r9, VCPU_LAST_INST
+ stwbrx r8, 0, r11
+
/* Unset guest mode. */
+after_inst_store:
li r0, KVM_GUEST_MODE_HOST_HV
stb r0, HSTATE_IN_GUEST(r13)
b guest_exit_cont
diff --git a/arch/powerpc/kvm/book3s_segment.S b/arch/powerpc/kvm/book3s_segment.S
index 1abe478..2ceed4c 100644
--- a/arch/powerpc/kvm/book3s_segment.S
+++ b/arch/powerpc/kvm/book3s_segment.S
@@ -287,8 +287,19 @@ ld_last_inst:
sync
#endif
+ ld r8, SVCPU_SHADOW_SRR1(r13)
+
+ /* r10 = vcpu->arch.msr & MSR_LE */
+ rldicl. r10, r8, 0, 63
+
stw r0, SVCPU_LAST_INST(r13)
+ beq no_ld_last_inst
+
+ /* swap and store the result */
+ addi r11, r13, SVCPU_LAST_INST
+ stwbrx r0, 0, r11
+
no_ld_last_inst:
/* Unset guest mode */
diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
index 751cd45..76d0a12 100644
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c
@@ -219,7 +219,6 @@ static int kvmppc_emulate_mfspr(struct kvm_vcpu *vcpu, int sprn, int rt)
* lmw
* stmw
*
- * XXX is_bigendian should depend on MMU mapping or MSR[LE]
*/
/* XXX Should probably auto-generate instruction decoding for a particular core
* from opcode tables in the future. */
@@ -232,6 +231,7 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
int sprn = get_sprn(inst);
enum emulation_result emulated = EMULATE_DONE;
int advance = 1;
+ int is_bigendian = kvmppc_is_bigendian(vcpu);
/* this default type might be overwritten by subcategories */
kvmppc_set_exit_type(vcpu, EMULATED_INST_EXITS);
@@ -266,47 +266,53 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
advance = 0;
break;
case OP_31_XOP_LWZX:
- emulated = kvmppc_handle_load(run, vcpu, rt, 4, 1);
+ emulated = kvmppc_handle_load(run, vcpu, rt, 4,
+ is_bigendian);
break;
case OP_31_XOP_LBZX:
- emulated = kvmppc_handle_load(run, vcpu, rt, 1, 1);
+ emulated = kvmppc_handle_load(run, vcpu, rt, 1,
+ is_bigendian);
break;
case OP_31_XOP_LBZUX:
- emulated = kvmppc_handle_load(run, vcpu, rt, 1, 1);
+ emulated = kvmppc_handle_load(run, vcpu, rt, 1,
+ is_bigendian);
kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
break;
case OP_31_XOP_STWX:
emulated = kvmppc_handle_store(run, vcpu,
kvmppc_get_gpr(vcpu, rs),
- 4, 1);
+ 4, is_bigendian);
break;
case OP_31_XOP_STBX:
emulated = kvmppc_handle_store(run, vcpu,
kvmppc_get_gpr(vcpu, rs),
- 1, 1);
+ 1, is_bigendian);
break;
case OP_31_XOP_STBUX:
emulated = kvmppc_handle_store(run, vcpu,
kvmppc_get_gpr(vcpu, rs),
- 1, 1);
+ 1, is_bigendian);
kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
break;
case OP_31_XOP_LHAX:
- emulated = kvmppc_handle_loads(run, vcpu, rt, 2, 1);
+ emulated = kvmppc_handle_loads(run, vcpu, rt, 2,
+ is_bigendian);
break;
case OP_31_XOP_LHZX:
- emulated = kvmppc_handle_load(run, vcpu, rt, 2, 1);
+ emulated = kvmppc_handle_load(run, vcpu, rt, 2,
+ is_bigendian);
break;
case OP_31_XOP_LHZUX:
- emulated = kvmppc_handle_load(run, vcpu, rt, 2, 1);
+ emulated = kvmppc_handle_load(run, vcpu, rt, 2,
+ is_bigendian);
kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
break;
@@ -317,13 +323,13 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
case OP_31_XOP_STHX:
emulated = kvmppc_handle_store(run, vcpu,
kvmppc_get_gpr(vcpu, rs),
- 2, 1);
+ 2, is_bigendian);
break;
case OP_31_XOP_STHUX:
emulated = kvmppc_handle_store(run, vcpu,
kvmppc_get_gpr(vcpu, rs),
- 2, 1);
+ 2, is_bigendian);
kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
break;
@@ -342,7 +348,8 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
break;
case OP_31_XOP_LWBRX:
- emulated = kvmppc_handle_load(run, vcpu, rt, 4, 0);
+ emulated = kvmppc_handle_load(run, vcpu, rt, 4,
+ !is_bigendian);
break;
case OP_31_XOP_TLBSYNC:
@@ -351,17 +358,18 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
case OP_31_XOP_STWBRX:
emulated = kvmppc_handle_store(run, vcpu,
kvmppc_get_gpr(vcpu, rs),
- 4, 0);
+ 4, !is_bigendian);
break;
case OP_31_XOP_LHBRX:
- emulated = kvmppc_handle_load(run, vcpu, rt, 2, 0);
+ emulated = kvmppc_handle_load(run, vcpu, rt, 2,
+ !is_bigendian);
break;
case OP_31_XOP_STHBRX:
emulated = kvmppc_handle_store(run, vcpu,
kvmppc_get_gpr(vcpu, rs),
- 2, 0);
+ 2, !is_bigendian);
break;
default:
@@ -371,33 +379,33 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
break;
case OP_LWZ:
- emulated = kvmppc_handle_load(run, vcpu, rt, 4, 1);
+ emulated = kvmppc_handle_load(run, vcpu, rt, 4, is_bigendian);
break;
/* TBD: Add support for other 64 bit load variants like ldu, ldux, ldx etc. */
case OP_LD:
rt = get_rt(inst);
- emulated = kvmppc_handle_load(run, vcpu, rt, 8, 1);
+ emulated = kvmppc_handle_load(run, vcpu, rt, 8, is_bigendian);
break;
case OP_LWZU:
- emulated = kvmppc_handle_load(run, vcpu, rt, 4, 1);
+ emulated = kvmppc_handle_load(run, vcpu, rt, 4, is_bigendian);
kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
break;
case OP_LBZ:
- emulated = kvmppc_handle_load(run, vcpu, rt, 1, 1);
+ emulated = kvmppc_handle_load(run, vcpu, rt, 1, is_bigendian);
break;
case OP_LBZU:
- emulated = kvmppc_handle_load(run, vcpu, rt, 1, 1);
+ emulated = kvmppc_handle_load(run, vcpu, rt, 1, is_bigendian);
kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
break;
case OP_STW:
emulated = kvmppc_handle_store(run, vcpu,
kvmppc_get_gpr(vcpu, rs),
- 4, 1);
+ 4, is_bigendian);
break;
/* TBD: Add support for other 64 bit store variants like stdu, stdux, stdx etc. */
@@ -405,57 +413,57 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
rs = get_rs(inst);
emulated = kvmppc_handle_store(run, vcpu,
kvmppc_get_gpr(vcpu, rs),
- 8, 1);
+ 8, is_bigendian);
break;
case OP_STWU:
emulated = kvmppc_handle_store(run, vcpu,
kvmppc_get_gpr(vcpu, rs),
- 4, 1);
+ 4, is_bigendian);
kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
break;
case OP_STB:
emulated = kvmppc_handle_store(run, vcpu,
kvmppc_get_gpr(vcpu, rs),
- 1, 1);
+ 1, is_bigendian);
break;
case OP_STBU:
emulated = kvmppc_handle_store(run, vcpu,
kvmppc_get_gpr(vcpu, rs),
- 1, 1);
+ 1, is_bigendian);
kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
break;
case OP_LHZ:
- emulated = kvmppc_handle_load(run, vcpu, rt, 2, 1);
+ emulated = kvmppc_handle_load(run, vcpu, rt, 2, is_bigendian);
break;
case OP_LHZU:
- emulated = kvmppc_handle_load(run, vcpu, rt, 2, 1);
+ emulated = kvmppc_handle_load(run, vcpu, rt, 2, is_bigendian);
kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
break;
case OP_LHA:
- emulated = kvmppc_handle_loads(run, vcpu, rt, 2, 1);
+ emulated = kvmppc_handle_loads(run, vcpu, rt, 2, is_bigendian);
break;
case OP_LHAU:
- emulated = kvmppc_handle_loads(run, vcpu, rt, 2, 1);
+ emulated = kvmppc_handle_loads(run, vcpu, rt, 2, is_bigendian);
kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
break;
case OP_STH:
emulated = kvmppc_handle_store(run, vcpu,
kvmppc_get_gpr(vcpu, rs),
- 2, 1);
+ 2, is_bigendian);
break;
case OP_STHU:
emulated = kvmppc_handle_store(run, vcpu,
kvmppc_get_gpr(vcpu, rs),
- 2, 1);
+ 2, is_bigendian);
kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
break;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] KVM: PPC: Book3S: MMIO emulation support for little endian guests
2013-10-07 14:27 ` [PATCH 3/3] KVM: PPC: Book3S: MMIO emulation support for little endian guests Cédric Le Goater
@ 2013-10-08 11:22 ` Paul Mackerras
2013-10-08 11:49 ` Cedric Le Goater
0 siblings, 1 reply; 6+ messages in thread
From: Paul Mackerras @ 2013-10-08 11:22 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: agraf, kvm-ppc, kvm
On Mon, Oct 07, 2013 at 04:27:47PM +0200, Cédric Le Goater wrote:
> MMIO emulation reads the last instruction executed by the guest
> and then emulates. If the guest is running in Little Endian mode,
> the instruction needs to be byte-swapped before being emulated.
>
> This patch stores the last instruction in the endian order of the
> host, primarily doing a byte-swap if needed. The common code
> which fetches 'last_inst' uses a helper routine kvmppc_need_byteswap().
> and the exit paths for the Book3S PV and HR guests use their own
> version in assembly.
>
> Finally, kvmppc_emulate_instruction() uses kvmppc_is_bigendian()
> to define in which endian order the mmio needs to be done.
>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
[snip]
> + ld r0, VCPU_MSR(r9)
> +
> + /* r10 = vcpu->arch.msr & MSR_LE */
> + rldicl. r10, r0, 0, 63
I would have written:
andi. r10, r0, MSR_LE
which doesn't need the comment, but really the two are equivalent.
> @@ -232,6 +231,7 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
> int sprn = get_sprn(inst);
> enum emulation_result emulated = EMULATE_DONE;
> int advance = 1;
> + int is_bigendian = kvmppc_is_bigendian(vcpu);
>
> /* this default type might be overwritten by subcategories */
> kvmppc_set_exit_type(vcpu, EMULATED_INST_EXITS);
> @@ -266,47 +266,53 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
> advance = 0;
> break;
> case OP_31_XOP_LWZX:
> - emulated = kvmppc_handle_load(run, vcpu, rt, 4, 1);
> + emulated = kvmppc_handle_load(run, vcpu, rt, 4,
> + is_bigendian);
I see you're still hitting all the call sites of kvmppc_handle_load(),
kvmppc_handle_loads() and kvmppc_handle_store(), rather than putting
the big-endian test inside kvmppc_handle_load() and
kvmppc_handle_store(), as in this untested patch:
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index f55e14c..171bce6 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -625,9 +625,13 @@ static void kvmppc_complete_mmio_load(struct kvm_vcpu *vcpu,
}
int kvmppc_handle_load(struct kvm_run *run, struct kvm_vcpu *vcpu,
- unsigned int rt, unsigned int bytes, int is_bigendian)
+ unsigned int rt, unsigned int bytes, int not_reverse)
{
int idx, ret;
+ int is_bigendian = not_reverse;
+
+ if (!kvmppc_is_bigendian(vcpu))
+ is_bigendian = !not_reverse;
if (bytes > sizeof(run->mmio.data)) {
printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__,
@@ -662,21 +666,25 @@ int kvmppc_handle_load(struct kvm_run *run, struct kvm_vcpu *vcpu,
/* Same as above, but sign extends */
int kvmppc_handle_loads(struct kvm_run *run, struct kvm_vcpu *vcpu,
- unsigned int rt, unsigned int bytes, int is_bigendian)
+ unsigned int rt, unsigned int bytes, int not_reverse)
{
int r;
vcpu->arch.mmio_sign_extend = 1;
- r = kvmppc_handle_load(run, vcpu, rt, bytes, is_bigendian);
+ r = kvmppc_handle_load(run, vcpu, rt, bytes, not_reverse);
return r;
}
int kvmppc_handle_store(struct kvm_run *run, struct kvm_vcpu *vcpu,
- u64 val, unsigned int bytes, int is_bigendian)
+ u64 val, unsigned int bytes, int not_reverse)
{
void *data = run->mmio.data;
int idx, ret;
+ int is_bigendian = not_reverse;
+
+ if (!kvmppc_is_bigendian(vcpu))
+ is_bigendian = !not_reverse;
if (bytes > sizeof(run->mmio.data)) {
printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__,
That seems simpler to me -- is there a reason not to do it that way?
Paul.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] KVM: PPC: Book3S: MMIO emulation support for little endian guests
2013-10-08 11:22 ` Paul Mackerras
@ 2013-10-08 11:49 ` Cedric Le Goater
0 siblings, 0 replies; 6+ messages in thread
From: Cedric Le Goater @ 2013-10-08 11:49 UTC (permalink / raw)
To: Paul Mackerras; +Cc: agraf, kvm-ppc, kvm
On 10/08/2013 01:22 PM, Paul Mackerras wrote:
> On Mon, Oct 07, 2013 at 04:27:47PM +0200, Cédric Le Goater wrote:
>> MMIO emulation reads the last instruction executed by the guest
>> and then emulates. If the guest is running in Little Endian mode,
>> the instruction needs to be byte-swapped before being emulated.
>>
>> This patch stores the last instruction in the endian order of the
>> host, primarily doing a byte-swap if needed. The common code
>> which fetches 'last_inst' uses a helper routine kvmppc_need_byteswap().
>> and the exit paths for the Book3S PV and HR guests use their own
>> version in assembly.
>>
>> Finally, kvmppc_emulate_instruction() uses kvmppc_is_bigendian()
>> to define in which endian order the mmio needs to be done.
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>
> [snip]
>
>> + ld r0, VCPU_MSR(r9)
>> +
>> + /* r10 = vcpu->arch.msr & MSR_LE */
>> + rldicl. r10, r0, 0, 63
>
> I would have written:
>
> andi. r10, r0, MSR_LE
>
> which doesn't need the comment, but really the two are equivalent.
Indeed, this is even better ... rldicl is a vestige of an earlier version
of this patch.
>> @@ -232,6 +231,7 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
>> int sprn = get_sprn(inst);
>> enum emulation_result emulated = EMULATE_DONE;
>> int advance = 1;
>> + int is_bigendian = kvmppc_is_bigendian(vcpu);
>>
>> /* this default type might be overwritten by subcategories */
>> kvmppc_set_exit_type(vcpu, EMULATED_INST_EXITS);
>> @@ -266,47 +266,53 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
>> advance = 0;
>> break;
>> case OP_31_XOP_LWZX:
>> - emulated = kvmppc_handle_load(run, vcpu, rt, 4, 1);
>> + emulated = kvmppc_handle_load(run, vcpu, rt, 4,
>> + is_bigendian);
>
> I see you're still hitting all the call sites of kvmppc_handle_load(),
> kvmppc_handle_loads() and kvmppc_handle_store(), rather than putting
> the big-endian test inside kvmppc_handle_load() and
> kvmppc_handle_store(), as in this untested patch:
>
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index f55e14c..171bce6 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -625,9 +625,13 @@ static void kvmppc_complete_mmio_load(struct kvm_vcpu *vcpu,
> }
>
> int kvmppc_handle_load(struct kvm_run *run, struct kvm_vcpu *vcpu,
> - unsigned int rt, unsigned int bytes, int is_bigendian)
> + unsigned int rt, unsigned int bytes, int not_reverse)
> {
> int idx, ret;
> + int is_bigendian = not_reverse;
> +
> + if (!kvmppc_is_bigendian(vcpu))
> + is_bigendian = !not_reverse;
>
> if (bytes > sizeof(run->mmio.data)) {
> printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__,
> @@ -662,21 +666,25 @@ int kvmppc_handle_load(struct kvm_run *run, struct kvm_vcpu *vcpu,
>
> /* Same as above, but sign extends */
> int kvmppc_handle_loads(struct kvm_run *run, struct kvm_vcpu *vcpu,
> - unsigned int rt, unsigned int bytes, int is_bigendian)
> + unsigned int rt, unsigned int bytes, int not_reverse)
> {
> int r;
>
> vcpu->arch.mmio_sign_extend = 1;
> - r = kvmppc_handle_load(run, vcpu, rt, bytes, is_bigendian);
> + r = kvmppc_handle_load(run, vcpu, rt, bytes, not_reverse);
>
> return r;
> }
>
> int kvmppc_handle_store(struct kvm_run *run, struct kvm_vcpu *vcpu,
> - u64 val, unsigned int bytes, int is_bigendian)
> + u64 val, unsigned int bytes, int not_reverse)
> {
> void *data = run->mmio.data;
> int idx, ret;
> + int is_bigendian = not_reverse;
> +
> + if (!kvmppc_is_bigendian(vcpu))
> + is_bigendian = !not_reverse;
>
> if (bytes > sizeof(run->mmio.data)) {
> printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__,
>
> That seems simpler to me -- is there a reason not to do it that way?
No. A part from the fact I did not quite get what you meant the first
time we talked about it. This is more elegant. I will resend a -v2.
Thanks for the review Paul,
C.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-10-08 11:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-07 14:27 [PATCH 0/3] KVM: PPC: Book3S: MMIO support for Little Endian guests Cédric Le Goater
2013-10-07 14:27 ` [PATCH 1/3] KVM: PPC: Book3S: add helper routine to load guest instructions Cédric Le Goater
2013-10-07 14:27 ` [PATCH 2/3] KVM: PPC: Book3S: add helper routines to detect endian order Cédric Le Goater
2013-10-07 14:27 ` [PATCH 3/3] KVM: PPC: Book3S: MMIO emulation support for little endian guests Cédric Le Goater
2013-10-08 11:22 ` Paul Mackerras
2013-10-08 11:49 ` Cedric Le Goater
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).