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

[Andreas Färber]

Am 17.05.2015 um 14:26 schrieb Alistair Francis:
> On Fri, May 15, 2015 at 9:15 PM, Andreas Färber <address@hidden> wrote:
>> Am 15.05.2015 um 08:36 schrieb 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:
>>>>>>>>>                          | 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.
>>
>> All targets had everything in env originally, so that's not really an
>> argument for anything. You need to decide what makes sense for your
>> target - icount is an example using negative offsets in CPUState now,
>> whereas TCGv* variables declared via offset from env remained in env.
> 
> I think env makes sense in this case, especially as the
> DEFINE_PROP_UINT8() macro defaults to setting variables in the env
> structure. Admittedly I haven't looked into it, so it might be just as
> easy to set variables in the MicroBlazeCPU.

In fact the CPU properties operate on MicroBlazeCPU. The legacy
properties thus need an explicit env.foo as macro argument.

But as usual things can be done in multiple steps.

Andreas

> I have another patch set ready to send out tomorrow with all of the
> variables in env. We can decide from there if they should be moved to
> the MicroBlazeCPU struct.
> 
> Thanks,
> 
> Alistair

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
21284 (AG Nürnberg)