Re: Xen 4.18 pvshim console issue (with patch)

[Jan Beulich <jbeulich@xxxxxxxx>]

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