On 01/09/2014 11:17 AM, Alexander Graf wrote:
>
> 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.
OK. I am taking the patch you just sent and work on a v8.
>> 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.
OK.
Thanks,
C.
--
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
