From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Graf Subject: Re: [PATCH v5 3/6] KVM: PPC: Book3S: MMIO emulation support for little endian guests Date: Wed, 08 Jan 2014 18:34:09 +0100 Message-ID: <52CD8C11.9020000@suse.de> References: <1383672128-26795-1-git-send-email-clg@fr.ibm.com> <1383672128-26795-4-git-send-email-clg@fr.ibm.com> <08276663-4EB5-4912-9F90-803E88DE4D1A@suse.de> <52CD899C.20305@fr.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Paul Mackerras , kvm-ppc@vger.kernel.org, "kvm@vger.kernel.org mailing list" To: Cedric Le Goater Return-path: In-Reply-To: <52CD899C.20305@fr.ibm.com> Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 01/08/2014 06:23 PM, Cedric Le Goater wrote: > On 01/02/2014 09:22 PM, Alexander Graf wrote: >> On 05.11.2013, at 18:22, C=E9dric 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_byteswa= p(). >>> and the exit paths for the Book3S PV and HR guests use their own >>> version in assembly. >>> >>> Finally, the meaning of the 'is_bigendian' argument of the >>> routines kvmppc_handle_load() of kvmppc_handle_store() is >>> slightly changed to represent an eventual reverse operation. This >>> is used in conjunction with kvmppc_is_bigendian() to determine if >>> the instruction being emulated should be byte-swapped. >>> >>> Signed-off-by: C=E9dric Le Goater >>> Signed-off-by: Paul Mackerras >>> --- >>> >>> Changes in v5: >>> >>> - changed register usage slightly (paulus@samba.org) >>> - added #ifdef CONFIG_PPC64 in book3s_segment.S (paulus@samba.org) >>> >>> 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 | 9 ++++++++- >>> arch/powerpc/include/asm/kvm_ppc.h | 10 +++++----- >>> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 9 +++++++++ >>> arch/powerpc/kvm/book3s_segment.S | 9 +++++++++ >>> arch/powerpc/kvm/emulate.c | 1 - >>> arch/powerpc/kvm/powerpc.c | 16 ++++++++++++---- >>> 6 files changed, 43 insertions(+), 11 deletions(-) >>> >>> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/i= nclude/asm/kvm_book3s.h >>> index 22ec875..ac06434 100644 >>> --- a/arch/powerpc/include/asm/kvm_book3s.h >>> +++ b/arch/powerpc/include/asm/kvm_book3s.h >>> @@ -283,7 +283,14 @@ static inline bool kvmppc_is_bigendian(struct = kvm_vcpu *vcpu) >>> static inline int kvmppc_ld32(struct kvm_vcpu *vcpu, ulong *eaddr, >>> u32 *ptr, bool data) >>> { >>> - return kvmppc_ld(vcpu, eaddr, sizeof(u32), ptr, data); >>> + int ret; >>> + >>> + ret =3D kvmppc_ld(vcpu, eaddr, sizeof(u32), ptr, data); >>> + >>> + if (kvmppc_need_byteswap(vcpu)) >>> + *ptr =3D swab32(*ptr); >>> + >>> + return ret; >>> } >>> >>> static inline u32 kvmppc_get_last_inst(struct kvm_vcpu *vcpu) >>> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/incl= ude/asm/kvm_ppc.h >>> index b15554a..3769a13 100644 >>> --- a/arch/powerpc/include/asm/kvm_ppc.h >>> +++ b/arch/powerpc/include/asm/kvm_ppc.h >>> @@ -53,13 +53,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); >>> + unsigned int rt, unsigned int bytes, >>> + int not_reverse); >>> extern 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); >>> 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 not_reverse); >>> >>> extern int kvmppc_emulate_instruction(struct kvm_run *run, >>> struct kvm_vcpu *vcpu); >>> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc= /kvm/book3s_hv_rmhandlers.S >>> index 77f1baa..89d4fbe 100644 >>> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S >>> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S >>> @@ -1404,10 +1404,19 @@ fast_interrupt_c_return: >>> lwz r8, 0(r10) >>> mtmsrd r3 >>> >>> + andi. r0, r11, MSR_LE >>> + >>> /* Store the result */ >>> stw r8, VCPU_LAST_INST(r9) >>> >>> + beq after_inst_store >>> + >>> + /* Swap and store the result */ >>> + addi r4, r9, VCPU_LAST_INST >>> + stwbrx r8, 0, r4 >>> + >> On v4 Paul mentioned that it would be dramatically more simple to lo= ad >> last_inst with host endianness and do any required fixups in >> kvmppc_get_last_inst() and I tend to agree. > Hmm, I am confused ... This is what the above code is doing : loading= the > guest last_inst with host endianness. Anyhow, I think I get what you = mean. > >> That also renders patch 1/6 moot, as you would simply always have a >> variable with the last_inst in host endianness and swap it regardles= s. >> >> Sorry to make you jump through so many iterations, but getting this >> right is incredibly hard. > It's ok. We are exploring alternatives. I rather talk about it and ge= t > this done. > >> Please rework the patches to not require any asm changes. > ok. I will send a patch without the SLE support for which I think I d= on't > fully understand the consequences. > >>> /* 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/b= ook3s_segment.S >>> index 1abe478..a942390 100644 >>> --- a/arch/powerpc/kvm/book3s_segment.S >>> +++ b/arch/powerpc/kvm/book3s_segment.S >>> @@ -289,6 +289,15 @@ ld_last_inst: >>> #endif >>> stw r0, SVCPU_LAST_INST(r13) >>> >>> +#ifdef CONFIG_PPC64 >>> + andi. r9, r4, MSR_LE >>> + beq no_ld_last_inst >>> + >>> + /* swap and store the result */ >>> + addi r9, r13, SVCPU_LAST_INST >>> + stwbrx r0, 0, r9 >>> +#endif >>> + >>> no_ld_last_inst: >>> >>> /* Unset guest mode */ >>> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.= c >>> index 751cd45..5e38004 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 par= ticular core >>> * from opcode tables in the future. */ >>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.= c >>> index 07c0106..6950f2b 100644 >>> --- a/arch/powerpc/kvm/powerpc.c >>> +++ b/arch/powerpc/kvm/powerpc.c >>> @@ -625,9 +625,13 @@ static void kvmppc_complete_mmio_load(struct k= vm_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) >> I'm not really happy with the "not_reverse" name. In the scope of th= is patch it's >> reasonably obvious what it tries to describe, but consider someone l= ooking at this >> function without a clue where we're swizzling endianness. The name d= oesn't even mention >> endianness. >> >> Naming is really hard. > yes. we should leave 'is_bigendian'. > >> How does "is_default_endian" sound? Then you can change the code bel= ow ... >> >>> { >>> int idx, ret; >>> + int is_bigendian =3D not_reverse; >>> + >>> + if (!kvmppc_is_bigendian(vcpu)) >>> + is_bigendian =3D !not_reverse; >> ... to >> >> if (kvmppc_is_bigendian(vcpu)) { >> /* Default endianness is "big endian". */ >> is_bigendian =3D is_default_endian; >> } else { >> /* Default endianness is "little endian". */ >> is_bigendian =3D !is_default_endian; >> } >> >> and suddenly things become reasonably clear for everyone I'd hope. > I think something like : > > + if (kvmppc_need_byteswap(vcpu)) > + is_bigendian =3D !is_bigendian; > + > > has a small footprint and is clear enough ? > > Thanks for the inputs, a (single) patch follows Not really. The argument means "use the normal endianness you would=20 usually use for memory access". It doesn't mean little or big endian=20 yet, as that's what we determine later. Keep in mind that gcc is really good at optimizing code like this, so=20 please don't try to be smart with variable reusage or any of the likes.= =20 In assembly this will all look identical, but the C representation=20 should be as self-documenting as possible. Alex