From mboxrd@z Thu Jan 1 00:00:00 1970 From: Cedric Le Goater Subject: Re: [PATCH v5 3/6] KVM: PPC: Book3S: MMIO emulation support for little endian guests Date: Wed, 08 Jan 2014 18:23:40 +0100 Message-ID: <52CD899C.20305@fr.ibm.com> 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> 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: In-Reply-To: <08276663-4EB5-4912-9F90-803E88DE4D1A@suse.de> Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 01/02/2014 09:22 PM, Alexander Graf wrote: >=20 > On 05.11.2013, at 18:22, C=E9dric Le Goater wrote: >=20 >> MMIO emulation reads the last instruction executed by the guest=20 >> and then emulates. If the guest is running in Little Endian mode,=20 >> 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, 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=20 >> 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/in= clude/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 k= vm_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/inclu= de/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 >> + >=20 > On v4 Paul mentioned that it would be dramatically more simple to loa= d=20 > last_inst with host endianness and do any required fixups in=20 > kvmppc_get_last_inst() and I tend to agree.=20 Hmm, I am confused ... This is what the above code is doing : loading t= he=20 guest last_inst with host endianness. Anyhow, I think I get what you me= an. > That also renders patch 1/6 moot, as you would simply always have a=20 > variable with the last_inst in host endianness and swap it regardless= =2E > > Sorry to make you jump through so many iterations, but getting this=20 > right is incredibly hard. It's ok. We are exploring alternatives. I rather talk about it and get=20 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 don= 't=20 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/bo= ok3s_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 part= icular 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 kv= m_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) >=20 > I'm not really happy with the "not_reverse" name. In the scope of thi= s patch it's=20 > reasonably obvious what it tries to describe, but consider someone lo= oking at this=20 > function without a clue where we're swizzling endianness. The name do= esn't even mention=20 > endianness. >=20 > Naming is really hard. yes. we should leave 'is_bigendian'.=20 > How does "is_default_endian" sound? Then you can change the code belo= w ... > >> { >> int idx, ret; >> + int is_bigendian =3D not_reverse; >> + >> + if (!kvmppc_is_bigendian(vcpu)) >> + is_bigendian =3D !not_reverse; >=20 > ... to >=20 > 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; > } >=20 > 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 ?=20 Thanks for the inputs, a (single) patch follows=20 C. > Alex >=20 >> >> 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, st= ruct 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 =3D 1; >> - r =3D kvmppc_handle_load(run, vcpu, rt, bytes, is_bigendian); >> + r =3D 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_bigendi= an) >> + u64 val, unsigned int bytes, int not_reverse) >> { >> void *data =3D run->mmio.data; >> int idx, ret; >> + int is_bigendian =3D not_reverse; >> + >> + if (!kvmppc_is_bigendian(vcpu)) >> + is_bigendian =3D !not_reverse; >> >> if (bytes > sizeof(run->mmio.data)) { >> printk(KERN_ERR "%s: bad MMIO length: %d\n", __func__, >> --=20 >> 1.7.10.4 >> >=20