Xen devel - Jan 2013 - [PATCH v2 1/2] xenconsoled: use grant references instead of map_foreign_range

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   | 48 ++++++++++++++++++++++++++++++++++++++-------
 tools/console/daemon/io.h   |  1 -
 tools/console/daemon/main.c |  2 --
 3 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 48fe151..e1720b6 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,18 @@ 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;
+	dom->ring_ref = -1;
+}
  
 static int domain_create_ring(struct domain *dom)
 {
@@ -522,9 +536,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 +744,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;
@@ -730,7 +752,7 @@ static void shutdown_domain(struct domain *d)
 
 static unsigned enum_pass = 0;
 
-void enum_domains(void)
+static void enum_domains(void)
 {
 	int domid = 1;
 	xc_dominfo_t dominfo;
@@ -957,6 +979,14 @@ void handle_io(void)
 		}
 	}
 
+	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_domains();
+
 	for (;;) {
 		struct domain *d, *n;
 		int max_fd = -1;
@@ -1097,6 +1127,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;
 }
 
diff --git a/tools/console/daemon/io.h b/tools/console/daemon/io.h
index 8fa04b6..f658bfc 100644
--- a/tools/console/daemon/io.h
+++ b/tools/console/daemon/io.h
@@ -21,7 +21,6 @@
 #ifndef CONSOLED_IO_H
 #define CONSOLED_IO_H
 
-void enum_domains(void);
 void handle_io(void);
 
 #endif
diff --git a/tools/console/daemon/main.c b/tools/console/daemon/main.c
index 789baa6..92d2fc4 100644
--- a/tools/console/daemon/main.c
+++ b/tools/console/daemon/main.c
@@ -161,8 +161,6 @@ int main(int argc, char **argv)
 	if (!xen_setup())
 		exit(1);
 
-	enum_domains();
-
 	handle_io();
 
 	closelog();
-- 
1.7.11.7

When the console is connected to a domain other than dom0, the console
device''s backend field should be set so the xenstore permissions for
the
console device reflect the domain that will be accessing them.

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/libxl/libxl_create.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index c6daec9..a8dfe61 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -967,6 +967,7 @@ static void domcreate_launch_dm(libxl__egc *egc,
libxl__multidev *multidev,
         ret = init_console_info(&console, 0);
         if ( ret )
             goto error_out;
+        console.backend_domid = state->console_domid;
         libxl__device_console_add(gc, domid, &console, state);
         libxl__device_console_dispose(&console);
 
@@ -999,6 +1000,7 @@ static void domcreate_launch_dm(libxl__egc *egc,
libxl__multidev *multidev,
                 d_config->num_vfbs, d_config->vfbs,
                 d_config->num_disks, &d_config->disks[0]);
 
+        console.backend_domid = state->console_domid;
         libxl__device_console_add(gc, domid, &console, state);
         libxl__device_console_dispose(&console);
 
-- 
1.7.11.7

On Thu, 10 Jan 2013, 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.
Unfortunately at the moment xc_dom_gnttab_init doesn''t work with an
autotranslated dom0, that is the only type of dom0 that you get on ARM.
As a consequence this patch would break xenconsoled on ARM. I expect the
same to happen with PVH as well.

I also have the same issue with xenstored and I was thinking about
writing a patch to make it gracefully fallback to xc_map_foreign_range
if xc_gnttab_map_grant_ref fails.

The reason why xc_dom_gnttab_init is broken is that it uses
xc_map_foreign_range to map the grant table page of the foreign domain,
that is not a regular gpfn page, therefore it fails.
Specifically on ARM it hits the case XENMAPSPACE_gmfn_foreign in
xenmem_add_to_physmap_one, then it fails the p2m lookup and returns
EINVAL.

Do you feel up for making xc_dom_gnttab_init work for an autotranslated
dom0?
Otherwise I think that for the moment is best to modify this patch to
gracefully fallback to xc_map_foreign_range. Then we need a couple of
other patches to do the same with xenstored and libxl.


> 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   | 48
++++++++++++++++++++++++++++++++++++++-------
>  tools/console/daemon/io.h   |  1 -
>  tools/console/daemon/main.c |  2 --
>  3 files changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 48fe151..e1720b6 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,18 @@ 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;
> +	dom->ring_ref = -1;
> +}
>   
>  static int domain_create_ring(struct domain *dom)
>  {
> @@ -522,9 +536,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 +744,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;
> @@ -730,7 +752,7 @@ static void shutdown_domain(struct domain *d)
>  
>  static unsigned enum_pass = 0;
>  
> -void enum_domains(void)
> +static void enum_domains(void)
>  {
>  	int domid = 1;
>  	xc_dominfo_t dominfo;
> @@ -957,6 +979,14 @@ void handle_io(void)
>  		}
>  	}
>  
> +	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_domains();
> +
>  	for (;;) {
>  		struct domain *d, *n;
>  		int max_fd = -1;
> @@ -1097,6 +1127,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;
>  }
>  
> diff --git a/tools/console/daemon/io.h b/tools/console/daemon/io.h
> index 8fa04b6..f658bfc 100644
> --- a/tools/console/daemon/io.h
> +++ b/tools/console/daemon/io.h
> @@ -21,7 +21,6 @@
>  #ifndef CONSOLED_IO_H
>  #define CONSOLED_IO_H
>  
> -void enum_domains(void);
>  void handle_io(void);
>  
>  #endif
> diff --git a/tools/console/daemon/main.c b/tools/console/daemon/main.c
> index 789baa6..92d2fc4 100644
> --- a/tools/console/daemon/main.c
> +++ b/tools/console/daemon/main.c
> @@ -161,8 +161,6 @@ int main(int argc, char **argv)
>  	if (!xen_setup())
>  		exit(1);
>  
> -	enum_domains();
> -
>  	handle_io();
>  
>  	closelog();
> -- 
> 1.7.11.7
>

On 01/11/2013 02:08 PM, Stefano Stabellini wrote:
> On Thu, 10 Jan 2013, 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.
> 
> Unfortunately at the moment xc_dom_gnttab_init doesn''t work with
an
> autotranslated dom0, that is the only type of dom0 that you get on ARM.
> As a consequence this patch would break xenconsoled on ARM. I expect the
> same to happen with PVH as well.
>
> I also have the same issue with xenstored and I was thinking about
> writing a patch to make it gracefully fallback to xc_map_foreign_range
> if xc_gnttab_map_grant_ref fails.
This patch should already do this, unlike xenstored where it only tries
xc_map_foreign_range when the grant table handle failed to open.
> The reason why xc_dom_gnttab_init is broken is that it uses
> xc_map_foreign_range to map the grant table page of the foreign domain,
> that is not a regular gpfn page, therefore it fails.
> Specifically on ARM it hits the case XENMAPSPACE_gmfn_foreign in
> xenmem_add_to_physmap_one, then it fails the p2m lookup and returns
> EINVAL.
I''m unsure why xc_map_foreign_range is being used to map a grant table
page: the only use of this function in xenconsoled is as a fallback for
when xc_gnttab_map_grant_ref fails (which sounds like the case on PVH/ARM),
but in this case the code should act the same as before this patch.
> Do you feel up for making xc_dom_gnttab_init work for an autotranslated
> dom0?
> Otherwise I think that for the moment is best to modify this patch to
> gracefully fallback to xc_map_foreign_range. Then we need a couple of
> other patches to do the same with xenstored and libxl.
xenstored also should fall back gracefully if the grant table fails to open
(a simple way test this on x86 is to fail to load the xen-gntdev module
before starting xenstored).  I haven''t tested this recently, but it
looks
correct from a glance.

I don''t think libxl does any grant table mapping: it seeds the
guest''s grant
table with some entries for xenstore and xenconsole, but never maps them.

-- 
Daniel De Graaf
National Security Agency

On Fri, 11 Jan 2013, Daniel De Graaf wrote:
> On 01/11/2013 02:08 PM, Stefano Stabellini wrote:
> > On Thu, 10 Jan 2013, 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.
> > 
> > Unfortunately at the moment xc_dom_gnttab_init doesn''t work
with an
> > autotranslated dom0, that is the only type of dom0 that you get on
ARM.
> > As a consequence this patch would break xenconsoled on ARM. I expect
the
> > same to happen with PVH as well.
> >
> > I also have the same issue with xenstored and I was thinking about
> > writing a patch to make it gracefully fallback to xc_map_foreign_range
> > if xc_gnttab_map_grant_ref fails.
> 
> This patch should already do this, unlike xenstored where it only tries
> xc_map_foreign_range when the grant table handle failed to open.
Yes, you are right, sorry for noticing it only now.

> > The reason why xc_dom_gnttab_init is broken is that it uses
> > xc_map_foreign_range to map the grant table page of the foreign
domain,
> > that is not a regular gpfn page, therefore it fails.
> > Specifically on ARM it hits the case XENMAPSPACE_gmfn_foreign in
> > xenmem_add_to_physmap_one, then it fails the p2m lookup and returns
> > EINVAL.
> 
> I''m unsure why xc_map_foreign_range is being used to map a grant
table
> page: the only use of this function in xenconsoled is as a fallback for
> when xc_gnttab_map_grant_ref fails (which sounds like the case on PVH/ARM),
> but in this case the code should act the same as before this patch.
It didn''t explain myself very well. Give a look at xc_dom_gnttab_seed:
right after xc_dom_gnttab_setup, it calls xc_map_foreign_range on it,
but the page returned by xc_dom_gnttab_setup is not going to be part of
the guest p2m, therefore xc_map_foreign_range will fail.
However xc_dom_gnttab_hvm_seed should work just fine.

> > Do you feel up for making xc_dom_gnttab_init work for an
autotranslated
> > dom0?
> > Otherwise I think that for the moment is best to modify this patch to
> > gracefully fallback to xc_map_foreign_range. Then we need a couple of
> > other patches to do the same with xenstored and libxl.
> 
> xenstored also should fall back gracefully if the grant table fails to open
> (a simple way test this on x86 is to fail to load the xen-gntdev module
> before starting xenstored).  I haven''t tested this recently, but
it looks
> correct from a glance.
Right, but in my case xenstored opens the grant table correctly. However
xc_gnttab_map_grant_ref would still fail and at that point xenstored is
unable to fall back to xc_map_foreign_range.

> I don''t think libxl does any grant table mapping: it seeds the
guest''s grant
> table with some entries for xenstore and xenconsole, but never maps them.
libxl doesn''t but libxc does in xc_dom_gnttab_seed.

In any case I managed to solve the problem on ARM, calling
xc_dom_gnttab_hvm_seed instead of xc_dom_gnttab_seed, and implementing
gnttab_create_shared_page and gnttab_shared_gmfn correctly in the
hypervisor. I am now able to use the grant table mapping for the
xenstore page. I hope that the same strategy is going to work correctly
for the console page in xenconsoled.

On Thu, 2013-01-10 at 17:28 +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>
I''ve acked + applied both of these patches.

I had to fixup a minor conflict with Wei''s select patch in io.c, please
check I''ve done the right thing.

On Thu, 2013-01-10 at 17:28 +0000, Daniel De Graaf
wrote:
> 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.
Have I missed the patch which writes that path?

On 01/17/2013 11:50 AM, Ian Campbell wrote:
> On Thu, 2013-01-10 at 17:28 +0000, Daniel De Graaf wrote:
>> 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.
> 
> Have I missed the patch which writes that path?
> 
There is currently no patch which writes to that path; this would be done
by the scripts that launch the domain that xenconsoled resides in. I
don''t
think having xenconsoled write its domain ID to that path is useful - it
would require waiting for the xenconsoled domain and daemon to start before
building other guests, whereas writing the path from dom0 allows setting
everything up first and letting xenconsoled catch up later.

-- 
Daniel De Graaf
National Security Agency

On Thu, 2013-01-17 at 16:58 +0000, Daniel De Graaf
wrote:
> On 01/17/2013 11:50 AM, Ian Campbell wrote:
> > On Thu, 2013-01-10 at 17:28 +0000, Daniel De Graaf wrote:
> >> 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.
> > 
> > Have I missed the patch which writes that path?
> > 
> 
> There is currently no patch which writes to that path; this would be done
> by the scripts that launch the domain that xenconsoled resides in. I
don''t
> think having xenconsoled write its domain ID to that path is useful - it
> would require waiting for the xenconsoled domain and daemon to start before
> building other guests, whereas writing the path from dom0 allows setting
> everything up first and letting xenconsoled catch up later.
That makes sense.

I have a follow up question though. Have I missed the patch which reads
this path ;-)

Ian.

On 01/17/2013 12:05 PM, Ian Campbell wrote:
> On Thu, 2013-01-17 at 16:58 +0000, Daniel De Graaf wrote:
>> On 01/17/2013 11:50 AM, Ian Campbell wrote:
>>> On Thu, 2013-01-10 at 17:28 +0000, Daniel De Graaf wrote:
>>>> 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.
>>>
>>> Have I missed the patch which writes that path?
>>>
>>
>> There is currently no patch which writes to that path; this would be
done
>> by the scripts that launch the domain that xenconsoled resides in. I
don''t
>> think having xenconsoled write its domain ID to that path is useful -
it
>> would require waiting for the xenconsoled domain and daemon to start
before
>> building other guests, whereas writing the path from dom0 allows
setting
>> everything up first and letting xenconsoled catch up later.
> 
> That makes sense.
> 
> I have a follow up question though. Have I missed the patch which reads
> this path ;-)
> 
> Ian.
> 
That was done in the same patch that reads xenstored''s path, and it was
put
in quite a while ago - the code is in tools/libxl/libxl_dom.c

-- 
Daniel De Graaf
National Security Agency

On Thu, 2013-01-17 at 17:15 +0000, Daniel De Graaf
wrote:
> That was done in the same patch that reads xenstored''s path, and
it
> was put in quite a while ago - the code is in tools/libxl/libxl_dom.c 
Ah, so it was. Great I don''t need to find it and commit it then ;-)

Ian.