| Message ID | 1efa3624771b5a0f265ce12a1ef6f39e7732fdf1.1431909583.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On Mon, May 18, 2015 at 4:13 PM, Alistair Francis <alistair.francis@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.francis@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. > + } 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. > 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? Regards, Peter > stackprot = 1; > } > > -- > 1.7.1 > >
On Mon, May 25, 2015 at 2:07 PM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Mon, May 18, 2015 at 4:13 PM, Alistair Francis > <alistair.francis@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.francis@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. > >> 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 >> >> >
On Mon, May 25, 2015 at 6:55 PM, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Mon, May 25, 2015 at 2:07 PM, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Mon, May 18, 2015 at 4:13 PM, Alistair Francis >> <alistair.francis@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.francis@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 >>> >>> >> >
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; + } 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); + 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) { stackprot = 1; }
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.francis@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(-)