Re: [Qemu-devel] [RFC v1 3/3] target-microblaze: Convert use-fpu to a CPU property

[Alistair Francis]

On Fri, May 15, 2015 at 4:32 PM, Peter Crosthwaite
<address@hidden> wrote:
> On Thu, May 14, 2015 at 10:56 PM, Alistair Francis
> <address@hidden> wrote:
>> On Fri, May 15, 2015 at 3:52 PM, Peter Crosthwaite
>> <address@hidden> wrote:
>>> On Thu, May 14, 2015 at 10:48 PM, Alistair Francis
>>> <address@hidden> wrote:
>>>> On Fri, May 15, 2015 at 3:22 PM, Peter Crosthwaite
>>>> <address@hidden> wrote:
>>>>> On Wed, May 13, 2015 at 11:08 PM, Alistair Francis
>>>>> <address@hidden> wrote:
>>>>>> Originally the use-fpu PVR bits were manually set for each machine. This
>>>>>> is a hassle and difficult to read, instead set them based on the CPU
>>>>>> properties.
>>>>>>
>>>>>> Signed-off-by: Alistair Francis <address@hidden>
>>>>>> ---
>>>>>>
>>>>>>  hw/microblaze/petalogix_ml605_mmu.c |    4 ++--
>>>>>>  target-microblaze/cpu-qom.h         |    1 +
>>>>>>  target-microblaze/cpu.c             |   13 +++++++++++--
>>>>>>  target-microblaze/translate.c       |    6 +++---
>>>>>>  4 files changed, 17 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/microblaze/petalogix_ml605_mmu.c 
>>>>>> b/hw/microblaze/petalogix_ml605_mmu.c
>>>>>> index 48c264b..f4e1cc5 100644
>>>>>> --- a/hw/microblaze/petalogix_ml605_mmu.c
>>>>>> +++ b/hw/microblaze/petalogix_ml605_mmu.c
>>>>>> @@ -71,9 +71,8 @@ static void machine_cpu_reset(MicroBlazeCPU *cpu)
>>>>>>      env->pvr.regs[10] = 0x0e000000; /* virtex 6 */
>>>>>>      /* setup pvr to match kernel setting */
>>>>>>      env->pvr.regs[5] |= PVR5_DCACHE_WRITEBACK_MASK;
>>>>>> -    env->pvr.regs[0] |= PVR0_USE_FPU_MASK | PVR0_ENDI;
>>>>>> +    env->pvr.regs[0] |= PVR0_ENDI;
>>>>>>      env->pvr.regs[0] = (env->pvr.regs[0] & ~PVR0_VERSION_MASK) | (0x14 
>>>>>> << 8);
>>>>>> -    env->pvr.regs[2] ^= PVR2_USE_FPU2_MASK;
>>>>>
>>>>> So I think this here will clear PVR2_FPU2 ...
>>>>>
>>>>>>      env->pvr.regs[4] = 0xc56b8000;
>>>>>>      env->pvr.regs[5] = 0xc56be000;
>>>>>>  }
>>>>>> @@ -95,6 +94,7 @@ petalogix_ml605_init(MachineState *machine)
>>>>>>
>>>>>>      /* init CPUs */
>>>>>>      cpu = MICROBLAZE_CPU(object_new(TYPE_MICROBLAZE_CPU));
>>>>>> +    object_property_set_int(OBJECT(cpu), 2, "xlnx.use-fpu", 
>>>>>> &error_abort);
>>>>>
>>>>> But here you are setting use-fpu to 2. Is this a functional change for
>>>>> ML605 board?
>>>>>
>>>>>>      object_property_set_bool(OBJECT(cpu), true, "realized", 
>>>>>> &error_abort);
>>>>>>
>>>>>>      /* Attach emulated BRAM through the LMB.  */
>>>>>> diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
>>>>>> index 7bc5b81..ce3a574 100644
>>>>>> --- a/target-microblaze/cpu-qom.h
>>>>>> +++ b/target-microblaze/cpu-qom.h
>>>>>> @@ -62,6 +62,7 @@ typedef struct MicroBlazeCPU {
>>>>>>      /* Microblaze Configuration Settings */
>>>>>>      struct {
>>>>>>          bool stackproc;
>>>>>> +        uint8_t usefpu;
>>>>>>      } cfg;
>>>>>>
>>>>>>      CPUMBState env;
>>>>>> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
>>>>>> index c08da19..c3a9a5d 100644
>>>>>> --- a/target-microblaze/cpu.c
>>>>>> +++ b/target-microblaze/cpu.c
>>>>>> @@ -89,10 +89,18 @@ static void mb_cpu_reset(CPUState *s)
>>>>>>                          | PVR2_USE_DIV_MASK \
>>>>>>                          | PVR2_USE_HW_MUL_MASK \
>>>>>>                          | PVR2_USE_MUL64_MASK \
>>>>>> -                        | PVR2_USE_FPU_MASK \
>>>>>> -                        | PVR2_USE_FPU2_MASK \
>>>>>
>>>>> If I have this straight, this suggests the property default value
>>>>> should be 2, and the ml605 board setting should be 1.
>>>>
>>>> You are correct, I will fix this.
>>>>
>>>>>
>>>>>>                          | PVR2_FPU_EXC_MASK \
>>>>>>                          | 0;
>>>>>> +
>>>>>> +    if (cpu->cfg.usefpu) {
>>>>>> +        env->pvr.regs[0] |= PVR0_USE_FPU_MASK;
>>>>>> +        env->pvr.regs[2] |= PVR2_USE_FPU_MASK;
>>>>>> +
>>>>>> +        if (cpu->cfg.usefpu > 1) {
>>>>>> +            env->pvr.regs[2] |= PVR2_USE_FPU2_MASK;
>>>>>> +        }
>>>>>> +    }
>>>>>
>>>>> This should be handled at realize time. See pl330_realize for example
>>>>> of realize-time PVR ("cfg" in that case) population.
>>>>
>>>> But the env state gets wiped out at reset, so the information will be lost.
>>>>
>>>
>>> Ahh, so the solution there is to do what ARM does and have a section
>>> at the back of the env which survives reset. Check the memset in
>>> arm_cpu_reset.
>>
>> Ok, just to clarify we want all of the pvr registers to be preserved on 
>> reset?
>>
>
> yes. But something that just occured to me, does it make sense to move
> it outside the env? into the CPUState? Andreas mentioned that fields
> in the CPU state before the env can be used with negative env* offsets
> by translated code. This means the PVR could just be pushed up to
> CPUState.

Do any other machines do that?

I'm happy with leaving it in the env (partly because I just did it)
and it also matches the way that ARM does it.

Thanks,

Alistair

>
> Regards,
> Peter
>
>> Thanks,
>>
>> Alistair
>>
>>>
>>> Regards,
>>> Peter
>>>
>>>> I can tidy up the if logic though.
>>>>
>>>>>
>>>>> To avoid churn, it might be easiest to convert the PVRs in chunks one
>>>>> by one (e.g. one patch for PVR0, then PVR2 etc etc. The existing |ing
>>>>> can be moved into realize for settings not worth propertyifying.
>>>>>
>>>>>> +
>>>>>>      env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp 
>>>>>> family.  */
>>>>>>      env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
>>>>>>
>>>>>> @@ -154,6 +162,7 @@ static Property mb_properties[] = {
>>>>>>      DEFINE_PROP_UINT32("xlnx.base-vectors", MicroBlazeCPU, 
>>>>>> base_vectors, 0),
>>>>>>      DEFINE_PROP_BOOL("xlnx.use-stack-protection", MicroBlazeCPU, 
>>>>>> cfg.stackproc,
>>>>>>                       true),
>>>>>> +    DEFINE_PROP_UINT8("xlnx.use-fpu", MicroBlazeCPU, cfg.usefpu, 1),
>>>>>
>>>>> No need for xlnx prefix (FDT generic should trim the prefix).
>>>>
>>>> I agree, but the base-vectors already had it. If the xlnx is unneeded
>>>> I'll remove
>>>> it from the base-vector as well.
>>>>
>>>> Thanks,
>>>>
>>>> Alistair
>>>>
>>>>>
>>>>> Regards,
>>>>> Peter
>>>>>
>>>>>>      DEFINE_PROP_END_OF_LIST(),
>>>>>>  };
>>>>>>
>>>>>> diff --git a/target-microblaze/translate.c 
>>>>>> b/target-microblaze/translate.c
>>>>>> index 4068946..36cfc5c 100644
>>>>>> --- a/target-microblaze/translate.c
>>>>>> +++ b/target-microblaze/translate.c
>>>>>> @@ -1415,11 +1415,11 @@ static int dec_check_fpuv2(DisasContext *dc)
>>>>>>
>>>>>>      r = dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU2_MASK;
>>>>>>
>>>>>> -    if (!r && (dc->tb_flags & MSR_EE_FLAG)) {
>>>>>> +    if ((dc->cpu->cfg.usefpu != 2) && (dc->tb_flags & MSR_EE_FLAG)) {
>>>>>>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_FPU);
>>>>>>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>>>>>>      }
>>>>>> -    return r;
>>>>>> +    return (dc->cpu->cfg.usefpu == 2) ? 0 : PVR2_USE_FPU2_MASK;
>>>>>>  }
>>>>>>
>>>>>>  static void dec_fpu(DisasContext *dc)
>>>>>> @@ -1428,7 +1428,7 @@ static void dec_fpu(DisasContext *dc)
>>>>>>
>>>>>>      if ((dc->tb_flags & MSR_EE_FLAG)
>>>>>>            && (dc->cpu->env.pvr.regs[2] & PVR2_ILL_OPCODE_EXC_MASK)
>>>>>> -          && !((dc->cpu->env.pvr.regs[2] & PVR2_USE_FPU_MASK))) {
>>>>>> +          && (dc->cpu->cfg.usefpu != 1)) {
>>>>>>          tcg_gen_movi_tl(cpu_SR[SR_ESR], ESR_EC_ILLEGAL_OP);
>>>>>>          t_gen_raise_exception(dc, EXCP_HW_EXCP);
>>>>>>          return;
>>>>>> --
>>>>>> 1.7.1
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>