[Xen-devel,08/10] xen/arm: vpl011: Modify the APIs in xenconsole to acces both PV and VCON consoles

Message ID 1491212673-13476-9-git-send-email-bhupinder.thakur@linaro.org
State New
Headers show
Series
  • pl011 emulation support in Xen
Related show

Commit Message

Bhupinder Thakur April 3, 2017, 9:44 a.m.
Xenconsole supports only PV console currently. To get access to emulated pl011
uart another backend console is required.

This patch modifies different data structures and APIs used
in xenconsole to support two console types: PV and VCON (virtual console).

Change summary:

1. Modify the domain structure to support two console types: PV and a
   virtual console (VCON).

2. Modify different APIs such as buffer_append() to take a new parameter
   console_type as input and operate on the data structures indexed by
   the console_type.

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 VCON
   events.

5. Add a new log_file for VCON console logs.

Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
---
 tools/console/daemon/io.c | 508 ++++++++++++++++++++++++++++++++--------------
 1 file changed, 356 insertions(+), 152 deletions(-)

Comments

Wei Liu April 12, 2017, 4:33 p.m. | #1
On Mon, Apr 03, 2017 at 03:14:31PM +0530, Bhupinder Thakur wrote:
> Xenconsole supports only PV console currently. To get access to emulated pl011
> uart another backend console is required.
> 
> This patch modifies different data structures and APIs used
> in xenconsole to support two console types: PV and VCON (virtual console).
> 
> Change summary:
> 
> 1. Modify the domain structure to support two console types: PV and a
>    virtual console (VCON).
> 
> 2. Modify different APIs such as buffer_append() to take a new parameter
>    console_type as input and operate on the data structures indexed by
>    the console_type.
> 
> 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 VCON
>    events.
> 
> 5. Add a new log_file for VCON console logs.
> 

It seems that this patch and previous one should be merged into one.

There are a lot of coding style issues, please fix all of them.

The code looks rather repetitive unfortunately. I believe a lot of the
repetitiveness can be avoided by using loops and pointers.

> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>  tools/console/daemon/io.c | 508 ++++++++++++++++++++++++++++++++--------------
>  1 file changed, 356 insertions(+), 152 deletions(-)
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 0cd1fee..b9be5a5 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -164,11 +164,39 @@ static int write_with_timestamp(int fd, const char *data, size_t sz,
>  	return 0;
>  }
>  
> -static void buffer_append(struct domain *dom)
> +/*
> + * This function checks if the data is allowed to be buffered for that console.
> + * If not then it marks it pending for later receiving.
> + */
> +static bool buffer_available(struct domain *dom, int console_type)
> +{
> +	if (discard_overflowed_data ||
> +		!dom->buffer[console_type].max_capacity ||
> +		dom->buffer[console_type].size < dom->buffer[console_type].max_capacity)

Line too long.

> +	{
> +		dom->console_data_pending &= ~(1<<console_type);
> +		return true;
> +	}
> +	else
> +	{
> +		dom->console_data_pending |= (1<<console_type);
> +		return false;
> +	}
> +}
> +
> +static void buffer_append(struct domain *dom, int console_type)
>  {
> -	struct buffer *buffer = &dom->buffer;
> +	struct buffer *buffer = &dom->buffer[console_type];
> +	struct xencons_interface *intf=dom->interface[console_type];
> +	xenevtchn_port_or_error_t port=dom->local_port[console_type];

Spaces around "=".

> +
>  	XENCONS_RING_IDX cons, prod, size;
> -	struct xencons_interface *intf = dom->interface;
> +
> +	/*
> +	 * check if more data is allowed to be buffered
> +	 */
> +	if (!buffer_available(dom, console_type))
> +		return;
>  
>  	cons = intf->out_cons;
>  	prod = intf->out_prod;
> @@ -193,22 +221,22 @@ static void buffer_append(struct domain *dom)
>  
>  	xen_mb();
>  	intf->out_cons = cons;
> -	xenevtchn_notify(dom->xce_handle, dom->local_port);
> +	xenevtchn_notify(dom->xce_handle, port);
>  
>  	/* Get the data to the logfile as early as possible because if
>  	 * no one is listening on the console pty then it will fill up
>  	 * and handle_tty_write will stop being called.
>  	 */
> -	if (dom->log_fd != -1) {
> +	if (dom->log_fd[console_type] != -1) {
>  		int logret;
>  		if (log_time_guest) {
>  			logret = write_with_timestamp(
> -				dom->log_fd,
> +				dom->log_fd[console_type],
>  				buffer->data + buffer->size - size,
>  				size, &log_time_guest_needts);
>  		} else {
>  			logret = write_all(
> -				dom->log_fd,
> +				dom->log_fd[console_type],
>  				buffer->data + buffer->size - size,
>  				size);
>  		}
> @@ -296,12 +324,13 @@ static int create_hv_log(void)
>  	return fd;
>  }
>  
> -static int create_domain_log(struct domain *dom)
> +static int create_domain_log(struct domain *dom, int console_type)
>  {
>  	char logfile[PATH_MAX];
>  	char *namepath, *data, *s;
>  	int fd;
>  	unsigned int len;
> +	char *console_name[]={"pv", "vcon"};
>  
>  	namepath = xs_get_domain_path(xs, dom->domid);
>  	s = realloc(namepath, strlen(namepath) + 6);
> @@ -320,7 +349,7 @@ static int create_domain_log(struct domain *dom)
>  		return -1;
>  	}
>  
> -	snprintf(logfile, PATH_MAX-1, "%s/guest-%s.log", log_dir, data);
> +	snprintf(logfile, PATH_MAX-1, "%s/guest-%s-%s.log", log_dir, console_name[console_type], data);

Line too long.

>  	free(data);
>  	logfile[PATH_MAX-1] = '\0';
>  
> @@ -344,14 +373,24 @@ static int create_domain_log(struct domain *dom)
>  
>  static void domain_close_tty(struct domain *dom)
>  {
> -	if (dom->master_fd != -1) {
> -		close(dom->master_fd);
> -		dom->master_fd = -1;
> +	if (dom->master_fd[CONSOLE_TYPE_PV] != -1) {
> +		close(dom->master_fd[CONSOLE_TYPE_PV]);
> +		dom->master_fd[CONSOLE_TYPE_PV] = -1;
> +	}
> +
> +	if (dom->slave_fd[CONSOLE_TYPE_PV] != -1) {
> +		close(dom->slave_fd[CONSOLE_TYPE_PV]);
> +		dom->slave_fd[CONSOLE_TYPE_PV] = -1;
> +	}
> +
> +	if (dom->master_fd[CONSOLE_TYPE_VCON] != -1) {
> +		close(dom->master_fd[CONSOLE_TYPE_VCON]);
> +		dom->master_fd[CONSOLE_TYPE_VCON] = -1;
>  	}
>  
> -	if (dom->slave_fd != -1) {
> -		close(dom->slave_fd);
> -		dom->slave_fd = -1;
> +	if (dom->slave_fd[CONSOLE_TYPE_VCON] != -1) {
> +		close(dom->slave_fd[CONSOLE_TYPE_VCON]);
> +		dom->slave_fd[CONSOLE_TYPE_VCON] = -1;
>  	}

You can use two loops to avoid repetitive snippets. But I'm fine with
the code as-is, too.

>  }
>  
> @@ -424,63 +463,86 @@ static int domain_create_tty(struct domain *dom)
>  	char *data;
>  	unsigned int len;
>  	struct termios term;
> +	int console_type=0;
>  
> -	assert(dom->slave_fd == -1);
> -	assert(dom->master_fd == -1);
> +	assert(dom->master_fd[CONSOLE_TYPE_PV] == -1);
> +	assert(dom->slave_fd[CONSOLE_TYPE_PV] == -1);
> +	assert(dom->master_fd[CONSOLE_TYPE_VCON] == -1);
> +	assert(dom->slave_fd[CONSOLE_TYPE_VCON] == -1);
>  
> -	if (openpty(&dom->master_fd, &dom->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;
> -	}
> +	/*
> +	 * Open pty for both PV and vcon consoles.
> +	 */
> +	for (console_type=0; console_type<MAX_CONSOLE; console_type++)

Spaces around "=" and "<".

> +	{
> +		if (openpty(&dom->master_fd[console_type], &dom->slave_fd[console_type], NULL, NULL, NULL) < 0) {

Line too long

> +			err = errno;
> +			dolog(LOG_ERR, "Failed to create tty for domain-%d "
> +				  "(errno = %i, %s)",
> +				  dom->domid, err, strerror(err));
> +			return 0;
> +		}
>  
> -	if (tcgetattr(dom->slave_fd, &term) < 0) {
> -		err = errno;
> -		dolog(LOG_ERR, "Failed to get tty attributes for domain-%d "
> -			"(errno = %i, %s)",
> -			dom->domid, err, strerror(err));
> -		goto out;
> -	}
> -	cfmakeraw(&term);
> -	if (tcsetattr(dom->slave_fd, TCSANOW, &term) < 0) {
> -		err = errno;
> -		dolog(LOG_ERR, "Failed to set tty attributes for domain-%d "
> -			"(errno = %i, %s)",
> -			dom->domid, err, strerror(err));
> -		goto out;
> -	}
> +		if (tcgetattr(dom->slave_fd[console_type], &term) < 0) {
> +			err = errno;
> +			dolog(LOG_ERR, "Failed to get tty attributes for domain-%d "
> +				"(errno = %i, %s)",
> +				dom->domid, err, strerror(err));
> +			goto out;
> +		}
> +		cfmakeraw(&term);
> +		if (tcsetattr(dom->slave_fd[console_type], TCSANOW, &term) < 0) {
> +			err = errno;
> +			dolog(LOG_ERR, "Failed to set tty attributes for domain-%d "
> +				"(errno = %i, %s)",
> +				dom->domid, err, strerror(err));
> +			goto out;
> +		}
>  
> -	if ((slave = ptsname(dom->master_fd)) == NULL) {
> -		err = errno;
> -		dolog(LOG_ERR, "Failed to get slave name for domain-%d "
> -		      "(errno = %i, %s)",
> -		      dom->domid, err, strerror(err));
> -		goto out;
> -	}
> +		if ((slave = ptsname(dom->master_fd[console_type])) == NULL) {
> +			err = errno;
> +			dolog(LOG_ERR, "Failed to get slave name for domain-%d "
> +				  "(errno = %i, %s)",
> +				  dom->domid, err, strerror(err));
> +			goto out;
> +		}
>  
> -	success = asprintf(&path, "%s/limit", dom->conspath) !=
> -		-1;
> -	if (!success)
> -		goto out;
> -	data = xs_read(xs, XBT_NULL, path, &len);
> -	if (data) {
> -		dom->buffer.max_capacity = strtoul(data, 0, 0);
> -		free(data);
> +		/*
> +		 * Initialize the console buffer capacity.
> +		 */
> +		success = asprintf(&path, "%s/limit", dom->conspath) !=
> +			-1;
> +		if (!success)
> +			goto out;
> +		data = xs_read(xs, XBT_NULL, path, &len);
> +		if (data) {
> +			dom->buffer[console_type].max_capacity = strtoul(data, 0, 0);
> +			free(data);
> +		}
> +		free(path);
> +
> +		/*
> +		 * Write console slave name to xen store.
> +		 */
> +		if (console_type == CONSOLE_TYPE_VCON)
> +			success = (asprintf(&path, "%s/vtty", dom->conspath) != -1);
> +		else
> +			success = (asprintf(&path, "%s/tty", dom->conspath) != -1);
> +
> +		if (!success)
> +			goto out;
> +		success = xs_write(xs, XBT_NULL, path, slave, strlen(slave));
> +		free(path);
> +
> +		if (fcntl(dom->master_fd[console_type], F_SETFL, O_NONBLOCK) == -1)
> +			goto out;
> +
> +		if (!dom->vcon_enabled)
> +			break;
>  	}
> -	free(path);
>  
> -	success = (asprintf(&path, "%s/tty", dom->conspath) != -1);
>  	if (!success)
>  		goto out;
> -	success = xs_write(xs, XBT_NULL, path, slave, strlen(slave));
> -	free(path);
> -	if (!success)
> -		goto out;
> -
> -	if (fcntl(dom->master_fd, F_SETFL, O_NONBLOCK) == -1)
> -		goto out;
>  
>  	return 1;
>  out:
> @@ -523,21 +585,53 @@ static int xs_gather(struct xs_handle *xs, const char *dir, ...)
>  	return ret;
>  }
>  
> -static void domain_unmap_interface(struct domain *dom)
> +static void domain_unmap_interface(struct domain *dom, int console_type)
>  {
> -	if (dom->interface == NULL)
> +	if (dom->interface[console_type] == NULL)
>  		return;
> -	if (xgt_handle && dom->ring_ref == -1)
> -		xengnttab_unmap(xgt_handle, dom->interface, 1);
> +	if (xgt_handle && dom->ring_ref[console_type] == -1)
> +		xengnttab_unmap(xgt_handle, dom->interface[console_type], 1);
>  	else
> -		munmap(dom->interface, XC_PAGE_SIZE);
> -	dom->interface = NULL;
> -	dom->ring_ref = -1;
> +		munmap(dom->interface[console_type], XC_PAGE_SIZE);
> +	dom->interface[console_type] = NULL;
> +	dom->ring_ref[console_type] = -1;
> +}
> +
> +static int bind_event_channel(struct domain *dom, int new_rport, int *lport, int *rport)

Line too long.

> +{
> +	int err = 0, rc;
> +
> +	/* Go no further if port has not changed and we are still bound. */
> +	if (new_rport == *rport) {
> +		xc_evtchn_status_t status = {
> +		.dom = DOMID_SELF,
> +		.port = *lport };
> +		if ((xc_evtchn_status(xc, &status) == 0) &&
> +			(status.status == EVTCHNSTAT_interdomain))
> +			goto out;
> +	}
> +
> +	*lport = -1;
> +	*rport = -1;
> +	rc = xenevtchn_bind_interdomain(dom->xce_handle,
> +									dom->domid, new_rport);

Indentation.

> +
> +	if (rc == -1) {
> +		err = errno;
> +		xenevtchn_close(dom->xce_handle);
> +		dom->xce_handle = NULL;
> +		goto out;
> +	}
> +
> +	*lport = rc;
> +	*rport = new_rport;
> +out:
> +	return err;
>  }
>   
>  static int domain_create_ring(struct domain *dom)
>  {
> -	int err, remote_port, ring_ref, rc;
> +	int err, remote_port, ring_ref, vcon_remote_port, vcon_ring_ref;
>  	char *type, path[PATH_MAX];
>  
>  	err = xs_gather(xs, dom->conspath,
> @@ -547,6 +641,17 @@ static int domain_create_ring(struct domain *dom)
>  	if (err)
>  		goto out;
>  
> +	/* vcon console is optional. */
> +	err = xs_gather(xs, dom->conspath,
> +		"vcon-ring-ref", "%u", &vcon_ring_ref,
> +		"vcon-port", "%i", &vcon_remote_port,
> +		NULL);
> +

Indentation.

> +	if (err || vcon_ring_ref == -1)
> +		dom->vcon_enabled = false;
> +	else
> +		dom->vcon_enabled = true;
> +
>  	snprintf(path, sizeof(path), "%s/type", dom->conspath);
>  	type = xs_read(xs, XBT_NULL, path, NULL);
>  	if (type && strcmp(type, "xenconsoled") != 0) {
> @@ -556,41 +661,51 @@ static int domain_create_ring(struct domain *dom)
>  	free(type);
>  
>  	/* If using ring_ref and it has changed, remap */
> -	if (ring_ref != dom->ring_ref && dom->ring_ref != -1)
> -		domain_unmap_interface(dom);
> +	if (ring_ref != dom->ring_ref[CONSOLE_TYPE_PV] && dom->ring_ref[CONSOLE_TYPE_PV] != -1)

Line too long.

> +		domain_unmap_interface(dom, CONSOLE_TYPE_PV);
> +
> +	/* If using vcon ring_ref and it has changed, remap */
> +	if (dom->vcon_enabled &&
> +		vcon_ring_ref != dom->ring_ref[CONSOLE_TYPE_VCON] &&
> +		dom->ring_ref[CONSOLE_TYPE_VCON] != -1 )
> +		domain_unmap_interface(dom, CONSOLE_TYPE_VCON);

Indentation.

>  
> -	if (!dom->interface && xgt_handle) {
> +	if (!dom->interface[CONSOLE_TYPE_PV] && xgt_handle) {
>  		/* Prefer using grant table */
> -		dom->interface = xengnttab_map_grant_ref(xgt_handle,
> -			dom->domid, GNTTAB_RESERVED_CONSOLE,
> -			PROT_READ|PROT_WRITE);
> -		dom->ring_ref = -1;
> +		dom->interface[CONSOLE_TYPE_PV] = xengnttab_map_grant_ref(xgt_handle,
> +																  dom->domid, GNTTAB_RESERVED_CONSOLE,
> +																  PROT_READ|PROT_WRITE);
> +		dom->ring_ref[CONSOLE_TYPE_PV] = -1;

Indentation.

>  	}
> -	if (!dom->interface) {
> +
> +	if (!dom->interface[CONSOLE_TYPE_PV]) {
>  		/* Fall back to xc_map_foreign_range */
> -		dom->interface = xc_map_foreign_range(
> +		dom->interface[CONSOLE_TYPE_PV] = xc_map_foreign_range(
>  			xc, dom->domid, XC_PAGE_SIZE,
>  			PROT_READ|PROT_WRITE,
>  			(unsigned long)ring_ref);
> -		if (dom->interface == NULL) {
> +		if (dom->interface[CONSOLE_TYPE_PV] == NULL) {
>  			err = EINVAL;
>  			goto out;
>  		}
> -		dom->ring_ref = ring_ref;
> +		dom->ring_ref[CONSOLE_TYPE_PV] = ring_ref;
>  	}
>  
> -	/* Go no further if port has not changed and we are still bound. */
> -	if (remote_port == dom->remote_port) {
> -		xc_evtchn_status_t status = {
> -			.dom = DOMID_SELF,
> -			.port = dom->local_port };
> -		if ((xc_evtchn_status(xc, &status) == 0) &&
> -		    (status.status == EVTCHNSTAT_interdomain))
> +	/* Map vcon console ring buffer. */
> +	if (dom->vcon_enabled && !dom->interface[CONSOLE_TYPE_VCON]) {
> +
> +		/* Fall back to xc_map_foreign_range */
> +		dom->interface[CONSOLE_TYPE_VCON] = xc_map_foreign_range(
> +		xc, dom->domid, XC_PAGE_SIZE,
> +		PROT_READ|PROT_WRITE,
> +		(unsigned long)vcon_ring_ref);
> +		if ( dom->interface[CONSOLE_TYPE_VCON] == NULL ) {
> +			err = EINVAL;
>  			goto out;
> +		}
> +		dom->ring_ref[CONSOLE_TYPE_VCON] = vcon_ring_ref;
>  	}
>  
> -	dom->local_port = -1;
> -	dom->remote_port = -1;
>  	if (dom->xce_handle != NULL)
>  		xenevtchn_close(dom->xce_handle);
>  
> @@ -602,31 +717,46 @@ static int domain_create_ring(struct domain *dom)
>  		goto out;
>  	}
>   
> -	rc = xenevtchn_bind_interdomain(dom->xce_handle,
> -		dom->domid, remote_port);
> -
> -	if (rc == -1) {
> -		err = errno;
> +	err = bind_event_channel(dom, remote_port,
> +							 &dom->local_port[CONSOLE_TYPE_PV],
> +							 &dom->remote_port[CONSOLE_TYPE_PV]);

Indentation.

> +	if (err)
> +	{
>  		xenevtchn_close(dom->xce_handle);
> -		dom->xce_handle = NULL;
>  		goto out;
>  	}
> -	dom->local_port = rc;
> -	dom->remote_port = remote_port;
>  
> -	if (dom->master_fd == -1) {
> +	if (dom->vcon_enabled)
> +	{
> +		err = bind_event_channel(dom, vcon_remote_port,
> +								 &dom->local_port[CONSOLE_TYPE_VCON],
> +								 &dom->remote_port[CONSOLE_TYPE_VCON]);

Indentation.

> +		if (err)
> +		{
> +			xenevtchn_close(dom->xce_handle);
> +			goto out;
> +		}
> +	}
> +
> +	if (dom->master_fd[CONSOLE_TYPE_PV] == -1) {
>  		if (!domain_create_tty(dom)) {
>  			err = errno;
>  			xenevtchn_close(dom->xce_handle);
>  			dom->xce_handle = NULL;
> -			dom->local_port = -1;
> -			dom->remote_port = -1;
> +			dom->local_port[CONSOLE_TYPE_PV] = -1;
> +			dom->remote_port[CONSOLE_TYPE_PV] = -1;
> +			dom->local_port[CONSOLE_TYPE_VCON] = -1;
> +			dom->remote_port[CONSOLE_TYPE_VCON] = -1;
> +			dom->vcon_enabled = false;
>  			goto out;
>  		}
>  	}
>  
> -	if (log_guest && (dom->log_fd == -1))
> -		dom->log_fd = create_domain_log(dom);
> +	if (log_guest && (dom->log_fd[CONSOLE_TYPE_PV] == -1))
> +		dom->log_fd[CONSOLE_TYPE_PV] = create_domain_log(dom, CONSOLE_TYPE_PV);
> +
> +	if (log_guest && dom->vcon_enabled && (dom->log_fd[CONSOLE_TYPE_VCON] == -1))
> +		dom->log_fd[CONSOLE_TYPE_VCON] = create_domain_log(dom, CONSOLE_TYPE_VCON);
>  
>   out:
>  	return err;
> @@ -681,17 +811,24 @@ static struct domain *create_domain(int domid)
>  	dom->conspath = s;
>  	strcat(dom->conspath, "/console");
>  
> -	dom->master_fd = -1;
> -	dom->master_pollfd_idx = -1;
> -	dom->slave_fd = -1;
> -	dom->log_fd = -1;
> +	dom->master_fd[CONSOLE_TYPE_PV] = -1;
> +	dom->master_pollfd_idx[CONSOLE_TYPE_PV] = -1;
> +	dom->slave_fd[CONSOLE_TYPE_PV] = -1;
> +	dom->master_fd[CONSOLE_TYPE_VCON] = -1;
> +	dom->master_pollfd_idx[CONSOLE_TYPE_VCON] = -1;
> +	dom->slave_fd[CONSOLE_TYPE_VCON] = -1;
> +	dom->log_fd[CONSOLE_TYPE_PV] = -1;
> +	dom->log_fd[CONSOLE_TYPE_VCON] = -1;
>  	dom->xce_pollfd_idx = -1;
>  
>  	dom->next_period = ((long long)ts.tv_sec * 1000) + (ts.tv_nsec / 1000000) + RATE_LIMIT_PERIOD;
>  
> -	dom->ring_ref = -1;
> -	dom->local_port = -1;
> -	dom->remote_port = -1;
> +	dom->ring_ref[CONSOLE_TYPE_PV] = -1;
> +	dom->ring_ref[CONSOLE_TYPE_VCON] = -1;
> +	dom->local_port[CONSOLE_TYPE_PV] = -1;
> +	dom->remote_port[CONSOLE_TYPE_PV] = -1;
> +	dom->local_port[CONSOLE_TYPE_VCON] = -1;
> +	dom->remote_port[CONSOLE_TYPE_VCON] = -1;
>  
>  	if (!watch_domain(dom, true))
>  		goto out;
> @@ -737,13 +874,21 @@ static void cleanup_domain(struct domain *d)
>  {
>  	domain_close_tty(d);
>  
> -	if (d->log_fd != -1) {
> -		close(d->log_fd);
> -		d->log_fd = -1;
> +	if (d->log_fd[CONSOLE_TYPE_PV] != -1) {
> +		close(d->log_fd[CONSOLE_TYPE_PV]);
> +		d->log_fd[CONSOLE_TYPE_PV] = -1;
> +	}
> +
> +	if (d->log_fd[CONSOLE_TYPE_VCON] != -1) {
> +		close(d->log_fd[CONSOLE_TYPE_VCON]);
> +		d->log_fd[CONSOLE_TYPE_VCON] = -1;
>  	}
>  
> -	free(d->buffer.data);
> -	d->buffer.data = NULL;
> +	free(d->buffer[CONSOLE_TYPE_PV].data);
> +	free(d->buffer[CONSOLE_TYPE_VCON].data);
> +
> +	d->buffer[CONSOLE_TYPE_PV].data = NULL;
> +	d->buffer[CONSOLE_TYPE_VCON].data = NULL;
>  
>  	free(d->conspath);
>  	d->conspath = NULL;
> @@ -755,7 +900,8 @@ static void shutdown_domain(struct domain *d)
>  {
>  	d->is_dead = true;
>  	watch_domain(d, false);
> -	domain_unmap_interface(d);
> +	domain_unmap_interface(d, CONSOLE_TYPE_PV);
> +	domain_unmap_interface(d, CONSOLE_TYPE_VCON);
>  	if (d->xce_handle != NULL)
>  		xenevtchn_close(d->xce_handle);
>  	d->xce_handle = NULL;
> @@ -786,9 +932,9 @@ static void enum_domains(void)
>  	}
>  }
>  
> -static int ring_free_bytes(struct domain *dom)
> +static int ring_free_bytes(struct domain *dom, int console_type)
>  {
> -	struct xencons_interface *intf = dom->interface;
> +	struct xencons_interface *intf = dom->interface[console_type];
>  	XENCONS_RING_IDX cons, prod, space;
>  
>  	cons = intf->in_cons;
> @@ -813,25 +959,26 @@ static void domain_handle_broken_tty(struct domain *dom, int recreate)
>  	}
>  }
>  
> -static void handle_tty_read(struct domain *dom)
> +static void handle_tty_read(struct domain *dom, int console_type)
>  {
>  	ssize_t len = 0;
>  	char msg[80];
>  	int i;
> -	struct xencons_interface *intf = dom->interface;
>  	XENCONS_RING_IDX prod;
> +	struct xencons_interface *intf=dom->interface[console_type];
> +	xenevtchn_port_or_error_t port=dom->local_port[console_type];
>  
>  	if (dom->is_dead)
>  		return;
>  
> -	len = ring_free_bytes(dom);
> +	len = ring_free_bytes(dom, console_type);
>  	if (len == 0)
>  		return;
>  
>  	if (len > sizeof(msg))
>  		len = sizeof(msg);
>  
> -	len = read(dom->master_fd, msg, len);
> +	len = read(dom->master_fd[console_type], msg, len);
>  	/*
>  	 * Note: on Solaris, len == 0 means the slave closed, and this
>  	 * is no problem, but Linux can't handle this usefully, so we
> @@ -847,28 +994,29 @@ static void handle_tty_read(struct domain *dom)
>  		}
>  		xen_wmb();
>  		intf->in_prod = prod;
> -		xenevtchn_notify(dom->xce_handle, dom->local_port);
> +		xenevtchn_notify(dom->xce_handle, port);
>  	} else {
>  		domain_close_tty(dom);
>  		shutdown_domain(dom);
>  	}
>  }
>  
> -static void handle_tty_write(struct domain *dom)
> +static void handle_tty_write(struct domain *dom, int console_type)
>  {
>  	ssize_t len;
>  
>  	if (dom->is_dead)
>  		return;
>  
> -	len = write(dom->master_fd, dom->buffer.data + dom->buffer.consumed,
> -		    dom->buffer.size - dom->buffer.consumed);
> +	len = write(dom->master_fd[console_type],
> +				dom->buffer[console_type].data + dom->buffer[console_type].consumed,
> +				dom->buffer[console_type].size - dom->buffer[console_type].consumed);
>   	if (len < 1) {
>  		dolog(LOG_DEBUG, "Write failed on domain %d: %zd, %d\n",
>  		      dom->domid, len, errno);
>  		domain_handle_broken_tty(dom, domain_is_valid(dom->domid));
>  	} else {
> -		buffer_advance(&dom->buffer, len);
> +		buffer_advance(&dom->buffer[console_type], len);
>  	}
>  }
>  
> @@ -884,7 +1032,10 @@ static void handle_ring_read(struct domain *dom)
>  
>  	dom->event_count++;
>  
> -	buffer_append(dom);
> +	if (port == dom->local_port[CONSOLE_TYPE_VCON])
> +		buffer_append(dom, CONSOLE_TYPE_VCON);
> +	else
> +		buffer_append(dom, CONSOLE_TYPE_PV);
>  
>  	if (dom->event_count < RATE_LIMIT_ALLOWANCE)
>  		(void)xenevtchn_unmask(dom->xce_handle, port);
> @@ -954,9 +1105,16 @@ static void handle_log_reload(void)
>  	if (log_guest) {
>  		struct domain *d;
>  		for (d = dom_head; d; d = d->next) {
> -			if (d->log_fd != -1)
> -				close(d->log_fd);
> -			d->log_fd = create_domain_log(d);
> +			if (d->log_fd[CONSOLE_TYPE_PV] != -1)
> +				close(d->log_fd[CONSOLE_TYPE_PV]);
> +			d->log_fd[CONSOLE_TYPE_PV] = create_domain_log(d, CONSOLE_TYPE_PV);
> +
> +			if (d->vcon_enabled)
> +			{
> +				if (d->log_fd[CONSOLE_TYPE_VCON] != -1)
> +					close(d->log_fd[CONSOLE_TYPE_VCON]);
> +				d->log_fd[CONSOLE_TYPE_PV] = create_domain_log(d, CONSOLE_TYPE_VCON);

Line too long.

> +			}
>  		}
>  	}
>  
> @@ -1074,7 +1232,9 @@ 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, d->local_port);
> +					(void)xenevtchn_unmask(d->xce_handle, d->local_port[CONSOLE_TYPE_PV]);
> +					if (d->vcon_enabled)
> +						(void)xenevtchn_unmask(d->xce_handle, d->local_port[CONSOLE_TYPE_VCON]);

Line too long.

>  				}
>  				d->event_count = 0;
>  			}
> @@ -1087,27 +1247,47 @@ void handle_io(void)
>  				    d->next_period < next_timeout)
>  					next_timeout = d->next_period;
>  			} else if (d->xce_handle != NULL) {
> -				if (discard_overflowed_data ||
> -				    !d->buffer.max_capacity ||
> -				    d->buffer.size < d->buffer.max_capacity) {
> +
> +				if (buffer_available(d, CONSOLE_TYPE_PV)) {
> +					int evtchn_fd = xenevtchn_fd(d->xce_handle);
> +					d->xce_pollfd_idx = set_fds(evtchn_fd,
> +												POLLIN|POLLPRI);

Indentation.

> +				}
> +
> +				if (buffer_available(d, CONSOLE_TYPE_VCON ) )

Extraneous space after CONSOLE_TYPE_VCON.

> +				{
>  					int evtchn_fd = xenevtchn_fd(d->xce_handle);
>  					d->xce_pollfd_idx = set_fds(evtchn_fd,
> -								    POLLIN|POLLPRI);
> +												POLLIN|POLLPRI);

Indentation.

>  				}
>  			}
>  
> -			if (d->master_fd != -1) {
> +			if (d->master_fd[CONSOLE_TYPE_PV] != -1) {
>  				short events = 0;
> -				if (!d->is_dead && ring_free_bytes(d))
> +				if (!d->is_dead && ring_free_bytes(d, CONSOLE_TYPE_PV))
>  					events |= POLLIN;
>  
> -				if (!buffer_empty(&d->buffer))
> +				if (!buffer_empty(&d->buffer[CONSOLE_TYPE_PV]))
>  					events |= POLLOUT;
>  
>  				if (events)
> -					d->master_pollfd_idx =
> -						set_fds(d->master_fd,
> -							events|POLLPRI);
> +					d->master_pollfd_idx[CONSOLE_TYPE_PV] =
> +						set_fds(d->master_fd[CONSOLE_TYPE_PV],
> +								events|POLLPRI);
> +			}
> +
> +			if (d->vcon_enabled && d->master_fd[CONSOLE_TYPE_VCON] != -1) {
> +				short events = 0;
> +				if (!d->is_dead && ring_free_bytes(d, CONSOLE_TYPE_VCON))
> +					events |= POLLIN;
> +
> +				if (!buffer_empty(&d->buffer[CONSOLE_TYPE_VCON]))
> +					events |= POLLOUT;
> +
> +				if (events)
> +					d->master_pollfd_idx[CONSOLE_TYPE_VCON] =
> +						set_fds(d->master_fd[CONSOLE_TYPE_VCON],
> +								events|POLLPRI);
>  			}
>  		}
>  
> @@ -1166,6 +1346,16 @@ void handle_io(void)
>  
>  		for (d = dom_head; d; d = n) {
>  			n = d->next;
> +
> +			/*
> +			 * Check if the data pending flag is set for any of the consoles.
> +			 * If yes then service those first.
> +			 */
> +			if ( d->console_data_pending & (1<<CONSOLE_TYPE_PV) )
> +				buffer_append(d, CONSOLE_TYPE_PV);
> +			else if ( d->console_data_pending & (1<<CONSOLE_TYPE_VCON) )
> +				buffer_append(d, CONSOLE_TYPE_VCON);
> +

Why? You seem to have skipped the ratelimit check here.

>  			if (d->event_count < RATE_LIMIT_ALLOWANCE) {
>  				if (d->xce_handle != NULL &&
>  				    d->xce_pollfd_idx != -1 &&
> @@ -1176,22 +1366,36 @@ void handle_io(void)
>  				    handle_ring_read(d);
>  			}
>  
> -			if (d->master_fd != -1 && d->master_pollfd_idx != -1) {
> -				if (fds[d->master_pollfd_idx].revents &
> +			if (d->master_fd[CONSOLE_TYPE_PV] != -1 && d->master_pollfd_idx[CONSOLE_TYPE_PV] != -1) {
> +				if (fds[d->master_pollfd_idx[CONSOLE_TYPE_PV]].revents &
> +				    ~(POLLIN|POLLOUT|POLLPRI))
> +					domain_handle_broken_tty(d, domain_is_valid(d->domid));
> +				else {
> +					if (fds[d->master_pollfd_idx[CONSOLE_TYPE_PV]].revents &
> +					    POLLIN)
> +						handle_tty_read(d, CONSOLE_TYPE_PV);
> +					if (fds[d->master_pollfd_idx[CONSOLE_TYPE_PV]].revents &
> +					    POLLOUT)
> +						handle_tty_write(d, CONSOLE_TYPE_PV);
> +				}
> +			}
> +
> +			if (d->master_fd[CONSOLE_TYPE_VCON] != -1 && d->master_pollfd_idx[CONSOLE_TYPE_VCON] != -1) {
> +				if (fds[d->master_pollfd_idx[CONSOLE_TYPE_VCON]].revents &
>  				    ~(POLLIN|POLLOUT|POLLPRI))
> -					domain_handle_broken_tty(d,
> -						   domain_is_valid(d->domid));
> +					domain_handle_broken_tty(d, domain_is_valid(d->domid));
>  				else {
> -					if (fds[d->master_pollfd_idx].revents &
> +					if (fds[d->master_pollfd_idx[CONSOLE_TYPE_VCON]].revents &
>  					    POLLIN)
> -						handle_tty_read(d);
> -					if (fds[d->master_pollfd_idx].revents &
> +						handle_tty_read(d, CONSOLE_TYPE_VCON);
> +					if (fds[d->master_pollfd_idx[CONSOLE_TYPE_VCON]].revents &
>  					    POLLOUT)
> -						handle_tty_write(d);
> +						handle_tty_write(d, CONSOLE_TYPE_VCON);
>  				}
>  			}
>  
> -			d->xce_pollfd_idx = d->master_pollfd_idx = -1;
> +			d->xce_pollfd_idx = d->master_pollfd_idx[CONSOLE_TYPE_PV] =
> +									d->master_pollfd_idx[CONSOLE_TYPE_VCON] = -1;
>  

Indentation and line too long.

>  			if (d->last_seen != enum_pass)
>  				shutdown_domain(d);
> -- 
> 2.7.4
>
Bhupinder Thakur April 24, 2017, 2:52 p.m. | #2
Hi Wei Liu,

Thanks for your comments.

On 12 April 2017 at 22:03, Wei Liu <wei.liu2@citrix.com> wrote:
> On Mon, Apr 03, 2017 at 03:14:31PM +0530, Bhupinder Thakur wrote:
>> Xenconsole supports only PV console currently. To get access to emulated pl011
>> uart another backend console is required.
>>
>> This patch modifies different data structures and APIs used
>> in xenconsole to support two console types: PV and VCON (virtual console).
>>
>> Change summary:
>>
>> 1. Modify the domain structure to support two console types: PV and a
>>    virtual console (VCON).
>>
>> 2. Modify different APIs such as buffer_append() to take a new parameter
>>    console_type as input and operate on the data structures indexed by
>>    the console_type.
>>
>> 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 VCON
>>    events.
>>
>> 5. Add a new log_file for VCON console logs.
>>
>
> It seems that this patch and previous one should be merged into one.
>
Yes I have merged this patch with the earlier one to maintain the bisectability.

> There are a lot of coding style issues, please fix all of them.
>
I will fix the coding style issues.

> The code looks rather repetitive unfortunately. I believe a lot of the
> repetitiveness can be avoided by using loops and pointers.
>
Stefano suggested to define an array of a console structure instead of
using different arrays for console related fields in the domain
structure.
I think that way the changes to the code will be much cleaner. I will
incorporate those changes.

>>  static void domain_close_tty(struct domain *dom)
>>  {
>> -     if (dom->master_fd != -1) {
>> -             close(dom->master_fd);
>> -             dom->master_fd = -1;
>> +     if (dom->master_fd[CONSOLE_TYPE_PV] != -1) {
>> +             close(dom->master_fd[CONSOLE_TYPE_PV]);
>> +             dom->master_fd[CONSOLE_TYPE_PV] = -1;
>> +     }
>> +
>> +     if (dom->slave_fd[CONSOLE_TYPE_PV] != -1) {
>> +             close(dom->slave_fd[CONSOLE_TYPE_PV]);
>> +             dom->slave_fd[CONSOLE_TYPE_PV] = -1;
>> +     }
>> +
>> +     if (dom->master_fd[CONSOLE_TYPE_VCON] != -1) {
>> +             close(dom->master_fd[CONSOLE_TYPE_VCON]);
>> +             dom->master_fd[CONSOLE_TYPE_VCON] = -1;
>>       }
>>
>> -     if (dom->slave_fd != -1) {
>> -             close(dom->slave_fd);
>> -             dom->slave_fd = -1;
>> +     if (dom->slave_fd[CONSOLE_TYPE_VCON] != -1) {
>> +             close(dom->slave_fd[CONSOLE_TYPE_VCON]);
>> +             dom->slave_fd[CONSOLE_TYPE_VCON] = -1;
>>       }
>
> You can use two loops to avoid repetitive snippets. But I'm fine with
> the code as-is, too.
I have added a new function console_close_tty() which is called for
both pv and vuart consoles. The console
is abstracted as a separate structure.

static void console_close_tty(struct console *con)
{
    if (con->master_fd != -1) {
        close(con->master_fd);
        con->master_fd = -1;
    }

    if (con->slave_fd != -1) {
        close(con->slave_fd);
        con->slave_fd = -1;
    }
}

Other functions such as buffer_append(), handle_tty_read() will also
operate on the console structure.

>> @@ -1166,6 +1346,16 @@ void handle_io(void)
>>
>>               for (d = dom_head; d; d = n) {
>>                       n = d->next;
>> +
>> +                     /*
>> +                      * Check if the data pending flag is set for any of the consoles.
>> +                      * If yes then service those first.
>> +                      */
>> +                     if ( d->console_data_pending & (1<<CONSOLE_TYPE_PV) )
>> +                             buffer_append(d, CONSOLE_TYPE_PV);
>> +                     else if ( d->console_data_pending & (1<<CONSOLE_TYPE_VCON) )
>> +                             buffer_append(d, CONSOLE_TYPE_VCON);
>> +
>
> Why? You seem to have skipped the ratelimit check here.

I have thought about this change and I think this check is not
required. The rate limit check will ensure that more data is not
appended to the buffer as it will keep the events masked and
handle_ring_read() will not be called. I will remove this check.

Regards,
Bhupinder

Patch

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 0cd1fee..b9be5a5 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -164,11 +164,39 @@  static int write_with_timestamp(int fd, const char *data, size_t sz,
 	return 0;
 }
 
-static void buffer_append(struct domain *dom)
+/*
+ * This function checks if the data is allowed to be buffered for that console.
+ * If not then it marks it pending for later receiving.
+ */
+static bool buffer_available(struct domain *dom, int console_type)
+{
+	if (discard_overflowed_data ||
+		!dom->buffer[console_type].max_capacity ||
+		dom->buffer[console_type].size < dom->buffer[console_type].max_capacity)
+	{
+		dom->console_data_pending &= ~(1<<console_type);
+		return true;
+	}
+	else
+	{
+		dom->console_data_pending |= (1<<console_type);
+		return false;
+	}
+}
+
+static void buffer_append(struct domain *dom, int console_type)
 {
-	struct buffer *buffer = &dom->buffer;
+	struct buffer *buffer = &dom->buffer[console_type];
+	struct xencons_interface *intf=dom->interface[console_type];
+	xenevtchn_port_or_error_t port=dom->local_port[console_type];
+
 	XENCONS_RING_IDX cons, prod, size;
-	struct xencons_interface *intf = dom->interface;
+
+	/*
+	 * check if more data is allowed to be buffered
+	 */
+	if (!buffer_available(dom, console_type))
+		return;
 
 	cons = intf->out_cons;
 	prod = intf->out_prod;
@@ -193,22 +221,22 @@  static void buffer_append(struct domain *dom)
 
 	xen_mb();
 	intf->out_cons = cons;
-	xenevtchn_notify(dom->xce_handle, dom->local_port);
+	xenevtchn_notify(dom->xce_handle, port);
 
 	/* Get the data to the logfile as early as possible because if
 	 * no one is listening on the console pty then it will fill up
 	 * and handle_tty_write will stop being called.
 	 */
-	if (dom->log_fd != -1) {
+	if (dom->log_fd[console_type] != -1) {
 		int logret;
 		if (log_time_guest) {
 			logret = write_with_timestamp(
-				dom->log_fd,
+				dom->log_fd[console_type],
 				buffer->data + buffer->size - size,
 				size, &log_time_guest_needts);
 		} else {
 			logret = write_all(
-				dom->log_fd,
+				dom->log_fd[console_type],
 				buffer->data + buffer->size - size,
 				size);
 		}
@@ -296,12 +324,13 @@  static int create_hv_log(void)
 	return fd;
 }
 
-static int create_domain_log(struct domain *dom)
+static int create_domain_log(struct domain *dom, int console_type)
 {
 	char logfile[PATH_MAX];
 	char *namepath, *data, *s;
 	int fd;
 	unsigned int len;
+	char *console_name[]={"pv", "vcon"};
 
 	namepath = xs_get_domain_path(xs, dom->domid);
 	s = realloc(namepath, strlen(namepath) + 6);
@@ -320,7 +349,7 @@  static int create_domain_log(struct domain *dom)
 		return -1;
 	}
 
-	snprintf(logfile, PATH_MAX-1, "%s/guest-%s.log", log_dir, data);
+	snprintf(logfile, PATH_MAX-1, "%s/guest-%s-%s.log", log_dir, console_name[console_type], data);
 	free(data);
 	logfile[PATH_MAX-1] = '\0';
 
@@ -344,14 +373,24 @@  static int create_domain_log(struct domain *dom)
 
 static void domain_close_tty(struct domain *dom)
 {
-	if (dom->master_fd != -1) {
-		close(dom->master_fd);
-		dom->master_fd = -1;
+	if (dom->master_fd[CONSOLE_TYPE_PV] != -1) {
+		close(dom->master_fd[CONSOLE_TYPE_PV]);
+		dom->master_fd[CONSOLE_TYPE_PV] = -1;
+	}
+
+	if (dom->slave_fd[CONSOLE_TYPE_PV] != -1) {
+		close(dom->slave_fd[CONSOLE_TYPE_PV]);
+		dom->slave_fd[CONSOLE_TYPE_PV] = -1;
+	}
+
+	if (dom->master_fd[CONSOLE_TYPE_VCON] != -1) {
+		close(dom->master_fd[CONSOLE_TYPE_VCON]);
+		dom->master_fd[CONSOLE_TYPE_VCON] = -1;
 	}
 
-	if (dom->slave_fd != -1) {
-		close(dom->slave_fd);
-		dom->slave_fd = -1;
+	if (dom->slave_fd[CONSOLE_TYPE_VCON] != -1) {
+		close(dom->slave_fd[CONSOLE_TYPE_VCON]);
+		dom->slave_fd[CONSOLE_TYPE_VCON] = -1;
 	}
 }
 
@@ -424,63 +463,86 @@  static int domain_create_tty(struct domain *dom)
 	char *data;
 	unsigned int len;
 	struct termios term;
+	int console_type=0;
 
-	assert(dom->slave_fd == -1);
-	assert(dom->master_fd == -1);
+	assert(dom->master_fd[CONSOLE_TYPE_PV] == -1);
+	assert(dom->slave_fd[CONSOLE_TYPE_PV] == -1);
+	assert(dom->master_fd[CONSOLE_TYPE_VCON] == -1);
+	assert(dom->slave_fd[CONSOLE_TYPE_VCON] == -1);
 
-	if (openpty(&dom->master_fd, &dom->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;
-	}
+	/*
+	 * Open pty for both PV and vcon consoles.
+	 */
+	for (console_type=0; console_type<MAX_CONSOLE; console_type++)
+	{
+		if (openpty(&dom->master_fd[console_type], &dom->slave_fd[console_type], 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;
+		}
 
-	if (tcgetattr(dom->slave_fd, &term) < 0) {
-		err = errno;
-		dolog(LOG_ERR, "Failed to get tty attributes for domain-%d "
-			"(errno = %i, %s)",
-			dom->domid, err, strerror(err));
-		goto out;
-	}
-	cfmakeraw(&term);
-	if (tcsetattr(dom->slave_fd, TCSANOW, &term) < 0) {
-		err = errno;
-		dolog(LOG_ERR, "Failed to set tty attributes for domain-%d "
-			"(errno = %i, %s)",
-			dom->domid, err, strerror(err));
-		goto out;
-	}
+		if (tcgetattr(dom->slave_fd[console_type], &term) < 0) {
+			err = errno;
+			dolog(LOG_ERR, "Failed to get tty attributes for domain-%d "
+				"(errno = %i, %s)",
+				dom->domid, err, strerror(err));
+			goto out;
+		}
+		cfmakeraw(&term);
+		if (tcsetattr(dom->slave_fd[console_type], TCSANOW, &term) < 0) {
+			err = errno;
+			dolog(LOG_ERR, "Failed to set tty attributes for domain-%d "
+				"(errno = %i, %s)",
+				dom->domid, err, strerror(err));
+			goto out;
+		}
 
-	if ((slave = ptsname(dom->master_fd)) == NULL) {
-		err = errno;
-		dolog(LOG_ERR, "Failed to get slave name for domain-%d "
-		      "(errno = %i, %s)",
-		      dom->domid, err, strerror(err));
-		goto out;
-	}
+		if ((slave = ptsname(dom->master_fd[console_type])) == NULL) {
+			err = errno;
+			dolog(LOG_ERR, "Failed to get slave name for domain-%d "
+				  "(errno = %i, %s)",
+				  dom->domid, err, strerror(err));
+			goto out;
+		}
 
-	success = asprintf(&path, "%s/limit", dom->conspath) !=
-		-1;
-	if (!success)
-		goto out;
-	data = xs_read(xs, XBT_NULL, path, &len);
-	if (data) {
-		dom->buffer.max_capacity = strtoul(data, 0, 0);
-		free(data);
+		/*
+		 * Initialize the console buffer capacity.
+		 */
+		success = asprintf(&path, "%s/limit", dom->conspath) !=
+			-1;
+		if (!success)
+			goto out;
+		data = xs_read(xs, XBT_NULL, path, &len);
+		if (data) {
+			dom->buffer[console_type].max_capacity = strtoul(data, 0, 0);
+			free(data);
+		}
+		free(path);
+
+		/*
+		 * Write console slave name to xen store.
+		 */
+		if (console_type == CONSOLE_TYPE_VCON)
+			success = (asprintf(&path, "%s/vtty", dom->conspath) != -1);
+		else
+			success = (asprintf(&path, "%s/tty", dom->conspath) != -1);
+
+		if (!success)
+			goto out;
+		success = xs_write(xs, XBT_NULL, path, slave, strlen(slave));
+		free(path);
+
+		if (fcntl(dom->master_fd[console_type], F_SETFL, O_NONBLOCK) == -1)
+			goto out;
+
+		if (!dom->vcon_enabled)
+			break;
 	}
-	free(path);
 
-	success = (asprintf(&path, "%s/tty", dom->conspath) != -1);
 	if (!success)
 		goto out;
-	success = xs_write(xs, XBT_NULL, path, slave, strlen(slave));
-	free(path);
-	if (!success)
-		goto out;
-
-	if (fcntl(dom->master_fd, F_SETFL, O_NONBLOCK) == -1)
-		goto out;
 
 	return 1;
 out:
@@ -523,21 +585,53 @@  static int xs_gather(struct xs_handle *xs, const char *dir, ...)
 	return ret;
 }
 
-static void domain_unmap_interface(struct domain *dom)
+static void domain_unmap_interface(struct domain *dom, int console_type)
 {
-	if (dom->interface == NULL)
+	if (dom->interface[console_type] == NULL)
 		return;
-	if (xgt_handle && dom->ring_ref == -1)
-		xengnttab_unmap(xgt_handle, dom->interface, 1);
+	if (xgt_handle && dom->ring_ref[console_type] == -1)
+		xengnttab_unmap(xgt_handle, dom->interface[console_type], 1);
 	else
-		munmap(dom->interface, XC_PAGE_SIZE);
-	dom->interface = NULL;
-	dom->ring_ref = -1;
+		munmap(dom->interface[console_type], XC_PAGE_SIZE);
+	dom->interface[console_type] = NULL;
+	dom->ring_ref[console_type] = -1;
+}
+
+static int bind_event_channel(struct domain *dom, int new_rport, int *lport, int *rport)
+{
+	int err = 0, rc;
+
+	/* Go no further if port has not changed and we are still bound. */
+	if (new_rport == *rport) {
+		xc_evtchn_status_t status = {
+		.dom = DOMID_SELF,
+		.port = *lport };
+		if ((xc_evtchn_status(xc, &status) == 0) &&
+			(status.status == EVTCHNSTAT_interdomain))
+			goto out;
+	}
+
+	*lport = -1;
+	*rport = -1;
+	rc = xenevtchn_bind_interdomain(dom->xce_handle,
+									dom->domid, new_rport);
+
+	if (rc == -1) {
+		err = errno;
+		xenevtchn_close(dom->xce_handle);
+		dom->xce_handle = NULL;
+		goto out;
+	}
+
+	*lport = rc;
+	*rport = new_rport;
+out:
+	return err;
 }
  
 static int domain_create_ring(struct domain *dom)
 {
-	int err, remote_port, ring_ref, rc;
+	int err, remote_port, ring_ref, vcon_remote_port, vcon_ring_ref;
 	char *type, path[PATH_MAX];
 
 	err = xs_gather(xs, dom->conspath,
@@ -547,6 +641,17 @@  static int domain_create_ring(struct domain *dom)
 	if (err)
 		goto out;
 
+	/* vcon console is optional. */
+	err = xs_gather(xs, dom->conspath,
+		"vcon-ring-ref", "%u", &vcon_ring_ref,
+		"vcon-port", "%i", &vcon_remote_port,
+		NULL);
+
+	if (err || vcon_ring_ref == -1)
+		dom->vcon_enabled = false;
+	else
+		dom->vcon_enabled = true;
+
 	snprintf(path, sizeof(path), "%s/type", dom->conspath);
 	type = xs_read(xs, XBT_NULL, path, NULL);
 	if (type && strcmp(type, "xenconsoled") != 0) {
@@ -556,41 +661,51 @@  static int domain_create_ring(struct domain *dom)
 	free(type);
 
 	/* If using ring_ref and it has changed, remap */
-	if (ring_ref != dom->ring_ref && dom->ring_ref != -1)
-		domain_unmap_interface(dom);
+	if (ring_ref != dom->ring_ref[CONSOLE_TYPE_PV] && dom->ring_ref[CONSOLE_TYPE_PV] != -1)
+		domain_unmap_interface(dom, CONSOLE_TYPE_PV);
+
+	/* If using vcon ring_ref and it has changed, remap */
+	if (dom->vcon_enabled &&
+		vcon_ring_ref != dom->ring_ref[CONSOLE_TYPE_VCON] &&
+		dom->ring_ref[CONSOLE_TYPE_VCON] != -1 )
+		domain_unmap_interface(dom, CONSOLE_TYPE_VCON);
 
-	if (!dom->interface && xgt_handle) {
+	if (!dom->interface[CONSOLE_TYPE_PV] && xgt_handle) {
 		/* Prefer using grant table */
-		dom->interface = xengnttab_map_grant_ref(xgt_handle,
-			dom->domid, GNTTAB_RESERVED_CONSOLE,
-			PROT_READ|PROT_WRITE);
-		dom->ring_ref = -1;
+		dom->interface[CONSOLE_TYPE_PV] = xengnttab_map_grant_ref(xgt_handle,
+																  dom->domid, GNTTAB_RESERVED_CONSOLE,
+																  PROT_READ|PROT_WRITE);
+		dom->ring_ref[CONSOLE_TYPE_PV] = -1;
 	}
-	if (!dom->interface) {
+
+	if (!dom->interface[CONSOLE_TYPE_PV]) {
 		/* Fall back to xc_map_foreign_range */
-		dom->interface = xc_map_foreign_range(
+		dom->interface[CONSOLE_TYPE_PV] = xc_map_foreign_range(
 			xc, dom->domid, XC_PAGE_SIZE,
 			PROT_READ|PROT_WRITE,
 			(unsigned long)ring_ref);
-		if (dom->interface == NULL) {
+		if (dom->interface[CONSOLE_TYPE_PV] == NULL) {
 			err = EINVAL;
 			goto out;
 		}
-		dom->ring_ref = ring_ref;
+		dom->ring_ref[CONSOLE_TYPE_PV] = ring_ref;
 	}
 
-	/* Go no further if port has not changed and we are still bound. */
-	if (remote_port == dom->remote_port) {
-		xc_evtchn_status_t status = {
-			.dom = DOMID_SELF,
-			.port = dom->local_port };
-		if ((xc_evtchn_status(xc, &status) == 0) &&
-		    (status.status == EVTCHNSTAT_interdomain))
+	/* Map vcon console ring buffer. */
+	if (dom->vcon_enabled && !dom->interface[CONSOLE_TYPE_VCON]) {
+
+		/* Fall back to xc_map_foreign_range */
+		dom->interface[CONSOLE_TYPE_VCON] = xc_map_foreign_range(
+		xc, dom->domid, XC_PAGE_SIZE,
+		PROT_READ|PROT_WRITE,
+		(unsigned long)vcon_ring_ref);
+		if ( dom->interface[CONSOLE_TYPE_VCON] == NULL ) {
+			err = EINVAL;
 			goto out;
+		}
+		dom->ring_ref[CONSOLE_TYPE_VCON] = vcon_ring_ref;
 	}
 
-	dom->local_port = -1;
-	dom->remote_port = -1;
 	if (dom->xce_handle != NULL)
 		xenevtchn_close(dom->xce_handle);
 
@@ -602,31 +717,46 @@  static int domain_create_ring(struct domain *dom)
 		goto out;
 	}
  
-	rc = xenevtchn_bind_interdomain(dom->xce_handle,
-		dom->domid, remote_port);
-
-	if (rc == -1) {
-		err = errno;
+	err = bind_event_channel(dom, remote_port,
+							 &dom->local_port[CONSOLE_TYPE_PV],
+							 &dom->remote_port[CONSOLE_TYPE_PV]);
+	if (err)
+	{
 		xenevtchn_close(dom->xce_handle);
-		dom->xce_handle = NULL;
 		goto out;
 	}
-	dom->local_port = rc;
-	dom->remote_port = remote_port;
 
-	if (dom->master_fd == -1) {
+	if (dom->vcon_enabled)
+	{
+		err = bind_event_channel(dom, vcon_remote_port,
+								 &dom->local_port[CONSOLE_TYPE_VCON],
+								 &dom->remote_port[CONSOLE_TYPE_VCON]);
+		if (err)
+		{
+			xenevtchn_close(dom->xce_handle);
+			goto out;
+		}
+	}
+
+	if (dom->master_fd[CONSOLE_TYPE_PV] == -1) {
 		if (!domain_create_tty(dom)) {
 			err = errno;
 			xenevtchn_close(dom->xce_handle);
 			dom->xce_handle = NULL;
-			dom->local_port = -1;
-			dom->remote_port = -1;
+			dom->local_port[CONSOLE_TYPE_PV] = -1;
+			dom->remote_port[CONSOLE_TYPE_PV] = -1;
+			dom->local_port[CONSOLE_TYPE_VCON] = -1;
+			dom->remote_port[CONSOLE_TYPE_VCON] = -1;
+			dom->vcon_enabled = false;
 			goto out;
 		}
 	}
 
-	if (log_guest && (dom->log_fd == -1))
-		dom->log_fd = create_domain_log(dom);
+	if (log_guest && (dom->log_fd[CONSOLE_TYPE_PV] == -1))
+		dom->log_fd[CONSOLE_TYPE_PV] = create_domain_log(dom, CONSOLE_TYPE_PV);
+
+	if (log_guest && dom->vcon_enabled && (dom->log_fd[CONSOLE_TYPE_VCON] == -1))
+		dom->log_fd[CONSOLE_TYPE_VCON] = create_domain_log(dom, CONSOLE_TYPE_VCON);
 
  out:
 	return err;
@@ -681,17 +811,24 @@  static struct domain *create_domain(int domid)
 	dom->conspath = s;
 	strcat(dom->conspath, "/console");
 
-	dom->master_fd = -1;
-	dom->master_pollfd_idx = -1;
-	dom->slave_fd = -1;
-	dom->log_fd = -1;
+	dom->master_fd[CONSOLE_TYPE_PV] = -1;
+	dom->master_pollfd_idx[CONSOLE_TYPE_PV] = -1;
+	dom->slave_fd[CONSOLE_TYPE_PV] = -1;
+	dom->master_fd[CONSOLE_TYPE_VCON] = -1;
+	dom->master_pollfd_idx[CONSOLE_TYPE_VCON] = -1;
+	dom->slave_fd[CONSOLE_TYPE_VCON] = -1;
+	dom->log_fd[CONSOLE_TYPE_PV] = -1;
+	dom->log_fd[CONSOLE_TYPE_VCON] = -1;
 	dom->xce_pollfd_idx = -1;
 
 	dom->next_period = ((long long)ts.tv_sec * 1000) + (ts.tv_nsec / 1000000) + RATE_LIMIT_PERIOD;
 
-	dom->ring_ref = -1;
-	dom->local_port = -1;
-	dom->remote_port = -1;
+	dom->ring_ref[CONSOLE_TYPE_PV] = -1;
+	dom->ring_ref[CONSOLE_TYPE_VCON] = -1;
+	dom->local_port[CONSOLE_TYPE_PV] = -1;
+	dom->remote_port[CONSOLE_TYPE_PV] = -1;
+	dom->local_port[CONSOLE_TYPE_VCON] = -1;
+	dom->remote_port[CONSOLE_TYPE_VCON] = -1;
 
 	if (!watch_domain(dom, true))
 		goto out;
@@ -737,13 +874,21 @@  static void cleanup_domain(struct domain *d)
 {
 	domain_close_tty(d);
 
-	if (d->log_fd != -1) {
-		close(d->log_fd);
-		d->log_fd = -1;
+	if (d->log_fd[CONSOLE_TYPE_PV] != -1) {
+		close(d->log_fd[CONSOLE_TYPE_PV]);
+		d->log_fd[CONSOLE_TYPE_PV] = -1;
+	}
+
+	if (d->log_fd[CONSOLE_TYPE_VCON] != -1) {
+		close(d->log_fd[CONSOLE_TYPE_VCON]);
+		d->log_fd[CONSOLE_TYPE_VCON] = -1;
 	}
 
-	free(d->buffer.data);
-	d->buffer.data = NULL;
+	free(d->buffer[CONSOLE_TYPE_PV].data);
+	free(d->buffer[CONSOLE_TYPE_VCON].data);
+
+	d->buffer[CONSOLE_TYPE_PV].data = NULL;
+	d->buffer[CONSOLE_TYPE_VCON].data = NULL;
 
 	free(d->conspath);
 	d->conspath = NULL;
@@ -755,7 +900,8 @@  static void shutdown_domain(struct domain *d)
 {
 	d->is_dead = true;
 	watch_domain(d, false);
-	domain_unmap_interface(d);
+	domain_unmap_interface(d, CONSOLE_TYPE_PV);
+	domain_unmap_interface(d, CONSOLE_TYPE_VCON);
 	if (d->xce_handle != NULL)
 		xenevtchn_close(d->xce_handle);
 	d->xce_handle = NULL;
@@ -786,9 +932,9 @@  static void enum_domains(void)
 	}
 }
 
-static int ring_free_bytes(struct domain *dom)
+static int ring_free_bytes(struct domain *dom, int console_type)
 {
-	struct xencons_interface *intf = dom->interface;
+	struct xencons_interface *intf = dom->interface[console_type];
 	XENCONS_RING_IDX cons, prod, space;
 
 	cons = intf->in_cons;
@@ -813,25 +959,26 @@  static void domain_handle_broken_tty(struct domain *dom, int recreate)
 	}
 }
 
-static void handle_tty_read(struct domain *dom)
+static void handle_tty_read(struct domain *dom, int console_type)
 {
 	ssize_t len = 0;
 	char msg[80];
 	int i;
-	struct xencons_interface *intf = dom->interface;
 	XENCONS_RING_IDX prod;
+	struct xencons_interface *intf=dom->interface[console_type];
+	xenevtchn_port_or_error_t port=dom->local_port[console_type];
 
 	if (dom->is_dead)
 		return;
 
-	len = ring_free_bytes(dom);
+	len = ring_free_bytes(dom, console_type);
 	if (len == 0)
 		return;
 
 	if (len > sizeof(msg))
 		len = sizeof(msg);
 
-	len = read(dom->master_fd, msg, len);
+	len = read(dom->master_fd[console_type], msg, len);
 	/*
 	 * Note: on Solaris, len == 0 means the slave closed, and this
 	 * is no problem, but Linux can't handle this usefully, so we
@@ -847,28 +994,29 @@  static void handle_tty_read(struct domain *dom)
 		}
 		xen_wmb();
 		intf->in_prod = prod;
-		xenevtchn_notify(dom->xce_handle, dom->local_port);
+		xenevtchn_notify(dom->xce_handle, port);
 	} else {
 		domain_close_tty(dom);
 		shutdown_domain(dom);
 	}
 }
 
-static void handle_tty_write(struct domain *dom)
+static void handle_tty_write(struct domain *dom, int console_type)
 {
 	ssize_t len;
 
 	if (dom->is_dead)
 		return;
 
-	len = write(dom->master_fd, dom->buffer.data + dom->buffer.consumed,
-		    dom->buffer.size - dom->buffer.consumed);
+	len = write(dom->master_fd[console_type],
+				dom->buffer[console_type].data + dom->buffer[console_type].consumed,
+				dom->buffer[console_type].size - dom->buffer[console_type].consumed);
  	if (len < 1) {
 		dolog(LOG_DEBUG, "Write failed on domain %d: %zd, %d\n",
 		      dom->domid, len, errno);
 		domain_handle_broken_tty(dom, domain_is_valid(dom->domid));
 	} else {
-		buffer_advance(&dom->buffer, len);
+		buffer_advance(&dom->buffer[console_type], len);
 	}
 }
 
@@ -884,7 +1032,10 @@  static void handle_ring_read(struct domain *dom)
 
 	dom->event_count++;
 
-	buffer_append(dom);
+	if (port == dom->local_port[CONSOLE_TYPE_VCON])
+		buffer_append(dom, CONSOLE_TYPE_VCON);
+	else
+		buffer_append(dom, CONSOLE_TYPE_PV);
 
 	if (dom->event_count < RATE_LIMIT_ALLOWANCE)
 		(void)xenevtchn_unmask(dom->xce_handle, port);
@@ -954,9 +1105,16 @@  static void handle_log_reload(void)
 	if (log_guest) {
 		struct domain *d;
 		for (d = dom_head; d; d = d->next) {
-			if (d->log_fd != -1)
-				close(d->log_fd);
-			d->log_fd = create_domain_log(d);
+			if (d->log_fd[CONSOLE_TYPE_PV] != -1)
+				close(d->log_fd[CONSOLE_TYPE_PV]);
+			d->log_fd[CONSOLE_TYPE_PV] = create_domain_log(d, CONSOLE_TYPE_PV);
+
+			if (d->vcon_enabled)
+			{
+				if (d->log_fd[CONSOLE_TYPE_VCON] != -1)
+					close(d->log_fd[CONSOLE_TYPE_VCON]);
+				d->log_fd[CONSOLE_TYPE_PV] = create_domain_log(d, CONSOLE_TYPE_VCON);
+			}
 		}
 	}
 
@@ -1074,7 +1232,9 @@  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, d->local_port);
+					(void)xenevtchn_unmask(d->xce_handle, d->local_port[CONSOLE_TYPE_PV]);
+					if (d->vcon_enabled)
+						(void)xenevtchn_unmask(d->xce_handle, d->local_port[CONSOLE_TYPE_VCON]);
 				}
 				d->event_count = 0;
 			}
@@ -1087,27 +1247,47 @@  void handle_io(void)
 				    d->next_period < next_timeout)
 					next_timeout = d->next_period;
 			} else if (d->xce_handle != NULL) {
-				if (discard_overflowed_data ||
-				    !d->buffer.max_capacity ||
-				    d->buffer.size < d->buffer.max_capacity) {
+
+				if (buffer_available(d, CONSOLE_TYPE_PV)) {
+					int evtchn_fd = xenevtchn_fd(d->xce_handle);
+					d->xce_pollfd_idx = set_fds(evtchn_fd,
+												POLLIN|POLLPRI);
+				}
+
+				if (buffer_available(d, CONSOLE_TYPE_VCON ) )
+				{
 					int evtchn_fd = xenevtchn_fd(d->xce_handle);
 					d->xce_pollfd_idx = set_fds(evtchn_fd,
-								    POLLIN|POLLPRI);
+												POLLIN|POLLPRI);
 				}
 			}
 
-			if (d->master_fd != -1) {
+			if (d->master_fd[CONSOLE_TYPE_PV] != -1) {
 				short events = 0;
-				if (!d->is_dead && ring_free_bytes(d))
+				if (!d->is_dead && ring_free_bytes(d, CONSOLE_TYPE_PV))
 					events |= POLLIN;
 
-				if (!buffer_empty(&d->buffer))
+				if (!buffer_empty(&d->buffer[CONSOLE_TYPE_PV]))
 					events |= POLLOUT;
 
 				if (events)
-					d->master_pollfd_idx =
-						set_fds(d->master_fd,
-							events|POLLPRI);
+					d->master_pollfd_idx[CONSOLE_TYPE_PV] =
+						set_fds(d->master_fd[CONSOLE_TYPE_PV],
+								events|POLLPRI);
+			}
+
+			if (d->vcon_enabled && d->master_fd[CONSOLE_TYPE_VCON] != -1) {
+				short events = 0;
+				if (!d->is_dead && ring_free_bytes(d, CONSOLE_TYPE_VCON))
+					events |= POLLIN;
+
+				if (!buffer_empty(&d->buffer[CONSOLE_TYPE_VCON]))
+					events |= POLLOUT;
+
+				if (events)
+					d->master_pollfd_idx[CONSOLE_TYPE_VCON] =
+						set_fds(d->master_fd[CONSOLE_TYPE_VCON],
+								events|POLLPRI);
 			}
 		}
 
@@ -1166,6 +1346,16 @@  void handle_io(void)
 
 		for (d = dom_head; d; d = n) {
 			n = d->next;
+
+			/*
+			 * Check if the data pending flag is set for any of the consoles.
+			 * If yes then service those first.
+			 */
+			if ( d->console_data_pending & (1<<CONSOLE_TYPE_PV) )
+				buffer_append(d, CONSOLE_TYPE_PV);
+			else if ( d->console_data_pending & (1<<CONSOLE_TYPE_VCON) )
+				buffer_append(d, CONSOLE_TYPE_VCON);
+
 			if (d->event_count < RATE_LIMIT_ALLOWANCE) {
 				if (d->xce_handle != NULL &&
 				    d->xce_pollfd_idx != -1 &&
@@ -1176,22 +1366,36 @@  void handle_io(void)
 				    handle_ring_read(d);
 			}
 
-			if (d->master_fd != -1 && d->master_pollfd_idx != -1) {
-				if (fds[d->master_pollfd_idx].revents &
+			if (d->master_fd[CONSOLE_TYPE_PV] != -1 && d->master_pollfd_idx[CONSOLE_TYPE_PV] != -1) {
+				if (fds[d->master_pollfd_idx[CONSOLE_TYPE_PV]].revents &
+				    ~(POLLIN|POLLOUT|POLLPRI))
+					domain_handle_broken_tty(d, domain_is_valid(d->domid));
+				else {
+					if (fds[d->master_pollfd_idx[CONSOLE_TYPE_PV]].revents &
+					    POLLIN)
+						handle_tty_read(d, CONSOLE_TYPE_PV);
+					if (fds[d->master_pollfd_idx[CONSOLE_TYPE_PV]].revents &
+					    POLLOUT)
+						handle_tty_write(d, CONSOLE_TYPE_PV);
+				}
+			}
+
+			if (d->master_fd[CONSOLE_TYPE_VCON] != -1 && d->master_pollfd_idx[CONSOLE_TYPE_VCON] != -1) {
+				if (fds[d->master_pollfd_idx[CONSOLE_TYPE_VCON]].revents &
 				    ~(POLLIN|POLLOUT|POLLPRI))
-					domain_handle_broken_tty(d,
-						   domain_is_valid(d->domid));
+					domain_handle_broken_tty(d, domain_is_valid(d->domid));
 				else {
-					if (fds[d->master_pollfd_idx].revents &
+					if (fds[d->master_pollfd_idx[CONSOLE_TYPE_VCON]].revents &
 					    POLLIN)
-						handle_tty_read(d);
-					if (fds[d->master_pollfd_idx].revents &
+						handle_tty_read(d, CONSOLE_TYPE_VCON);
+					if (fds[d->master_pollfd_idx[CONSOLE_TYPE_VCON]].revents &
 					    POLLOUT)
-						handle_tty_write(d);
+						handle_tty_write(d, CONSOLE_TYPE_VCON);
 				}
 			}
 
-			d->xce_pollfd_idx = d->master_pollfd_idx = -1;
+			d->xce_pollfd_idx = d->master_pollfd_idx[CONSOLE_TYPE_PV] =
+									d->master_pollfd_idx[CONSOLE_TYPE_VCON] = -1;
 
 			if (d->last_seen != enum_pass)
 				shutdown_domain(d);