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

[Tom Musta <tommusta@gmail.com>]

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)

> 
>> +    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).