[PATCH v2] xen/console: Skip switching serial input to non existing domains

[PATCH v2] xen/console: Skip switching serial input to non existing domains

[Michal Orzel]

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. This problem can be elaborated
further to all the domains that no longer exist.

Modify switch_serial_input() so that we skip switching serial input to
non existing domains.

For now, to minimize the required changes and to match the current
behavior with hwdom, the default input goes to the first real domain.
The choice is more or less arbitrary since dom0less domUs are supposedly
equal. This will be handled in the future by adding support in boot time
configuration for marking a specific domain preferred in terms of
directing serial input to.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
Changes in v2:
 - was: xen/console: Handle true dom0less case when switching serial input
 - use a more generic approach to handle all non-existing domains
---
 xen/drivers/char/console.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

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);
     }
 
-- 
2.25.1

Re: [PATCH v2] xen/console: Skip switching serial input to non existing domains

[Jan Beulich]

On 16.03.2023 11:26, Michal Orzel wrote:
> --- 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);
>      }

While at the first glance (when you sent it in reply to v1) it looked okay,
I'm afraid it really isn't: Please consider what happens when the last of
the DomU-s doesn't exist anymore. (You don't really check whether it still
exists, because the range check comes ahead of the existence one.) In that
case you want to move from second-to-last to Xen. I expect the entire
if/else construct wants to be inside the loop.

Jan

Re: [PATCH v2] xen/console: Skip switching serial input to non existing domains

[Michal Orzel]

On 16/03/2023 12:11, Jan Beulich wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On 16.03.2023 11:26, Michal Orzel wrote:
>> --- 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);
>>      }
> 
> While at the first glance (when you sent it in reply to v1) it looked okay,
> I'm afraid it really isn't: Please consider what happens when the last of
> the DomU-s doesn't exist anymore. (You don't really check whether it still
> exists, because the range check comes ahead of the existence one.) In that
> case you want to move from second-to-last to Xen. I expect the entire
> if/else construct wants to be inside the loop.
I did this deliberately because I do not think the situation you describe is possible
(i.e. no domains at all - Xen still usable). With hardware domain in place, we can e.g. destroy the domain
which would invoke domain_kill() -> domain_destroy() that would free domain struct.
Without hwdom, the domain cannot kill/destroy itself. It can do the shutdown but it will not
destroy it (at least this is what I tested). So I do not think there can be a scenario where
there is not a single domain while Xen running and be usable.

~Michal

Re: [PATCH v2] xen/console: Skip switching serial input to non existing domains

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 2636 bytes --]

On Thu, Mar 16, 2023 at 2:15 PM Michal Orzel <michal.orzel@amd.com> wrote:

>
>
> On 16/03/2023 12:11, Jan Beulich wrote:
> > Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> >
> >
> > On 16.03.2023 11:26, Michal Orzel wrote:
> >> --- 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);
> >>      }
> >
> > While at the first glance (when you sent it in reply to v1) it looked
> okay,
> > I'm afraid it really isn't: Please consider what happens when the last of
> > the DomU-s doesn't exist anymore. (You don't really check whether it
> still
> > exists, because the range check comes ahead of the existence one.) In
> that
> > case you want to move from second-to-last to Xen. I expect the entire
> > if/else construct wants to be inside the loop.
> I did this deliberately because I do not think the situation you describe
> is possible
> (i.e. no domains at all - Xen still usable). With hardware domain in
> place, we can e.g. destroy the domain
> which would invoke domain_kill() -> domain_destroy() that would free
> domain struct.
> Without hwdom, the domain cannot kill/destroy itself. It can do the
> shutdown but it will not
> destroy it (at least this is what I tested). So I do not think there can
> be a scenario where
> there is not a single domain while Xen running and be usable.


We've actually been discussing something like this.  Consider if someone
wanted to use Xen as part of a system architected like Amazon's Nitro: You
could have a DPU that ran the "toolstack", and gave Xen commands to create
or destroy domains.  It could dynamically create SRIOV PCI devices to be
passed directly through to guests.  In this case, you might run into a
situation where no VMs existed, and yet the system was not dead.

 -George

[-- Attachment #2: Type: text/html, Size: 3384 bytes --]

Re: [PATCH v2] xen/console: Skip switching serial input to non existing domains

[Jan Beulich]

On 16.03.2023 15:15, Michal Orzel wrote:
> 
> 
> On 16/03/2023 12:11, Jan Beulich wrote:
>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>
>>
>> On 16.03.2023 11:26, Michal Orzel wrote:
>>> --- 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);
>>>      }
>>
>> While at the first glance (when you sent it in reply to v1) it looked okay,
>> I'm afraid it really isn't: Please consider what happens when the last of
>> the DomU-s doesn't exist anymore. (You don't really check whether it still
>> exists, because the range check comes ahead of the existence one.) In that
>> case you want to move from second-to-last to Xen. I expect the entire
>> if/else construct wants to be inside the loop.
> I did this deliberately because I do not think the situation you describe is possible
> (i.e. no domains at all - Xen still usable). With hardware domain in place, we can e.g. destroy the domain
> which would invoke domain_kill() -> domain_destroy() that would free domain struct.
> Without hwdom, the domain cannot kill/destroy itself. It can do the shutdown but it will not
> destroy it (at least this is what I tested). So I do not think there can be a scenario where
> there is not a single domain while Xen running and be usable.

I didn't talk about "no domain left at all", but about the case where the
domain with the highest domain ID is gone.

Jan

Re: [PATCH v2] xen/console: Skip switching serial input to non existing domains

[Stefano Stabellini]

On Thu, 16 Mar 2023, Jan Beulich wrote:
> On 16.03.2023 11:26, Michal Orzel wrote:
> > --- 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);
> >      }
> 
> While at the first glance (when you sent it in reply to v1) it looked okay,
> I'm afraid it really isn't: Please consider what happens when the last of
> the DomU-s doesn't exist anymore. (You don't really check whether it still
> exists, because the range check comes ahead of the existence one.) In that
> case you want to move from second-to-last to Xen. I expect the entire
> if/else construct wants to be inside the loop.

I don't think we need another loop, just a check if we found a domain or
not. E.g.:


    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);
            console_rx = next_rx;
            printk("*** Serial input to DOM%d", console_rx - 1);
            break;
        }

        next_rx++;
    }

    /* no domain found */
    console_rx = 0;
    printk("*** Serial input to Xen");

Re: [PATCH v2] xen/console: Skip switching serial input to non existing domains

[Jan Beulich]

On 16.03.2023 23:59, Stefano Stabellini wrote:
> On Thu, 16 Mar 2023, Jan Beulich wrote:
>> On 16.03.2023 11:26, Michal Orzel wrote:
>>> --- 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);
>>>      }
>>
>> While at the first glance (when you sent it in reply to v1) it looked okay,
>> I'm afraid it really isn't: Please consider what happens when the last of
>> the DomU-s doesn't exist anymore. (You don't really check whether it still
>> exists, because the range check comes ahead of the existence one.) In that
>> case you want to move from second-to-last to Xen. I expect the entire
>> if/else construct wants to be inside the loop.
> 
> I don't think we need another loop, just a check if we found a domain or

I didn't say "another loop", but I suggested that the loop needs to be
around the if/else. Of course this can be transformed into equivalent
forms, like ...

> not. E.g.:
> 
> 
>     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);
>             console_rx = next_rx;
>             printk("*** Serial input to DOM%d", console_rx - 1);
>             break;
>         }
> 
>         next_rx++;
>     }
> 
>     /* no domain found */
>     console_rx = 0;
>     printk("*** Serial input to Xen");

... what you suggest (or at least almost, because the way it's written
we'd always switch to Xen).

Jan

Re: [PATCH v2] xen/console: Skip switching serial input to non existing domains

[Michal Orzel]

On 17/03/2023 09:36, Jan Beulich wrote:
> 
> 
> On 16.03.2023 23:59, Stefano Stabellini wrote:
>> On Thu, 16 Mar 2023, Jan Beulich wrote:
>>> On 16.03.2023 11:26, Michal Orzel wrote:
>>>> --- 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);
>>>>      }
>>>
>>> While at the first glance (when you sent it in reply to v1) it looked okay,
>>> I'm afraid it really isn't: Please consider what happens when the last of
>>> the DomU-s doesn't exist anymore. (You don't really check whether it still
>>> exists, because the range check comes ahead of the existence one.) In that
>>> case you want to move from second-to-last to Xen. I expect the entire
>>> if/else construct wants to be inside the loop.
>>
>> I don't think we need another loop, just a check if we found a domain or
> 
> I didn't say "another loop", but I suggested that the loop needs to be
> around the if/else. Of course this can be transformed into equivalent
> forms, like ...
> 
>> not. E.g.:
>>
>>
>>     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);
>>             console_rx = next_rx;
>>             printk("*** Serial input to DOM%d", console_rx - 1);
>>             break;
>>         }
>>
>>         next_rx++;
>>     }
>>
>>     /* no domain found */
>>     console_rx = 0;
>>     printk("*** Serial input to Xen");
> 
> ... what you suggest (or at least almost, because the way it's written
> we'd always switch to Xen).

I would prefer a loop with if/else inside. If you are ok with the following code
that handles all the cases, I will push a patch in a minute:

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 51e5408f2114..96ec3bbcf541 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -483,15 +483,34 @@ struct domain *console_input_domain(void)
 
 static void switch_serial_input(void)
 {
-    if ( console_rx == max_init_domid + 1 )
-    {
-        console_rx = 0;
-        printk("*** Serial input to Xen");
-    }
-    else
+    unsigned int next_rx = console_rx + 1;
+
+    /*
+     * Rotate among Xen, dom0 and boot-time created domUs while skipping
+     * switching serial input to non existing domains.
+     */
+    while ( next_rx <= max_init_domid + 2 )
     {
-        console_rx++;
-        printk("*** Serial input to DOM%d", console_rx - 1);
+        if ( next_rx == max_init_domid + 2 )
+        {
+            console_rx = 0;
+            printk("*** Serial input to Xen");
+            break;
+        }
+        else
+        {
+            struct domain *d = rcu_lock_domain_by_id(next_rx - 1);
+
+            if ( d )
+            {
+                rcu_unlock_domain(d);
+                console_rx = next_rx;
+                printk("*** Serial input to DOM%d", console_rx - 1);
+                break;
+            }
+
+            next_rx++;
+        }
     }
 
     if ( switch_code )

~Michal

Re: [PATCH v2] xen/console: Skip switching serial input to non existing domains

[Jan Beulich]

On 17.03.2023 10:32, Michal Orzel wrote:
> 
> 
> On 17/03/2023 09:36, Jan Beulich wrote:
>>
>>
>> On 16.03.2023 23:59, Stefano Stabellini wrote:
>>> On Thu, 16 Mar 2023, Jan Beulich wrote:
>>>> On 16.03.2023 11:26, Michal Orzel wrote:
>>>>> --- 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);
>>>>>      }
>>>>
>>>> While at the first glance (when you sent it in reply to v1) it looked okay,
>>>> I'm afraid it really isn't: Please consider what happens when the last of
>>>> the DomU-s doesn't exist anymore. (You don't really check whether it still
>>>> exists, because the range check comes ahead of the existence one.) In that
>>>> case you want to move from second-to-last to Xen. I expect the entire
>>>> if/else construct wants to be inside the loop.
>>>
>>> I don't think we need another loop, just a check if we found a domain or
>>
>> I didn't say "another loop", but I suggested that the loop needs to be
>> around the if/else. Of course this can be transformed into equivalent
>> forms, like ...
>>
>>> not. E.g.:
>>>
>>>
>>>     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);
>>>             console_rx = next_rx;
>>>             printk("*** Serial input to DOM%d", console_rx - 1);
>>>             break;
>>>         }
>>>
>>>         next_rx++;
>>>     }
>>>
>>>     /* no domain found */
>>>     console_rx = 0;
>>>     printk("*** Serial input to Xen");
>>
>> ... what you suggest (or at least almost, because the way it's written
>> we'd always switch to Xen).
> 
> I would prefer a loop with if/else inside. If you are ok with the following code
> that handles all the cases, I will push a patch in a minute:

Looks roughly okay, but recall I said so also when you "pre-posted" the
previous version.

Jan