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