Report the values of x86 segment registers to remote debuggers.

[jhb]

Summary

Report the values of x86 segment registers to remote debuggers.

Ensure that the upper 16 bits of segment registers manually saved in trapframes are
cleared by explicitly pushing a zero and then moving the segment register into the
low 16 bits. Certain Intel processors treat a push of a segment register as a move
of the segment register into the low 16 bits leaving the upper 16 bits of the word in
the stack unchanged.

Test Plan

• Check 'info registers' in a remote kgdb session for both amd64 and i386.
• This also fixes the junk in 'show registers' in ddb on i386.

[kib]

I do not know the remote gdb protocol, never looked at it. Would it make sense to put constants, like register numbers, into some header with symbolic names, or is it just empty work ?

sys/i386/i386/gdb_machdep.c

62 ↗

(On Diff #5991)

Weird. ddb correctly reported the values of the %es and %fs to me.

ddb forgets to show %gs, though; it is absent from the trapframe.

sys/i386/i386/machdep.c

2870 ↗

(On Diff #5991)

Yes, this looks right; %gs is not reloaded on the kernel entry.

[jhb]

I don't think it is worth adding constants for the remote protocol. Or at least, I would prefer they were documented in some other header that FreeBSD could borrow. GDB does have headers with these constants, but they are GPLd of course.

sys/i386/i386/gdb_machdep.c

62 ↗

(On Diff #5991)

Hmm, I will recheck in ddb what I see when I see junk in kgdb.

I will also look at fixing ddb.

[jhb]

sys/i386/i386/gdb_machdep.c

62 ↗

(On Diff #5991)

I get junk in ddb:

db> show reg
cs 0x20
ds 0x28
es 0xe03a0028
fs 0xbfbf0008
gs 0x1b
ss 0x28

[jhb]

sys/i386/i386/gdb_machdep.c

62 ↗

(On Diff #5991)

I checked the SDM and this is a "feature" of 32-bit PUSHes of segment registers (in the description of PUSH in vol2):

If the source operand is an immediate of size less than the operand size, a sign-extended
value is pushed on the stack. If the source operand is a segment register (16 bits) and the
operand size is 64-bits, a zero- extended value is pushed on the stack; if the operand size
is 32-bits, either a zero-extended value is pushed on the stack or the segment selector is
written on the stack using a 16-bit move. For the last case, all recent Core and Atom
processors perform a 16-bit move, leaving the upper portion of the stack location
unmodified.

[kib]

sys/i386/i386/gdb_machdep.c

62 ↗

(On Diff #5991)

Ok, so it is not a complete garbage, just the upper half-word is useless. For registers other than %es/%fs, it is probably zero that was already on the stack.

The issue is much less critical than it sound initially.

[jhb]

Yes, though I want to figure out a way to fix it. One option would be to fix the frame by replacing the push with a push of 0 and a mov. The other route is to fix the consumers (ddb and kgdb) to just read the low 16 bits. I already have ddb fixed in my pending patches, (needed a similar fix for the 16-bit seg regs on amd64) so will probably go that route.

[jhb]

• Clear the upper 16 bits of segment registers in the kdb_frame.

[jhb]

Clearing the flags in kdb_frame directly seemed to be the least evil of the choices I considered:

1. changing the asm code to push 0 and mov the seg reg explicitly instead of push seg reg
2. changing gdb_cpu_getreg() to modify the kdb_frame on each register read.
3. setting aside additional static vars for each seg reg that "shadow" tf_[cdef]s and have gdb_cpu_setreg() copy tf_Xs to the static variable and return a pointer to that instead

[kib]

In D2743#53308, @jhb wrote:

Clearing the flags in kdb_frame directly seemed to be the least evil of the choices I considered:

1. changing the asm code to push 0 and mov the seg reg explicitly instead of push seg reg

Why is #1 (i.e. single central place where the cleanup happens) not better than doing it in only one place, leaving other
potential consumers with garbage ?

2. changing gdb_cpu_getreg() to modify the kdb_frame on each register read.
3. setting aside additional static vars for each seg reg that "shadow" tf_[cdef]s and have gdb_cpu_setreg() copy tf_Xs to the static variable and return a pointer to that instead

[jhb]

In D2743#53309, @kib wrote:

In D2743#53308, @jhb wrote:

Clearing the flags in kdb_frame directly seemed to be the least evil of the choices I considered:

1. changing the asm code to push 0 and mov the seg reg explicitly instead of push seg reg

Why is #1 (i.e. single central place where the cleanup happens) not better than doing it in only one place, leaving other
potential consumers with garbage ?

I guess I was worried about the runtime overhead since very few things actually consume
these values. If you prefer #1 I can do that however.

[kib]

In D2743#53310, @jhb wrote:

In D2743#53309, @kib wrote:

In D2743#53308, @jhb wrote:

Clearing the flags in kdb_frame directly seemed to be the least evil of the choices I considered:

1. changing the asm code to push 0 and mov the seg reg explicitly instead of push seg reg

Why is #1 (i.e. single central place where the cleanup happens) not better than doing it in only one place, leaving other
potential consumers with garbage ?

I guess I was worried about the runtime overhead since very few things actually consume
these values. If you prefer #1 I can do that however.

Common wisdom says that stack is mapped to exclusively owned cache lines. Also, I bet that write buffers in modern CPUs are sophisticated enough to handle merge of 16- and 32- bit writes. I would prefer to fix this at one place.

[jhb]

• Explicitly push 0 and mov seg regs onto the stack.
• Remove the hack to clear the upper 16-bits of seg regs from kdb_frame.

[kib]

To be blameless, the patch to zero the upper half-word of the segment registers should be split from the kgdb/ddb part.

[jhb]

Agreed, I will split it out separately.