Re: [PATCH 02/24] Add MIPS32 FPU64 GDB target descriptions

[James Hogan <james dot hogan at imgtec dot com>]

Hi Maciej,

On Wed, Oct 12, 2016 at 01:42:17PM +0100, Maciej W. Rozycki wrote:
> On Mon, 27 Jun 2016, Bhushan Attarde wrote:
> 
> >     Add a couple of new target descriptions for MIPS32 targets with 64-bit
> >     floating point registers, with and without DSP.
> > 
> >     These are identical to their 32-bit FPU counterparts except that the FP
> >     registers are 64-bits long to allow for debugging of F64=1 MIPS32 targets
> >     such as MIPS32r2 compatible cores, and they include the Config5 CP0
> >     register which has an FRE bit.
> 
>  Hmm, has Linux kernel support for CP0.Config5 accesses gone upstream 
> already?  Can you give me an upstream commit ID and/or reference to the 
> discussion where it has been approved if so?

I don't think it did go upstream yet.

> 
>  More importantly, what do we need CP0.Config5 access for in the first 
> place?  It looks to me like this bit is irrelevant to GDB as it does not 
> affect the native (raw) register format.  So the only use would be to let 
> the user running a debugging session switch between the FRE and NFRE modes 
> without the need to poke at CP1C.FRE or CP1C.NFRE registers with a CTC1 
> instruction, which by itself makes sense to me, but needs a further 
> consideration.

It allows the FRE bit to be read (I seem to remember this was the only
bit actually exposed through ptrace by the patch).

FRE simply causes certain instructions (all single precision FP
arithmetic instructions and FP word loads/stores) to trap to the kernel
so that it can emulate a variation/subset of FR=0, so the debugger would
use it to decide how to decode the single precision FP registers based
on the double precision FP registers (iirc).

> 
>  Additionally exposing CP0.Config5 may have security implications, 
> especially as parts of the register have not been defined yet in the 
> architectures and we'd have to force architecture maintainers somehow to 
> ask us every time they intend to add a bit to this register to check if 
> this has security implications and has to be avoided and/or explicitly 
> handled in software.

yes, as above it explicity only shows certain bits. I'm fine with the
api changing if necessary though since it isn't upstream.

cheers
James

> 
>  So overall it looks to me like we should avoid it.  What we ought to do 
> instead is -- since we need to extend structures anyway -- I think adding 
> all the remaining CP1C registers.  This would cover the earlier CP1C.UFR 
> and CP1C.UNFR registers as well as any future additions, which would be 
> further qualified with CP1.FIR bits to remove unimplemented registers from 
> the view.  Given the unusual semantics of these registers I'd be tempted 
> to map both pairs to the read register only and pretend it to be r/w with 
> the LSB directly mapping to CP0.Status.FR and CP0.Config5.FRE as with 
> CFC1.  Access would be r/o if user access to the selected bit was not 
> permitted.
> 
>  This would provide the necessary semantics and avoid any security 
> implications as GDB would only be able to poke/peek at the same bits the 
> debuggee itself could.
> 
>  This could be sorted out the same way in core files.
> 
>  Finally, do we need to have these bits recorded in a signal frame?
> 
> >     F64 targets have an FR bit in the status register to specify the effective
> >     register size, and the FRE bit is used with FR=1 to enable a compatibility
> >     mode which is similar to FR=0 but with usable odd doubles which don't
> >     alias with any other FP registers.
> 
>  This seems to contradict to how I read the architecture specification, 
> which IIUC states that CP0.Config5.FRE=1 merely causes all single FP 
> operations to trap.  All 32 64-bit registers are usable for doubles with 
> CP0.Status.FR=1 regardless of the state of CP0.Config5.FRE.  Have I missed 
> anything here?  Can you please elaborate and perhaps rephrase the sentence 
> so that the implications for GDB are more clearly seen?
> 
>  Further comments as to the change itself follow.
> 
> >     gdb/ChangeLog:
> > 
> >     	* features/Makefile (WHICH): Add mips-fpu64-linux and
> >     	mips-fpu64-dsp-linux and Sort microblaze out of the way
> 
>  Mid-sentence capitalisation issue here.
> 
> >         of mips/mips64
> 
>  Indentation and missing full stop.
> 
> >     	(mips-fpu64-expedite): Set.
> >     	(mips-fpu64-dsp-expedite): Set.
> >     	* features/mips-fpu64-dsp-linux.xml: New file.
> >     	* features/mips-fpu64-linux.xml: New file.
> >     	* features/mips-fpu64.xml: New file.
> >     	* features/mips-fpu64-dsp-linux.c: New generated file.
> >     	* features/mips-fpu64-linux.c: New generated file.
> >     	* regformats/mips-fpu64-dsp-linux.dat: New generated file.
> >     	* regformats/mips-fpu64-linux.dat: New generated file.
> > 
> >     gdb/gdbserver/ChangeLog:
> > 
> >     	* Makefile.in (clean): Delete mips-fpu64-linux.c and
> >     	mips-fpu64-dsp-linux.c.
> 
>  I think "Delete" is confusing here: are you deleting from the rule or 
> deleting files?  Please rephrase to make it unambiguous.  Maybe: "Add a 
> deletion of..."?
> 
> > diff --git a/gdb/features/Makefile b/gdb/features/Makefile
> > index 10173cf..b6baaf66 100644
> > --- a/gdb/features/Makefile
> > +++ b/gdb/features/Makefile
> > @@ -57,8 +57,8 @@ WHICH = aarch64 \
> >  	i386/x32 i386/x32-linux \
> >  	i386/x32-avx i386/x32-avx-linux \
> >  	i386/x32-avx512 i386/x32-avx512-linux \
> > -	mips-linux mips-dsp-linux \
> >  	microblaze-with-stack-protect \
> > +	mips-linux mips-dsp-linux mips-fpu64-linux mips-fpu64-dsp-linux \
> >  	mips64-linux mips64-dsp-linux \
> >  	nios2-linux \
> >  	rs6000/powerpc-32 \
> > @@ -100,11 +100,13 @@ i386/x32-avx-expedite = rbp,rsp,rip
> >  i386/x32-avx-linux-expedite = rbp,rsp,rip
> >  i386/x32-avx512-expedite = rbp,rsp,rip
> >  i386/x32-avx512-linux-expedite = rbp,rsp,rip
> > +microblaze-expedite = r1,rpc
> >  mips-expedite = r29,pc
> >  mips-dsp-expedite = r29,pc
> > +mips-fpu64-expedite = r29,pc
> > +mips-fpu64-dsp-expedite = r29,pc
> >  mips64-expedite = r29,pc
> >  mips64-dsp-expedite = r29,pc
> > -microblaze-expedite = r1,rpc
> >  nios2-linux-expedite = sp,pc
> >  powerpc-expedite = r1,pc
> >  rs6000/powerpc-cell32l-expedite = r1,pc,r0,orig_r3,r4
> 
>  Please split Microblaze changes off to a separate preparatory patch.  
> They can probably go in right away as obviously correct, but as a newcomer 
> please allow 48 hours for people to chime in before committing.
> 
>  Speaking of which -- I don't see you listed in gdb/MAINTAINERS, so can 
> you please clarify your status WRT the FSF copyright assignment?  Are you 
> covered by a company assignment?  If your paperwork has been cleared, then  
> I can commit your change on your behalf.
> 
> > diff --git a/gdb/features/mips-fpu64-dsp-linux.xml b/gdb/features/mips-fpu64-dsp-linux.xml
> > new file mode 100644
> > index 0000000..f0de5ce
> > --- /dev/null
> > +++ b/gdb/features/mips-fpu64-dsp-linux.xml
> > @@ -0,0 +1,20 @@
> > +<?xml version="1.0"?>
> > +<!-- Copyright (C) 2012-2013 Free Software Foundation, Inc.
> 
>  You need to list the current year in copyright notices for newly 
> submitted files, i.e.:
> 
> <!-- Copyright (C) 2016 Free Software Foundation, Inc.
> 
> Likewise throughout.
> 
> > diff --git a/gdb/features/mips-fpu64.xml b/gdb/features/mips-fpu64.xml
> > new file mode 100644
> > index 0000000..88dc9d9
> > --- /dev/null
> > +++ b/gdb/features/mips-fpu64.xml
> > @@ -0,0 +1,45 @@
> > +<?xml version="1.0"?>
> > +<!-- Copyright (C) 2007-2013 Free Software Foundation, Inc.
> > +
> > +     Copying and distribution of this file, with or without modification,
> > +     are permitted in any medium without royalty provided the copyright
> > +     notice and this notice are preserved.  -->
> > +
> > +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > +<feature name="org.gnu.gdb.mips.fpu">
> > +  <reg name="f0" bitsize="64" type="ieee_double"/>
> > +  <reg name="f1" bitsize="64" type="ieee_double"/>
> > +  <reg name="f2" bitsize="64" type="ieee_double"/>
> > +  <reg name="f3" bitsize="64" type="ieee_double"/>
> > +  <reg name="f4" bitsize="64" type="ieee_double"/>
> > +  <reg name="f5" bitsize="64" type="ieee_double"/>
> > +  <reg name="f6" bitsize="64" type="ieee_double"/>
> > +  <reg name="f7" bitsize="64" type="ieee_double"/>
> > +  <reg name="f8" bitsize="64" type="ieee_double"/>
> > +  <reg name="f9" bitsize="64" type="ieee_double"/>
> > +  <reg name="f10" bitsize="64" type="ieee_double"/>
> > +  <reg name="f11" bitsize="64" type="ieee_double"/>
> > +  <reg name="f12" bitsize="64" type="ieee_double"/>
> > +  <reg name="f13" bitsize="64" type="ieee_double"/>
> > +  <reg name="f14" bitsize="64" type="ieee_double"/>
> > +  <reg name="f15" bitsize="64" type="ieee_double"/>
> > +  <reg name="f16" bitsize="64" type="ieee_double"/>
> > +  <reg name="f17" bitsize="64" type="ieee_double"/>
> > +  <reg name="f18" bitsize="64" type="ieee_double"/>
> > +  <reg name="f19" bitsize="64" type="ieee_double"/>
> > +  <reg name="f20" bitsize="64" type="ieee_double"/>
> > +  <reg name="f21" bitsize="64" type="ieee_double"/>
> > +  <reg name="f22" bitsize="64" type="ieee_double"/>
> > +  <reg name="f23" bitsize="64" type="ieee_double"/>
> > +  <reg name="f24" bitsize="64" type="ieee_double"/>
> > +  <reg name="f25" bitsize="64" type="ieee_double"/>
> > +  <reg name="f26" bitsize="64" type="ieee_double"/>
> > +  <reg name="f27" bitsize="64" type="ieee_double"/>
> > +  <reg name="f28" bitsize="64" type="ieee_double"/>
> > +  <reg name="f29" bitsize="64" type="ieee_double"/>
> > +  <reg name="f30" bitsize="64" type="ieee_double"/>
> > +  <reg name="f31" bitsize="64" type="ieee_double"/>
> > +
> > +  <reg name="fcsr" bitsize="32" group="float"/>
> > +  <reg name="fir" bitsize="32" group="float"/>
> > +</feature>
> 
>  This is AFAICT the same as mips64-fpu.xml, except for the sizes of `fcsr' 
> and `fir', wrongly set to 64 bits in the latter file.  I think we ought to 
> avoid the unnecessary duplication, and just use a single template for the 
> FPU, which is the same in the 64-bit FP case regardless of the machine 
> word width of the CPU.
> 
>  Can you therefore please just reuse mips64-fpu.xml here, which I think 
> should be renamed at the same time to mips-fpu64.xml, as with your 
> proposal (GIT will detect a rename and show it properly with diffs and 
> other stats)?  For historical reasons we need to keep $fcsr and $fir at 64 
> bits in XML descriptions, as explained in commit 78b86327b530 ("mips-tdep: 
> Make FCRs always 32-bit"), made specifically to address the inconsistency 
> discovered in the context of your submission; none of any further FCRs 
> added will have any historical implications though, so they ought to be 
> 32-bit as in hardware.
> 
>  You'll have to adjust the callers of `mips_collect_register_32bit' and 
> `mips_supply_register_32bit' in `gdbserver' accordingly so that 
> `use_64bit' is set according to the FPU rather than the CPU model then.  
> I don't think you'll need to adjust anything in mips-linux-tdep.c, however 
> please double-check.
> 
> > diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
> > index 1e874e3..9913d6a 100644
> > --- a/gdb/gdbserver/Makefile.in
> > +++ b/gdb/gdbserver/Makefile.in
> > @@ -357,7 +357,8 @@ clean:
> >  	rm -f reg-tilegx.c reg-tilegx32.c
> >  	rm -f arm-with-iwmmxt.c
> >  	rm -f arm-with-vfpv2.c arm-with-vfpv3.c arm-with-neon.c
> > -	rm -f mips-linux.c mips-dsp-linux.c
> > +	rm -f mips-linux.c mips-dsp-linux.c mips-fpu64-linux.c
> > +	rm -f mips-fpu64-dsp-linux.c
> >  	rm -f mips64-linux.c mips64-dsp-linux.c
> >  	rm -f nios2-linux.c
> >  	rm -f powerpc-32.c powerpc-32l.c powerpc-64l.c powerpc-e500l.c
> 
>  This will then have to reflect the removal of mips64-fpu.xml of course.
> 
> > index a54b9e7..422fd90 100644
> > --- a/gdb/gdbserver/configure.srv
> > +++ b/gdb/gdbserver/configure.srv
> > @@ -187,15 +187,20 @@ case "${target}" in
> >  			;;
> >    mips*-*-linux*)	srv_regobj="mips-linux.o"
> >  			srv_regobj="${srv_regobj} mips-dsp-linux.o"
> > +			srv_regobj="${srv_regobj} mips-fpu64-linux.o"
> > +			srv_regobj="${srv_regobj} mips-fpu64-dsp-linux.o"
> >  			srv_regobj="${srv_regobj} mips64-linux.o"
> >  			srv_regobj="${srv_regobj} mips64-dsp-linux.o"
> >  			srv_tgtobj="$srv_linux_obj linux-mips-low.o"
> >  			srv_tgtobj="${srv_tgtobj} mips-linux-watch.o"
> >  			srv_xmlfiles="mips-linux.xml"
> >  			srv_xmlfiles="${srv_xmlfiles} mips-dsp-linux.xml"
> > +			srv_xmlfiles="${srv_xmlfiles} mips-fpu64-linux.xml"
> > +			srv_xmlfiles="${srv_xmlfiles} mips-fpu64-dsp-linux.xml"
> >  			srv_xmlfiles="${srv_xmlfiles} mips-cpu.xml"
> >  			srv_xmlfiles="${srv_xmlfiles} mips-cp0.xml"
> >  			srv_xmlfiles="${srv_xmlfiles} mips-fpu.xml"
> > +			srv_xmlfiles="${srv_xmlfiles} mips-fpu64.xml"
> >  			srv_xmlfiles="${srv_xmlfiles} mips-dsp.xml"
> >  			srv_xmlfiles="${srv_xmlfiles} mips64-linux.xml"
> >  			srv_xmlfiles="${srv_xmlfiles} mips64-dsp-linux.xml"
> 
>  Likewise.
> 
>   Maciej

Attachment: signature.asc
Description: Digital signature