| Message ID | 20230316102635.6497-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] xen/console: Skip switching serial input to non existing domains | expand |
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
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
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
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
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");
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
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
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
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); }
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(-)