Daniel De Graaf
2012-Dec-13 16:08 UTC
[PATCH] 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 | 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
Ian Campbell
2013-Jan-10 16:11 UTC
Re: [PATCH] xenconsoled: use grant references instead of map_foreign_range
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.
Daniel De Graaf
2013-Jan-10 16:23 UTC
Re: [PATCH] xenconsoled: use grant references instead of map_foreign_range
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
Ian Campbell
2013-Jan-10 16:29 UTC
Re: [PATCH] xenconsoled: use grant references instead of map_foreign_range
> >> + 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.>
Possibly Parallel Threads
- [PATCH v2 1/2] xenconsoled: use grant references instead of map_foreign_range
- [PATCH] Switch to poll in xenconsoled's io loop.
- [PATCH] xenconsoled: ignore spurious watch event
- [PATCH] Xenconsoled should ignore spurious watch event. Otherwise, it can rebind to the same evtchn of a dying domU during suspending and cause below error message:
- [PATCH 2/3] [RFC] User-space grant table device - changes to libxc