Re: [PATCH 07/10 v2] xen/arm: vpl011: Add support for vuart in xenconsole

From: Bhupinder Thakur <bhupinder.thakur@linaro.org>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wei.liu2@citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	Jan Beulich <jbeulich@suse.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 07/10 v2] xen/arm: vpl011: Add support for vuart in xenconsole
Date: Mon, 8 May 2017 11:48:07 +0530	[thread overview]
Message-ID: <CACtJ1JSBqzKcnV_XwQs93YAqtKb9-50Qe8FmJ73ag45Cv2nfxw@mail.gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.10.1704281540080.2895@sstabellini-ThinkPad-X260>

Hi Stefano,

On 29 April 2017 at 04:40, Stefano Stabellini <sstabellini@kernel.org> wrote:
> On Fri, 28 Apr 2017, Bhupinder Thakur wrote:
>> Xenconsole supports only PV console currently. This patch adds support
>> for vuart, which allows emulated pl011 uart to be accessed as a console.
>>
>> This patch modifies different data structures and APIs used
>> in xenconsole to support two console types: PV and VUART.
>>
>> Change summary:
>>
>> 1. Split the domain structure into a console structure and the
>>    domain structure. Each PV and VUART will have seprate console
>>    structures.
>>
>> 2. Modify different APIs such as buffer_append() etc. to take
>>    console structure as input and perform per console specific
>>    operations.
>>
>> 3. Modfications in domain_create_ring():
>>     - Bind to the vpl011 event channel obtained from the xen store as a
>>       new parameter.
>>     - Map the PFN to its address space to be used as IN/OUT ring
>>       buffers.
>>       It obtains the PFN from the xen store as a new parameter
>>
>> 4. Modifications in handle_ring_read() to handle both PV and VUART
>>    events.
>>
>> 5. Add a new log_file for VUART console logs.
>
> This patch is too big. It might be best to split this patch in two: one
> to refactor the code to introduce struct console, and the other to
> introduce the vuart console.  It would make it far easier to review.
>
Ok. I have split the changes into two patches as suggested.

>
>> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
>> ---
>>
>> Changes since v1:
>>
>> - Split the domain struture to a separate console structure
>> - Modified the functions to operate on the console struture
>> - Replaced repetitive per console code with generic code
>>
>>  tools/console/daemon/io.c | 514 ++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 365 insertions(+), 149 deletions(-)
>>
>> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
>> index 7e6a886..55fda37 100644
>> --- a/tools/console/daemon/io.c
>> +++ b/tools/console/daemon/io.c
>> @@ -61,6 +61,10 @@
>>  /* Duration of each time period in ms */
>>  #define RATE_LIMIT_PERIOD 200
>>
>> +#define MAX_CONSOLE 2
>> +#define CONSOLE_TYPE_PV 0
>> +#define CONSOLE_TYPE_VUART 1
>
> It would be nice to protect this by something like:
>
> #ifdef CONFIG_ARM64 && CONFIG_ACPI
>
Why is there a dependency on ACPI?

> so that we don't waste memory in all other cases. The end result would
> be to have an console array of only one element on arm32 and x86 and
> when acpi is disabled.
>
>
>>  extern int log_reload;
>>  extern int log_guest;
>>  extern int log_hv;
>> @@ -89,29 +93,75 @@ struct buffer {
>>       size_t max_capacity;
>>  };
>>
>> -struct domain {
>> -     int domid;
>> +struct console {
>> +     char *name;
>> +     char *ttyname;
>>       int master_fd;
>>       int master_pollfd_idx;
>>       int slave_fd;
>>       int log_fd;
>> -     bool is_dead;
>> -     unsigned last_seen;
>>       struct buffer buffer;
>> -     struct domain *next;
>> -     char *conspath;
>>       int ring_ref;
>>       xenevtchn_port_or_error_t local_port;
>>       xenevtchn_port_or_error_t remote_port;
>> +     struct xencons_interface *interface;
>> +     struct domain *d;  /* Reference to the domain it is contained in. */
>> +};
>> +
>> +struct domain {
>> +     int domid;
>> +     bool is_dead;
>> +     unsigned last_seen;
>> +     struct domain *next;
>> +     char *conspath;
>>       xenevtchn_handle *xce_handle;
>>       int xce_pollfd_idx;
>> -     struct xencons_interface *interface;
>>       int event_count;
>>       long long next_period;
>> +     struct console console[MAX_CONSOLE];
>>  };
>>
>>
>> -     snprintf(logfile, PATH_MAX-1, "%s/guest-%s.log", log_dir, data);
>> +     snprintf(logfile, PATH_MAX-1, "%s/guest-%s-%s.log",
>> +                      log_dir, con->name, data);
>
> I am OK with this, but I wonder if changing the log name will create any
> troubles to existing management software.
Ok. i will keep the log filename unchanged for pv logs.
For vuart I will create a new directory /console/vuart where the log
file will be created.

>
>
>>       free(data);
>>       logfile[PATH_MAX-1] = '\0';
>>
>> @@ -336,19 +401,24 @@ static int create_domain_log(struct domain *dom)
>>       return fd;
>>  }
>>
>> -static void domain_close_tty(struct domain *dom)
>> +static void console_close_tty(struct console *con)
>>  {
>> -     if (dom->master_fd != -1) {
>> -             close(dom->master_fd);
>> -             dom->master_fd = -1;
>> +     if (con->master_fd != -1) {
>> +             close(con->master_fd);
>> +             con->master_fd = -1;
>>       }
>>
>> -     if (dom->slave_fd != -1) {
>> -             close(dom->slave_fd);
>> -             dom->slave_fd = -1;
>> +     if (con->slave_fd != -1) {
>> +             close(con->slave_fd);
>> +             con->slave_fd = -1;
>>       }
>>  }
>>
>> +static void domain_close_tty(struct domain *dom)
>> +{
>> +     console_iter_no_check(dom, console_close_tty);
>> +}
>> +
>>  #ifdef __sun__
>>  static int openpty(int *amaster, int *aslave, char *name,
>>                  struct termios *termp, struct winsize *winp)
>> @@ -409,7 +479,7 @@ void cfmakeraw(struct termios *termios_p)
>>  }
>>  #endif /* __sun__ */
>>
>> -static int domain_create_tty(struct domain *dom)
>> +static int console_create_tty(struct console *con)
>>  {
>>       const char *slave;
>>       char *path;
>> @@ -418,19 +488,23 @@ static int domain_create_tty(struct domain *dom)
>>       char *data;
>>       unsigned int len;
>>       struct termios term;
>> +     struct domain *dom = con->d;
>> +
>> +     if (!console_enabled(con))
>> +             return 1;
>>
>> -     assert(dom->slave_fd == -1);
>> -     assert(dom->master_fd == -1);
>> +     assert(con->master_fd == -1);
>> +     assert(con->slave_fd == -1);
>>
>> -     if (openpty(&dom->master_fd, &dom->slave_fd, NULL, NULL, NULL) < 0) {
>> +     if (openpty(&con->master_fd, &con->slave_fd, NULL, NULL, NULL) < 0) {
>>               err = errno;
>>               dolog(LOG_ERR, "Failed to create tty for domain-%d "
>> -                   "(errno = %i, %s)",
>> -                   dom->domid, err, strerror(err));
>> -             return 0;
>> +                       "(errno = %i, %s)",
>> +                       dom->domid, err, strerror(err));
>> +             goto out;
>
> I noticed that you turned the return into a goto out, why?
>
>
I will replace it with a goto.

>> -     buffer_append(dom);
>> +     if (port == vuart_con->local_port)
>> +             buffer_append(vuart_con);
>> +     else
>> +             buffer_append(pv_con);
>
> I would do it with a loop, without hardcoding the check:>
> for (i = 0; i < max_console; i++) {
>     if (dom->console[i].local_port == port)
>         buffer_append(&dom->console[i]);
> }
>
>
ok.

Regards,
Bhupinder

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