From 740c824fe8c552d44f355fa3dd8a80bab8284b0e Mon Sep 17 00:00:00 2001 From: David LeGare Date: Tue, 12 Apr 2022 15:52:50 +0000 Subject: [PATCH] Update to gdbstub 0.6.1 Update the GDB stub implementation to the 0.6 version of the gdbstub crate API, attempting to preserve the current behavior as much as possible. Hardware breakpoints and single stepping still work, but some existing issues with software breakpoints are still present. BUG=None TEST=Manual Cq-Depend: chromium:3578400 Change-Id: I522242a1a2055ecdf47b2010a615dc9e0136ebd0 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3578025 Tested-by: kokoro Auto-Submit: David LeGare Reviewed-by: Keiichi Watanabe Commit-Queue: Keiichi Watanabe --- Cargo.toml | 4 +- arch/Cargo.toml | 2 +- src/gdb.rs | 216 ++++++++++++++++++++++++++++-------------- vm_control/Cargo.toml | 2 +- x86_64/Cargo.toml | 2 +- 5 files changed, 152 insertions(+), 74 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9cfd258660..92583b6922 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -162,8 +162,8 @@ data_model = "*" devices = { path = "devices" } disk = { path = "disk" } enumn = "0.1.0" -gdbstub = { version = "0.5.0", optional = true } -gdbstub_arch = { version = "0.1.0", optional = true } +gdbstub = { version = "0.6.1", optional = true } +gdbstub_arch = { version = "0.2.2", optional = true } rutabaga_gfx = { path = "rutabaga_gfx"} hypervisor = { path = "hypervisor" } kernel_cmdline = { path = "kernel_cmdline" } diff --git a/arch/Cargo.toml b/arch/Cargo.toml index bd776158c1..51b89aa8c5 100644 --- a/arch/Cargo.toml +++ b/arch/Cargo.toml @@ -14,7 +14,7 @@ acpi_tables = { path = "../acpi_tables" } anyhow = "*" base = { path = "../base" } devices = { path = "../devices" } -gdbstub_arch = { version = "0.1.0", optional = true } +gdbstub_arch = { version = "0.2.2", optional = true } hypervisor = { path = "../hypervisor" } kernel_cmdline = { path = "../kernel_cmdline" } libc = "*" diff --git a/src/gdb.rs b/src/gdb.rs index 9972512d6b..8ade45c1db 100644 --- a/src/gdb.rs +++ b/src/gdb.rs @@ -15,14 +15,20 @@ use vm_control::{ use vm_memory::GuestAddress; use gdbstub::arch::Arch; -use gdbstub::target::ext::base::singlethread::{ResumeAction, SingleThreadOps, StopReason}; -use gdbstub::target::ext::base::{BaseOps, GdbInterrupt}; +use gdbstub::common::Signal; +use gdbstub::conn::{Connection, ConnectionExt}; +use gdbstub::stub::run_blocking::BlockingEventLoop; +use gdbstub::stub::{run_blocking, SingleThreadStopReason}; +use gdbstub::target::ext::base::singlethread::{ + SingleThreadBase, SingleThreadResume, SingleThreadResumeOps, SingleThreadSingleStep, + SingleThreadSingleStepOps, +}; +use gdbstub::target::ext::base::BaseOps; use gdbstub::target::ext::breakpoints::{ Breakpoints, BreakpointsOps, HwBreakpoint, HwBreakpointOps, }; use gdbstub::target::TargetError::NonFatal; use gdbstub::target::{Target, TargetResult}; -use gdbstub::Connection; #[cfg(target_arch = "x86_64")] use gdbstub_arch::x86::X86_64_SSE as GdbArch; use remain::sorted; @@ -51,10 +57,10 @@ pub fn gdb_thread(mut gdbstub: GdbStub, port: u32) { }; info!("GDB connected from {}", addr); - let connection: Box> = Box::new(stream); - let mut gdb = gdbstub::GdbStub::new(connection); + let connection: Box> = Box::new(stream); + let gdb = gdbstub::stub::GdbStub::new(connection); - match gdb.run(&mut gdbstub) { + match gdb.run_blocking::(&mut gdbstub) { Ok(reason) => { info!("GDB session closed: {:?}", reason); } @@ -95,6 +101,7 @@ pub struct GdbStub { vcpu_com: Vec>, from_vcpu: mpsc::Receiver, + single_step: bool, hw_breakpoints: Vec, } @@ -108,6 +115,7 @@ impl GdbStub { vm_tube: Mutex::new(vm_tube), vcpu_com, from_vcpu, + single_step: false, hw_breakpoints: Default::default(), } } @@ -134,8 +142,7 @@ impl GdbStub { } impl Target for GdbStub { - // TODO(keiichiw): Replace `()` with `X86_64CoreRegId` when we update the gdbstub crate. - type Arch = GdbArch<()>; + type Arch = GdbArch; type Error = &'static str; fn base_ops(&mut self) -> BaseOps { @@ -143,70 +150,24 @@ impl Target for GdbStub { } // TODO(keiichiw): sw_breakpoint, hw_watchpoint, extended_mode, monitor_cmd, section_offsets - fn breakpoints(&mut self) -> Option> { + fn support_breakpoints(&mut self) -> Option> { Some(self) } + + // TODO(crbug.com/1141812): Remove this override once proper software breakpoint + // support has been added. + // + // Overriding this method to return `true` allows GDB to implement software + // breakpoints by writing breakpoint instructions directly into the target's + // instruction stream. This will be redundant once proper software breakpoints + // have been implemented. See the trait method's docs for more information: + // https://docs.rs/gdbstub/0.6.1/gdbstub/target/trait.Target.html#method.guard_rail_implicit_sw_breakpoints + fn guard_rail_implicit_sw_breakpoints(&self) -> bool { + true + } } -impl SingleThreadOps for GdbStub { - fn resume( - &mut self, - action: ResumeAction, - check_gdb_interrupt: GdbInterrupt, - ) -> Result, Self::Error> { - let single_step = ResumeAction::Step == action; - - if single_step { - match self.vcpu_request(VcpuControl::Debug(VcpuDebug::EnableSinglestep)) { - Ok(VcpuDebugStatus::CommandComplete) => {} - Ok(s) => { - error!("Unexpected vCPU response for EnableSinglestep: {:?}", s); - return Err("Unexpected vCPU response for EnableSinglestep"); - } - Err(e) => { - error!("Failed to request EnableSinglestep: {}", e); - return Err("Failed to request EnableSinglestep"); - } - }; - } - - self.vm_request(VmRequest::Resume).map_err(|e| { - error!("Failed to resume the target: {}", e); - "Failed to resume the target" - })?; - - let mut check_gdb_interrupt = check_gdb_interrupt.no_async(); - // Polling - loop { - // TODO(keiichiw): handle error? - if let Ok(msg) = self - .from_vcpu - .recv_timeout(std::time::Duration::from_millis(100)) - { - match msg.msg { - VcpuDebugStatus::HitBreakPoint => { - if single_step { - return Ok(StopReason::DoneStep); - } else { - return Ok(StopReason::HwBreak); - } - } - status => { - error!("Unexpected VcpuDebugStatus: {:?}", status); - } - } - } - - if check_gdb_interrupt.pending() { - self.vm_request(VmRequest::Suspend).map_err(|e| { - error!("Failed to suspend the target: {}", e); - "Failed to suspend the target" - })?; - return Ok(StopReason::GdbInterrupt); - } - } - } - +impl SingleThreadBase for GdbStub { fn read_registers( &mut self, regs: &mut ::Registers, @@ -292,10 +253,58 @@ impl SingleThreadOps for GdbStub { } } } + + #[inline(always)] + fn support_resume(&mut self) -> Option> { + Some(self) + } +} + +impl SingleThreadResume for GdbStub { + fn resume(&mut self, _signal: Option) -> Result<(), Self::Error> { + // TODO: Handle any incoming signal. + + self.vm_request(VmRequest::Resume).map_err(|e| { + error!("Failed to resume the target: {}", e); + "Failed to resume the target" + }) + } + + #[inline(always)] + fn support_single_step(&mut self) -> Option> { + Some(self) + } +} + +impl SingleThreadSingleStep for GdbStub { + fn step(&mut self, _signal: Option) -> Result<(), Self::Error> { + // TODO: Handle any incoming signal. + + match self.vcpu_request(VcpuControl::Debug(VcpuDebug::EnableSinglestep)) { + Ok(VcpuDebugStatus::CommandComplete) => { + self.single_step = true; + } + Ok(s) => { + error!("Unexpected vCPU response for EnableSinglestep: {:?}", s); + return Err("Unexpected vCPU response for EnableSinglestep"); + } + Err(e) => { + error!("Failed to request EnableSinglestep: {}", e); + return Err("Failed to request EnableSinglestep"); + } + }; + + self.vm_request(VmRequest::Resume).map_err(|e| { + error!("Failed to resume the target: {}", e); + "Failed to resume the target" + })?; + + Ok(()) + } } impl Breakpoints for GdbStub { - fn hw_breakpoint(&mut self) -> Option> { + fn support_hw_breakpoint(&mut self) -> Option> { Some(self) } } @@ -354,3 +363,72 @@ impl HwBreakpoint for GdbStub { } } } + +struct GdbStubEventLoop; + +impl BlockingEventLoop for GdbStubEventLoop { + type Target = GdbStub; + type Connection = Box>; + type StopReason = SingleThreadStopReason; + + fn wait_for_stop_reason( + target: &mut Self::Target, + conn: &mut Self::Connection, + ) -> Result< + run_blocking::Event, + run_blocking::WaitForStopReasonError< + ::Error, + ::Error, + >, + > { + loop { + // TODO(keiichiw): handle error? + if let Ok(msg) = target + .from_vcpu + .recv_timeout(std::time::Duration::from_millis(100)) + { + match msg.msg { + VcpuDebugStatus::HitBreakPoint => { + if target.single_step { + target.single_step = false; + return Ok(run_blocking::Event::TargetStopped( + SingleThreadStopReason::DoneStep, + )); + } else { + return Ok(run_blocking::Event::TargetStopped( + SingleThreadStopReason::HwBreak(()), + )); + } + } + status => { + error!("Unexpected VcpuDebugStatus: {:?}", status); + } + } + } + + // If no message was received within the timeout check for incoming data from + // the GDB client. + if conn + .peek() + .map_err(run_blocking::WaitForStopReasonError::Connection)? + .is_some() + { + let byte = conn + .read() + .map_err(run_blocking::WaitForStopReasonError::Connection)?; + return Ok(run_blocking::Event::IncomingData(byte)); + } + } + } + + fn on_interrupt( + target: &mut Self::Target, + ) -> Result, ::Error> { + target.vm_request(VmRequest::Suspend).map_err(|e| { + error!("Failed to suspend the target: {}", e); + "Failed to suspend the target" + })?; + + Ok(Some(SingleThreadStopReason::Signal(Signal::SIGINT))) + } +} diff --git a/vm_control/Cargo.toml b/vm_control/Cargo.toml index 5135d29c5e..874baf9ccd 100644 --- a/vm_control/Cargo.toml +++ b/vm_control/Cargo.toml @@ -11,7 +11,7 @@ gdb = ["gdbstub_arch"] balloon_control = { path = "../common/balloon_control" } base = { path = "../base" } data_model = { path = "../common/data_model" } -gdbstub_arch = { version = "0.1.0", optional = true } +gdbstub_arch = { version = "0.2.2", optional = true } hypervisor = { path = "../hypervisor" } libc = "*" remain = "*" diff --git a/x86_64/Cargo.toml b/x86_64/Cargo.toml index a285dab210..c75c563bd8 100644 --- a/x86_64/Cargo.toml +++ b/x86_64/Cargo.toml @@ -13,7 +13,7 @@ arch = { path = "../arch" } assertions = { path = "../common/assertions" } data_model = { path = "../common/data_model" } devices = { path = "../devices" } -gdbstub_arch = { version = "0.1.0", optional = true } +gdbstub_arch = { version = "0.2.2", optional = true } hypervisor = { path = "../hypervisor" } kernel_cmdline = { path = "../kernel_cmdline" } kernel_loader = { path = "../kernel_loader" }