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

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

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