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

[Alistair Francis]

On Mon, May 18, 2015 at 1:12 AM, Andreas Färber <address@hidden> wrote:
> 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.

I confused myself with that last comment and forgot that we were talking
about the pvr regs and not the configuration variables. So it doesn't matter
what the DEFINE_PROP_UINT8() macro does.

I'll send my patches now and we can keep discussing it there.

Thanks,

Alistair

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