Re: [Qemu-devel] [PATCH RESEND v1 3/5] target-microblaze: Allow the stack protection to be disabled

[Alistair Francis]

On Mon, May 25, 2015 at 2:07 PM, Peter Crosthwaite
<address@hidden> wrote:
> On Mon, May 18, 2015 at 4:13 PM, Alistair Francis
> <address@hidden> wrote:
>> Microblaze stack protection is configurable and isn't always enabled.
>> This patch allows the stack protection to be disabled from the CPU
>> properties.
>>
>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>> Changes since RFC:
>>  - Move the cfg.stackproc check into translate.c
>>  - Set the PVR register
>>
>>  target-microblaze/cpu-qom.h   |    5 +++++
>>  target-microblaze/cpu.c       |    5 +++++
>>  target-microblaze/cpu.h       |    1 +
>>  target-microblaze/translate.c |    2 +-
>>  4 files changed, 12 insertions(+), 1 deletions(-)
>>
>> diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
>> index e3e0701..7bc5b81 100644
>> --- a/target-microblaze/cpu-qom.h
>> +++ b/target-microblaze/cpu-qom.h
>> @@ -59,6 +59,11 @@ typedef struct MicroBlazeCPU {
>>      uint32_t base_vectors;
>>      /*< public >*/
>>
>> +    /* Microblaze Configuration Settings */
>> +    struct {
>> +        bool stackproc;
>
> stackprot? Although should we just verbatim match to the TRMs name for
> these variables (dropping the redundant leading C_)? That would make
> this "use_stack_protection" and match to QOM property name exactly.

I think "use_stack_protection" is too long, it makes it harder to stay under
the 80 characters for each line.

I'll change it to 'stackprot' though.

>
>> +    } cfg;
>> +
>>      CPUMBState env;
>>  } MicroBlazeCPU;
>>
>> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
>> index 555bc4c..4deb256 100644
>> --- a/target-microblaze/cpu.c
>> +++ b/target-microblaze/cpu.c
>> @@ -117,6 +117,9 @@ static void mb_cpu_realizefn(DeviceState *dev, Error 
>> **errp)
>>                          | PVR2_USE_FPU2_MASK \
>>                          | PVR2_FPU_EXC_MASK \
>>                          | 0;
>> +
>> +    env->pvr.regs[0] |= (cpu->cfg.stackproc ? PVR0_SPROT_MASK : 0);
>> +
>
> Parentheses not needed.

True, but then they get re-added in the next patch as more
configuration variables get added.

I can remove them from this one.

>
>>      env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  */
>>      env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
>>
>> @@ -159,6 +162,8 @@ static const VMStateDescription vmstate_mb_cpu = {
>>
>>  static Property mb_properties[] = {
>>      DEFINE_PROP_UINT32("xlnx.base-vectors", MicroBlazeCPU, base_vectors, 0),
>> +    DEFINE_PROP_BOOL("use-stack-protection", MicroBlazeCPU, cfg.stackproc,
>> +                     true),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>> diff --git a/target-microblaze/cpu.h b/target-microblaze/cpu.h
>> index e4c1cde..481f463 100644
>> --- a/target-microblaze/cpu.h
>> +++ b/target-microblaze/cpu.h
>> @@ -128,6 +128,7 @@ typedef struct CPUMBState CPUMBState;
>>  #define PVR0_FAULT                     0x00100000
>>  #define PVR0_VERSION_MASK               0x0000FF00
>>  #define PVR0_USER1_MASK                 0x000000FF
>> +#define PVR0_SPROT_MASK                 0x00000001
>>
>>  /* User 2 PVR mask */
>>  #define PVR1_USER2_MASK                 0xFFFFFFFF
>> diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
>> index 4068946..19faf40 100644
>> --- a/target-microblaze/translate.c
>> +++ b/target-microblaze/translate.c
>> @@ -862,7 +862,7 @@ static inline TCGv *compute_ldst_addr(DisasContext *dc, 
>> TCGv *t)
>>      int stackprot = 0;
>>
>>      /* All load/stores use ra.  */
>> -    if (dc->ra == 1) {
>> +    if (dc->ra == 1 && dc->cpu->cfg.stackproc) {
>
> There is a similar logic below for dc->rb:
>
>         if (dc->rb == 1) {
>             stackprot = 1;
>         }
>
> Should it have the same guard?

Yes, it should. I just missed that one.

Thanks,

Alistair

>
> Regards,
> Peter
>
>>          stackprot = 1;
>>      }
>>
>> --
>> 1.7.1
>>
>>
>