Add support for Hyperlight KVM guest debugging using gdb #111

dblnz · 2024-12-13T17:54:02Z

The current implementation supports:

add/remove HW breakpoints (max 4)
add/remove SW breakpoints
read/write registers
read/write addresses
step/continue
get code offset from target

The overall architecture is designed to work like a Request - Response protocol from the gdb thread to the hypervisor handler over a DebugCommChannel.

All the functionality is implemented on the hypervisor side so it has access to the shared memory and VcpuFd
The gdb thread uses gdbstub to handle the communication with the gdb client
when the gdb client requests the supported features mentioned above, a request is sent forward to the hypervisor handler for the running sandbox to run

devigned

Super excited to see this landing. Nice work! My feedback mostly consists of nits and questions.

docs/how-to-debug-a-hyperlight-guest.md

src/hyperlight_host/src/hypervisor/gdb/mod.rs

src/hyperlight_host/src/hypervisor/gdb/target.rs

src/hyperlight_host/src/hypervisor/hypervisor_handler.rs

src/hyperlight_host/src/hypervisor/kvm.rs

src/hyperlight_host/src/hypervisor/hypervisor_handler.rs

Signed-off-by: Doru Blânzeanu <[email protected]>

…en gdb feature is on Signed-off-by: Doru Blânzeanu <[email protected]>

Signed-off-by: Doru Blânzeanu <[email protected]>

- this type will be used by the gdb and the hypervisor handler to send requests and receive responses Signed-off-by: Doru Blânzeanu <[email protected]>

- the target implements the traits to provide callbacks for the gdb commands Signed-off-by: Doru Blânzeanu <[email protected]>

- adds a specific handler for the vcpu exit debug that waits for debug messages and processes them Signed-off-by: Doru Blânzeanu <[email protected]>

Signed-off-by: Doru Blânzeanu <[email protected]>

- also adds handling of gdb client disconnect by resuming execution Signed-off-by: Doru Blânzeanu <[email protected]>

Signed-off-by: Doru Blânzeanu <[email protected]>

ludfjig

This is awesome!! Looked briefly though everything and I will give a more in-depth review tomorrow

src/hyperlight_host/Cargo.toml

src/hyperlight_host/src/hypervisor/gdb/mod.rs

src/hyperlight_host/src/sandbox/config.rs

src/hyperlight_host/src/hypervisor/gdb/x86_64_target.rs

- improve documentation - improve gdb errors by adding a specific error variant for when the rflags conversion from u64 to u32 fails - fix clippy issue on windows Signed-off-by: Doru Blânzeanu <[email protected]>

- the user experience is not great as the fields and methods related to gdb port are not accessible unless the gdb feature is turned on. - if one runs the build command in the cli and provides the --features gdb the IDE will not know that the feature is enabled and will show errors as it doesn't know about the fields/methods Signed-off-by: Doru Blânzeanu <[email protected]>

- when using visual studio code, sometimes it requests to read from unexpected addresses which can cause an error on the hypervisor side. This fix signals this to the gdb thread which marks it as a non-fatal error Signed-off-by: Doru Blânzeanu <[email protected]>

ludfjig

This is awesome. I tried it out and it works great. Left some more comments, feel free to dismiss if you disagree with any of them. I realize some of them might be hard to implement as well. GREAT WORK!!

ludfjig · 2025-02-06T22:49:08Z

src/hyperlight_host/src/sandbox/config.rs

+    #[instrument(skip_all, parent = Span::current(), level= "Trace")]
+    /// Sets the configuration for the guest debug
+    pub fn set_guest_debug_port(&mut self, port: u16) {
+        self.guest_debug_port = Some(max(port, Self::MIN_GUEST_DEBUG_PORT));


I called this function but forgot to enable the gdb feature. The code ran fine, but didn't wait for gdb to connect, and I got confused. In my opinion, maybe we should feature-gate this function, or return error somewhere (maybe not in this func) if a user is trying to use a sandbox with this setting set, without using the gdb feature. Maybe others have better ideas. Maybe some tests for this could be good to

This was the previous intention, however, I disabled it with the last commit.
The reasoning behind was that the rust-analyzer didn't know about the feature being enabled and it showed errors in IDE as it doesn't know the methods.
I am not sure which is the best approach, you might be right with being confusing when it doesn't do anything.

You should be able to enable the feature for rust-analyzer in .vscode/settings.json

ludfjig · 2025-02-06T23:25:37Z

docs/how-to-debug-a-hyperlight-guest.md

+
+### End to end example
+
+Using the [Sandbox configuration](#sandbox-configuration) above to configure the [hello-world](https://github.com/hyperlight-dev/hyperlight/blob/main/src/hyperlight_host/examples/hello-world/main.rs) example


I think it would be useful if we created a brand new example for this instead of using hello-world. I read through this doc but missed the part where it said "Using the sandbox configuration above", so I was confused why it wasn't working. What do you think?

Good point, I can add another example

ludfjig · 2025-02-06T23:33:33Z

docs/how-to-debug-a-hyperlight-guest.md

+```bash
+# Terminal 2
+$ cat .gdbinit
+file file src/tests/rust_guests/bin/debug/simpleguest


file file typo?

ludfjig · 2025-02-07T04:41:11Z

src/hyperlight_host/Cargo.toml

@@ -128,6 +130,8 @@ kvm = ["dep:kvm-bindings", "dep:kvm-ioctls"]
 mshv2 = ["dep:mshv-bindings2", "dep:mshv-ioctls2"]
 mshv3 = ["dep:mshv-bindings3", "dep:mshv-ioctls3"]
 inprocess = []
+# This enables compilation of gdb stub for easy debug in the guest


I think you can drop the first part of this comment, since we probably don't care too much about what compilation it enables, but rather that it allows for easy gdb debugging of a guest.

ludfjig · 2025-02-07T04:49:04Z

src/hyperlight_host/src/hypervisor/kvm.rs

+            self.dbg_cfg.arch.debugreg = [0; 8];
+            for (k, addr) in addrs.iter().enumerate() {
+                self.dbg_cfg.arch.debugreg[k] = *addr;
+                self.dbg_cfg.arch.debugreg[7] |= 1 << (k * 2);


Would you mind adding a comment about what these bit operations does/means?

ludfjig · 2025-02-07T05:18:30Z

src/hyperlight_host/src/hypervisor/kvm.rs

+            if self.debug.is_some() {
+                log::debug!("Setting entrypoint bp {:X}", self.entrypoint);
+                let mut entrypoint_debug = KvmDebug::new();
+                entrypoint_debug.hw_breakpoints.push(self.entrypoint);


Could we make an method on KvmDebug for adding a breakpoint that does error checking (>4 breapoints), instead of manually modifying the vec? It seems fragile to do the error checking during the set_breakpoints, and to expose the inner vec like this.

good point, I'll change it 👍

ludfjig · 2025-02-07T05:22:18Z

src/hyperlight_host/src/hypervisor/kvm.rs

+            let addr = self.translate_gva(addr)?;
+
+            if let Some(debug) = self.debug.as_mut() {
+                if debug.hw_breakpoints.contains(&addr) {


Seems like the .contains is unnecessary given that you use .position later anyway. You might be able to simplify this whole block using hw_breakpoints.retain(|a| a != addrv) or something along those lines

thanks for the suggestion

ludfjig · 2025-02-07T05:24:26Z

src/hyperlight_host/src/hypervisor/gdb/event_loop.rs

+                }
+            }
+
+            if conn.peek().map(|b| b.is_some()).unwrap_or(false) {


I'm not sure I really understand what this is doing. Maybe a comment here could help

I'll add a comment

ludfjig · 2025-02-07T05:29:02Z

src/hyperlight_host/src/hypervisor/gdb/mod.rs

+/// Enumerates the possible responses that a hypervisor can provide to a debugger
+#[derive(Debug)]
+pub enum DebugResponse {
+    AddHwBreakpoint(bool),


Do we need all of these responses, or would it be sufficient to combine some of them into something like Success()? I looked briefly and I didn't see AddHwBreakpoint and AddSwBreakpoint being used for anything for example. ( can see DebugResponse::Step | DebugResponse::Continue | DebugResponse::DisableDebug are being used though)

The need stems from the return value the add_sw_breakpoint and the others expect.
An example signature is:

fn add_sw_breakpoint( &mut self, addr: <Self::Arch as Arch>::Usize, _kind: <Self::Arch as Arch>::BreakpointKind, ) -> TargetResult<bool, Self> {

and it expects true/false on the success path signaling whether the breakpoint is active, true being it was just added, false being it was already active and the new call didn't actually do anything.

I could put all of them together to make a generic response that returns Result<bool, Error> but I am not sure it would make it look a lot better getting rid of only 3 variants. Let me know what you think

ludfjig · 2025-02-07T05:37:41Z

docs/how-to-debug-a-hyperlight-guest.md

+enable pretty-printer
+layout src
+
+$ gdb


This markdown is great. One thing this gdb feature introduces is some complexity regarding threading. Would you be able to write down (or maybe create a diagram) for how the threads interact with each other, for example the handler thread and so on? It's a bit confusing to me which thread waits for what thread, etc :D. A simple list about what happens in what order when you create a sandbox with gdb enabled would be sufficient

this is a good idea, I thought about adding a diagram but I was afraid it would get desynchronized with the code when there would be a change as the documentation usually doesn't get updated unless it is close to the code being changed.

I'll work on a diagram to help with the understanding of the inner workings of the feature.

dblnz added the kind/enhancement New feature or request label Dec 13, 2024

dblnz self-assigned this Dec 13, 2024

dblnz marked this pull request as draft December 13, 2024 17:54

dblnz force-pushed the gdb-support-latest branch 2 times, most recently from be21b85 to ca61def Compare December 19, 2024 11:41

devigned requested changes Jan 22, 2025

View reviewed changes

dblnz force-pushed the gdb-support-latest branch from 4a44b2e to 16492de Compare January 22, 2025 15:40

dblnz marked this pull request as ready for review January 22, 2025 16:07

dblnz changed the title ~~Initial support for gdb debugging in Hyperlight KVM guest~~ Add support for Hyperlight KVM guest debugging using gdb Jan 22, 2025

danbugs reviewed Jan 28, 2025

View reviewed changes

src/hyperlight_host/src/hypervisor/hypervisor_handler.rs Outdated Show resolved Hide resolved

src/hyperlight_host/src/hypervisor/hypervisor_handler.rs Outdated Show resolved Hide resolved

dblnz added 17 commits February 4, 2025 22:54

add gdb cargo feature

90a3f91

Signed-off-by: Doru Blânzeanu <[email protected]>

disable timeouts for messaging between host and hypervisor handler wh…

1913aa5

…en gdb feature is on Signed-off-by: Doru Blânzeanu <[email protected]>

add guest debug port to sandbox configuration

c5aeada

Signed-off-by: Doru Blânzeanu <[email protected]>

add gdb debug tread creation method

579b324

Signed-off-by: Doru Blânzeanu <[email protected]>

add debug communication channel type

535dd75

- this type will be used by the gdb and the hypervisor handler to send requests and receive responses Signed-off-by: Doru Blânzeanu <[email protected]>

add hyperlight sandbox target to handle gdb commands support

1203ff4

- the target implements the traits to provide callbacks for the gdb commands Signed-off-by: Doru Blânzeanu <[email protected]>

handle debug vcpu exit on kvm side

ba57971

- adds a specific handler for the vcpu exit debug that waits for debug messages and processes them Signed-off-by: Doru Blânzeanu <[email protected]>

add kvm debug configuration that handles vCPU interraction

a417206

Signed-off-by: Doru Blânzeanu <[email protected]>

add read/write registers support

bfe70b0

Signed-off-by: Doru Blânzeanu <[email protected]>

hw breakpoint and resume support added

5c22657

Signed-off-by: Doru Blânzeanu <[email protected]>

add stepping support

a1b0a5d

- also adds handling of gdb client disconnect by resuming execution Signed-off-by: Doru Blânzeanu <[email protected]>

add a way to read/write host shared mem when debugging

d9a364b

Signed-off-by: Doru Blânzeanu <[email protected]>

add read/write address and text section offset support

e550a84

Signed-off-by: Doru Blânzeanu <[email protected]>

handlers fixup

b175f41

Signed-off-by: Doru Blânzeanu <[email protected]>

add sw breakpoint support

a4bb6dd

Signed-off-by: Doru Blânzeanu <[email protected]>

add ci checks for the gdb feature

5476cdd

Signed-off-by: Doru Blânzeanu <[email protected]>

add documentation about the gdb debugging feature

d0db26e

Signed-off-by: Doru Blânzeanu <[email protected]>

dblnz force-pushed the gdb-support-latest branch from 2a36f06 to d0db26e Compare February 4, 2025 21:07

ludfjig previously approved these changes Feb 5, 2025

View reviewed changes

fix issues related to clippy, typos and feedback

af0a52a

- improve documentation - improve gdb errors by adding a specific error variant for when the rflags conversion from u64 to u32 fails - fix clippy issue on windows Signed-off-by: Doru Blânzeanu <[email protected]>

dblnz dismissed ludfjig’s stale review via 6cc6184 February 5, 2025 12:57

dblnz force-pushed the gdb-support-latest branch from 17a5d14 to 98ac60e Compare February 5, 2025 16:06

ludfjig approved these changes Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Hyperlight KVM guest debugging using gdb #111

Add support for Hyperlight KVM guest debugging using gdb #111

dblnz commented Dec 13, 2024 •

edited

Loading

devigned left a comment

ludfjig left a comment

ludfjig left a comment

ludfjig Feb 6, 2025

dblnz Feb 7, 2025

ludfjig Feb 7, 2025

ludfjig Feb 6, 2025

dblnz Feb 7, 2025

ludfjig Feb 6, 2025

ludfjig Feb 7, 2025

ludfjig Feb 7, 2025 •

edited

Loading

ludfjig Feb 7, 2025

dblnz Feb 7, 2025

ludfjig Feb 7, 2025

dblnz Feb 7, 2025

ludfjig Feb 7, 2025

dblnz Feb 7, 2025

ludfjig Feb 7, 2025

dblnz Feb 7, 2025 •

edited

Loading

ludfjig Feb 7, 2025 •

edited

Loading

dblnz Feb 7, 2025


		### End to end example

		Using the [Sandbox configuration](#sandbox-configuration) above to configure the [hello-world](https://github.com/hyperlight-dev/hyperlight/blob/main/src/hyperlight_host/examples/hello-world/main.rs) example

Add support for Hyperlight KVM guest debugging using gdb #111

Are you sure you want to change the base?

Add support for Hyperlight KVM guest debugging using gdb #111

Conversation

dblnz commented Dec 13, 2024 • edited Loading

devigned left a comment

Choose a reason for hiding this comment

ludfjig left a comment

Choose a reason for hiding this comment

ludfjig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ludfjig Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblnz Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

ludfjig Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblnz commented Dec 13, 2024 •

edited

Loading

ludfjig Feb 7, 2025 •

edited

Loading

dblnz Feb 7, 2025 •

edited

Loading

ludfjig Feb 7, 2025 •

edited

Loading