[Qemu-devel] [PATCH v3] xen_console: support the new extended xenstore protocol

[Qemu-devel] [PATCH v3] xen_console: support the new extended xenstore protocol

[stefano.stabellini]

From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Since CS 21994 on xen-unstable.hg and CS
466608f3a32e1f9808acdf832a5843af37e5fcec on qemu-xen-unstable.git, few
changes have been introduced to the PV console xenstore protocol, as
described by the document docs/misc/console.txt under xen-unstable.hg.

>From the Qemu point of view, very few modifications are needed to
correctly support the protocol: read from xenstore the "output" node
that tell us what the output of the PV console is going to be.
In case the output is a tty, write to xenstore the device name.

Changes in v2:

- fix error paths: free malloc'ed strings and close the xenstore
connection before returning;

- remove useless snprintf in xenstore_store_pv_console_info if i == 0.

Changes in v3:

- replace xs_daemon_open/xs_daemon_close with xs_open/xs_close.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 hw/xen.h         |    1 +
 hw/xen_console.c |   16 +++++++++-----
 xen-all.c        |   60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 6 deletions(-)

diff --git a/hw/xen.h b/hw/xen.h
index d435ca0..dad0ca0 100644
--- a/hw/xen.h
+++ b/hw/xen.h
@@ -50,6 +50,7 @@ qemu_irq *xen_interrupt_controller_init(void);
 int xen_init(void);
 int xen_hvm_init(void);
 void xen_vcpu_init(void);
+void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
 
 #if defined(NEED_CPU_H) && !defined(CONFIG_USER_ONLY)
 void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size);
diff --git a/hw/xen_console.c b/hw/xen_console.c
index 519d5f5..e81afcd 100644
--- a/hw/xen_console.c
+++ b/hw/xen_console.c
@@ -179,8 +179,9 @@ static void xencons_send(struct XenConsole *con)
 static int con_init(struct XenDevice *xendev)
 {
     struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
-    char *type, *dom;
+    char *type, *dom, label[32];
     int ret = 0;
+    const char *output;
 
     /* setup */
     dom = xs_get_domain_path(xenstore, con->xendev.dom);
@@ -194,11 +195,14 @@ static int con_init(struct XenDevice *xendev)
         goto out;
     }
 
-    if (!serial_hds[con->xendev.dev])
-	xen_be_printf(xendev, 1, "WARNING: serial line %d not configured\n",
-                      con->xendev.dev);
-    else
-        con->chr = serial_hds[con->xendev.dev];
+    output = xenstore_read_str(con->console, "output");
+    /* output is a pty by default */
+    if (output == NULL) {
+        output = "pty";
+    }
+    snprintf(label, sizeof(label), "xencons%d", con->xendev.dev);
+    con->chr = qemu_chr_open(label, output, NULL);
+    xenstore_store_pv_console_info(con->xendev.dev, con->chr);
 
 out:
     qemu_free(type);
diff --git a/xen-all.c b/xen-all.c
index 6099bff..3fd04ef 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -737,6 +737,66 @@ static void cpu_handle_ioreq(void *opaque)
     }
 }
 
+static int store_dev_info(int domid, CharDriverState *cs, const char *string)
+{
+    struct xs_handle *xs = NULL;
+    char *path = NULL;
+    char *newpath = NULL;
+    char *pts = NULL;
+    int ret = -1;
+
+    /* Only continue if we're talking to a pty. */
+    if (strncmp(cs->filename, "pty:", 4)) {
+        return 0;
+    }
+    pts = cs->filename + 4;
+
+    /* We now have everything we need to set the xenstore entry. */
+    xs = xs_open(0);
+    if (xs == NULL) {
+        fprintf(stderr, "Could not contact XenStore\n");
+        goto out;
+    }
+
+    path = xs_get_domain_path(xs, domid);
+    if (path == NULL) {
+        fprintf(stderr, "xs_get_domain_path() error\n");
+        goto out;
+    }
+    newpath = realloc(path, (strlen(path) + strlen(string) +
+                strlen("/tty") + 1));
+    if (newpath == NULL) {
+        fprintf(stderr, "realloc error\n");
+        goto out;
+    }
+    path = newpath;
+
+    strcat(path, string);
+    strcat(path, "/tty");
+    if (!xs_write(xs, XBT_NULL, path, pts, strlen(pts))) {
+        fprintf(stderr, "xs_write for '%s' fail", string);
+        goto out;
+    }
+    ret = 0;
+
+out:
+    free(path);
+    xs_close(xs);
+
+    return ret;
+}
+
+void xenstore_store_pv_console_info(int i, CharDriverState *chr)
+{
+    if (i == 0) {
+        store_dev_info(xen_domid, chr, "/console");
+    } else {
+        char buf[32];
+        snprintf(buf, sizeof(buf), "/device/console/%d", i);
+        store_dev_info(xen_domid, chr, buf);
+    }
+}
+
 static void xenstore_record_dm_state(XenIOState *s, const char *state)
 {
     char path[50];
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH v3] xen_console: support the new extended xenstore protocol

[Alexander Graf]

On 06/29/2011 01:12 PM, stefano.stabellini@eu.citrix.com wrote:
> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>
> Since CS 21994 on xen-unstable.hg and CS
> 466608f3a32e1f9808acdf832a5843af37e5fcec on qemu-xen-unstable.git, few
> changes have been introduced to the PV console xenstore protocol, as
> described by the document docs/misc/console.txt under xen-unstable.hg.
>
>  From the Qemu point of view, very few modifications are needed to
> correctly support the protocol: read from xenstore the "output" node
> that tell us what the output of the PV console is going to be.
> In case the output is a tty, write to xenstore the device name.
>
> Changes in v2:
>
> - fix error paths: free malloc'ed strings and close the xenstore
> connection before returning;
>
> - remove useless snprintf in xenstore_store_pv_console_info if i == 0.
>
> Changes in v3:
>
> - replace xs_daemon_open/xs_daemon_close with xs_open/xs_close.
>
> Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> ---
>   hw/xen.h         |    1 +
>   hw/xen_console.c |   16 +++++++++-----
>   xen-all.c        |   60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 71 insertions(+), 6 deletions(-)
>
> diff --git a/hw/xen.h b/hw/xen.h
> index d435ca0..dad0ca0 100644
> --- a/hw/xen.h
> +++ b/hw/xen.h
> @@ -50,6 +50,7 @@ qemu_irq *xen_interrupt_controller_init(void);
>   int xen_init(void);
>   int xen_hvm_init(void);
>   void xen_vcpu_init(void);
> +void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
>
>   #if defined(NEED_CPU_H)&&  !defined(CONFIG_USER_ONLY)
>   void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size);
> diff --git a/hw/xen_console.c b/hw/xen_console.c
> index 519d5f5..e81afcd 100644
> --- a/hw/xen_console.c
> +++ b/hw/xen_console.c
> @@ -179,8 +179,9 @@ static void xencons_send(struct XenConsole *con)
>   static int con_init(struct XenDevice *xendev)
>   {
>       struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
> -    char *type, *dom;
> +    char *type, *dom, label[32];
>       int ret = 0;
> +    const char *output;
>
>       /* setup */
>       dom = xs_get_domain_path(xenstore, con->xendev.dom);
> @@ -194,11 +195,14 @@ static int con_init(struct XenDevice *xendev)
>           goto out;
>       }
>
> -    if (!serial_hds[con->xendev.dev])
> -	xen_be_printf(xendev, 1, "WARNING: serial line %d not configured\n",
> -                      con->xendev.dev);
> -    else
> -        con->chr = serial_hds[con->xendev.dev];
> +    output = xenstore_read_str(con->console, "output");
> +    /* output is a pty by default */
> +    if (output == NULL) {
> +        output = "pty";

Not sure I understand. Why would a pty be the default? stdio makes a lot 
more sense, no?

> +    }
> +    snprintf(label, sizeof(label), "xencons%d", con->xendev.dev);
> +    con->chr = qemu_chr_open(label, output, NULL);
> +    xenstore_store_pv_console_info(con->xendev.dev, con->chr);
>
>   out:
>       qemu_free(type);
> diff --git a/xen-all.c b/xen-all.c
> index 6099bff..3fd04ef 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -737,6 +737,66 @@ static void cpu_handle_ioreq(void *opaque)
>       }
>   }
>
> +static int store_dev_info(int domid, CharDriverState *cs, const char *string)
> +{
> +    struct xs_handle *xs = NULL;
> +    char *path = NULL;
> +    char *newpath = NULL;
> +    char *pts = NULL;
> +    int ret = -1;
> +
> +    /* Only continue if we're talking to a pty. */
> +    if (strncmp(cs->filename, "pty:", 4)) {

What's wrong with !pty?


Alex

Re: [Qemu-devel] [PATCH v3] xen_console: support the new extended xenstore protocol

[Stefano Stabellini]

On Thu, 30 Jun 2011, Alexander Graf wrote:
> On 06/29/2011 01:12 PM, stefano.stabellini@eu.citrix.com wrote:
> > From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> >
> > Since CS 21994 on xen-unstable.hg and CS
> > 466608f3a32e1f9808acdf832a5843af37e5fcec on qemu-xen-unstable.git, few
> > changes have been introduced to the PV console xenstore protocol, as
> > described by the document docs/misc/console.txt under xen-unstable.hg.
> >
> >  From the Qemu point of view, very few modifications are needed to
> > correctly support the protocol: read from xenstore the "output" node
> > that tell us what the output of the PV console is going to be.
> > In case the output is a tty, write to xenstore the device name.
> >
> > Changes in v2:
> >
> > - fix error paths: free malloc'ed strings and close the xenstore
> > connection before returning;
> >
> > - remove useless snprintf in xenstore_store_pv_console_info if i == 0.
> >
> > Changes in v3:
> >
> > - replace xs_daemon_open/xs_daemon_close with xs_open/xs_close.
> >
> > Signed-off-by: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> > ---
> >   hw/xen.h         |    1 +
> >   hw/xen_console.c |   16 +++++++++-----
> >   xen-all.c        |   60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 71 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/xen.h b/hw/xen.h
> > index d435ca0..dad0ca0 100644
> > --- a/hw/xen.h
> > +++ b/hw/xen.h
> > @@ -50,6 +50,7 @@ qemu_irq *xen_interrupt_controller_init(void);
> >   int xen_init(void);
> >   int xen_hvm_init(void);
> >   void xen_vcpu_init(void);
> > +void xenstore_store_pv_console_info(int i, struct CharDriverState *chr);
> >
> >   #if defined(NEED_CPU_H)&&  !defined(CONFIG_USER_ONLY)
> >   void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size);
> > diff --git a/hw/xen_console.c b/hw/xen_console.c
> > index 519d5f5..e81afcd 100644
> > --- a/hw/xen_console.c
> > +++ b/hw/xen_console.c
> > @@ -179,8 +179,9 @@ static void xencons_send(struct XenConsole *con)
> >   static int con_init(struct XenDevice *xendev)
> >   {
> >       struct XenConsole *con = container_of(xendev, struct XenConsole, xendev);
> > -    char *type, *dom;
> > +    char *type, *dom, label[32];
> >       int ret = 0;
> > +    const char *output;
> >
> >       /* setup */
> >       dom = xs_get_domain_path(xenstore, con->xendev.dom);
> > @@ -194,11 +195,14 @@ static int con_init(struct XenDevice *xendev)
> >           goto out;
> >       }
> >
> > -    if (!serial_hds[con->xendev.dev])
> > -	xen_be_printf(xendev, 1, "WARNING: serial line %d not configured\n",
> > -                      con->xendev.dev);
> > -    else
> > -        con->chr = serial_hds[con->xendev.dev];
> > +    output = xenstore_read_str(con->console, "output");
> > +    /* output is a pty by default */
> > +    if (output == NULL) {
> > +        output = "pty";
> 
> Not sure I understand. Why would a pty be the default? stdio makes a lot 
> more sense, no?

ptys are always been the default in the xen world. Besides qemu is
always executed by the toolstack, so stdio might be used for something
else...


> > +    }
> > +    snprintf(label, sizeof(label), "xencons%d", con->xendev.dev);
> > +    con->chr = qemu_chr_open(label, output, NULL);
> > +    xenstore_store_pv_console_info(con->xendev.dev, con->chr);
> >
> >   out:
> >       qemu_free(type);
> > diff --git a/xen-all.c b/xen-all.c
> > index 6099bff..3fd04ef 100644
> > --- a/xen-all.c
> > +++ b/xen-all.c
> > @@ -737,6 +737,66 @@ static void cpu_handle_ioreq(void *opaque)
> >       }
> >   }
> >
> > +static int store_dev_info(int domid, CharDriverState *cs, const char *string)
> > +{
> > +    struct xs_handle *xs = NULL;
> > +    char *path = NULL;
> > +    char *newpath = NULL;
> > +    char *pts = NULL;
> > +    int ret = -1;
> > +
> > +    /* Only continue if we're talking to a pty. */
> > +    if (strncmp(cs->filename, "pty:", 4)) {
> 
> What's wrong with !pty?

Nothing wrong, we don't need to write anything back to xenstore in that
case.

Re: [Qemu-devel] [PATCH v3] xen_console: support the new extended xenstore protocol

[Alexander Graf]

On 06/29/2011 01:12 PM, stefano.stabellini@eu.citrix.com wrote:
> From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
>
> Since CS 21994 on xen-unstable.hg and CS
> 466608f3a32e1f9808acdf832a5843af37e5fcec on qemu-xen-unstable.git, few
> changes have been introduced to the PV console xenstore protocol, as
> described by the document docs/misc/console.txt under xen-unstable.hg.
>
>  From the Qemu point of view, very few modifications are needed to
> correctly support the protocol: read from xenstore the "output" node
> that tell us what the output of the PV console is going to be.
> In case the output is a tty, write to xenstore the device name.
>
> Changes in v2:
>
> - fix error paths: free malloc'ed strings and close the xenstore
> connection before returning;
>
> - remove useless snprintf in xenstore_store_pv_console_info if i == 0.
>
> Changes in v3:
>
> - replace xs_daemon_open/xs_daemon_close with xs_open/xs_close.


On SLES11SP1:

cc1: warnings being treated as errors
/studio/tmp/agraf/xen-all.c: In function ‘store_dev_info’:
/studio/tmp/agraf/xen-all.c:755: error: implicit declaration of function 
‘xs_open’
/studio/tmp/agraf/xen-all.c:755: error: nested extern declaration of 
‘xs_open’
/studio/tmp/agraf/xen-all.c:755: error: assignment makes pointer from 
integer without a cast
/studio/tmp/agraf/xen-all.c:784: error: implicit declaration of function 
‘xs_close’
/studio/tmp/agraf/xen-all.c:784: error: nested extern declaration of 
‘xs_close’


Alex

Re: [Qemu-devel] [PATCH v3] xen_console: support the new extended xenstore protocol

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1714 bytes --]

On Thu, 30 Jun 2011, Alexander Graf wrote:
> On 06/29/2011 01:12 PM, stefano.stabellini@eu.citrix.com wrote:
> > From: Stefano Stabellini<stefano.stabellini@eu.citrix.com>
> >
> > Since CS 21994 on xen-unstable.hg and CS
> > 466608f3a32e1f9808acdf832a5843af37e5fcec on qemu-xen-unstable.git, few
> > changes have been introduced to the PV console xenstore protocol, as
> > described by the document docs/misc/console.txt under xen-unstable.hg.
> >
> >  From the Qemu point of view, very few modifications are needed to
> > correctly support the protocol: read from xenstore the "output" node
> > that tell us what the output of the PV console is going to be.
> > In case the output is a tty, write to xenstore the device name.
> >
> > Changes in v2:
> >
> > - fix error paths: free malloc'ed strings and close the xenstore
> > connection before returning;
> >
> > - remove useless snprintf in xenstore_store_pv_console_info if i == 0.
> >
> > Changes in v3:
> >
> > - replace xs_daemon_open/xs_daemon_close with xs_open/xs_close.
> 
> 
> On SLES11SP1:
> 
> cc1: warnings being treated as errors
> /studio/tmp/agraf/xen-all.c: In function ‘store_dev_info’:
> /studio/tmp/agraf/xen-all.c:755: error: implicit declaration of function 
> ‘xs_open’
> /studio/tmp/agraf/xen-all.c:755: error: nested extern declaration of 
> ‘xs_open’
> /studio/tmp/agraf/xen-all.c:755: error: assignment makes pointer from 
> integer without a cast
> /studio/tmp/agraf/xen-all.c:784: error: implicit declaration of function 
> ‘xs_close’
> /studio/tmp/agraf/xen-all.c:784: error: nested extern declaration of 
> ‘xs_close’

I'll send an update with two simple implementations of xs_close and
xs_open in case xen < 4.1