gdbstub_arch: Add support for AArch64 #109
Conversation
|
Thank you for the contribution! This is certainly one of the more interesting Arch implementations I've seen, and I'm looking forward to merging it in :) wrt. clippy and rustfmt: I think it's totally fine to slap a wrt. some of the validation: I will continue to iterate on the specific wording on the PR template, and make it clear that in the case of adding a new Your analysis regarding target.xml is spot on, and it seems you've got a good handle on the different challenges + solutions to supporting target-specific subsets of system registers. And indeed, this issue falls under the umbrella of #12, which is currently the longest-standing unsolved issue in One (currently unimplemented) feature that could be particularly useful for AArch64 is A rough sketch of how this might work at the Arch level (for demonstrative purposes - not necessarily a final design I'd want to see implemented): trait Sysregs {
fn xml() -> &'static str {}
}
enum AArch64NoSysregs {}
impl Sysregs for AArch64NoSysregs {
fn xml() -> &'static str { "" }
}
enum AArch64<S: Sysregs = AArch64NoSysregs> {}
impl<S: Sysregs> Arch for AArch64<S> {
// support non-`target.xml` annexes
fn target_description_xml(annex: &[u8]) -> Option<&'static str> {
match annex {
b"target.xml" => { /* return static string which includes a <xi:include href="sysregs.xml"/> tag */ }
b"sysregs.xml" => { S::xml() }
}
}
}You could imagine extending ...of course, as exciting as all this sounds, this would require quite a bit of core So, with that being said, assuming we want to land this in
(note that I would not want to introduce arch-specific compile time feature flags to I'm OK with any of those 3 options, so it really just depends what you're up for. Note that regardless which option we go for, you'll need to beef-up the docs on the AArch64 type / module to discuss some of the nuance in supporting the large number of system registers. Or, in other words: most of the stuff in the PR description + some stuff from this follow-up comment should become part of the docs. |
That's a great idea: ship all of the XML in the
+1
Okay, let me suggest a fourth one, then! I'm not sure I follow your code example so please let me know if I'm missing something but instead of having a per-sysreg granularity (which I assume from your example), what do you think of introducing an We could then have one All of the XML-handling code remains abstracted away in BTW, would this necessarily need to break the "API" (e.g. by extending the
Making Ideally, the macro should be architecture-agnostic.
Will do! |
I read through your suggestion, and yeah, I think that'd work! Quite elegantly at that, I might add! There is one thing you didn't touch on in your proposal that merits some thought: how this would affect After all, that type is supposed to represent all possible (core) register types a Arch exposes. Right now, this is generally modeled as a
Until we have some kind of implementation on the table, it's hard to judge whether something requires a breaking API change. That said... my gut feeling is that any contortions we might do to avoid a breaking change would be overly complicated vs. just biting the bullet and tweaking APIs directly. FWIW, releasing 0.7 isn't a huge deal, as solving #12 is as good of an excuse as any for a breaking release! Not to mention there are already a trail of small API papercuts that I wouldn't mind closing out in 0.7 as well :)
For clarity's sake, what I was proposing was a more moderate approach of That said, it would be cool to have some kind of comptime and/or runtime XML generator, as this would get us closer to unifying classic LLDB register definitions with target.xml (#103), but this is very much in the realm of long-term "nice to have". With all that said, you didn't actually give me a straight answer for what you wanted to do in the short term with this PR 😅 Would you like to land things as-is (using one of those options I mentioned above), or hold-off until we have some kind of dynamic target feature selection story? |
From my basic understanding of the GDB protocol (you surely know more than I do), I had assumed that In this context (which might not hold for other architectures), I don't see the point of extending From my PoV, I believe that the feature-based split shouldn't transpire beyond the So, to answer your questions: it shouldn't if my assumptions about AArch64 are valid across architectures.
Yes, I think that's the right way to look at it.
I can tell: https://github.com/daniel5151/gdbstub/milestone/3 :)
Yes, that's what I had in mind, similar to what
Right, I'd probably like to land things as-is so that you can release a new (minor) version of the crate and I get unblocked on the |
Ahh, yeah, unfortunately, this doesn't hold for other architectures. The contents of the In practice, this means that many targets send their core registers along with their FPU registers as part of the After all, while your use case doesn't benefit from sending FPU state back and forth on each bulk register transfer, that's not strictly true in all cases. Indeed, check out some of the EDIT: I re-read my initial comment, and I can see where the confusion was! When I said "core", I was using it in the sense of "registers that aren't high-regnum sysregs", rather than the formal "core" feature in the target.xml. My bad - I should've been using more specific terminology.
Nothing, aside from perf :) Way back in the day, the (aside: it'd be great if And really, this touches on an interesting point: the decision as to which registers should be sent as part of the bulk
Sure, sounds good. Seems CI is failing right now, but if you can resolve those, we can go ahead and merge it in + push out a release :)
Yeah, sounds good! |
Besides Hi, I am working on the AArch64 GDB support of CH: cloud-hypervisor/cloud-hypervisor#4355. And I kept monitoring this PR and the knowledgeable discussion. It's nice to know that the first-stage implementation was agreed. |
Thanks, I now understand that my misunderstanding came from my assumption that the GDB client somehow knew about "important registers" and was programmed to expect them from
Absolutely! It should be the exact opposite of what I implemented, actually: FP&SIMD registers should be part of
Agreed, for example in the case of But I don't see a way to control that at run-time or even in the
Right, I've silenced I have pushed a few
|
I'm not sure I follow. As discussed earlier, there really isn't any such things as "architecture mandated registers", and it's entirely up to the discretion of the GDB stub to decide what registers it wants to report to the GDB client. i.e: you're well within your right to back a Aarch64 target.xml that's missing FP defns back to the GDB client, and never report FP registers. The only thing that'll happen is... you won't see those registers in your GDB client (even in the target is still using them under-the-hood). In any case, wrt "how can we tell beforehand if it's going to be used by the debugged code": you're the one deciding the workloads! If you know you'll never need FP registers, feel free to leave them out. If you think you'll need them... include them. Maybe even structure the code to support a runtime switch that toggles them on/off depending on what specific workload you're debugging. And fwiw, all the same logic applies to MIPS as well. And indeed, more concretely: there's nothing stopping someone from using the base Mips arch without DSP regs on a system with DSP regs. All that means is they'd be missing out on extra debug context.
I typically just squash-merge all PRs, so feel free to leave them as-is.
Hmmmmm... While there is an argument to be made that the "no panic" guarantee shouldn't necessarily extend to Instead, I'd just swap out the unreachable for a
Nah, that's fine. Though fwiw, I would just inline the Also, bump from earlier in this PR discussion:
No need to go overboard or anything, but please add some module/type level docs (when appropriate) that direct users to the big lump of constants that are included in the code + draw attention to the fact that the default (oh, and link back to this PR in the docs - lots of useful context in this thread). |
Implement gdbstub::arch::Arch for the 64-bit mode of the ARMv8-A architecture. Signed-off-by: Pierre-Clément Tosi <[email protected]>
Sounds good, I dropped 3796559
Is that possible for a
I've added some documentation for the sysregs, which is the peculiarity of this architecture. |
I suppose it's fine to leave things as-is then. For the sake of consistency, I'm not too keen on having individual Ignore that CI failure. The CI will alway uses the latest Rust version, and it seems 1.63 comes with a new clippy lint that I'll need to fix. I think this should be good to merge! Before I do that though, it'd be nice if @michael2012z could chip in and validate this implementation as well. I won't block the PR on it or anything, but giving them a few days to chime in seems reasonable :) |
I am glad to do that. A little work is needed in my code to adapt to this PR. Will feedback late today or tomorrow. |
|
I integrated the code of the PR to my development branch: https://github.com/michael2012z/cloud-hypervisor/tree/aarch64-gdb Everything looks good! Thank you both for this excellent code. |
|
Thanks, @michael2012z!
It's not pretty but at least, it works; I'll try to improve that: #12 (comment) |
|
PR is merged, and |
Description
Implement gdbstub::arch::Arch for the 64-bit mode of the ARMv8-A architecture.
The noticeable difference between AArch64 and other supported architectures (incl. RISC-V, AFAICT) is the very high number of system registers (theoretically up to ~64k, currently ~800) that the architecture provides. When debugging baremetal code, having access to those registers through the debugger is essential.
To keep the amount of compiled code low, we represent all registers through a
RegId-compliant enum where system registers contain their internalu16"opcode" (this is the value used by the ISA to encode the register into an instruction) and use theregnumXML value to teach the GDB client about thoseu16opcodes i.e. when the client sends a'p'or'P'packet togdbstub,.from_raw_id()is trivial as it simply needs to wrap theregnumin the corresponding Rustenum. This provides a future-proof system with no risk of clash with core registers (as the architecture guarantees that the opcode's top bit will always be1). We also provide compile-timeconstinstances of theSystemvariant to greatly improve readability ofgdbstub's client; although impressive in terms of LoC, these constant values should be 0-cost once compiled.The list of registers (code and XML entries) was auto-generated from official ARM register descriptions.
Note that the XML itself represents a large increase in
.rodatasize; a more dynamic approach would involve the target figuring out, for each register, if it is supported. This would improve the experience of the GDB client as only relevant registers would be reported throughinfo registersbut would prevent us from encapsulating the "regnumhack" described above ingdbstub_archas theTarget(or similar) would then be responsible for transmitting the XML (e.g. by implementingTargetDescriptionXmlOverride), would increase start-up time, and client code would need to make sure that.from_raw_id()doesn't break (although providing a.to_raw_id()would probably be enough).It would also most likely duplicate code between
gdbstubclients (the library should be where such code is centralized) and makegdbstubharder to use on AArch64, compared to other architectures. A simple way to reduce the size of the XML as it is here, if wanted, could be to group system registers in logical groups (e.g. those accessible from EL1, EL2 only, EL3 only, debug registers, ...) andinclude_str!based on acfgbut that would add architecture-specific features to the crate, which we might not want(?)API Stability
Note that
AArch64RegIdexposes a.len()method publicly, which can be useful to client code as AArch64 registers can be of different sizes.Checklist
rustdocformatting looks good (viacargo doc) -> how forgdbstub_arch?examples/armv4twithRUST_LOG=trace+ any relevant GDB output under the "Validation" section below -> not relevant./example_no_std/check_size.shbefore/after changes under the "Validation" section below -> not relevantArchimplementationcrosvmCLs).Validation
GDB output
On a AArch64
Target:armv4t output
Necessary?
Before/After `./example_no_std/check_size.sh` output
No difference, as expected:
Before
After