From mboxrd@z Thu Jan 1 00:00:00 1970 From: Cedric Le Goater Subject: Re: [PATCH v7] KVM: PPC: Book3S: MMIO emulation support for little endian guests Date: Thu, 09 Jan 2014 11:33:53 +0100 Message-ID: <52CE7B11.8000603@fr.ibm.com> References: <52CD899C.20305@fr.ibm.com> <1389261756-2767-1-git-send-email-clg@fr.ibm.com> <2A8A3EA7-E7E9-4B1B-B68F-18B44699A22E@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Paul Mackerras , kvm-ppc@vger.kernel.org, "kvm@vger.kernel.org mailing list" To: Alexander Graf Return-path: Received: from e06smtp15.uk.ibm.com ([195.75.94.111]:57833 "EHLO e06smtp15.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752098AbaAIKeD (ORCPT ); Thu, 9 Jan 2014 05:34:03 -0500 Received: from /spool/local by e06smtp15.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 9 Jan 2014 10:34:01 -0000 In-Reply-To: <2A8A3EA7-E7E9-4B1B-B68F-18B44699A22E@suse.de> Sender: kvm-owner@vger.kernel.org List-ID: On 01/09/2014 11:17 AM, Alexander Graf wrote: >=20 > On 09.01.2014, at 11:02, C=E9dric Le Goater wrote: >=20 >> 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=E9dric Le Goater >> --- >> >> How's that ? As the changes were small, I kept them in one patch=20 >> but I can split if necessary.=20 >> >> This patch was tested for Big Endian and Little Endian HV guests=20 >> 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@samba.org) >> - added #ifdef CONFIG_PPC64 in book3s_segment.S (paulus@samba.org) >> - 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 Gra= f) >> >> 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=20 >> kvmppc_handle_load() by changing the is_bigendian parameter >> meaning. (Paul Mackerras) >> =09 >> 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/in= clude/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_vc= pu *vcpu) >> return vcpu->arch.pc; >> } >> >> +static inline bool kvmppc_need_byteswap(struct kvm_vcpu *vcpu) >> +{ >> + return (vcpu->arch.shared->msr & MSR_LE) !=3D (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 =3D kvmppc_get_pc(vcpu); >> @@ -280,7 +291,7 @@ static inline u32 kvmppc_get_last_inst(struct kv= m_vcpu *vcpu) >> if (vcpu->arch.last_inst =3D=3D KVM_INST_FETCH_FAILED) >> kvmppc_ld(vcpu, &pc, sizeof(u32), &vcpu->arch.last_inst, false); >> >> - return vcpu->arch.last_inst; >=20 > I would prefer if you just explicitly put the contents of kvmppc_byte= swap_last_inst() here. >=20 >> + return kvmppc_byteswap_last_inst(vcpu); >> } >> >> /* >> @@ -297,7 +308,7 @@ static inline u32 kvmppc_get_last_sc(struct kvm_= vcpu *vcpu) >=20 > ... 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.=20 >> if (vcpu->arch.last_inst =3D=3D 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/inclu= de/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) !=3D !!is_store) >> + if (instruction_is_store(kvmppc_byteswap_last_inst(vcpu)) !=3D !!i= s_store) >=20 > This can safely be kvmppc_get_last_inst() at this point, because we'r= e definitely not hitting the KVM_INST_FETCH_FAILED code path anymore. OK. Thanks, C.=20