Hardware api 2 #7
Conversation
Beginning to port the hardware debug API to aarch64. Currently, I have broken the existing code into a 32 bit and 64 bit variant with no overlap between the two, although they share a decent amount of code. I intend to make everything functional before pulling out the common code and structuring it better. At the moment, the breakpoint_001 test passes. Signed-off-by: Alwin Joshy <joshyalwin@gmail.com>
Watchpoints implemented and passing seL4test. Single stepping implementation compiles but has not been tested. Signed-off-by: Alwin Joshy <joshyalwin@gmail.com>
Signed-off-by: Alwin Joshy <joshyalwin@gmail.com>
Signed-off-by: Alwin Joshy <joshyalwin@gmail.com>
Signed-off-by: Alwin Joshy <joshyalwin@gmail.com>
Signed-off-by: Alwin Joshy <joshyalwin@gmail.com>
|
Currently the diff is very hard to review because the code moves from e.g. If you use |
Yeah, it's quite painful. The original code seems to have been written with only ARMv7 in mind, which makes it painful to refactor nicely to include aarch64. I'll see what I can do to make it easier to review but I have a feeling it's going to be kind of ugly no matter what. Edit: I didn't know about |
Use If you want to split a file, you need to do something complicated (see e.g. here, but make sure you do all this on a temporary branch so you won't accidentally mess up everything). But if we end up with some kind of ugly, I'd prefer ifdefs with at least correct history and easier to review diffs. We can do the proper file moves and splitting at the very end. |
|
Looking at the size of the diff, that might be a good idea. But whether it's one or two PRs doesn't matter much, it's how easy it is to check for correctness that counts. The main way to achieve that is to separate big moves that touch a lot of lines, but don't change content, from actual changes in content. The advantage of one PR is that you see why things are done (well, theoretically). What you have in #8 is already much better, but instead of copying After that you want a commit that removes duplicate lines, with no other changes in the moved files (it's okay to move declarations from one header to another, for instance, and do anything else to make it compile). It's a lot easier to convince git to keep both files if you do the duplicate line removal in two separate commits, one in your main branch and on in the mv branch, as it will notice a conflict and you can resolve it by keeping At this point you should have split the file in two and everything should still work, without any code content changes anywhere. You can delete your temporary branch now. And then you want to commit any actual changes in code, like removing And lastly, you could add aarch64 support, but if that's a lot it can be done in a separate PR too. |
|
Okay thanks, I'll give that a go |
|
@Indanz One thing I don't really get in the remove duplication step is, what about static functions that are used by both arm/machine/debug.c and arm/32/machine/debug.c? Should I just leave a copy of the function in both files in this step to get it compiling and clean it up by making it non-static and adding it to the header file in the next commit? |
|
I would only keep the version you want to keep after the merge. Don't try to make it compile pre-merge. I forgot about static functions, those you could make non-static when removing the unused code, or do it in the merge itself, or in a separate commit. It doesn't matter much because the change is local and self-contained, unlike the refactoring. |
|
@Indanz I tried something like what you said in seL4#1198. Normal merging didn't really work, so I ended up doing the octopus merge solution from https://devblogs.microsoft.com/oldnewthing/20190917-00/?p=102894. |
|
Review-wise, that looks exactly like I meant, very good. However, I think history gets lost for the new file (git blame shows you as author for moved files, for instance). I'll try something tomorrow to see if I can convince git to do what I want. |
|
That's weird, I swear it was right at some point. Something might have messed up when I cherry-picked the aarch64 changes from another branch. |
|
Okay, I think I got the history right at least, please check I didn't mess anything up, see my refactor_hw_debug_aarch32_attempt_iz branch. I did:
|
|
@Indanz Looks good, there are a couple of TODOs Ieft in there that I should fix, but if you're happy with the git aspect, we can move on to a submitting an actual PR after I fix those. EDIT: Done, you can find the extra stuff here: seL4/seL4@master...alwin-joshy:refactor_hw_debug_aarch32_attempt_iz. I'll probably squash this one with the last one if you're okay with the changes. |
|
I'd do an interactive rebase and add the 32-bit fixes to "change files to allow compilation" and squash the 64-bit fixes with the last commit. So, other than the horrible DEBUG_GENERATE_*_FN macro, what I don't like are the unexplained changes to the current 32-bit code. Those should be done in a separate commits with explanations I think. Same for cosmetic changes like renaming all fields: Either don't do that at all, or at least do it in a separate commit. |
|
@Indanz Okay, I think I've done that. Not too sure which changes to aarch32 code you mean, I think the only real changes I've made to aarch32 are the names of the fields, which I've now put into a seperate commit, and the Arch_setupBcr/Wcr functions, which I changed because there would have been duplicated code in the aarch32 and aarch64 implementations. |
No description provided.