Re: [PATCH v2 20/35] xen/console: introduce console_owner_domid()

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Thursday, December 12th, 2024 at 2:18 AM, Roger Pau Monné 
<roger.pau@xxxxxxxxxx> wrote:

>
>
> On Thu, Dec 05, 2024 at 08:41:50PM -0800, Denis Mukhin via B4 Relay wrote:
>
> > From: Denis Mukhin dmukhin@xxxxxxxx
> >
> > console_owner_domid() is introduced to obtain the "console owner" domain ID.
> >
> > The call is used in NS8250 emulator to identify the case when physical xen
> > console focus is owned by the domain w/ NS8250 emulator, in which case,
> > messages from guest OS are formatted w/o '(XEN)' prefix.
>
>
> Nit: it would be better to not use abbreviations such as w/ or w/o in
> commit messages.

Muscle memory; cleaned up in v3.

>
> > Signed-off-by: Denis Mukhin dmukhin@xxxxxxxx
> > ---
> > xen/drivers/char/console.c | 5 +++++
> > xen/include/xen/console.h | 1 +
> > 2 files changed, 6 insertions(+)
> >
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index 
> > 52cf64dbf6fd18d599cb88835d03501a23b3e3c4..a8ab5c2bcb98e4cadf9ad2c9ad28d297977d0557
> >  100644
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -498,6 +498,11 @@ struct domain *rcu_lock_domain_console_owner(void)
> > return rcu_lock_domain_console_by_id(console_owner);
> > }
> >
> > +domid_t console_owner_domid(void)
> > +{
> > + return console_owner;
> > +}
> > +
> > static bool console_owner_possible(domid_t domid)
> > {
> > struct domain *d;
> > diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
> > index 
> > 57c482cfbf2da15b011e64841ea086e779f4588d..83be5794aff6630beaad46f910fcc0fc6d833808
> >  100644
> > --- a/xen/include/xen/console.h
> > +++ b/xen/include/xen/console.h
> > @@ -33,6 +33,7 @@ void console_end_log_everything(void);
> >
> > struct domain *rcu_lock_domain_console_owner(void);
> > int console_set_owner(domid_t);
> > +domid_t console_owner_domid(void);
>
>
> I would expect that either the caller already has a domain locked, or
> uses rcu_lock_domain_console_owner() to obtain the domain and then get
> the domid? (d->domain_id?)

If the console focus in the guest, there's no need to prefix each guest OS
printout w/ "(XEN)" and a timestamp. To identify that current ns8250 emulator
is the one who's printing to the physical UART, I introduced this call.

I think ideally, domain should have a flag saying "currently has console
focus" which is set by the console focus switch logic in console driver.

>
>
> It's hard to tell why you need such way to get the console input
> target domid in such a way without seeing a caller to the function.

Sorry for that, addressed in v3.
I ended up dropping the patch after addressing all the feedback.

>
> Thanks, Roger.