Re: [Qemu-devel] [PATCH RESEND v1 2/5] target-microblaze: Preserve the pvr registers during reset

[Alistair Francis <alistair.francis@xilinx.com>]

On Mon, May 25, 2015 at 1:53 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Mon, May 18, 2015 at 4:13 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> Move the Microblaze PVR registers to the end of the CPUMBState
>> and preserve them during reset. This is similar to what the
>> QEMU ARM model does with some of it's registers.
>>
>> This allows the Microblaze PVR registers to only be set once
>> at realise instead of constantly at reset.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>> NOTE: The individial machine resets still write to the PVR
>> registers on each reset. This is no longer required as it only
>> needs to be done once. Instead of moving them now, they are
>> being left there and will be removed when they are all
>> converted to the standard CPU properties.
>>
>>  target-microblaze/cpu.c |   43 +++++++++++++++++++++++++------------------
>>  target-microblaze/cpu.h |   10 ++++++----
>>  2 files changed, 31 insertions(+), 22 deletions(-)
>>
>> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
>> index 67e3182..555bc4c 100644
>> --- a/target-microblaze/cpu.c
>> +++ b/target-microblaze/cpu.c
>> @@ -63,13 +63,37 @@ static void mb_cpu_reset(CPUState *s)
>>
>>      mcc->parent_reset(s);
>>
>> -    memset(env, 0, sizeof(CPUMBState));
>> +    memset(env, 0, offsetof(CPUMBState, pvr));
>>      env->res_addr = RES_ADDR_NONE;
>>      tlb_flush(s, 1);
>>
>>      /* Disable stack protector.  */
>>      env->shr = ~0;
>>
>> +#if defined(CONFIG_USER_ONLY)
>> +    /* start in user mode with interrupts enabled.  */
>> +    env->sregs[SR_MSR] = MSR_EE | MSR_IE | MSR_VM | MSR_UM;
>> +#else
>> +    env->sregs[SR_MSR] = 0;
>> +    mmu_init(&env->mmu);
>> +    env->mmu.c_mmu = 3;
>> +    env->mmu.c_mmu_tlb_access = 3;
>> +    env->mmu.c_mmu_zones = 16;
>> +#endif
>> +}
>> +
>> +static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
>> +{
>> +    CPUState *cs = CPU(dev);
>> +    MicroBlazeCPUClass *mcc = MICROBLAZE_CPU_GET_CLASS(dev);
>> +    MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
>> +    CPUMBState *env = &cpu->env;
>> +
>> +    cpu_reset(cs);
>
> So git did a poor job of showing the code motion of this patch as this
> is really context. But as follow up we should consider removing this
> as realize functions should not call resets.

Yeah, it really struggled with this patch for some reason.

I can still remove them from this patch. I'm touching it anyway

>
>> +    qemu_init_vcpu(cs);
>> +
>> +    memset(&env->pvr, 0, sizeof(env->pvr));
>> +
>
> Similar, 0 memsets in realize not needed but it is ultimately just
> context with the strange way git has done this patch.

Same with this.

>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Thanks

Alistair

>
> Regards,
> Peter
>
>>      env->pvr.regs[0] = PVR0_PVR_FULL_MASK \
>>                         | PVR0_USE_BARREL_MASK \
>>                         | PVR0_USE_DIV_MASK \
>> @@ -99,25 +123,8 @@ static void mb_cpu_reset(CPUState *s)
>>      env->sregs[SR_PC] = cpu->base_vectors;
>>
>>  #if defined(CONFIG_USER_ONLY)
>> -    /* start in user mode with interrupts enabled.  */
>> -    env->sregs[SR_MSR] = MSR_EE | MSR_IE | MSR_VM | MSR_UM;
>>      env->pvr.regs[10] = 0x0c000000; /* Spartan 3a dsp.  */
>> -#else
>> -    env->sregs[SR_MSR] = 0;
>> -    mmu_init(&env->mmu);
>> -    env->mmu.c_mmu = 3;
>> -    env->mmu.c_mmu_tlb_access = 3;
>> -    env->mmu.c_mmu_zones = 16;
>>  #endif
>> -}
>> -
>> -static void mb_cpu_realizefn(DeviceState *dev, Error **errp)
>> -{
>> -    CPUState *cs = CPU(dev);
>> -    MicroBlazeCPUClass *mcc = MICROBLAZE_CPU_GET_CLASS(dev);
>> -
>> -    cpu_reset(cs);
>> -    qemu_init_vcpu(cs);
>>
>>      mcc->parent_realize(dev, errp);
>>  }
>> diff --git a/target-microblaze/cpu.h b/target-microblaze/cpu.h
>> index 4ea04ac..e4c1cde 100644
>> --- a/target-microblaze/cpu.h
>> +++ b/target-microblaze/cpu.h
>> @@ -260,16 +260,18 @@ struct CPUMBState {
>>  #define IFLAGS_TB_MASK  (D_FLAG | IMM_FLAG | DRTI_FLAG | DRTE_FLAG | DRTB_FLAG)
>>      uint32_t iflags;
>>
>> -    struct {
>> -        uint32_t regs[16];
>> -    } pvr;
>> -
>>  #if !defined(CONFIG_USER_ONLY)
>>      /* Unified MMU.  */
>>      struct microblaze_mmu mmu;
>>  #endif
>>
>>      CPU_COMMON
>> +
>> +    /* These fields are preserved on reset.  */
>> +
>> +    struct {
>> +        uint32_t regs[16];
>> +    } pvr;
>>  };
>>
>>  #include "cpu-qom.h"
>> --
>> 1.7.1
>>
>>
>