Re: [Xen-devel] [PATCH 10/14 v4] xen/arm: vpl011: Modify xenconsole to support multiple consoles

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 6 Jun 2017, Bhupinder Thakur wrote:
> This patch adds the support for multiple consoles and introduces the iterator
> functions to operate on multiple consoles.
> 
> This patch is in preparation to support a new vuart console.
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>
> ---
> CC: ij
> CC: wl
> CC: ss
> CC: jg
> 
> Changes since v3:
> - The changes in xenconsole have been split into four patches. This is the 
> third patch.
> 
>  tools/console/daemon/io.c | 364 
> +++++++++++++++++++++++++++++++++-------------
>  1 file changed, 263 insertions(+), 101 deletions(-)
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index c5dd08d..db73e10 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -90,12 +90,15 @@ struct buffer {
>  };
>  
>  struct console {
> +     char *xsname;

How is xsname useful? It doesn't look like it is used anywhere except
init, right?


> +     char *ttyname;
>       int master_fd;
>       int master_pollfd_idx;
>       int slave_fd;
>       int log_fd;
>       struct buffer buffer;
> -     char *conspath;
> +     char *xspath;

A simple trick to make patch easier to handle is to separate out changes
like this one: renaming conspath to xspath causes a lot of churn, which
ends up all mixed up with other meaningful changes.


> +     char *log_suffix;
>       int ring_ref;
>       xenevtchn_port_or_error_t local_port;
>       xenevtchn_port_or_error_t remote_port;
> @@ -103,6 +106,23 @@ struct console {
>       struct domain *d;
>  };
>  
> +struct console_data {
> +     char *xsname;
> +     char *ttyname;
> +     char *log_suffix;
> +};
> +
> +static struct console_data console_data[] = {
> +
> +     {
> +             .xsname = "/console",
> +             .ttyname = "tty",
> +             .log_suffix = "",
> +     },
> +};
> +
> +#define MAX_CONSOLE (sizeof(console_data)/sizeof(struct console_data))
> +
>  struct domain {
>       int domid;
>       bool is_dead;
> @@ -112,11 +132,90 @@ struct domain {
>       int xce_pollfd_idx;
>       int event_count;
>       long long next_period;
> -     struct console console;
> +     struct console console[MAX_CONSOLE];
>  };
>  
> +static void buffer_append(struct console *con, unsigned int data)
>  {
>       struct buffer *buffer = &con->buffer;
> +     struct xencons_interface *intf = con->interface;
> +     xenevtchn_port_or_error_t rxport = (xenevtchn_port_or_error_t)data;
>       struct domain *dom = con->d;
>       XENCONS_RING_IDX cons, prod, size;
> -     struct xencons_interface *intf = con->interface;
> +
> +     /* If incoming data is not for the current console then ignore. */
> +     if (con->local_port != rxport)
> +             return;
>  
>       cons = intf->out_cons;
>       prod = intf->out_prod;
> @@ -427,6 +541,9 @@ static int console_create_tty(struct console *con)
>       struct termios term;
>       struct domain *dom = con->d;
>  
> +     if (!console_enabled(con))
> +             return 1;

Is this actually useful? It doesn't look like the rest code changed in
regards to console_create_tty.



>       assert(con->slave_fd == -1);
>       assert(con->master_fd == -1);
>  
> @@ -594,15 +711,16 @@ static int console_create_ring(struct console *con)
>  
>       con->local_port = -1;
>       con->remote_port = -1;
> -     if (dom->xce_handle != NULL)
> -             xenevtchn_close(dom->xce_handle);
>  
> -     /* Opening evtchn independently for each console is a bit
> -      * wasteful, but that's how the code is structured... */
> -     dom->xce_handle = xenevtchn_open(NULL, 0);
> -     if (dom->xce_handle == NULL) {
> -             err = errno;
> -             goto out;
> +     if (dom->xce_handle == NULL)
> +     {
> +             /* Opening evtchn independently for each console is a bit
> +              * wasteful, but that's how the code is structured... */
> +             dom->xce_handle = xenevtchn_open(NULL, 0);
> +             if (dom->xce_handle == NULL) {
> +                     err = errno;
> +                     goto out;
> +             }

I think we need to do this per console actually, see below


>       }
>   
>       rc = xenevtchn_bind_interdomain(dom->xce_handle,
> @@ -1092,14 +1282,13 @@ void handle_io(void)
>                       if ((now+5) > d->next_period) {
>                               d->next_period = now + RATE_LIMIT_PERIOD;
>                               if (d->event_count >= RATE_LIMIT_ALLOWANCE) {
> -                                     (void)xenevtchn_unmask(d->xce_handle, 
> con->local_port);
> +                                     console_iter_void_arg1(d, 
> console_event_unmask);
>                               }
>                               d->event_count = 0;
>                       }
>               }
> @@ -1107,28 +1296,15 @@ void handle_io(void)
>                                   d->next_period < next_timeout)
>                                       next_timeout = d->next_period;
>                       } else if (d->xce_handle != NULL) {
> -                             if (discard_overflowed_data ||
> -                                 !con->buffer.max_capacity ||
> -                                 con->buffer.size < 
> con->buffer.max_capacity) {
> -                                     int evtchn_fd = 
> xenevtchn_fd(d->xce_handle);
> -                                     d->xce_pollfd_idx = set_fds(evtchn_fd,
> -                                                                 
> POLLIN|POLLPRI);
> +                                     if (console_iter_bool_arg1(d, 
> buffer_available))
> +                                     {
> +                                             int evtchn_fd = 
> xenevtchn_fd(d->xce_handle);
> +                                             d->xce_pollfd_idx = 
> set_fds(evtchn_fd,
> +                                                                             
>                         POLLIN|POLLPRI);
> +                                     }
>                               }

Is there a reason why we have one xce_pollfd_idx, xce_handle,
next_period and event_count per domain, rather than per console?

It is strange to set xce_pollfd_idx if at least one console of the
domain has enough buffer availability. Similarly, it is strange to count
the next_period on a per domain basis, regardless of the number of
consoles. It would be natural to do it at the console level.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel