On Mon, May 25, 2015 at 6:55 PM, Alistair Francis
<alistair.fran...@xilinx.com> wrote:
> On Mon, May 25, 2015 at 2:07 PM, Peter Crosthwaite
> <peter.crosthwa...@xilinx.com> wrote:
>> On Mon, May 18, 2015 at 4:13 PM, Alistair Francis
>> <alistair.fran...@xilinx.com> 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 <alistair.fran...@xilinx.com>
>>> ---
>>> 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.
>
Nah just leave them. I get it now.
Regards,
Peter
>>
>>> 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
>>>
>>>
>>
>
