Re: [PATCH v1 2/8] xen/console: introduce console input permission

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Wednesday, March 26th, 2025 at 6:44 AM, Jan Beulich <jbeulich@xxxxxxxx> 
wrote:

>
>
> On 19.03.2025 00:36, dmkhn@xxxxxxxxx wrote:
>
> > @@ -564,10 +586,25 @@ static void __serial_rx(char c)
> > /* Deliver input to the PV shim console. */
> > rc = consoled_guest_tx(c);
> >
> > - if ( rc )
> > + switch ( rc )
> > + {
> > + case 0:
> > + break;
> > +
> > + case -EBUSY: /* Loopback mode /
> > + case -ENOSPC: / FIFO is full */
> > guest_printk(d,
> > XENLOG_WARNING "failed to process console input: %d\n",
> > rc);
> > + break;
> > +
> > + default:
> > + d->console.input_allowed = false;
>
>
> This aspect isn't mentioned / justified in the description, and I also
> can't deduce why you would do so. Or to put it differently, why you'd
> then not also take away input focus from this domain, for it no longer
> being eligible to have focus.

My idea was to explicitly distinguish "recoverable" errors, such as
"FIFO is full", from "emulator logical" errors when input character cannot
be delivered to the domain because of an error in emulator, so those are
easily seen in the debug logs.

I re-inspected return values, the only values currently supported are:
  (1) 0: success
  (2) -ENODEV: domain is not configured to have vUART
  (3) -ENOSPC: FIFO is full

-EBUSY is supposed to mean "vUART is in loopback mode" and it is leaked
from the future NS16550 emulator code.

I will drop that change in v2.

>
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -512,7 +512,7 @@ struct domain
> > bool auto_node_affinity;
> > /* Is this guest fully privileged (aka dom0)? /
> > bool is_privileged;
> > - / Can this guest access the Xen console? /
> > + / XSM: permission to use HYPERCALL_console_io hypercall /
> > bool is_console;
> > / Is this guest being debugged by dom0? */
> > bool debugger_attached;
> > @@ -651,6 +651,12 @@ struct domain
> > unsigned int num_llc_colors;
> > const unsigned int llc_colors;
> > #endif
> > +
> > + / Console settings. /
> > + struct {
> > + / Permission to own physical console input. */
> > + bool input_allowed;
> > + } console;
>
>
> Are further fields going to be added to this sub-struct? If not, is having
> a sub-struct here actually worth it?

I was thinking about grouping all console-related fields in struct domain
here in the future: e.g. move pbuf-related fields under "struct console".

>
> Jan