RE: [Patch, microblaze]: Add slr and shr regs

[Ajit Kumar Agarwal <ajit dot kumar dot agarwal at xilinx dot com>]

Sorry for the late response as I was on Vacation. Please find my response inlined below.

-----Original Message-----
From: Michael Eager [mailto:eager@eagerm.com] 
Sent: Thursday, June 05, 2014 9:24 PM
To: Ajit Kumar Agarwal; gdb-patches@sourceware.org
Cc: Vinod Kathail; Vidhumouli Hunsigida; Nagaraju Mekala
Subject: Re: [Patch, microblaze]: Add slr and shr regs

On 05/27/14 00:46, Ajit Kumar Agarwal wrote:
>
>
> -----Original Message-----
> From: Michael Eager [mailto:eager@eagercon.com]
> Sent: Tuesday, May 27, 2014 12:05 PM
> To: Ajit Kumar Agarwal; gdb-patches@sourceware.org; Yao Qi; Pedro 
> Alves
> Cc: Joel Brobecker; Vinod Kathail; Vidhumouli Hunsigida; Nagaraju 
> Mekala
> Subject: Re: [Patch, microblaze]: Add slr and shr regs
>
> On 05/26/14 03:04, Ajit Kumar Agarwal wrote:
>>
>> Based on the feedback and incorporated all review comment, updated the patch.

Hi Ajit --

I've reviewed your patch in more detail.

There are a number of issues which need to be addressed.


> Update the problem description and here it is.
>
>      [Patch, microblaze]: Add slr and shr regs
>


>         Prior to version 2013.1, XMD's gdbserver stub returned 57 registers in
>         response to GDB's G request.  Starting with version 2013.1, XMD added the
>         slr and shr register, for a count of 59 registers.  This patch adds
>         these registers to the expected G response.

>>XMD identifies itself with the version of EDK/SDK in which it was released, not with the version number you mention.

>>I reviewed the MicroBlaze architecture documents for EDK 9.1i and EDK 14.2i.  It appears that at some time after EDK 9.1i, the MicroBlaze processor architecture was extended to add >>two new stack protection
>>registers: slr (stack low register) and shr (stack high registers).
>>It isn't clear from the documentation I have at hand which MicroBlaze processor version contained this architectural change.  EDK 9.1i mentions versions through V6.00, while EDK 14.2i >>mentions versions from v7.30 through v8.40.

>>Evidently, as part of this architectural change, XMD's gdbserver stub released with the corresponding EDK version was modified to return these additional registers.

>>Your patch needs to be modified to support multiple versions of the MicroBlaze architecture.  It is generally not acceptable to break support for one processor version when you add >>support for a different processor version.

>>Your patch should also mention that it changes behavior of gdbserver to match XMD.

SLR/SHR was added in MicroBlaze v8.10.a, EDK 13.1.  XMD's gdbserver stub releasesed before this design throws an error mentioning gdb is not supported  for the version below v8.10.a, EDK 13.1.

The slr(stack low register) and shr(stack high registers) are implemented based on C_USE_STACK_PROTECTION is set 0/1. Microblaze being the reconfigurable architecture the design can be selected with and without these registers. Its hard to identify in gdb whether these registers is being implemented for the design or not. In XMD where the gdb client connects to the local host we always display the shr and shl registers irrespective of  C_USE_STACK_PROTECTION is set or not. In the case where the design is not implemented with these register we always display the content to be 0 or ? to the user.  gdb will also display these registers when the C_USE_STACK_PROTECTION is set 0/1. 

>> ChangeLog:
>> 2014-05-26 Ajit Agarwal <ajitkum@xilinx.com>
>>
>>           * Makefile.in (microblaze-linux.c): New rule.

There is no new Makefile rule in the patch.

>>           * microblaze-tdep.c (microblaze_register_names): Add
>>           the rshr and rslr register names.

This needs to be modified to support the MicroBlaze versions which are currently supported (EDK 9.1i) as well as more recent versions.

>>           * microblaze-tdep.h (microblaze_reg_num): Add
>>           field MICROBLAZE_SLR_REGNUM and MICROBLAZE_SHR_REGNUM
>>           MICROBLAZE_NUM_REGS.
>>           (microblaze_frame_cache): Use of MICROBLAZE_NUM_REGS.

Same as above.

>>           * features/microblaze-cpu.xml: New file.
>>
>>           * features/microblaze-linux.c: New file.
>>
>>           * features/microblaze-linux.xml: New file.
>>
>>           * regformats/reg-microblaze.dat: New file.
>>
>>           * features/Makefile (microblaze-linux): Add
>>           microblaze-linux and microblaze-expedite.

>>I do not have any way to test microblaze-linux.  Make sure that your patch works with both -target=microblaze-linux and -target=microblaze-xilinx-gnu, and with both EDK 9.1i and >>14.2i, at a minimum.  Please provide documentation which show that this patch works using gdbserver on a Linux target, as well as using XMD.

Same as the Explanation above. The native Linux support patch is not yet  submitted and we will submitting the patches after this patch.  XMD testing is done and it works fine.

When you resubmit the patch, please make sure that you check for whitespace errors.  Make sure that you address all of the questions and comments made here and by other reviewers.

-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077