|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 07/10 v2] xen/arm: vpl011: Add support for vuart in xenconsole
Hi Stefano,
On 29 April 2017 at 04:40, Stefano Stabellini <sstabellini@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>
>> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |