On 09.01.2014, at 11:02, Cédric Le Goater <clg@xxxxxxxxxx> wrote:
> MMIO emulation reads the last instruction executed by the guest
> and then emulates. If the guest is running in Little Endian order,
> or more generally in a different endian order of the host, the
> instruction needs to be byte-swapped before being emulated.
>
> This patch adds a helper routine which tests the endian order of
> the host and the guest in order to decide whether a byteswap is
> needed or not. It is then used to byteswap the last instruction
> of the guest in the endian order of the host before MMIO emulation
> is performed.
>
> Finally, kvmppc_handle_load() of kvmppc_handle_store() are modified
> to reverse the endianness of the MMIO if required.
>
> Signed-off-by: Cédric Le Goater <clg@xxxxxxxxxx>
> ---
>
> How's that ? As the changes were small, I kept them in one patch
> but I can split if necessary.
>
> This patch was tested for Big Endian and Little Endian HV guests
> and Big Endian PR guests on 3.13 (plus a h_set_mode hack)
>
> Cheers,
>
> C.
>
> Changes in v7:
>
> - replaced is_bigendian by is_default_endian (Alexander Graf)
>
> Changes in v6:
>
> - removed asm changes (Alexander Graf)
> - byteswap last_inst when used in kvmppc_get_last_inst()
> - postponed Split Little Endian support
>
> Changes in v5:
>
> - changed register usage slightly (paulus@xxxxxxxxx)
> - added #ifdef CONFIG_PPC64 in book3s_segment.S (paulus@xxxxxxxxx)
> - added support for little endian host
> - added support for Split Little Endian (SLE)
>
> Changes in v4:
>
> - got rid of useless helper routine kvmppc_ld_inst(). (Alexander Graf)
>
> Changes in v3:
>
> - moved kvmppc_need_byteswap() in kvmppc_ld32. It previously was in
> kvmppc_ld_inst(). (Alexander Graf)
>
> Changes in v2:
>
> - replaced rldicl. by andi. to test the MSR_LE bit in the guest
> exit paths. (Paul Mackerras)
>
> - moved the byte swapping logic to kvmppc_handle_load() and
> kvmppc_handle_load() by changing the is_bigendian parameter
> meaning. (Paul Mackerras)
>
> arch/powerpc/include/asm/kvm_book3s.h | 15 +++++++++++++--
> arch/powerpc/include/asm/kvm_ppc.h | 7 ++++---
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 2 +-
> arch/powerpc/kvm/emulate.c | 1 -
> arch/powerpc/kvm/powerpc.c | 28 ++++++++++++++++++++++++----
> 5 files changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
> index bc23b1ba7980..00499f5f16bc 100644
> --- a/arch/powerpc/include/asm/kvm_book3s.h
> +++ b/arch/powerpc/include/asm/kvm_book3s.h
> @@ -271,6 +271,17 @@ 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) != (MSR_KERNEL & MSR_LE);
> +}
> +
> +static inline u32 kvmppc_byteswap_last_inst(struct kvm_vcpu *vcpu)
> +{
> + 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)
> {
> ulong pc = kvmppc_get_pc(vcpu);
> @@ -280,7 +291,7 @@ static inline u32 kvmppc_get_last_inst(struct kvm_vcpu *vcpu)
> if (vcpu->arch.last_inst == KVM_INST_FETCH_FAILED)
> kvmppc_ld(vcpu, &pc, sizeof(u32), &vcpu->arch.last_inst, false);
>
> - return vcpu->arch.last_inst;
I would prefer if you just explicitly put the contents of kvmppc_byteswap_last_inst() here.
> + return kvmppc_byteswap_last_inst(vcpu);
> }
>
> /*
> @@ -297,7 +308,7 @@ static inline u32 kvmppc_get_last_sc(struct kvm_vcpu *vcpu)
... and instead converge the two functions into one. In fact, let me quickly hack up a patch for that.
> if (vcpu->arch.last_inst == KVM_INST_FETCH_FAILED)
> kvmppc_ld(vcpu, &pc, sizeof(u32), &vcpu->arch.last_inst, false);
>
> - return vcpu->arch.last_inst;
> + return kvmppc_byteswap_last_inst(vcpu);
> }
>
> static inline ulong kvmppc_get_fault_dar(struct kvm_vcpu *vcpu)
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index c8317fbf92c4..629277df4798 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -54,12 +54,13 @@ extern void kvmppc_handler_highmem(void);
> extern void kvmppc_dump_vcpu(struct kvm_vcpu *vcpu);
> extern int kvmppc_handle_load(struct kvm_run *run, struct kvm_vcpu *vcpu,
> unsigned int rt, unsigned int bytes,
> - int is_bigendian);
> + int is_default_endian);
> extern int kvmppc_handle_loads(struct kvm_run *run, struct kvm_vcpu *vcpu,
> unsigned int rt, unsigned int bytes,
> - int is_bigendian);
> + int is_default_endian);
> extern 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 is_default_endian);
>
> extern int kvmppc_emulate_instruction(struct kvm_run *run,
> struct kvm_vcpu *vcpu);
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 79e992d8c823..ff10fba29878 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -562,7 +562,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(vcpu->arch.last_inst) != !!is_store)
> + if (instruction_is_store(kvmppc_byteswap_last_inst(vcpu)) != !!is_store)
This can safely be kvmppc_get_last_inst() at this point, because we're definitely not hitting the KVM_INST_FETCH_FAILED code path anymore.
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
