|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 09/11] xen/arm: vpl011: Modify domain_create_ring in xenconsole to map the ring buffer and event channel
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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |