On 18.10.2023 15:36, Manuel Bouyer wrote: > On Wed, Oct 18, 2023 at 03:24:22PM +0200, Jan Beulich wrote: >> On 18.10.2023 13:20, Manuel Bouyer wrote: >>> On Wed, Oct 18, 2023 at 11:44:22AM +0100, Andrew Cooper wrote: >>>> On 18/10/2023 11:38 am, Manuel Bouyer wrote: >>>>> Hello, >>>>> With Xen 4.18, a PV domain running under pvshim doesn't get console input. >>>>> This is because the domain id in pvshim isn't 0 (and on x86 >>>>> max_init_domid is >>>>> hardwired to 0), so console_input_domain() will never select that domain >>>>> as input. >>>>> >>>>> The attached patch fixes it by translating 0 to the real domain id for >>>>> pvshim, but there may be a better way to do this. >>>>> >>>> >>>> Thankyou for the report. >>>> >>>> First, CC'ing Henry as 4.18 release manager. >>>> >>>> There have been changes in how this works recently in 4.18, notably c/s >>>> c2581c58bec96. >>> >>> Yes, it looks like this one introduced the problem. >>> Before this, we would switch console_rx to 1 without checking if >>> domain (console_rx - 1) exists, and console_rx == 1 is a special case >>> in __serial_rx() >>> >>>> >>>> However, it's not obvious whether this worked in 4.17 or not. i.e. >>>> whether it's a regression in 4.18, or whether it's been broken since PV >>>> Shim was introduced. >>> >>> I don't know for 4.16 or 4.17 but I can tell that it's working in 4.15 >> >> >From looking at the code, it doesn't look like it would: There >> switch_serial_input() toggles console_rx between 0 and 1 afaict, and >> console_input_domain() handles value 0 as "Xen" (like in 4.18), while >> otherwise it calls rcu_lock_domain_by_id(console_rx - 1) (i.e. trying >> to get hold of Dom0's domain structure, not Dom1's). > > Well, I have a 4.15.5 installation in production and I can tell you that > with PV+PVSHIM, the console is working, for sure. > > AFAIK console_input_domain() is called only on ARM, from > vpl011_write_data_xen(). It's never called for x86.
Oh, indeed. I took your patch touching the function as meaning it's used on x86. This then means my earlier suggestion won't work, as we need console_rx to have the value 1 for input to be accepted from Dom1. Which in turn means your change to switch_serial_input(), suitably cleaned up, is then likely the best we can do, despite me not really liking the shim special casing. As to cleaning up - besides the build, indentation, and style issues I think you want to drop the "&& pv_shim" part of the condition (as get_initial_domain_id() takes care of that already), and ideally you'd also move the new "domid" variable into the more narrow scope. If you don't feel like providing a proper (updated) patch, I'm happy to take over, but to retain indication of your initial work I'd need you to permit me to add your S-o-b (alongside mine). Jan
