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

[Alistair Francis]

On Fri, May 29, 2015 at 3:35 PM, Alistair Francis
<address@hidden> wrote:
> On Thu, May 28, 2015 at 4:17 PM, Edgar E. Iglesias
> <address@hidden> wrote:
>> On Thu, May 28, 2015 at 03:37:42PM +1000, Alistair Francis wrote:
>>> Microblaze stack protection is configurable and isn't always enabled.
>>> This patch allows the stack protection to be disabled/enabled from the
>>> CPU properties.
>>>
>>> The stack protection is disabled by default as by default the Microblaze
>>> machines enable the MMU and stack protection can't be enabled if the
>>> MMU is.
>>>
>>> Signed-off-by: Alistair Francis <address@hidden>
>>
>> Hi Alistair,
>>
>>
>>> ---
>>> V2:
>>>  - Change the variable name to stackprot
>>>  - Include protection for the second time stack protection
>>>    is enabled
>>>  - Disable stack protection by default
>>> 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 |    4 ++--
>>>  4 files changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target-microblaze/cpu-qom.h b/target-microblaze/cpu-qom.h
>>> index e3e0701..e08adb9 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 stackprot;
>>> +    } cfg;
>>> +
>>>      CPUMBState env;
>>>  } MicroBlazeCPU;
>>>
>>> diff --git a/target-microblaze/cpu.c b/target-microblaze/cpu.c
>>> index 95be540..ead2fcd 100644
>>> --- a/target-microblaze/cpu.c
>>> +++ b/target-microblaze/cpu.c
>>> @@ -114,6 +114,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.stackprot ? PVR0_SPROT_MASK : 0);
>>
>> Could you please skip the parentheses.
>
> Hey Edgar,
>
> Yeah I will. They get re-added straight away, which is why I left them in.
>
>>
>>> +
>>>      env->pvr.regs[10] = 0x0c000000; /* Default to spartan 3a dsp family.  
>>> */
>>>      env->pvr.regs[11] = PVR11_USE_MMU | (16 << 17);
>>>
>>> @@ -156,6 +159,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.stackprot,
>>> +                     false),
>>
>> I think the change to default false should be done in a separate patch
>> as it changes the function behaviour of the default CPU.
>
> Fair enough. I will leave it here and add a patch at the end that disables it.

On that note, should the current machines enable it?

It has always been enabled for them, but they do support MMU's so it
should be disabled.

Thanks,

Alistair

>
> Thanks,
>
> Alistair
>
>>
>>
>>
>>
>>>      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..bd10b40 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.stackprot) {
>>>          stackprot = 1;
>>>      }
>>>
>>> @@ -875,7 +875,7 @@ static inline TCGv *compute_ldst_addr(DisasContext *dc, 
>>> TCGv *t)
>>>              return &cpu_R[dc->ra];
>>>          }
>>>
>>> -        if (dc->rb == 1) {
>>> +        if (dc->rb == 1 && dc->cpu->cfg.stackprot) {
>>>              stackprot = 1;
>>>          }
>>>
>>> --
>>> 1.7.1
>>>
>>