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

[Alexander Graf <agraf@suse.de>]

On 04/04/2014 08:58 AM, Alexey Kardashevskiy wrote:
> 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.

Anything that 970 can work with should be handled in the 970 case as 
well, yes. Keep in mind that we also have POWER5 that can run PR KVM.

Maybe it'd be better to extract those into helper functions that we just 
call from the respective proc init functions? That way we at least stay 
consistent.


Alex