[Qemu-devel] [PATCH] qemu-xen: make use of xenstore relative paths

[Qemu-devel] [PATCH] qemu-xen: make use of xenstore relative paths

[Roger Pau Monne]

Qemu has several hardcoded xenstore paths that are only valid on Dom0.
Attempts to launch a Qemu instance (to act as a userspace backend for
PV disks) will fail because Qemu is not able to access those paths
when running on a domain different than Dom0.

Instead make the xenstore paths relative to the domain where Qemu is
actually running.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: xen-devel@lists.xenproject.org
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen_backend.c |   19 ++++++-------------
 xen-all.c        |    2 +-
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/hw/xen_backend.c b/hw/xen_backend.c
index 008cdb3..e220606 100644
--- a/hw/xen_backend.c
+++ b/hw/xen_backend.c
@@ -205,7 +205,6 @@ static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
                                            struct XenDevOps *ops)
 {
     struct XenDevice *xendev;
-    char *dom0;
 
     xendev = xen_be_find_xendev(type, dom, dev);
     if (xendev) {
@@ -219,12 +218,10 @@ static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
     xendev->dev   = dev;
     xendev->ops   = ops;
 
-    dom0 = xs_get_domain_path(xenstore, 0);
-    snprintf(xendev->be, sizeof(xendev->be), "%s/backend/%s/%d/%d",
-             dom0, xendev->type, xendev->dom, xendev->dev);
+    snprintf(xendev->be, sizeof(xendev->be), "backend/%s/%d/%d",
+             xendev->type, xendev->dom, xendev->dev);
     snprintf(xendev->name, sizeof(xendev->name), "%s-%d",
              xendev->type, xendev->dev);
-    free(dom0);
 
     xendev->debug      = debug;
     xendev->local_port = -1;
@@ -570,14 +567,12 @@ static int xenstore_scan(const char *type, int dom, struct XenDevOps *ops)
 {
     struct XenDevice *xendev;
     char path[XEN_BUFSIZE], token[XEN_BUFSIZE];
-    char **dev = NULL, *dom0;
+    char **dev = NULL;
     unsigned int cdev, j;
 
     /* setup watch */
-    dom0 = xs_get_domain_path(xenstore, 0);
     snprintf(token, sizeof(token), "be:%p:%d:%p", type, dom, ops);
-    snprintf(path, sizeof(path), "%s/backend/%s/%d", dom0, type, dom);
-    free(dom0);
+    snprintf(path, sizeof(path), "backend/%s/%d", type, dom);
     if (!xs_watch(xenstore, path, token)) {
         xen_be_printf(NULL, 0, "xen be: watching backend path (%s) failed\n", path);
         return -1;
@@ -603,12 +598,10 @@ static void xenstore_update_be(char *watch, char *type, int dom,
                                struct XenDevOps *ops)
 {
     struct XenDevice *xendev;
-    char path[XEN_BUFSIZE], *dom0, *bepath;
+    char path[XEN_BUFSIZE], *bepath;
     unsigned int len, dev;
 
-    dom0 = xs_get_domain_path(xenstore, 0);
-    len = snprintf(path, sizeof(path), "%s/backend/%s/%d", dom0, type, dom);
-    free(dom0);
+    len = snprintf(path, sizeof(path), "backend/%s/%d", type, dom);
     if (strncmp(path, watch, len) != 0) {
         return;
     }
diff --git a/xen-all.c b/xen-all.c
index 15be8ed..99666f9 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -967,7 +967,7 @@ static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
         exit(1);
     }
 
-    snprintf(path, sizeof (path), "/local/domain/0/device-model/%u/state", xen_domid);
+    snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
     if (!xs_write(xs, XBT_NULL, path, state, strlen(state))) {
         fprintf(stderr, "error recording dm state\n");
         exit(1);
-- 
1.7.7.5 (Apple Git-26)

Re: [Qemu-devel] [PATCH] qemu-xen: make use of xenstore relative paths

[Anthony PERARD]

On Wed, Sep 18, 2013 at 09:50:58PM +0200, Roger Pau Monne wrote:
> Qemu has several hardcoded xenstore paths that are only valid on Dom0.
> Attempts to launch a Qemu instance (to act as a userspace backend for
> PV disks) will fail because Qemu is not able to access those paths
> when running on a domain different than Dom0.
> 
> Instead make the xenstore paths relative to the domain where Qemu is
> actually running.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: xen-devel@lists.xenproject.org
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

This look fine. One issue with the patch: the file xen_backend.c have
been moved to hw/xen/xen_backend.c.

I've also tryied it in a stubdomain, and it does not boot anymore
because the qemu in the stubdom can not read the state.  I have tried
again without the change in xen-all.c, and the stubdom does not complain
anymore. So in the change in xenstore_record_dm_state() needed as well?


> ---
>  hw/xen_backend.c |   19 ++++++-------------
>  xen-all.c        |    2 +-
>  2 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/xen_backend.c b/hw/xen_backend.c
> index 008cdb3..e220606 100644
> --- a/hw/xen_backend.c
> +++ b/hw/xen_backend.c
> @@ -205,7 +205,6 @@ static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
>                                             struct XenDevOps *ops)
>  {
>      struct XenDevice *xendev;
> -    char *dom0;
>  
>      xendev = xen_be_find_xendev(type, dom, dev);
>      if (xendev) {
> @@ -219,12 +218,10 @@ static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
>      xendev->dev   = dev;
>      xendev->ops   = ops;
>  
> -    dom0 = xs_get_domain_path(xenstore, 0);
> -    snprintf(xendev->be, sizeof(xendev->be), "%s/backend/%s/%d/%d",
> -             dom0, xendev->type, xendev->dom, xendev->dev);
> +    snprintf(xendev->be, sizeof(xendev->be), "backend/%s/%d/%d",
> +             xendev->type, xendev->dom, xendev->dev);
>      snprintf(xendev->name, sizeof(xendev->name), "%s-%d",
>               xendev->type, xendev->dev);
> -    free(dom0);
>  
>      xendev->debug      = debug;
>      xendev->local_port = -1;
> @@ -570,14 +567,12 @@ static int xenstore_scan(const char *type, int dom, struct XenDevOps *ops)
>  {
>      struct XenDevice *xendev;
>      char path[XEN_BUFSIZE], token[XEN_BUFSIZE];
> -    char **dev = NULL, *dom0;
> +    char **dev = NULL;
>      unsigned int cdev, j;
>  
>      /* setup watch */
> -    dom0 = xs_get_domain_path(xenstore, 0);
>      snprintf(token, sizeof(token), "be:%p:%d:%p", type, dom, ops);
> -    snprintf(path, sizeof(path), "%s/backend/%s/%d", dom0, type, dom);
> -    free(dom0);
> +    snprintf(path, sizeof(path), "backend/%s/%d", type, dom);
>      if (!xs_watch(xenstore, path, token)) {
>          xen_be_printf(NULL, 0, "xen be: watching backend path (%s) failed\n", path);
>          return -1;
> @@ -603,12 +598,10 @@ static void xenstore_update_be(char *watch, char *type, int dom,
>                                 struct XenDevOps *ops)
>  {
>      struct XenDevice *xendev;
> -    char path[XEN_BUFSIZE], *dom0, *bepath;
> +    char path[XEN_BUFSIZE], *bepath;
>      unsigned int len, dev;
>  
> -    dom0 = xs_get_domain_path(xenstore, 0);
> -    len = snprintf(path, sizeof(path), "%s/backend/%s/%d", dom0, type, dom);
> -    free(dom0);
> +    len = snprintf(path, sizeof(path), "backend/%s/%d", type, dom);
>      if (strncmp(path, watch, len) != 0) {
>          return;
>      }
> diff --git a/xen-all.c b/xen-all.c
> index 15be8ed..99666f9 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -967,7 +967,7 @@ static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
>          exit(1);
>      }
>  
> -    snprintf(path, sizeof (path), "/local/domain/0/device-model/%u/state", xen_domid);
> +    snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
>      if (!xs_write(xs, XBT_NULL, path, state, strlen(state))) {
>          fprintf(stderr, "error recording dm state\n");
>          exit(1);
> -- 
> 1.7.7.5 (Apple Git-26)
> 

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH] qemu-xen: make use of xenstore relative paths

[Roger Pau Monné]

On 26/09/13 18:46, Anthony PERARD wrote:
> On Wed, Sep 18, 2013 at 09:50:58PM +0200, Roger Pau Monne wrote:
>> Qemu has several hardcoded xenstore paths that are only valid on Dom0.
>> Attempts to launch a Qemu instance (to act as a userspace backend for
>> PV disks) will fail because Qemu is not able to access those paths
>> when running on a domain different than Dom0.
>>
>> Instead make the xenstore paths relative to the domain where Qemu is
>> actually running.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: xen-devel@lists.xenproject.org
>> Cc: Anthony PERARD <anthony.perard@citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> This look fine. One issue with the patch: the file xen_backend.c have
> been moved to hw/xen/xen_backend.c.

Thanks, this is based on the stable Qemu version in Xen tree, I should
have done the change on top of the main qemu.git repo.

> I've also tryied it in a stubdomain, and it does not boot anymore
> because the qemu in the stubdom can not read the state.  I have tried
> again without the change in xen-all.c, and the stubdom does not complain
> anymore. So in the change in xenstore_record_dm_state() needed as well?

Yes, if we run a Qemu instance inside a driver domain it wouldn't make
much sense IMHO to write the state of that Qemu instance on a xenstore
path that belongs to the Dom0, and also we would need to give the driver
domain permissions to write on a xenstore path that's inside the Dom0
xenstore path, which doesn't seem like a good idea.

To make Qemu work on a domain different than Dom0 you will also need the
following patch from my driver domain series:

http://marc.info/?l=xen-devel&m=137993233817018

If not the guest is unable to create the device-model/<domid>/state
xenstore entry. For stubdomains would it be really hard to change the
Dom0 to check for /local/domain/<stubdom_id>/device-model/<domid>/state
instead of /local/domain/0/device-model/<domid>/state?

Re: [Qemu-devel] [PATCH] qemu-xen: make use of xenstore relative paths

[Anthony PERARD]

On Thu, Sep 26, 2013 at 07:20:31PM +0200, Roger Pau Monné wrote:
> On 26/09/13 18:46, Anthony PERARD wrote:
> > On Wed, Sep 18, 2013 at 09:50:58PM +0200, Roger Pau Monne wrote:
> >> Qemu has several hardcoded xenstore paths that are only valid on Dom0.
> >> Attempts to launch a Qemu instance (to act as a userspace backend for
> >> PV disks) will fail because Qemu is not able to access those paths
> >> when running on a domain different than Dom0.
> >>
> >> Instead make the xenstore paths relative to the domain where Qemu is
> >> actually running.
> >>
> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> Cc: xen-devel@lists.xenproject.org
> >> Cc: Anthony PERARD <anthony.perard@citrix.com>
> >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > This look fine. One issue with the patch: the file xen_backend.c have
> > been moved to hw/xen/xen_backend.c.
> 
> Thanks, this is based on the stable Qemu version in Xen tree, I should
> have done the change on top of the main qemu.git repo.
> 
> > I've also tryied it in a stubdomain, and it does not boot anymore
> > because the qemu in the stubdom can not read the state.  I have tried
> > again without the change in xen-all.c, and the stubdom does not complain
> > anymore. So in the change in xenstore_record_dm_state() needed as well?
> 
> Yes, if we run a Qemu instance inside a driver domain it wouldn't make
> much sense IMHO to write the state of that Qemu instance on a xenstore
> path that belongs to the Dom0, and also we would need to give the driver
> domain permissions to write on a xenstore path that's inside the Dom0
> xenstore path, which doesn't seem like a good idea.
> 
> To make Qemu work on a domain different than Dom0 you will also need the
> following patch from my driver domain series:
> 
> http://marc.info/?l=xen-devel&m=137993233817018
> 
> If not the guest is unable to create the device-model/<domid>/state
> xenstore entry. For stubdomains would it be really hard to change the
> Dom0 to check for /local/domain/<stubdom_id>/device-model/<domid>/state
> instead of /local/domain/0/device-model/<domid>/state?

I have tried with the patch applied to libxl, and stubdom work fine with
the changes to the xenstore paths. So, once the xen_backend.c file is in
the new path, you can add my:
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD