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

[Peter Crosthwaite]

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.

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
>>>>>
>>>>>
>>>>
>>>
>>
>