From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40045) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WVy55-0006fS-Rj for qemu-devel@nongnu.org; Fri, 04 Apr 2014 02:58:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WVy4u-00086y-Uj for qemu-devel@nongnu.org; Fri, 04 Apr 2014 02:58:27 -0400 Received: from mail-pa0-f52.google.com ([209.85.220.52]:47289) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WVy4u-00086r-MG for qemu-devel@nongnu.org; Fri, 04 Apr 2014 02:58:16 -0400 Received: by mail-pa0-f52.google.com with SMTP id rd3so3049146pab.11 for ; Thu, 03 Apr 2014 23:58:15 -0700 (PDT) Message-ID: <533E5802.6000900@ozlabs.ru> Date: Fri, 04 Apr 2014 17:58:10 +1100 From: Alexey Kardashevskiy 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> <533DB2AE.6040003@gmail.com> In-Reply-To: <533DB2AE.6040003@gmail.com> Content-Type: text/plain; charset=KOI8-R 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: Tom Musta , Alexander Graf , qemu-devel@nongnu.org Cc: qemu-ppc@nongnu.org On 04/04/2014 06:12 AM, Tom Musta wrote: > 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) Yes, this is better. Thanks! >> >>> + 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? Nope. linux-headers/asm-powerpc/kvm.h: #define KVM_REG_PPC_TM_CR (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x60) #define KVM_REG_PPC_TM_LR (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x61) #define KVM_REG_PPC_TM_CTR (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x62) #define KVM_REG_PPC_TM_FPSCR (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x63) #define KVM_REG_PPC_TM_AMR (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x64) #define KVM_REG_PPC_TM_PPR (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x65) #define KVM_REG_PPC_TM_VRSAVE (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x66) #define KVM_REG_PPC_TM_VSCR (KVM_REG_PPC_TM | KVM_REG_SIZE_U32 | 0x67) #define KVM_REG_PPC_TM_DSCR (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x68) #define KVM_REG_PPC_TM_TAR (KVM_REG_PPC_TM | KVM_REG_SIZE_U64 | 0x69) For the reason unknown (Paul is not in the office to ask) all TM-related registers are defined as 64bit and only VSCR is 32bit. And this is in the host kernel already. >>> 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). Yes. VRSAVE, SIAR, SDAR are even defined for 970 (Alex, should I add them to 970 definitions?), PPR is not defined in any 970 spec but is in 2.04..2.07. -- Alexey