On 11.01.2024 16:24, Andrew Cooper wrote: > On 11/01/2024 12:11 pm, Jan Beulich wrote: >>>> Have >>>> handle_keypress() make the pointer available via a per-CPU variable, >>>> thus eliminating the need to pass it to all IRQ key handlers, making >>>> sure that a console-invoked key's handling can still nest inside a >>>> sysctl-invoked one's. >>> I know this is the current behaviour, and I'm not suggesting altering it >>> in this patch, but the sysctl was added so you had a way of using debug >>> keys without necessarily having a working serial connection. >>> >>> It was never expected or intended for both mechanisms to work >>> concurrently, and I don't think we need to take any care to make/keep it >>> working. >> Well, all it takes is the saving and restoring of keypress_regs in >> handle_keypress(). You you really think it would be better to risk >> a cash, but not doing that tiny bit of extra work? > > I presume you mean crash?
Oops, yes, I do. > I'm not advocating for leaving something explicitly unsafe, but I'm also > looking to see if we can avoid having keypress_regs to begin with. i.e. > I think we've already got unnecessary complexity, and it would be good > to reduce it. >[...] >>> This just leaves dump regs, which I think can safely use get_irq_regs() >>> || guest_cpu_user_regs(). All it wants is something to dump_execstate() >>> to, which just wants to be the start of the path which led here. >> I don't think so - consider the case of 'd' hitting while handling an >> interrupt (and, say, stuck there in an infinite loop with IRQs enabled). >> We'd then wrongly dump the context of what the earlier IRQ interrupted. > > The serial IRQ producing the 'd' keypress will push a irq frame, which > is what will be returned by get_irq_regs(). Hmm, yes. I wonder what I was thinking ... > It does occur to me that we're trying to accommodate for two behaviours > here. > > For a real keypress, we want to dump from the the point the interrupt > hit because that's the interesting bit of stack to see. For a SYSCTL, > there's nothing, and we're using BUGFRAME_run_fn to generate one. There's three forms of handle_keypress() invocations really, and hence why (after having dropped the regs parameter already) I re-instated it. As an aside - no, sysctl handling does not generate an exception frame. Is uses guest_cpu_user_regs() (and imo validly so). > So actually we just simply want "regs = get_irq_regs();" here and retain > prior NULL check, don't we? As per above, after I had it that way first, I backed off to accommodate all present use forms of handle_keypress(). But dealing with that (in whichever way we may end up deeming workable) can be separate anyway, afaict. Jan
