Page MenuHomeFreeBSD

bhyve: Add support for XML register definitions
ClosedPublic

Authored by markj on Jan 30 2024, 3:26 PM.
Tags
None
Referenced Files
F102560341: D43666.diff
Thu, Nov 14, 2:03 AM
Unknown Object (File)
Fri, Nov 8, 10:40 PM
Unknown Object (File)
Wed, Nov 6, 7:40 AM
Unknown Object (File)
Wed, Nov 6, 7:40 AM
Unknown Object (File)
Wed, Nov 6, 7:40 AM
Unknown Object (File)
Wed, Nov 6, 7:40 AM
Unknown Object (File)
Wed, Nov 6, 7:40 AM
Unknown Object (File)
Mon, Nov 4, 9:41 AM

Details

Summary

This is useful for exposing additional registers to debuggers.

The stub indicates support by including the string
"qXfer:features:read+" in its feature list. The debugger queries for
target descriptions by sending the query "qXfer:features:read:" followed
by a file path.

The XML definitions are copied from QEMU and installed to
/usr/share/bhyve/gdb.

Note that we currently don't handle the SIMD registers at all, since
that's of somewhat limited utility (for me at least) and since that
requires new ioctls to fetch the register values.

Sponsored by: Innovate UK

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 55709
Build 52598: arc lint + arc unit

Event Timeline

markj requested review of this revision.Jan 30 2024, 3:26 PM
jrtc27 added inline comments.
usr.sbin/bhyve/gdb.c
1672

O_RESOLVE_BENEATH?

usr.sbin/bhyve/gdb.c
1672

That's implicit since this code runs in a capsicum sandbox, but yes we might as well specify the flag as well.

usr.sbin/bhyve/gdb.c
1658

The simpler approach would be to just generate this file during the build and put it in /usr/share/bhyve/gdb, then you don't need any special-casing here and unmap goes away. Is there a particular reason you didn't?

1672

Not for WITHOUT_CAPSICUM though, which is an option

usr.sbin/bhyve/gdb.c
1658

I may need to specify multiple includes before the closing </target> tag. I specifically need to do that for CHERI, and at the time it was easier to just hard-code target.xml like this. I agree that this would be nicer to generate this at build time nonetheless, I'll give it a try.

1672

From src.conf.5:

WITHOUT_CAPSICUM
        This option has no effect.

See commit c24c117b96445ecdbd4bf9856f88118b376296c3.

The bhyve code still uses WITHOUT_CAPSICUM everywhere but I believe that's just to ease porting to, e.g., illumos.

markj marked 3 inline comments as done.

Apply Jessica's suggestions.

Hmm, I think we shouldn't advertise registers we don't yet support in the XML file.

FWIW, the authoritative XML files are in the GDB repository (and are permissively licensed): https://sourceware.org/git/?p=binutils-gdb.git;a=tree;f=gdb/features/i386;h=8984465fec9388b100aeaa6158b305231a72777e;hb=9c175474a81dc6422e0a57e747bf9dfbacf8270d

I would rather use GDB's files and add a local "feature" for the system registers we supply as an extension.

If we ever support XSAVE registers we will have to generate a dynamic top-level file by only including the relevant FP/vector register features rather than shipping the top-level file as a static file in /usr/share/bhyve/gdb.

In D43666#996481, @jhb wrote:

Hmm, I think we shouldn't advertise registers we don't yet support in the XML file.

You mean, we shouldn't advertise the FP/SIMD registers in the XML? Yes, that's fair, I can remove them.

FWIW, the authoritative XML files are in the GDB repository (and are permissively licensed): https://sourceware.org/git/?p=binutils-gdb.git;a=tree;f=gdb/features/i386;h=8984465fec9388b100aeaa6158b305231a72777e;hb=9c175474a81dc6422e0a57e747bf9dfbacf8270d

I would rather use GDB's files and add a local "feature" for the system registers we supply as an extension.

What's the advantage of using GDB's files? We'd have to modify them to get some useful registers, like CR* and kgsbase, which aren't listed there at all.

So GDB has specific knowledge about register features and requires that given features include certain registers (e.g. the "avx" feature has to include all of the ymm registers, and the "core" feature has to include all of the GPRs). Using GDB's files will ensure we aren't missing required registers.

There is a "segments" feature for fsbase and gsbase btw.

For registers we add (in particular the system registers) we really should just create a new "feature" and place our additional registers there. The additional feature can be named something like "org.freebsd.bhyve.i386.system" for example for things like "cr0", etc.

In terms of CHERI, CHERI GDB requires that the cap registers are part of a specific feature as well and not just lumped into the "core" registers.

In D43666#996748, @jhb wrote:

So GDB has specific knowledge about register features and requires that given features include certain registers (e.g. the "avx" feature has to include all of the ymm registers, and the "core" feature has to include all of the GPRs). Using GDB's files will ensure we aren't missing required registers.

There is a "segments" feature for fsbase and gsbase btw.

For registers we add (in particular the system registers) we really should just create a new "feature" and place our additional registers there. The additional feature can be named something like "org.freebsd.bhyve.i386.system" for example for things like "cr0", etc.

In terms of CHERI, CHERI GDB requires that the cap registers are part of a specific feature as well and not just lumped into the "core" registers.

Based on our conversation earlier this week and some experiments, I think I'd rather punt on this for now. In particular, I don't have any plans to add support for fetching FP or vector registers.

There are some annoyances with using gdb's XML definitions directly:

  • I can't include multiple <feature> elements in the same document for some reason.
  • If I provide 64bit-core.xml, I have to include the i387 registers in the XML, otherwise gdb gives me a "Architecture rejected target-supplied description" warning and falls back to the hard-coded description. But we don't actually provide those registers.
  • To implement the 'p' query, gdb needs to know the index of each register, but that starts to get awkward when we have multiple files (I'd need to add a third one for control registers) and ranges of registers that we don't implement.

So I'd rather just lump everything in one file for now. When it comes time to add dynamic selection (e.g., optional extensions), this scheme will need to be reworked somewhat anyway (e.g., target.xml will need to be dynamically generated), and in the meantime I think the splitting that you're suggesting doesn't do much for us.

Remove definitions for vector registers that we don't support.

The i387 register definitions need to be kept, otherwise gdb rejects
the target description.

We can refine the XML later, this is a good starting point for now.

This revision is now accepted and ready to land.Feb 20 2024, 9:20 PM