On 24/01/2024 7:28 am, Jan Beulich wrote: > On 24.01.2024 02:34, Stefano Stabellini wrote: >> I managed to get back to read the mailing list and noticed this patch. >> >> Is it still relevant and needs to be reviewed? >> >> Are there any outstanding disagreements between maintainers on the >> approach to take here? Or should I just go ahead and review it? > It is still relevant from my pov, and everything that may be controversial > is said ...
BUGFRAME_* cannot legitimately modify the interrupted context. Two are fatal paths, and other two are side-effect-less as far as C can tell. So the infrastructure ought to take a const pointer. The reason why this pointer is non-const is to do with the interaction of the serial and keyhandler infrastructures. Because we're adjusting that for other reasons, I was hoping it would subsequently be easy to switch Xen to being properly const in this regard. Turns out it is: https://gitlab.com/xen-project/people/andyhhp/xen/-/commit/4f857075005da1d28632e4f9198c2e7d0f404b9a with a couple of caveats. (Only the buster-gcc-ibt run failed, so I've got some cf_check-ing to adjust, but all the other builds worked fine). To make the serial code compile, I ended up having to revert patch 2 of the regs series, which I believe is safe to do following patch 3-5 which un-plumb the regs pointer deeper in the call chain. If this is turns out to be true, then the patch ought to be added and reverted in the same series so it isn't left hanging about after the fact. The _$X_poll() functions are used in timer context, which means there's an outer regs context already latched, and that's arguably a better context to use anyway for 'd'. This in turn allows us to remove a #UD from a fast(ish) path, and remove some per-cpu or static variables which are just used for non-standard parameter passing because run_in_exception_handler() doesn't let you pass any. This leaves the '%' debugger infrastructure. Being a debugger, it's making arbitrary changes anyway and I'd much rather cast away constness for a debugger, than to keep everything else mutable when it oughtn't to be. If absolutely nothing else, registration and handling '%' ought to be from x86 code rather than common code, which would remove the do_debugger_trap_fatal() layering violation. But, the more I look into the gdbstub the more I'm convinced that it doesn't work. For example, this gem: /* Resuming after we've stopped used to work, but more through luck than any actual intention. It doesn't at the moment. */ >From c/s b69f92f3012 in July 2004, and more specifically the commit which added the gdbstub functionality to begin with. I.e. it doesn't appear to have ever supported more than "poke around in the crashed state". In the 2 decades that noone has fixed this, we've gained far better technologies for doing this, such as running it in a VM. I am going to submit some patches deleting gdbstub. It clearly had not much value to begin with, and is not definitely not worth the problems it is creating in adjacent code these days. ~Andrew
