From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46738) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YyDDb-00049z-Kl for qemu-devel@nongnu.org; Fri, 29 May 2015 01:52:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YyDDa-0005ha-D3 for qemu-devel@nongnu.org; Fri, 29 May 2015 01:52:31 -0400 Received: from mail-oi0-x232.google.com ([2607:f8b0:4003:c06::232]:33687) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YyDDa-0005hT-6o for qemu-devel@nongnu.org; Fri, 29 May 2015 01:52:30 -0400 Received: by oiww2 with SMTP id w2so49081035oiw.0 for ; Thu, 28 May 2015 22:52:29 -0700 (PDT) MIME-Version: 1.0 Sender: alistair23@gmail.com In-Reply-To: <20150529054215.GA30952@toto> References: <84e456db1be3f771dea4020092662a9e80c4cf54.1432790821.git.alistair.francis@xilinx.com> <20150528061733.GQ30952@toto> <20150529054215.GA30952@toto> From: Alistair Francis Date: Fri, 29 May 2015 15:51:59 +1000 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v2 3/5] target-microblaze: Allow the stack protection to be disabled/enabled List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Edgar E. Iglesias" Cc: Peter Crosthwaite , Richard Henderson , "qemu-devel@nongnu.org Developers" , =?UTF-8?Q?Andreas_F=C3=A4rber?= , Alistair Francis On Fri, May 29, 2015 at 3:42 PM, Edgar E. Iglesias wrote: > On Fri, May 29, 2015 at 03:39:32PM +1000, Alistair Francis wrote: >> On Fri, May 29, 2015 at 3:35 PM, Alistair Francis >> wrote: >> > On Thu, May 28, 2015 at 4:17 PM, Edgar E. Iglesias >> > 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 >> >> >> >> 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. > > > Right, stackprot should be disabled as the MMU is on. > I wasn't aware that the stackprot was not used in combination with > the MMU at the time... Ok, I will disable it by default and for all of the current machines. Thanks, Alistair > > Cheers, > Edgar > > >> >> 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 >> >>> >> >> >