[PATCH] xenconsoled: use grant references instead of map_foreign_range

[PATCH] xenconsoled: use grant references instead of map_foreign_range

[Daniel De Graaf]

Grant references for the xenstore and xenconsole shared pages exist, but
currently only xenstore uses these references.  Change the xenconsole
daemon to prefer using the grant reference over map_foreign_range when
mapping the shared console ring.

This allows xenconsoled to be run in a domain other than dom0 if set up
correctly - for libxl, the xenstore path /tool/xenconsoled/domid
specifies the domain containing xenconsoled.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/console/daemon/io.c | 45 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 48fe151..e24247d 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -24,6 +24,7 @@
 #include "io.h"
 #include <xenstore.h>
 #include <xen/io/console.h>
+#include <xen/grant_table.h>
 
 #include <stdlib.h>
 #include <errno.h>
@@ -69,6 +70,7 @@ static int log_hv_fd = -1;
 static evtchn_port_or_error_t log_hv_evtchn = -1;
 static xc_interface *xch; /* why does xenconsoled have two xc handles ? */
 static xc_evtchn *xce_handle = NULL;
+static xc_gnttab *xcg_handle = NULL;
 
 struct buffer {
 	char *data;
@@ -501,6 +503,17 @@ static int xs_gather(struct xs_handle *xs, const char *dir, ...)
 	va_end(ap);
 	return ret;
 }
+
+static void domain_unmap_interface(struct domain *dom)
+{
+	if (dom->interface == NULL)
+		return;
+	if (xcg_handle && dom->ring_ref == -1)
+		xc_gnttab_munmap(xcg_handle, dom->interface, 1);
+	else
+		munmap(dom->interface, getpagesize());
+	dom->interface = NULL;
+}
  
 static int domain_create_ring(struct domain *dom)
 {
@@ -522,9 +535,19 @@ static int domain_create_ring(struct domain *dom)
 	}
 	free(type);
 
-	if (ring_ref != dom->ring_ref) {
-		if (dom->interface != NULL)
-			munmap(dom->interface, getpagesize());
+	/* If using ring_ref and it has changed, remap */
+	if (ring_ref != dom->ring_ref && dom->ring_ref != -1)
+		domain_unmap_interface(dom);
+
+	if (!dom->interface && xcg_handle) {
+		/* Prefer using grant table */
+		dom->interface = xc_gnttab_map_grant_ref(xcg_handle,
+			dom->domid, GNTTAB_RESERVED_CONSOLE,
+			PROT_READ|PROT_WRITE);
+		dom->ring_ref = -1;
+	}
+	if (!dom->interface) {
+		/* Fall back to xc_map_foreign_range */
 		dom->interface = xc_map_foreign_range(
 			xc, dom->domid, getpagesize(),
 			PROT_READ|PROT_WRITE,
@@ -720,9 +743,7 @@ static void shutdown_domain(struct domain *d)
 {
 	d->is_dead = true;
 	watch_domain(d, false);
-	if (d->interface != NULL)
-		munmap(d->interface, getpagesize());
-	d->interface = NULL;
+	domain_unmap_interface(d);
 	if (d->xce_handle != NULL)
 		xc_evtchn_close(d->xce_handle);
 	d->xce_handle = NULL;
@@ -736,6 +757,13 @@ void enum_domains(void)
 	xc_dominfo_t dominfo;
 	struct domain *dom;
 
+	if (enum_pass == 0) {
+		xcg_handle = xc_gnttab_open(NULL, 0);
+		if (xcg_handle == NULL) {
+			dolog(LOG_DEBUG, "Failed to open xcg handle: %d (%s)",
+				  errno, strerror(errno));
+		}
+	}
 	enum_pass++;
 
 	while (xc_domain_getinfo(xc, domid, 1, &dominfo) == 1) {
@@ -946,6 +974,7 @@ void handle_io(void)
 			      errno, strerror(errno));
 			goto out;
 		}
+
 		log_hv_fd = create_hv_log();
 		if (log_hv_fd == -1)
 			goto out;
@@ -1097,6 +1126,10 @@ void handle_io(void)
 		xc_evtchn_close(xce_handle);
 		xce_handle = NULL;
 	}
+	if (xcg_handle != NULL) {
+		xc_gnttab_close(xcg_handle);
+		xcg_handle = NULL;
+	}
 	log_hv_evtchn = -1;
 }
 
-- 
1.7.11.7

Re: [PATCH] xenconsoled: use grant references instead of map_foreign_range

[Ian Campbell]

On Thu, 2012-12-13 at 16:08 +0000, Daniel De Graaf wrote:
> Grant references for the xenstore and xenconsole shared pages exist, but
> currently only xenstore uses these references.  Change the xenconsole
> daemon to prefer using the grant reference over map_foreign_range when
> mapping the shared console ring.
> 
> This allows xenconsoled to be run in a domain other than dom0 if set up
> correctly - for libxl, the xenstore path /tool/xenconsoled/domid
> specifies the domain containing xenconsoled.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/console/daemon/io.c | 45 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 48fe151..e24247d 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -24,6 +24,7 @@
>  #include "io.h"
>  #include <xenstore.h>
>  #include <xen/io/console.h>
> +#include <xen/grant_table.h>
>  
>  #include <stdlib.h>
>  #include <errno.h>
> @@ -69,6 +70,7 @@ static int log_hv_fd = -1;
>  static evtchn_port_or_error_t log_hv_evtchn = -1;
>  static xc_interface *xch; /* why does xenconsoled have two xc handles ? */
>  static xc_evtchn *xce_handle = NULL;
> +static xc_gnttab *xcg_handle = NULL;
>  
>  struct buffer {
>  	char *data;
> @@ -501,6 +503,17 @@ static int xs_gather(struct xs_handle *xs, const char *dir, ...)
>  	va_end(ap);
>  	return ret;
>  }
> +
> +static void domain_unmap_interface(struct domain *dom)
> +{
> +	if (dom->interface == NULL)
> +		return;
> +	if (xcg_handle && dom->ring_ref == -1)
> +		xc_gnttab_munmap(xcg_handle, dom->interface, 1);
> +	else
> +		munmap(dom->interface, getpagesize());
> +	dom->interface = NULL;
> +}
>   
>  static int domain_create_ring(struct domain *dom)
>  {
> @@ -522,9 +535,19 @@ static int domain_create_ring(struct domain *dom)
>  	}
>  	free(type);
>  
> -	if (ring_ref != dom->ring_ref) {
> -		if (dom->interface != NULL)
> -			munmap(dom->interface, getpagesize());
> +	/* If using ring_ref and it has changed, remap */
> +	if (ring_ref != dom->ring_ref && dom->ring_ref != -1)
> +		domain_unmap_interface(dom);
> +
> +	if (!dom->interface && xcg_handle) {
> +		/* Prefer using grant table */
> +		dom->interface = xc_gnttab_map_grant_ref(xcg_handle,
> +			dom->domid, GNTTAB_RESERVED_CONSOLE,
> +			PROT_READ|PROT_WRITE);
> +		dom->ring_ref = -1;
> +	}
> +	if (!dom->interface) {
> +		/* Fall back to xc_map_foreign_range */

If you get here and then xc_map_foreign_range fails nothing will reset
dom->ring_ref to -1, so in the case where it has changed and you are
remapping it will retain its previous value. That's going to cause
confusion if the ring ref changes again I think? Easily fixed by
resetting ring_ref in domain_unmap_interface.

Hrm, that's probably a preexisting issue now I think about it.

I notice that nothing checks the error returns from this function. Oh
well.

>  	d->xce_handle = NULL;
> @@ -736,6 +757,13 @@ void enum_domains(void)
>  	xc_dominfo_t dominfo;
>  	struct domain *dom;
>  
> +	if (enum_pass == 0) {
> +		xcg_handle = xc_gnttab_open(NULL, 0);
> +		if (xcg_handle == NULL) {
> +			dolog(LOG_DEBUG, "Failed to open xcg handle: %d (%s)",
> +				  errno, strerror(errno));
> +		}
> +	}

I think it would be preferable to open this along with the other handles
in handle_io.

Ian.

Re: [PATCH] xenconsoled: use grant references instead of map_foreign_range

[Daniel De Graaf]

On 01/10/2013 11:11 AM, Ian Campbell wrote:
> On Thu, 2012-12-13 at 16:08 +0000, Daniel De Graaf wrote:
>> Grant references for the xenstore and xenconsole shared pages exist, but
>> currently only xenstore uses these references.  Change the xenconsole
>> daemon to prefer using the grant reference over map_foreign_range when
>> mapping the shared console ring.
>>
>> This allows xenconsoled to be run in a domain other than dom0 if set up
>> correctly - for libxl, the xenstore path /tool/xenconsoled/domid
>> specifies the domain containing xenconsoled.
>>
>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> ---
>>  tools/console/daemon/io.c | 45 +++++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 39 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
>> index 48fe151..e24247d 100644
>> --- a/tools/console/daemon/io.c
>> +++ b/tools/console/daemon/io.c
>> @@ -24,6 +24,7 @@
>>  #include "io.h"
>>  #include <xenstore.h>
>>  #include <xen/io/console.h>
>> +#include <xen/grant_table.h>
>>  
>>  #include <stdlib.h>
>>  #include <errno.h>
>> @@ -69,6 +70,7 @@ static int log_hv_fd = -1;
>>  static evtchn_port_or_error_t log_hv_evtchn = -1;
>>  static xc_interface *xch; /* why does xenconsoled have two xc handles ? */
>>  static xc_evtchn *xce_handle = NULL;
>> +static xc_gnttab *xcg_handle = NULL;
>>  
>>  struct buffer {
>>  	char *data;
>> @@ -501,6 +503,17 @@ static int xs_gather(struct xs_handle *xs, const char *dir, ...)
>>  	va_end(ap);
>>  	return ret;
>>  }
>> +
>> +static void domain_unmap_interface(struct domain *dom)
>> +{
>> +	if (dom->interface == NULL)
>> +		return;
>> +	if (xcg_handle && dom->ring_ref == -1)
>> +		xc_gnttab_munmap(xcg_handle, dom->interface, 1);
>> +	else
>> +		munmap(dom->interface, getpagesize());
>> +	dom->interface = NULL;
>> +}
>>   
>>  static int domain_create_ring(struct domain *dom)
>>  {
>> @@ -522,9 +535,19 @@ static int domain_create_ring(struct domain *dom)
>>  	}
>>  	free(type);
>>  
>> -	if (ring_ref != dom->ring_ref) {
>> -		if (dom->interface != NULL)
>> -			munmap(dom->interface, getpagesize());
>> +	/* If using ring_ref and it has changed, remap */
>> +	if (ring_ref != dom->ring_ref && dom->ring_ref != -1)
>> +		domain_unmap_interface(dom);
>> +
>> +	if (!dom->interface && xcg_handle) {
>> +		/* Prefer using grant table */
>> +		dom->interface = xc_gnttab_map_grant_ref(xcg_handle,
>> +			dom->domid, GNTTAB_RESERVED_CONSOLE,
>> +			PROT_READ|PROT_WRITE);
>> +		dom->ring_ref = -1;
>> +	}
>> +	if (!dom->interface) {
>> +		/* Fall back to xc_map_foreign_range */
> 
> If you get here and then xc_map_foreign_range fails nothing will reset
> dom->ring_ref to -1, so in the case where it has changed and you are
> remapping it will retain its previous value. That's going to cause
> confusion if the ring ref changes again I think? Easily fixed by
> resetting ring_ref in domain_unmap_interface.

Yep, will add that next version.
 
> Hrm, that's probably a preexisting issue now I think about it.
>
> I notice that nothing checks the error returns from this function. Oh
> well.
> 
>>  	d->xce_handle = NULL;
>> @@ -736,6 +757,13 @@ void enum_domains(void)
>>  	xc_dominfo_t dominfo;
>>  	struct domain *dom;
>>  
>> +	if (enum_pass == 0) {
>> +		xcg_handle = xc_gnttab_open(NULL, 0);
>> +		if (xcg_handle == NULL) {
>> +			dolog(LOG_DEBUG, "Failed to open xcg handle: %d (%s)",
>> +				  errno, strerror(errno));
>> +		}
>> +	}
> 
> I think it would be preferable to open this along with the other handles
> in handle_io.
>
> Ian.
> 

That's too late: the grant table file descriptor must be open when enum_domains
is run, and main() calls enum_domains() just before it calls handle_io().

I could move that call to enum_domains() into handle_io() if that's preferred;
it seems like that would be cleaner, especially since that's where the cleanup
is located.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH] xenconsoled: use grant references instead of map_foreign_range

[Ian Campbell]

> >> +	if (!dom->interface) {
> >> +		/* Fall back to xc_map_foreign_range */
> > 
> > If you get here and then xc_map_foreign_range fails nothing will reset
> > dom->ring_ref to -1, so in the case where it has changed and you are
> > remapping it will retain its previous value. That's going to cause
> > confusion if the ring ref changes again I think? Easily fixed by
> > resetting ring_ref in domain_unmap_interface.
> 
> Yep, will add that next version.

Thanks.

> > Hrm, that's probably a preexisting issue now I think about it.
> >
> > I notice that nothing checks the error returns from this function. Oh
> > well.
> > 
> >>  	d->xce_handle = NULL;
> >> @@ -736,6 +757,13 @@ void enum_domains(void)
> >>  	xc_dominfo_t dominfo;
> >>  	struct domain *dom;
> >>  
> >> +	if (enum_pass == 0) {
> >> +		xcg_handle = xc_gnttab_open(NULL, 0);
> >> +		if (xcg_handle == NULL) {
> >> +			dolog(LOG_DEBUG, "Failed to open xcg handle: %d (%s)",
> >> +				  errno, strerror(errno));
> >> +		}
> >> +	}
> > 
> > I think it would be preferable to open this along with the other handles
> > in handle_io.
> >
> That's too late: the grant table file descriptor must be open when enum_domains
> is run, and main() calls enum_domains() just before it calls handle_io().

OK.

> I could move that call to enum_domains() into handle_io() if that's preferred;
> it seems like that would be cleaner, especially since that's where the cleanup
> is located.

Either that or moving the xc_gnttab_open and cleanup into main() would
be good I think.

>