At the moment, we direct serial input to hardware domain by default.
This does not make any sense when running in true dom0less mode, since
such domain does not exist. As a result, users wishing to write to
an emulated UART of a domU are always forced to execute CTRL-AAA first.
The same issue is when rotating among serial inputs, where we always
have to go through hardware domain case.

Modify switch_serial_input() so that, if there is no hardware domain
(i.e. true dom0less case) and console_rx is 1 (i.e. input to hwdom),
we skip it and move to the next serial input.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/drivers/char/console.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index e8468c121ad0..3bbab35dc460 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -491,6 +491,14 @@ static void switch_serial_input(void)
     else
     {
         console_rx++;
+
+        /*
+         * Skip switching serial input to hardware domain if it does not exist
+         * (i.e. true dom0less mode).
+         */
+        if ( !hardware_domain && (console_rx == 1) )
+            console_rx++;
+
         printk("*** Serial input to DOM%d", console_rx - 1);
     }
 
-- 
2.25.1

On 14.03.2023 15:27, Michal Orzel wrote:
> At the moment, we direct serial input to hardware domain by default.
> This does not make any sense when running in true dom0less mode, since
> such domain does not exist. As a result, users wishing to write to
> an emulated UART of a domU are always forced to execute CTRL-AAA first.
> The same issue is when rotating among serial inputs, where we always
> have to go through hardware domain case.
> 
> Modify switch_serial_input() so that, if there is no hardware domain
> (i.e. true dom0less case) and console_rx is 1 (i.e. input to hwdom),
> we skip it and move to the next serial input.
> 
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Makes sense, but I think there are more things to consider.

> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -491,6 +491,14 @@ static void switch_serial_input(void)
>      else
>      {
>          console_rx++;
> +
> +        /*
> +         * Skip switching serial input to hardware domain if it does not exist
> +         * (i.e. true dom0less mode).
> +         */
> +        if ( !hardware_domain && (console_rx == 1) )
> +            console_rx++;

The consumers of this variable aren't really serialized with this
updating. That's probably okay-ish prior to your change, but now
there can be two updates in rapid succession. I think it would be
better if the variable was written only once here.

>          printk("*** Serial input to DOM%d", console_rx - 1);

When invoked from console_endboot() this will now switch to Dom1,
i.e. that domain becomes kind of "preferred", which I think is
wrong. Instead I think in such a case we should direct input to
Xen by default.

And then I have a question about Dom<N> lifetime in dom0less: Can
such domains be short-lived? If yes, what you do here for Dom0
would likely want generalizing, to skip any domain that doesn't
exist (anymore).

Jan

On 14/03/2023 16:17, Jan Beulich wrote:
> 
> 
> On 14.03.2023 15:27, Michal Orzel wrote:
>> At the moment, we direct serial input to hardware domain by default.
>> This does not make any sense when running in true dom0less mode, since
>> such domain does not exist. As a result, users wishing to write to
>> an emulated UART of a domU are always forced to execute CTRL-AAA first.
>> The same issue is when rotating among serial inputs, where we always
>> have to go through hardware domain case.
>>
>> Modify switch_serial_input() so that, if there is no hardware domain
>> (i.e. true dom0less case) and console_rx is 1 (i.e. input to hwdom),
>> we skip it and move to the next serial input.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> 
> Makes sense, but I think there are more things to consider.
> 
>> --- a/xen/drivers/char/console.c
>> +++ b/xen/drivers/char/console.c
>> @@ -491,6 +491,14 @@ static void switch_serial_input(void)
>>      else
>>      {
>>          console_rx++;
>> +
>> +        /*
>> +         * Skip switching serial input to hardware domain if it does not exist
>> +         * (i.e. true dom0less mode).
>> +         */
>> +        if ( !hardware_domain && (console_rx == 1) )
>> +            console_rx++;
> 
> The consumers of this variable aren't really serialized with this
> updating. That's probably okay-ish prior to your change, but now
> there can be two updates in rapid succession. I think it would be
> better if the variable was written only once here.
ok, makes sense.

> 
>>          printk("*** Serial input to DOM%d", console_rx - 1);
> 
> When invoked from console_endboot() this will now switch to Dom1,
> i.e. that domain becomes kind of "preferred", which I think is
> wrong. Instead I think in such a case we should direct input to
> Xen by default.
Switching serial input to the first usable domain is the major motivation behind this patch.
The number of times I got pinged by users with *apparent* Xen issue on true dom0less
just because input was directed to dom0 which was not there (not everyone seems to read the
boot logs) forced me to create this patch and manifests that this is not the behavior user wants.
Switching to Xen console would not help at all. Also, we already have a way to set switch code to 'x'
to default serial input to Xen.
So I think what I did (switching to the first existing domain) should be the default behavior (just like it was done for dom0).

> 
> And then I have a question about Dom<N> lifetime in dom0less: Can
> such domains be short-lived? If yes, what you do here for Dom0
> would likely want generalizing, to skip any domain that doesn't
> exist (anymore).
I think there is nothing preventing such domains not to be short-lived and checking
for existence is a good idea. So all in all I would do the following modifications:

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index e8468c121ad0..d843b8baf162 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -490,7 +490,24 @@ static void switch_serial_input(void)
     }
     else
     {
-        console_rx++;
+        unsigned int next_rx = console_rx + 1;
+
+        /* Skip switching serial input to non existing domains */
+        while ( next_rx < max_init_domid + 1 )
+        {
+            struct domain *d = rcu_lock_domain_by_id(next_rx - 1);
+
+            if ( d )
+            {
+                rcu_unlock_domain(d);
+                break;
+            }
+
+            next_rx++;
+        }
+
+        console_rx = next_rx;
+
         printk("*** Serial input to DOM%d", console_rx - 1);
     }


~Michal

On 15.03.2023 13:34, Michal Orzel wrote:
> On 14/03/2023 16:17, Jan Beulich wrote:
>> On 14.03.2023 15:27, Michal Orzel wrote:
>>> --- a/xen/drivers/char/console.c
>>> +++ b/xen/drivers/char/console.c
>>> @@ -491,6 +491,14 @@ static void switch_serial_input(void)
>>>      else
>>>      {
>>>          console_rx++;
>>> +
>>> +        /*
>>> +         * Skip switching serial input to hardware domain if it does not exist
>>> +         * (i.e. true dom0less mode).
>>> +         */
>>> +        if ( !hardware_domain && (console_rx == 1) )
>>> +            console_rx++;
>>
>> The consumers of this variable aren't really serialized with this
>> updating. That's probably okay-ish prior to your change, but now
>> there can be two updates in rapid succession. I think it would be
>> better if the variable was written only once here.
> ok, makes sense.
> 
>>
>>>          printk("*** Serial input to DOM%d", console_rx - 1);
>>
>> When invoked from console_endboot() this will now switch to Dom1,
>> i.e. that domain becomes kind of "preferred", which I think is
>> wrong. Instead I think in such a case we should direct input to
>> Xen by default.
> Switching serial input to the first usable domain is the major motivation behind this patch.
> The number of times I got pinged by users with *apparent* Xen issue on true dom0less
> just because input was directed to dom0 which was not there (not everyone seems to read the
> boot logs) forced me to create this patch and manifests that this is not the behavior user wants.
> Switching to Xen console would not help at all. Also, we already have a way to set switch code to 'x'
> to default serial input to Xen.
> So I think what I did (switching to the first existing domain) should be the default behavior (just like it was done for dom0).

Well, I'm not going to stand in the way, but if one of several supposedly
equal domains is to be "preferred" in some way, then I for one would
expect justification for doing so. If that's the route to go, then the
patch snippet you provided looks good to me.

Jan

On 15/03/2023 14:11, Jan Beulich wrote:
> 
> 
> On 15.03.2023 13:34, Michal Orzel wrote:
>> On 14/03/2023 16:17, Jan Beulich wrote:
>>> On 14.03.2023 15:27, Michal Orzel wrote:
>>>> --- a/xen/drivers/char/console.c
>>>> +++ b/xen/drivers/char/console.c
>>>> @@ -491,6 +491,14 @@ static void switch_serial_input(void)
>>>>      else
>>>>      {
>>>>          console_rx++;
>>>> +
>>>> +        /*
>>>> +         * Skip switching serial input to hardware domain if it does not exist
>>>> +         * (i.e. true dom0less mode).
>>>> +         */
>>>> +        if ( !hardware_domain && (console_rx == 1) )
>>>> +            console_rx++;
>>>
>>> The consumers of this variable aren't really serialized with this
>>> updating. That's probably okay-ish prior to your change, but now
>>> there can be two updates in rapid succession. I think it would be
>>> better if the variable was written only once here.
>> ok, makes sense.
>>
>>>
>>>>          printk("*** Serial input to DOM%d", console_rx - 1);
>>>
>>> When invoked from console_endboot() this will now switch to Dom1,
>>> i.e. that domain becomes kind of "preferred", which I think is
>>> wrong. Instead I think in such a case we should direct input to
>>> Xen by default.
>> Switching serial input to the first usable domain is the major motivation behind this patch.
>> The number of times I got pinged by users with *apparent* Xen issue on true dom0less
>> just because input was directed to dom0 which was not there (not everyone seems to read the
>> boot logs) forced me to create this patch and manifests that this is not the behavior user wants.
>> Switching to Xen console would not help at all. Also, we already have a way to set switch code to 'x'
>> to default serial input to Xen.
>> So I think what I did (switching to the first existing domain) should be the default behavior (just like it was done for dom0).
> 
> Well, I'm not going to stand in the way, but if one of several supposedly
> equal domains is to be "preferred" in some way, then I for one would
> expect justification for doing so. If that's the route to go, then the
> patch snippet you provided looks good to me.
Well, the explanation is that we are directing input to the first existing domain
which also is the first domain created (this is what users expect at least by default).
This for now creates simplest/cleanest solution that matches the current behavior
with dom0 and solves the users inconveniences I mentioned earlier.
There is no other real preference apart from being first domain created and to help keep the order simple.

In the future, we are going to allow specifying which domain to direct serial input to if users
want to modify the default behavior. This way, we will have all the flexibility needed:
-default -> first created
-conswitch=ax -> Xen
-dt property -> dom<N>

~Michal

On Wed, 15 Mar 2023, Michal Orzel wrote:
> On 15/03/2023 14:11, Jan Beulich wrote:
> > On 15.03.2023 13:34, Michal Orzel wrote:
> >> On 14/03/2023 16:17, Jan Beulich wrote:
> >>> On 14.03.2023 15:27, Michal Orzel wrote:
> >>>> --- a/xen/drivers/char/console.c
> >>>> +++ b/xen/drivers/char/console.c
> >>>> @@ -491,6 +491,14 @@ static void switch_serial_input(void)
> >>>>      else
> >>>>      {
> >>>>          console_rx++;
> >>>> +
> >>>> +        /*
> >>>> +         * Skip switching serial input to hardware domain if it does not exist
> >>>> +         * (i.e. true dom0less mode).
> >>>> +         */
> >>>> +        if ( !hardware_domain && (console_rx == 1) )
> >>>> +            console_rx++;
> >>>
> >>> The consumers of this variable aren't really serialized with this
> >>> updating. That's probably okay-ish prior to your change, but now
> >>> there can be two updates in rapid succession. I think it would be
> >>> better if the variable was written only once here.
> >> ok, makes sense.
> >>
> >>>
> >>>>          printk("*** Serial input to DOM%d", console_rx - 1);
> >>>
> >>> When invoked from console_endboot() this will now switch to Dom1,
> >>> i.e. that domain becomes kind of "preferred", which I think is
> >>> wrong. Instead I think in such a case we should direct input to
> >>> Xen by default.
> >> Switching serial input to the first usable domain is the major motivation behind this patch.
> >> The number of times I got pinged by users with *apparent* Xen issue on true dom0less
> >> just because input was directed to dom0 which was not there (not everyone seems to read the
> >> boot logs) forced me to create this patch and manifests that this is not the behavior user wants.
> >> Switching to Xen console would not help at all. Also, we already have a way to set switch code to 'x'
> >> to default serial input to Xen.
> >> So I think what I did (switching to the first existing domain) should be the default behavior (just like it was done for dom0).
> > 
> > Well, I'm not going to stand in the way, but if one of several supposedly
> > equal domains is to be "preferred" in some way, then I for one would
> > expect justification for doing so. If that's the route to go, then the
> > patch snippet you provided looks good to me.
> Well, the explanation is that we are directing input to the first existing domain
> which also is the first domain created (this is what users expect at least by default).
> This for now creates simplest/cleanest solution that matches the current behavior
> with dom0 and solves the users inconveniences I mentioned earlier.
> There is no other real preference apart from being first domain created and to help keep the order simple.

My two cents. Given the feedback we are getting from users, I think it
is important that the input goes to a real valid domain (not Xen, not
Dom0 that doesn't exist). "Which dom0less domain?" is a valid question,
and I don't know what would be the best answer. But any of those domains
would be better than what we have today.

On 16.03.2023 02:49, Stefano Stabellini wrote:
> On Wed, 15 Mar 2023, Michal Orzel wrote:
>> On 15/03/2023 14:11, Jan Beulich wrote:
>>> On 15.03.2023 13:34, Michal Orzel wrote:
>>>> On 14/03/2023 16:17, Jan Beulich wrote:
>>>>> On 14.03.2023 15:27, Michal Orzel wrote:
>>>>>> --- a/xen/drivers/char/console.c
>>>>>> +++ b/xen/drivers/char/console.c
>>>>>> @@ -491,6 +491,14 @@ static void switch_serial_input(void)
>>>>>>      else
>>>>>>      {
>>>>>>          console_rx++;
>>>>>> +
>>>>>> +        /*
>>>>>> +         * Skip switching serial input to hardware domain if it does not exist
>>>>>> +         * (i.e. true dom0less mode).
>>>>>> +         */
>>>>>> +        if ( !hardware_domain && (console_rx == 1) )
>>>>>> +            console_rx++;
>>>>>
>>>>> The consumers of this variable aren't really serialized with this
>>>>> updating. That's probably okay-ish prior to your change, but now
>>>>> there can be two updates in rapid succession. I think it would be
>>>>> better if the variable was written only once here.
>>>> ok, makes sense.
>>>>
>>>>>
>>>>>>          printk("*** Serial input to DOM%d", console_rx - 1);
>>>>>
>>>>> When invoked from console_endboot() this will now switch to Dom1,
>>>>> i.e. that domain becomes kind of "preferred", which I think is
>>>>> wrong. Instead I think in such a case we should direct input to
>>>>> Xen by default.
>>>> Switching serial input to the first usable domain is the major motivation behind this patch.
>>>> The number of times I got pinged by users with *apparent* Xen issue on true dom0less
>>>> just because input was directed to dom0 which was not there (not everyone seems to read the
>>>> boot logs) forced me to create this patch and manifests that this is not the behavior user wants.
>>>> Switching to Xen console would not help at all. Also, we already have a way to set switch code to 'x'
>>>> to default serial input to Xen.
>>>> So I think what I did (switching to the first existing domain) should be the default behavior (just like it was done for dom0).
>>>
>>> Well, I'm not going to stand in the way, but if one of several supposedly
>>> equal domains is to be "preferred" in some way, then I for one would
>>> expect justification for doing so. If that's the route to go, then the
>>> patch snippet you provided looks good to me.
>> Well, the explanation is that we are directing input to the first existing domain
>> which also is the first domain created (this is what users expect at least by default).
>> This for now creates simplest/cleanest solution that matches the current behavior
>> with dom0 and solves the users inconveniences I mentioned earlier.
>> There is no other real preference apart from being first domain created and to help keep the order simple.
> 
> My two cents. Given the feedback we are getting from users, I think it
> is important that the input goes to a real valid domain (not Xen, not
> Dom0 that doesn't exist). "Which dom0less domain?" is a valid question,
> and I don't know what would be the best answer. But any of those domains
> would be better than what we have today.

Could boot time configuration perhaps indicate which domain to "prefer"?
Otherwise I'm pretty inclined to suggest to make it actually random ...

Jan

On 16/03/2023 08:24, Jan Beulich wrote:
> 
> 
> On 16.03.2023 02:49, Stefano Stabellini wrote:
>> On Wed, 15 Mar 2023, Michal Orzel wrote:
>>> On 15/03/2023 14:11, Jan Beulich wrote:
>>>> On 15.03.2023 13:34, Michal Orzel wrote:
>>>>> On 14/03/2023 16:17, Jan Beulich wrote:
>>>>>> On 14.03.2023 15:27, Michal Orzel wrote:
>>>>>>> --- a/xen/drivers/char/console.c
>>>>>>> +++ b/xen/drivers/char/console.c
>>>>>>> @@ -491,6 +491,14 @@ static void switch_serial_input(void)
>>>>>>>      else
>>>>>>>      {
>>>>>>>          console_rx++;
>>>>>>> +
>>>>>>> +        /*
>>>>>>> +         * Skip switching serial input to hardware domain if it does not exist
>>>>>>> +         * (i.e. true dom0less mode).
>>>>>>> +         */
>>>>>>> +        if ( !hardware_domain && (console_rx == 1) )
>>>>>>> +            console_rx++;
>>>>>>
>>>>>> The consumers of this variable aren't really serialized with this
>>>>>> updating. That's probably okay-ish prior to your change, but now
>>>>>> there can be two updates in rapid succession. I think it would be
>>>>>> better if the variable was written only once here.
>>>>> ok, makes sense.
>>>>>
>>>>>>
>>>>>>>          printk("*** Serial input to DOM%d", console_rx - 1);
>>>>>>
>>>>>> When invoked from console_endboot() this will now switch to Dom1,
>>>>>> i.e. that domain becomes kind of "preferred", which I think is
>>>>>> wrong. Instead I think in such a case we should direct input to
>>>>>> Xen by default.
>>>>> Switching serial input to the first usable domain is the major motivation behind this patch.
>>>>> The number of times I got pinged by users with *apparent* Xen issue on true dom0less
>>>>> just because input was directed to dom0 which was not there (not everyone seems to read the
>>>>> boot logs) forced me to create this patch and manifests that this is not the behavior user wants.
>>>>> Switching to Xen console would not help at all. Also, we already have a way to set switch code to 'x'
>>>>> to default serial input to Xen.
>>>>> So I think what I did (switching to the first existing domain) should be the default behavior (just like it was done for dom0).
>>>>
>>>> Well, I'm not going to stand in the way, but if one of several supposedly
>>>> equal domains is to be "preferred" in some way, then I for one would
>>>> expect justification for doing so. If that's the route to go, then the
>>>> patch snippet you provided looks good to me.
>>> Well, the explanation is that we are directing input to the first existing domain
>>> which also is the first domain created (this is what users expect at least by default).
>>> This for now creates simplest/cleanest solution that matches the current behavior
>>> with dom0 and solves the users inconveniences I mentioned earlier.
>>> There is no other real preference apart from being first domain created and to help keep the order simple.
>>
>> My two cents. Given the feedback we are getting from users, I think it
>> is important that the input goes to a real valid domain (not Xen, not
>> Dom0 that doesn't exist). "Which dom0less domain?" is a valid question,
>> and I don't know what would be the best answer. But any of those domains
>> would be better than what we have today.
> 
> Could boot time configuration perhaps indicate which domain to "prefer"?
> Otherwise I'm pretty inclined to suggest to make it actually random ...
Random is not great because in a system with e.g. 20 domUs and directing to e.g. 10th domU
user would have to go through a lot of domains executing CTRL-AAA several times. Also, for me
it would be difficult to reason about as such approach is definitely not something users expect.

May I ask so that we proceed with a patch I proposed to start from the first usable domain
to match the current behavior and to help users?. In the meantime I will start working on
a support for indicating which domain to prefer.
All in all, the new patch would touch console_endboot() and not switch_serial_input().

~Michal

On 16.03.2023 08:51, Michal Orzel wrote:
> On 16/03/2023 08:24, Jan Beulich wrote:
>> On 16.03.2023 02:49, Stefano Stabellini wrote:
>>> On Wed, 15 Mar 2023, Michal Orzel wrote:
>>>> On 15/03/2023 14:11, Jan Beulich wrote:
>>>>> On 15.03.2023 13:34, Michal Orzel wrote:
>>>>>> On 14/03/2023 16:17, Jan Beulich wrote:
>>>>>>> On 14.03.2023 15:27, Michal Orzel wrote:
>>>>>>>> --- a/xen/drivers/char/console.c
>>>>>>>> +++ b/xen/drivers/char/console.c
>>>>>>>> @@ -491,6 +491,14 @@ static void switch_serial_input(void)
>>>>>>>>      else
>>>>>>>>      {
>>>>>>>>          console_rx++;
>>>>>>>> +
>>>>>>>> +        /*
>>>>>>>> +         * Skip switching serial input to hardware domain if it does not exist
>>>>>>>> +         * (i.e. true dom0less mode).
>>>>>>>> +         */
>>>>>>>> +        if ( !hardware_domain && (console_rx == 1) )
>>>>>>>> +            console_rx++;
>>>>>>>
>>>>>>> The consumers of this variable aren't really serialized with this
>>>>>>> updating. That's probably okay-ish prior to your change, but now
>>>>>>> there can be two updates in rapid succession. I think it would be
>>>>>>> better if the variable was written only once here.
>>>>>> ok, makes sense.
>>>>>>
>>>>>>>
>>>>>>>>          printk("*** Serial input to DOM%d", console_rx - 1);
>>>>>>>
>>>>>>> When invoked from console_endboot() this will now switch to Dom1,
>>>>>>> i.e. that domain becomes kind of "preferred", which I think is
>>>>>>> wrong. Instead I think in such a case we should direct input to
>>>>>>> Xen by default.
>>>>>> Switching serial input to the first usable domain is the major motivation behind this patch.
>>>>>> The number of times I got pinged by users with *apparent* Xen issue on true dom0less
>>>>>> just because input was directed to dom0 which was not there (not everyone seems to read the
>>>>>> boot logs) forced me to create this patch and manifests that this is not the behavior user wants.
>>>>>> Switching to Xen console would not help at all. Also, we already have a way to set switch code to 'x'
>>>>>> to default serial input to Xen.
>>>>>> So I think what I did (switching to the first existing domain) should be the default behavior (just like it was done for dom0).
>>>>>
>>>>> Well, I'm not going to stand in the way, but if one of several supposedly
>>>>> equal domains is to be "preferred" in some way, then I for one would
>>>>> expect justification for doing so. If that's the route to go, then the
>>>>> patch snippet you provided looks good to me.
>>>> Well, the explanation is that we are directing input to the first existing domain
>>>> which also is the first domain created (this is what users expect at least by default).
>>>> This for now creates simplest/cleanest solution that matches the current behavior
>>>> with dom0 and solves the users inconveniences I mentioned earlier.
>>>> There is no other real preference apart from being first domain created and to help keep the order simple.
>>>
>>> My two cents. Given the feedback we are getting from users, I think it
>>> is important that the input goes to a real valid domain (not Xen, not
>>> Dom0 that doesn't exist). "Which dom0less domain?" is a valid question,
>>> and I don't know what would be the best answer. But any of those domains
>>> would be better than what we have today.
>>
>> Could boot time configuration perhaps indicate which domain to "prefer"?
>> Otherwise I'm pretty inclined to suggest to make it actually random ...
> Random is not great because in a system with e.g. 20 domUs and directing to e.g. 10th domU
> user would have to go through a lot of domains executing CTRL-AAA several times. Also, for me
> it would be difficult to reason about as such approach is definitely not something users expect.
> 
> May I ask so that we proceed with a patch I proposed to start from the first usable domain
> to match the current behavior and to help users?. In the meantime I will start working on
> a support for indicating which domain to prefer.

So long as the description states clearly that the choice is arbitrary, I
wouldn't (further) object to such a change.

> All in all, the new patch would touch console_endboot() and not switch_serial_input().

Of course.

Jan