Re: [PATCH 09/11] xen/arm: vpl011: Modify domain_create_ring in xenconsole to map the ring buffer and event channel

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Feb 21, 2017 at 04:56:06PM +0530, Bhupinder Thakur wrote:
> Modfication 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
> 
> Signed-off-by: Bhupinder Thakur <bhupinder.thakur@linaro.org>
> ---
>  tools/console/daemon/io.c | 128 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 110 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 7e6a886..b1aa615 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -101,13 +101,18 @@ struct domain {
>  	struct domain *next;
>  	char *conspath;
>  	int ring_ref;
> +	int vpl011_ring_ref;
>  	xenevtchn_port_or_error_t local_port;
>  	xenevtchn_port_or_error_t remote_port;
> +	xenevtchn_port_or_error_t vpl011_local_port;
> +	xenevtchn_port_or_error_t vpl011_remote_port;
>  	xenevtchn_handle *xce_handle;
>  	int xce_pollfd_idx;
>  	struct xencons_interface *interface;
> +	struct xencons_interface *vpl011_interface;
>  	int event_count;
>  	long long next_period;
> +	bool vpl011_initialized;
>  };
>  
>  static struct domain *dom_head;
> @@ -529,9 +534,58 @@ static void domain_unmap_interface(struct domain *dom)
>  	dom->ring_ref = -1;
>  }
>   
> +static void domain_unmap_vpl011_interface(struct domain *dom)
> +{
> +	if ( dom->vpl011_interface == NULL )
> +        return;
> +
> +	if ( xgt_handle && dom->vpl011_ring_ref == -1 )
> +		xengnttab_unmap(xgt_handle, dom->vpl011_interface, 1);

This code uses its own styleguide, which means:
 - Use tabs instead of spaces. And only one here (and in the above return)
 - No spaces between ( ).


> +	else
> +		munmap(dom->vpl011_interface, XC_PAGE_SIZE);
> +	dom->vpl011_interface = NULL;
> +	dom->vpl011_ring_ref = -1;
> +}


Hm on second thought, this looks like domain_unmap_interface. Can you just
replicate it to be like it?

Or better yet. Could you change the domain_unmap_interface to
accept two parameters: a void pointer to the interface and
 a pointer to the ring_ref?

That way you could do:
 domain_unmap_interface(&dom->interface, &dom->ring_ref);
 domain_unmap_interface(&dom->vpl011_interface, &dom->vpl011_ring_ref);

And you would reuse the function?


> +
> +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 ) {

Again, no spaces here.

> +		xc_evtchn_status_t status = {
> +		.dom = DOMID_SELF,
> +		.port = *lport };

> +	if ((xc_evtchn_status(xc, &status) == 0) &&

Shouldn't this have an tab in front it? It is part of the 'if (new_rport == *rport)' conditional
after all.

> +		(status.status == EVTCHNSTAT_interdomain))
> +		goto out;
> +	}
> +
> +	/* initialize the ports */

Please remove this comment
> +	*lport = -1;
> +	*rport = -1;
> + 
> +	/* bind to new remote port */

And this one..
> +	rc = xenevtchn_bind_interdomain(dom->xce_handle,
> +	dom->domid, new_rport);

Huh? that should have been moved to be under (.

> +
> +	if ( rc == -1 ) {

Wrong style guide. No spaces.

> +		err = errno;
> +		xenevtchn_close(dom->xce_handle);
> +		dom->xce_handle = NULL;
> +		goto out;
> +	}
> +
> +	/* store new local and remote event channel ports */

Please remove this comment.

> +	*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, vpl011_remote_port, vpl011_ring_ref;
>  	char *type, path[PATH_MAX];
>  
>  	err = xs_gather(xs, dom->conspath,
> @@ -541,6 +595,20 @@ static int domain_create_ring(struct domain *dom)
>  	if (err)
>  		goto out;
>  
> +	/*
> +	* if the vpl011 parameters are not available or are not initialized
> +	* the vpl011 console is not available

I would just change this to:

/* vpl011 is optional. */

> +	*/
> +	err = xs_gather(xs, dom->conspath,
> +		"vpl011-ring-ref", "%u", &vpl011_ring_ref,
> +		"vpl011-port", "%i", &vpl011_remote_port,

How come you don't use %u for both?

> +		NULL);
> +
> +	if ( err || vpl011_ring_ref == -1 )
> +		dom->vpl011_initialized = false;
> +	else
> +		dom->vpl011_initialized = true;
> +
>  	snprintf(path, sizeof(path), "%s/type", dom->conspath);
>  	type = xs_read(xs, XBT_NULL, path, NULL);
>  	if (type && strcmp(type, "xenconsoled") != 0) {
> @@ -553,6 +621,12 @@ static int domain_create_ring(struct domain *dom)
>  	if (ring_ref != dom->ring_ref && dom->ring_ref != -1)
>  		domain_unmap_interface(dom);
>  
> +	/* If using vpl011 ring_ref and it has changed, remap */
> +	if ( dom->vpl011_initialized && 
> +		vpl011_ring_ref != dom->vpl011_ring_ref && 
> +		dom->vpl011_ring_ref != -1 )

Something is off here. Or maybe it is my editor. But I would think that the condtionals
 should be on the same line. Also, no spaces here.

> +		domain_unmap_vpl011_interface(dom);
> +
>  	if (!dom->interface && xgt_handle) {
>  		/* Prefer using grant table */
>  		dom->interface = xengnttab_map_grant_ref(xgt_handle,
> @@ -560,6 +634,8 @@ static int domain_create_ring(struct domain *dom)
>  			PROT_READ|PROT_WRITE);
>  		dom->ring_ref = -1;
>  	}
> +
> +	/* map PV console ring buffer */

?? Please remove that.
>  	if (!dom->interface) {
>  		/* Fall back to xc_map_foreign_range */
>  		dom->interface = xc_map_foreign_range(
> @@ -573,18 +649,21 @@ static int domain_create_ring(struct domain *dom)
>  		dom->ring_ref = 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 vpl011 console ring buffer */

s/map/Map/ and also add period at the end please.

> +	if ( dom->vpl011_initialized && !dom->vpl011_interface ) {
> +
> +		/* Fall back to xc_map_foreign_range */
> +		dom->vpl011_interface = xc_map_foreign_range(
> +		xc, dom->domid, XC_PAGE_SIZE,
> +		PROT_READ|PROT_WRITE,
> +		(unsigned long)vpl011_ring_ref);

What happend here? Is my editor mixed up? I would think that those
would a bit shifted over?

> +		if ( dom->vpl011_interface == NULL ) {

No spaces ..
> +			err = EINVAL;
>  			goto out;
> +		}
> +		dom->vpl011_ring_ref = vpl011_ring_ref;
>  	}
>  
> -	dom->local_port = -1;
> -	dom->remote_port = -1;
>  	if (dom->xce_handle != NULL)
>  		xenevtchn_close(dom->xce_handle);
>  
> @@ -596,17 +675,24 @@ 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;
> +	/* bind PV console channel */
> +	err = bind_event_channel(dom, remote_port, &dom->local_port, &dom->remote_port);
> +	if (err)
> +	{

{ is on the same line as if (err) 

>  		xenevtchn_close(dom->xce_handle);
> -		dom->xce_handle = NULL;

Why? We are still closing it..

>  		goto out;
>  	}
> -	dom->local_port = rc;
> -	dom->remote_port = remote_port;
> +
> +	/* bind vpl011 console channel */

Uppercase please and period.

> +	if ( dom->vpl011_initialized )
> +	{

{ is on the same line as if in this code.

> +		err = bind_event_channel(dom, vpl011_remote_port, &dom->vpl011_local_port, &dom->vpl011_remote_port);
> +		if (err)
> +		{

Ditto.
> +			xenevtchn_close(dom->xce_handle);

No xce_handle = NULL?

> +			goto out;
> +		}
> +	}
>  
>  	if (dom->master_fd == -1) {
>  		if (!domain_create_tty(dom)) {
> @@ -615,6 +701,9 @@ static int domain_create_ring(struct domain *dom)
>  			dom->xce_handle = NULL;
>  			dom->local_port = -1;
>  			dom->remote_port = -1;
> +			dom->vpl011_local_port = -1;
> +			dom->vpl011_remote_port = -1;
> +			dom->vpl011_initialized = false;
>  			goto out;
>  		}
>  	}
> @@ -684,8 +773,11 @@ static struct domain *create_domain(int domid)
>  	dom->next_period = ((long long)ts.tv_sec * 1000) + (ts.tv_nsec / 1000000) + RATE_LIMIT_PERIOD;
>  
>  	dom->ring_ref = -1;
> +	dom->vpl011_ring_ref = -1;
>  	dom->local_port = -1;
>  	dom->remote_port = -1;
> +	dom->vpl011_local_port = -1;
> +	dom->vpl011_remote_port = -1;
>  
>  	if (!watch_domain(dom, true))
>  		goto out;
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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