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

[Michael Eager <eager@eagerm.com>]

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.

>> 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.

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