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

[Bhupinder Thakur <bhupinder.thakur@xxxxxxxxxx>]

Hi Stefano,

Thanks for your comments.

>> 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?
Yes. I will remove it from the console structure.

>
>
>> +     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.
>
I thought that xspath is a more appropriate name than conspath because
it tells that it is related to
xenstore. I could keep it unchanged. Let me know. Else I will
introduce this in a separate patch.

>
>> +     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.
I think this check in not required as it would be called only if the
console was initialized.
I will remove the check.
>
>
>
>>       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.

I tried to reuse the same event channel for handling multiple consoles
since an event channel can handle multiple connections by allocating
unique local ports. Considering that there will not be many consoles
active at the same time, I thought it might be ok to reuse the same
event channel.

I agree that it is natural to make this per console. Let me know if I
should make it per console.

Regards,
Bhupinder

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