On 25.01.2024 02:14, Andrew Cooper wrote: > 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.
Hmm, I'm not sure I see how reverting that would end up working. However, aiui you need to revert primarily for the non-const-ness of the pointers involved in [gs]et_irq_regs(). I wonder whether, if we followed your underlying thought here, those shouldn't be const-ified then anyway. > 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'. If the timer happens to run on an idle vCPU, what "outer regs context" would we have there? > 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. All fine. Still I wonder whether in the meantime this patch isn't an improvement on its own, and hence whether the const couldn't sensibly be added subsequently. Jan
