openvmm: add basic gdbstub support for aarch64 #307

jstarks · 2024-11-13T00:18:35Z

Plumb aarch64 support, using the gdbstub_arch crate to provide the register definitions.

Only register reads and memory access are supported. Register updates and breakpoints are not.

Plumb aarch64 support, using the `gdbstub_arch` crate to provide the register definitions. Only register reads and memory access are supported. Register updates and breakpoints are not.

daprilik · 2024-11-13T01:25:50Z

vmm_core/vmm_core_defs/src/debug_rpc.rs

+#[derive(Debug, Protobuf)]
+pub enum DebuggerVpState {
+    X86_64(X86VpState),
+    Aarch64(Aarch64VpState),


unfortunate that x86 has to "pay" due to ARM support being added here...

are you completely opposed to having this arch gated?

I am not completely opposed. But aren't we trying to minimize cfg(guest_arch = ...)?

I think my worry is that we're making different design choices based on how "impactful" the changes are.

Correct me if I'm wrong, but we use compile-time gates in things like, say, core virt_ infrastructure, precisely because we don't want to carry x86 specific stuff on ARM (and vice-versa). We could've done something similar to this approach, where things like CPU topology would be arch-based enums, but we didn't do that.

I think whichever way we go in this code is gonna be reasonable, but nailing down some specific guidelines / philosophy around multi-arch code would be good moving forwards.

Fair. Collecting some thoughts here... I think general there are three different approaches we can take:

Define types/traits/functions that are the union of the arch-specific details.

Define types/traits/functions that use generics to describe arch-specific details.

Use cfg(guest_arch = ...) to just gate stuff at compile time.

I think each of these have their uses, and I think we can probably be a bit more prescriptive about when you should reach for which tool.

For example, I think we generally should avoid the use of cfg in cross-worker communication types (like the ones defined here), because that makes it hard for us to ever support a cross-arch VMM via emulation--even if the VM worker binary needs to be built multiple times to support multiple arches, we should not require multiple builds of client binaries (such as petri).

It's also just a good principle that the build configuration should not affect the cross-process protocol--we ran into problems with petri being built with different features from openvmm and having protocol incompatibilities. It's easier to have a rule forbidding all cfg in protocol types than to explain that it's just feature that's prohibited.

I also think we should generally avoid arch generics in protocol types, since generics are viral. In this case, we'd have to propagate the arch up through... DebugRequest, VmProxy, State, DebuggerWorker...

So that leaves us with unions, which I think make sense here.

But for traits and functions, different constraints are at play. There, we have experimented with guest_arch and generics to a greater degree. I think we'll want to revisit some of our decisions there over time, and maybe use the union technique in a few more places.

That's a very insightful comment John, thank you. Thinking about it in terms of worker code vs. "client" code is not something I had considered, but makes a lot of sense.

Could you send a PR up which adds these thoughts to https://openvmm.dev/guide/dev_guide/contrib/code.html, so we can codify some of this thinking? Or, if you'd like to drive-by include it as part of this PR (which looks like it'll need at least one more revision anyways), that'd be great.

I'll do this separately. Tracking via #320 so I don't forget.

daprilik · 2024-11-14T21:44:47Z

vmm_core/virt_hvf/src/lib.rs

@@ -641,7 +641,7 @@ impl<'p> virt::Processor for HvfProcessor<'p> {
        _vtl: Vtl,
        _state: Option<&virt::x86::DebugState>,
    ) -> Result<(), Self::Error> {
-        todo!()
+        Ok(())


meta: this seems life a x86 specific function that should prob get tweaked / yoinked from the virt::Processor API (or put behind an IDET or something)

I think this just gets generalized once we support hw breakpoints on ARM.

daprilik · 2024-11-14T21:48:33Z

vmm_core/src/partition_unit/vp_set.rs

+        let DebuggerVpState::Aarch64(_state) = state else {
+            anyhow::bail!("wrong architecture")
+        };
+        anyhow::bail!("todo");


is this hard to do, or just annoying to do? would be nice to just get it out of the way now if its just the latter

I kept reading, and see that there's a solid amount of wiring here. Seems reasonable to punt for now.

I'll wire it up here because it's straightforward. But I won't bother with the end-to-end for now.

daprilik · 2024-11-14T21:50:43Z

workers/debug_worker/src/gdb/targets/target_aarch64/mod.rs

@@ -0,0 +1,95 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+


missing mod comment

daprilik · 2024-11-14T21:51:34Z

workers/debug_worker/src/gdb/targets/target_aarch64/mod.rs

+            },
+            pc: state.pc,
+            cpsr: state.cpsr as u32,
+            v: [0; 32],


are these just TODO, or...?

Yeah. I'll add a comment.

daprilik · 2024-11-14T21:53:06Z

A few misc comments, but broadly LGTM

jstarks requested review from a team as code owners November 13, 2024 00:18

openvmm: add basic gdbstub support for aarch64

350adcd

Plumb aarch64 support, using the `gdbstub_arch` crate to provide the register definitions. Only register reads and memory access are supported. Register updates and breakpoints are not.

jstarks force-pushed the aarch64_gdb branch from a4658b5 to 350adcd Compare November 13, 2024 00:25

fix

c541890

daprilik reviewed Nov 13, 2024

View reviewed changes

paravisor

16915ba

romank-msft previously approved these changes Nov 13, 2024

View reviewed changes

fixes

da0cc49

jstarks dismissed romank-msft’s stale review via da0cc49 November 13, 2024 21:49

jstarks mentioned this pull request Nov 13, 2024

Update coding guidelines on guest_arch #320

Open

clippy

e98ca87

jstarks enabled auto-merge (squash) November 14, 2024 06:38

jstarks requested a review from daprilik November 14, 2024 21:38

daprilik reviewed Nov 14, 2024

View reviewed changes

feedback

d6f6f8b

daprilik approved these changes Nov 17, 2024

View reviewed changes

jstarks merged commit f9ebf91 into microsoft:main Nov 17, 2024
24 checks passed

jstarks deleted the aarch64_gdb branch November 17, 2024 22:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openvmm: add basic gdbstub support for aarch64 #307

openvmm: add basic gdbstub support for aarch64 #307

jstarks commented Nov 13, 2024

daprilik Nov 13, 2024

jstarks Nov 13, 2024

daprilik Nov 13, 2024

jstarks Nov 13, 2024

daprilik Nov 13, 2024

jstarks Nov 13, 2024

daprilik Nov 14, 2024

jstarks Nov 14, 2024

daprilik Nov 14, 2024

daprilik Nov 14, 2024

jstarks Nov 14, 2024

daprilik Nov 14, 2024

daprilik Nov 14, 2024

jstarks Nov 14, 2024

daprilik commented Nov 14, 2024

		@@ -0,0 +1,95 @@
		// Copyright (c) Microsoft Corporation.
		// Licensed under the MIT License.

openvmm: add basic gdbstub support for aarch64 #307

openvmm: add basic gdbstub support for aarch64 #307

Conversation

jstarks commented Nov 13, 2024

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

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

daprilik commented Nov 14, 2024