Bug 251022 - [patch] GDB stub in the kernel reports incorrect length of the ARM FPA registers
Summary: [patch] GDB stub in the kernel reports incorrect length of the ARM FPA registers
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: arm (show other bugs)
Version: CURRENT
Hardware: arm Any
: --- Affects Only Me
Assignee: Mark Johnston
Keywords: patch
Depends on:
Blocks: 251463
  Show dependency treegraph
Reported: 2020-11-10 17:30 UTC by Dmitry Salychev
Modified: 2020-12-11 00:26 UTC (History)
2 users (show)

See Also:

Hardcoded length of the FPA registers (979 bytes, patch)
2020-11-10 17:30 UTC, Dmitry Salychev
no flags Details | Diff
Hardcoded length of the FPA registers (613 bytes, patch)
2020-11-26 21:00 UTC, Dmitry Salychev
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Salychev 2020-11-10 17:30:20 UTC
Created attachment 219527 [details]
Hardcoded length of the FPA registers

A little intro: I've an x86-64 computer running 13.0-CURRENT r366999 which
plays a role of a debugger system running kgdb, and BeagleBone Black
running my custom build (review D5986) of 13.0-CURRENT r367528 as a target.

I've tried to connect kgdb via a serial link to the target in order to
run an external kernel debugger and got:

	(kgdb) set serial baud 115200
	(kgdb) set debug remote 1
	(kgdb) target remote /dev/cuaU1
	Remote debugging using /dev/cuaU1
	Sending packet: $qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;fork-events+;vfork-events+;exec-events+;vContSupported+;QThreadEvents+;no-resumed+;xmlRegisters=i386#6a...Ack
	Sending packet: $g#67...Ack
	Packet received:
	Truncated register 19 in remote 'g' packet

I wasn't able to proceed any further because kgdb wasn't connected to the target.
Comment 1 Dmitry Salychev 2020-11-10 17:39:00 UTC
After several experiments with patched kgdb I discovered that this is what reported by the kernel as a response to the "g" packet:

Packet received:
00000000 (r0)
8805a8c0 (r1)
84eb1fd1 (r2)
13000060 (r3)
ecec1fd1 (r4)
585987c0 (r5)
00080400 (r6)
6c7a31c0 (r7)
ecec1fd1 (r8)
00000000 (r9)
00000000 (r10)
58ec1fd1 (r11)
01000000 (r12)
48ec1fd1 (r13 svc_sp)
147b31c0 (r14 svc_lr)
307b31c0 (r15 pc)
xxxxxxxx (r16, f0)
xxxxxxxx (r17, f1)
xxxxxxxx (r18, f2)
xxxxxxxx (r19, f3)
xxxxxxxx (r20, f4)
xxxxxxxx (r21, f5)
xxxxxxxx (r22, f6)
xxxxxxxx (r23, f7)
xxxxxxxx (r24)
13000060 (r25 spsr)

But kgdb expects:

Register 0: size=4, offset=0
Register 1: size=4, offset=4
Register 2: size=4, offset=8
Register 3: size=4, offset=12
Register 4: size=4, offset=16
Register 5: size=4, offset=20
Register 6: size=4, offset=24
Register 7: size=4, offset=28
Register 8: size=4, offset=32
Register 9: size=4, offset=36
Register 10: size=4, offset=40
Register 11: size=4, offset=44
Register 12: size=4, offset=48
Register 13: size=4, offset=52
Register 14: size=4, offset=56
Register 15: size=4, offset=60
Register 16: size=12, offset=64
Register 17: size=12, offset=76
Register 18: size=12, offset=88
Register 19: size=12, offset=100
Register 20: size=12, offset=112
Register 21: size=12, offset=124
Register 22: size=12, offset=136
Register 23: size=12, offset=148
Register 24: size=4, offset=160
Register 25: size=4, offset=164

According to the [1] f0-f7 are FPA registers which were implemented in only few devices [2], but GDB still reserves space for them for compatibility.

[1] https://developer.arm.com/documentation/dui0056/d/using-the-procedure-call-standard/floating-point-options/the-fpa-architecture

[2] https://retrocomputing.stackexchange.com/questions/13400/history-of-arm-linux-and-fpa
Comment 2 Dmitry Salychev 2020-11-10 17:43:06 UTC
It looks like each register can hold a single-precision, double-precision, or extended-precision value which doesn't answer a question why GDB expects 96-bit registers to me. However, I just want to let kgdb work out-of-the-box in future.
Comment 3 Mark Johnston freebsd_committer 2020-11-24 18:36:39 UTC
Seems reasonable to me.  Register 24 is still not handled in the patch, isn't it a problem?

The patch also increases the gdb buffer size.  I guess it gives a significant throughput improvement?  It is probably not too harmful to bump the buffer size, arm kernel configs do not enable GDB.
Comment 4 Dmitry Salychev 2020-11-24 20:10:10 UTC
(In reply to Mark Johnston from comment #3)

Hello Mark,

kgdb expects 4 bytes for Register 24, so no problem here with the existing implementation of gdb_cpu_regsz():

static __inline size_t
gdb_cpu_regsz(int regnum __unused)
	return (sizeof(int));

I didn't test a throughput tbh, but increased GDB_BUFSZ in order to make enough space for these 96-bit registers as I thought. It looks like I haven't even had to touch it in case of the register values transmission.
Comment 5 Mark Johnston freebsd_committer 2020-11-24 20:17:24 UTC
(In reply to Dmitry Salychev from comment #4)
Ahh, I see.  In that case I think the code should be added to gdb_cpu_regsz() instead.

I don't quite parse the last statement.  The GDB_BUFSZ adjustment is unnecessary?
Comment 6 Dmitry Salychev 2020-11-26 21:00:48 UTC
Created attachment 220011 [details]
Hardcoded length of the FPA registers

gdb_cpu_regsz() modified to report a length of the FPA registers which is expected by GDB (96-bit).
Comment 7 Dmitry Salychev 2020-11-26 21:06:46 UTC
(In reply to Mark Johnston from comment #5)

It's a good idea to modify gdb_cpu_regsz()! Btw, you've parsed my statement correctly. GDB_BUFSZ defines a size of the packets in bytes which can be accepted by the gdb stub and not transmitted.

I've attached a new patch with only necessary changes.
Comment 8 commit-hook freebsd_committer 2020-11-27 16:35:48 UTC
A commit references this bug:

Author: markj
Date: Fri Nov 27 16:35:44 UTC 2020
New revision: 368108
URL: https://svnweb.freebsd.org/changeset/base/368108

  arm: Correctly report the size of FPA registers to GDB

  Modern ARM systems do not have an FPA unit but GDB reserves register
  indices for FPA registers and expects the stub to know their sizes.

  PR:		251022
  Submitted by:	Dmitry Salychev <dsl@mcusim.org>
  MFC after:	2 weeks

Comment 9 Mark Johnston freebsd_committer 2020-11-27 16:36:32 UTC
Thanks Dmitry, I committed the patch with a few cosmetic tweaks.
Comment 10 Dmitry Salychev 2020-11-27 17:46:24 UTC
(In reply to Mark Johnston from comment #9)

Thanks Mark! :) It looks like a first patch of mine found its way to FreeBSD.

Btw, I'll create a review for https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=251053 as we discussed and open a new PR with another patch which allows to modify stack pointer, link register and r0-r12 in ARM target from kgdb during this weekend.
Comment 11 Mark Johnston freebsd_committer 2020-11-28 03:21:18 UTC
(In reply to Dmitry Salychev from comment #10)
Sounds good.  Please ping me and I can help get them reviewed and committed.  I am not an ARM expert so we might need to find a better reviewer for anything complicated.
Comment 12 commit-hook freebsd_committer 2020-12-11 00:26:05 UTC
A commit references this bug:

Author: markj
Date: Fri Dec 11 00:25:35 UTC 2020
New revision: 368535
URL: https://svnweb.freebsd.org/changeset/base/368535

  MFC r368108:
  arm: Correctly report the size of FPA registers to GDB

  PR:	251022

_U  stable/12/