Re: [Qemu-devel] [PATCH 4/9] target-ppc: Implement "compat" CPU option

[Alexander Graf <agraf@suse.de>]

On 21.05.14 09:59, Alexey Kardashevskiy wrote:
> On 05/21/2014 05:29 PM, Alexander Graf wrote:
>> On 21.05.14 08:57, Alexey Kardashevskiy wrote:
>>> On 05/17/2014 06:47 AM, Alexander Graf wrote:
>>>> On 16.05.14 17:17, Alexey Kardashevskiy wrote:
>>>>> On 05/17/2014 12:05 AM, Alexander Graf wrote:
>>>>>> On 15.05.14 13:28, Alexey Kardashevskiy wrote:
>>>>>>> This adds basic support for the "compat" CPU option. By specifying
>>>>>>> the compat property, the user can manually switch guest CPU mode from
>>>>>>> "raw" to "architected".
>>>>>>>
>>>>>>> Since the actual compatibility mode is not implemented yet, this does
>>>>>>> not change the existing behavior.
>>>>>>>
>>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>>> ---
>>>>>>>      hw/ppc/spapr.c              | 15 ++++++++++++++-
>>>>>>>      target-ppc/cpu-models.h     |  6 ++++++
>>>>>>>      target-ppc/cpu-qom.h        |  2 ++
>>>>>>>      target-ppc/cpu.h            |  3 +++
>>>>>>>      target-ppc/translate_init.c | 35 +++++++++++++++++++++++++++++++++++
>>>>>>>      5 files changed, 60 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>>>> index 0f8bd95..61d0675 100644
>>>>>>> --- a/hw/ppc/spapr.c
>>>>>>> +++ b/hw/ppc/spapr.c
>>>>>>> @@ -212,7 +212,8 @@ static int spapr_fixup_cpu_dt(void *fdt,
>>>>>>> sPAPREnvironment *spapr)
>>>>>>>            CPU_FOREACH(cpu) {
>>>>>>>              DeviceClass *dc = DEVICE_GET_CLASS(cpu);
>>>>>>> -        int index = ppc_get_vcpu_dt_id(POWERPC_CPU(cpu));
>>>>>>> +        PowerPCCPU *pcpu = POWERPC_CPU(cpu);
>>>>>>> +        int index = ppc_get_vcpu_dt_id(pcpu);
>>>>>>>              uint32_t associativity[] = {cpu_to_be32(0x5),
>>>>>>>                                          cpu_to_be32(0x0),
>>>>>>>                                          cpu_to_be32(0x0),
>>>>>>> @@ -249,6 +250,14 @@ static int spapr_fixup_cpu_dt(void *fdt,
>>>>>>> sPAPREnvironment *spapr)
>>>>>>>                  return ret;
>>>>>>>              }
>>>>>>>      +        if (pcpu->cpu_version) {
>>>>>>> +            ret = fdt_setprop(fdt, offset, "cpu-version",
>>>>>>> +                              &pcpu->cpu_version,
>>>>>>> sizeof(pcpu->cpu_version));
>>>>>>> +            if (ret < 0) {
>>>>>>> +                return ret;
>>>>>>> +            }
>>>>>>> +        }
>>>>>>> +
>>>>>>>              /* Build interrupt servers and gservers properties */
>>>>>>>              for (i = 0; i < smp_threads; i++) {
>>>>>>>                  servers_prop[i] = cpu_to_be32(index + i);
>>>>>>> @@ -1278,6 +1287,10 @@ static void ppc_spapr_init(QEMUMachineInitArgs
>>>>>>> *args)
>>>>>>>                  kvmppc_set_papr(cpu);
>>>>>>>              }
>>>>>>>      +        if (cpu->max_compat) {
>>>>>>> +            ppc_set_compat(cpu, cpu->max_compat);
>>>>>>> +        }
>>>>>>> +
>>>>>>>              xics_cpu_setup(spapr->icp, cpu);
>>>>>>>                qemu_register_reset(spapr_cpu_reset, cpu);
>>>>>>> diff --git a/target-ppc/cpu-models.h b/target-ppc/cpu-models.h
>>>>>>> index 33d2c51..45c0028 100644
>>>>>>> --- a/target-ppc/cpu-models.h
>>>>>>> +++ b/target-ppc/cpu-models.h
>>>>>>> @@ -753,4 +753,10 @@ enum {
>>>>>>>          POWERPC_SVR_8641D              = 0x80900121,
>>>>>>>      };
>>>>>>>      +/* Processor Compatibility mask (PCR) */
>>>>>>> +enum {
>>>>>>> +    POWERPC_ISA_COMPAT_2_05 = 0x02,
>>>>>>> +    POWERPC_ISA_COMPAT_2_06 = 0x04,
>>>>>>> +};
>>>>>>> +
>>>>>>>      #endif
>>>>>>> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
>>>>>>> index 2a8515b..dfd1419 100644
>>>>>>> --- a/target-ppc/cpu-qom.h
>>>>>>> +++ b/target-ppc/cpu-qom.h
>>>>>>> @@ -83,6 +83,7 @@ typedef struct PowerPCCPUClass {
>>>>>>>       * @env: #CPUPPCState
>>>>>>>       * @cpu_dt_id: CPU index used in the device tree. KVM uses this
>>>>>>> index too
>>>>>>>       * @max_compat: Maximal supported logical PVR from the command line
>>>>>>> + * @cpu_version: Current logical PVR, zero if in "raw" mode
>>>>>>>       *
>>>>>>>       * A PowerPC CPU.
>>>>>>>       */
>>>>>>> @@ -94,6 +95,7 @@ struct PowerPCCPU {
>>>>>>>          CPUPPCState env;
>>>>>>>          int cpu_dt_id;
>>>>>>>          uint32_t max_compat;
>>>>>>> +    uint32_t cpu_version;
>>>>>>>      };
>>>>>>>        static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
>>>>>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>>>>>> index d498340..f61675a 100644
>>>>>>> --- a/target-ppc/cpu.h
>>>>>>> +++ b/target-ppc/cpu.h
>>>>>>> @@ -1123,6 +1123,8 @@ void ppc_store_msr (CPUPPCState *env, target_ulong
>>>>>>> value);
>>>>>>>        void ppc_cpu_list (FILE *f, fprintf_function cpu_fprintf);
>>>>>>>      +void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version);
>>>>>>> +
>>>>>>>      /* Time-base and decrementer management */
>>>>>>>      #ifndef NO_CPU_IO_DEFS
>>>>>>>      uint64_t cpu_ppc_load_tbl (CPUPPCState *env);
>>>>>>> @@ -1338,6 +1340,7 @@ static inline int cpu_mmu_index (CPUPPCState *env)
>>>>>>>      #define SPR_LPCR              (0x13E)
>>>>>>>      #define SPR_BOOKE_DVC2        (0x13F)
>>>>>>>      #define SPR_BOOKE_TSR         (0x150)
>>>>>>> +#define SPR_PCR               (0x152)
>>>>>>>      #define SPR_BOOKE_TCR         (0x154)
>>>>>>>      #define SPR_BOOKE_TLB0PS      (0x158)
>>>>>>>      #define SPR_BOOKE_TLB1PS      (0x159)
>>>>>>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>>>>>>> index 6f61b34..c4bd5de 100644
>>>>>>> --- a/target-ppc/translate_init.c
>>>>>>> +++ b/target-ppc/translate_init.c
>>>>>>> @@ -7803,6 +7803,15 @@ static void init_proc_POWER7 (CPUPPCState *env)
>>>>>>>          /* Can't find information on what this should be on reset.  This
>>>>>>>           * value is the one used by 74xx processors. */
>>>>>>>          vscr_init(env, 0x00010000);
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * Register PCR to report POWERPC_EXCP_PRIV_REG instead of
>>>>>>> +     * POWERPC_EXCP_INVAL_SPR.
>>>>>>> +     */
>>>>>>> +    spr_register(env, SPR_PCR, "PCR",
>>>>>>> +                 SPR_NOACCESS, SPR_NOACCESS,
>>>>>>> +                 SPR_NOACCESS, SPR_NOACCESS,
>>>>>>> +                 0x00000000);
>>>>>>>      }
>>>>>>>        POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
>>>>>>> @@ -8880,6 +8889,32 @@ static void ppc_cpu_unrealizefn(DeviceState *dev,
>>>>>>> Error **errp)
>>>>>>>          }
>>>>>>>      }
>>>>>>>      +void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version)
>>>>>>> +{
>>>>>>> +    CPUPPCState *env = &cpu->env;
>>>>>>> +
>>>>>>> +    cpu->cpu_version = cpu_version;
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * Calculate PCR value from virtual PVR.
>>>>>>> +     * TODO: support actual compatibility in TCG.
>>>>>>> +     */
>>>>>>> +    switch (cpu_version) {
>>>>>>> +    case CPU_POWERPC_LOGICAL_2_05:
>>>>>>> +        env->spr[SPR_PCR] = POWERPC_ISA_COMPAT_2_05;
>>>>>>> +        break;
>>>>>>> +    case CPU_POWERPC_LOGICAL_2_06:
>>>>>>> +        env->spr[SPR_PCR] = POWERPC_ISA_COMPAT_2_06;
>>>>>>> +        break;
>>>>>>> +    case CPU_POWERPC_LOGICAL_2_06_PLUS:
>>>>>>> +        env->spr[SPR_PCR] = POWERPC_ISA_COMPAT_2_06;
>>>>>>> +        break;
>>>>>> Don't we have to disable all those fancy p8 features too?
>>>>>>
>>>>>> #define   PCR_VEC_DIS   (1ul << (63-0)) /* Vec. disable (bit NA since
>>>>>> POWER8) */
>>>>>> #define   PCR_VSX_DIS   (1ul << (63-1)) /* VSX disable (bit NA since
>>>>>> POWER8) */
>>>>>> #define   PCR_TM_DIS    (1ul << (63-2)) /* Trans. memory disable
>>>>>> (POWER8) */
>>>>> Yep, makes sense to set "TM" for POWER < 8 and VSX+VEC for POWER < 7.
>>>>>
>>>>>> In fact, we probably want those bits always set when compat < power8.
>>>>> Why? VSX is present on power7. VEC is defined in 2.06 too.
>>>> True. So how does pre-p8 treat those p8 and above bits?
>>> PowerISA 2.07 says if 2.06 mode is selected, the TM bit does not matter
>>> anymore - TM will be disabled because 2.06 does not define it at all. The
>>> same is true for VSX and 2.05 mode. So just setting a mode must be ok.
>> And for POWER8-on-POWER9 (or whatever it will be called) we can just set
>> PCR to the respective feature disable bits. I think that should work.
> I lost you here. I suspect it will also have "2.07" compatibility mode and
> we will use that instead of disabling feature one by one.

Considering that the architects added by-feature bits, I would assume 
they won't introduce a 2.07 bit but rather make every new feature 
disableable one by one. But maybe I'm wrong - the point is that either 
way will work and is future proof ;).


Alex