Re: [Qemu-devel] [PATCH 3/4] KVM: PPC: Support POWER8 registers

[Alexey Kardashevskiy]

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:
> 
> <snip>
> 
>>
>> Tom, please have a look through this as well :).
>>
>>> ---
> 
> <snip>
> 
>>> --- 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