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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> ---
>  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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel