From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59571) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WVn4R-0004XL-Rw for qemu-devel@nongnu.org; Thu, 03 Apr 2014 15:13:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WVn4H-0005eb-3R for qemu-devel@nongnu.org; Thu, 03 Apr 2014 15:13:03 -0400 Message-ID: <533DB2AE.6040003@gmail.com> Date: Thu, 03 Apr 2014 14:12:46 -0500 From: Tom Musta MIME-Version: 1.0 References: <1396530891-6352-1-git-send-email-aik@ozlabs.ru> <1396530891-6352-4-git-send-email-aik@ozlabs.ru> <533D6334.2080208@suse.de> In-Reply-To: <533D6334.2080208@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/4] KVM: PPC: Support POWER8 registers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf , Alexey Kardashevskiy , qemu-devel@nongnu.org Cc: qemu-ppc@nongnu.org On 4/3/2014 8:33 AM, Alexander Graf wrote: > > On 03.04.14 15:14, Alexey Kardashevskiy wrote: >> This enabled KVM and migration support for a number of POWER8 registers: > > Tom, please have a look through this as well :). > >> --- >> --- a/target-ppc/cpu.h >> +++ b/target-ppc/cpu.h >> @@ -426,6 +426,8 @@ struct ppc_slb_t { >> #define MSR_TAG 62 /* Tag-active mode (POWERx ?) */ >> #define MSR_ISF 61 /* Sixty-four-bit interrupt mode on 630 */ >> #define MSR_SHV 60 /* hypervisor state hflags */ >> +#define MSR_TS 33 /* Transactional state, 2 bits (Book3s) */ > > 2 bits means you want to add another define or at least a comment at bit 34. I find it rather counterintuitive to declare bit 33 MSR_TS as 2-bit wide too - you'd expect (3 << MSR_TS) gives you the right mask, but then it'd have to be 34, no? Is this better? #define MSR_TS0 34 #define MSR_TS1 33 You should also add a decoder: #define msr_ts ((env->msr >> MSR_TS1) & 3) > >> + target_ulong tm_gpr[32]; >> + ppc_avr_t tm_vsr[64]; >> + uint64_t tm_cr; >> + uint64_t tm_lr; >> + uint64_t tm_ctr; >> + uint64_t tm_fpscr; >> + uint64_t tm_amr; >> + uint64_t tm_ppr; >> + uint64_t tm_vrsave; >> + uint32_t tm_vscr; >> + uint64_t tm_dscr; >> + uint64_t tm_tar; >> }; If vscr is declared as 32 bits, should CR and VRSAVE also be 32-bits? >> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c >> index 9974b10..ead69fa 100644 >> --- a/target-ppc/kvm.c >> +++ b/target-ppc/kvm.c >> @@ -866,6 +866,25 @@ int kvm_arch_put_registers(CPUState *cs, int level) >> } >> #ifdef TARGET_PPC64 >> + if ((cpu->env.msr >> MSR_TS) & 3) { > > Ah, it works because you're shifting the other direction. That works. How about we just introduce an msr_ts() helper similar to the other lower case helpers to make this obvious? > Agreed. >> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c >> index 1627bb0..4b20c29 100644 >> --- a/target-ppc/translate_init.c >> +++ b/target-ppc/translate_init.c >> @@ -7025,14 +7025,22 @@ static void init_proc_POWER7 (CPUPPCState *env) >> SPR_NOACCESS, SPR_NOACCESS, >> &spr_read_generic, SPR_NOACCESS, >> 0x80800000); >> - spr_register(env, SPR_VRSAVE, "SPR_VRSAVE", >> - &spr_read_generic, &spr_write_generic, >> - &spr_read_generic, &spr_write_generic, >> - 0x00000000); >> - spr_register(env, SPR_PPR, "PPR", >> - &spr_read_generic, &spr_write_generic, >> - &spr_read_generic, &spr_write_generic, >> - 0x00000000); >> + spr_register_kvm(env, SPR_VRSAVE, "VRSAVE", >> + &spr_read_generic, &spr_write_generic, >> + &spr_read_generic, &spr_write_generic, >> + KVM_REG_PPC_VRSAVE, 0x00000000); >> + spr_register_kvm(env, SPR_PPR, "PPR", >> + &spr_read_generic, &spr_write_generic, >> + &spr_read_generic, &spr_write_generic, >> + KVM_REG_PPC_PPR, 0x00000000); >> + spr_register_kvm(env, SPR_BOOK3S_SIAR, "SIAR", >> + &spr_read_generic, &spr_write_generic, >> + &spr_read_generic, &spr_write_generic, >> + KVM_REG_PPC_SIAR, 0x00000000); >> + spr_register_kvm(env, SPR_BOOK3S_SDAR, "SDAR", >> + &spr_read_generic, &spr_write_generic, >> + &spr_read_generic, &spr_write_generic, >> + KVM_REG_PPC_SDAR, 0x00000000); >> /* Logical partitionning */ >> spr_register_kvm(env, SPR_LPCR, "LPCR", >> SPR_NOACCESS, SPR_NOACCESS, > These need to go into P8 as well? (see my comment for patch 2).