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

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

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

>
> If moving something to the end of env is an option, then moving it to
> MicroBlazeCPU (not CPUState!) would seem better, especially when it's
> not supposed to be reset. Going from CPUMBState *env to MicroBlazeCPU
> *cpu is a cheap offset operation, whereas CPUState *cs involves a QOM cast.
>
> Regards,
> Andreas
>
> --
> SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB
> 21284 (AG Nürnberg)
>