[PATCH] mini-os: Fix xenbus initialisation

[PATCH] mini-os: Fix xenbus initialisation

[Samuel Thibault]

mini-os: Fix xenbus initialisation

This fixes xenbus initialization of blkfront, netfront and pcifront
by uniformizing with fbfront: after writing parameters, set state to
initialised, then wait for backend to switch to connect state, and then
only read its parameter and switch to the connect state.

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

diff -r ebd84be3420a extras/mini-os/blkfront.c
--- a/extras/mini-os/blkfront.c	Tue Mar 30 18:31:39 2010 +0100
+++ b/extras/mini-os/blkfront.c	Tue Apr 06 01:24:28 2010 +0200
@@ -152,13 +152,12 @@
     }
 
     snprintf(path, sizeof(path), "%s/state", nodename);
-    err = xenbus_switch_state(xbt, path, XenbusStateConnected);
+    err = xenbus_switch_state(xbt, path, XenbusStateInitialised);
     if (err) {
-        message = "switching state";
+        printk("error writing blk initialized on %s: %s\n", path, err);
         goto abort_transaction;
     }
 
-
     err = xenbus_transaction_end(xbt, 0, &retry);
     if (err) free(err);
     if (retry) {
@@ -189,6 +188,7 @@
     {
         XenbusState state;
         char path[strlen(dev->backend) + 1 + 19 + 1];
+        char frontpath[strlen(nodename) + 1 + 6 + 1];
         snprintf(path, sizeof(path), "%s/mode", dev->backend);
         msg = xenbus_read(XBT_NIL, path, &c);
         if (msg) {
@@ -232,7 +232,18 @@
         dev->info.flush = xenbus_read_integer(path);
 
         *info = dev->info;
+
+        printk("%s connected\n", dev->backend);
+
+        snprintf(frontpath, sizeof(frontpath), "%s/state", nodename);
+        if((err = xenbus_switch_state(XBT_NIL, frontpath, XenbusStateConnected))
+            != NULL) {
+            printk("error switching state: %s\n", err);
+            xenbus_unwatch_path_token(XBT_NIL, path, path);
+            goto error;
+        }
     }
+
     unmask_evtchn(dev->evtchn);
 
     printk("%u sectors of %u bytes\n", dev->info.sectors, dev->info.sector_size);
diff -r ebd84be3420a extras/mini-os/fbfront.c
--- a/extras/mini-os/fbfront.c	Tue Mar 30 18:31:39 2010 +0100
+++ b/extras/mini-os/fbfront.c	Tue Apr 06 01:24:28 2010 +0200
@@ -126,8 +126,8 @@
     snprintf(path, sizeof(path), "%s/state", nodename);
     err = xenbus_switch_state(xbt, path, XenbusStateInitialised);
     if (err) {
-        printk("error writing initialized: %s\n", err);
-        free(err);
+        printk("error writing fb initialized: %s\n", err);
+        goto abort_transaction;
     }
 
     err = xenbus_transaction_end(xbt, 0, &retry);
@@ -189,6 +189,7 @@
     printk("************************** KBDFRONT\n");
 
     return dev;
+
 error:
     free(msg);
     free(err);
diff -r ebd84be3420a extras/mini-os/netfront.c
--- a/extras/mini-os/netfront.c	Tue Mar 30 18:31:39 2010 +0100
+++ b/extras/mini-os/netfront.c	Tue Apr 06 01:24:28 2010 +0200
@@ -407,9 +407,9 @@
     }
 
     snprintf(path, sizeof(path), "%s/state", nodename);
-    err = xenbus_switch_state(xbt, path, XenbusStateConnected);
+    err = xenbus_switch_state(xbt, path, XenbusStateInitialised);
     if (err) {
-        message = "switching state";
+        printk("error writing net initialized: %s\n", err);
         goto abort_transaction;
     }
 
@@ -445,6 +445,7 @@
     {
         XenbusState state;
         char path[strlen(dev->backend) + 1 + 5 + 1];
+        char frontpath[strlen(nodename) + 1 + 6 + 1];
         snprintf(path, sizeof(path), "%s/state", dev->backend);
 
         xenbus_watch_path_token(XBT_NIL, path, path, &dev->events);
@@ -463,6 +464,16 @@
             snprintf(path, sizeof(path), "%s/ip", dev->backend);
             xenbus_read(XBT_NIL, path, ip);
         }
+
+        printk("%s connected\n", dev->backend);
+
+        snprintf(frontpath, sizeof(frontpath), "%s/state", nodename);
+        if((err = xenbus_switch_state(XBT_NIL, frontpath, XenbusStateConnected))
+            != NULL) {
+            printk("error switching state: %s\n", err);
+            xenbus_unwatch_path_token(XBT_NIL, path, path);
+            goto error;
+        }
     }
 
     printk("**************************\n");
@@ -481,6 +492,7 @@
             &rawmac[5]);
 
     return dev;
+
 error:
     free(msg);
     free(err);
diff -r ebd84be3420a extras/mini-os/pcifront.c
--- a/extras/mini-os/pcifront.c	Tue Mar 30 18:31:39 2010 +0100
+++ b/extras/mini-os/pcifront.c	Tue Apr 06 01:24:28 2010 +0200
@@ -143,7 +143,7 @@
     char* err;
     char* message=NULL;
     int retry=0;
-    char* msg;
+    char* msg = NULL;
     char* nodename = _nodename ? _nodename : "device/pci/0";
     int dom;
 
@@ -206,7 +206,7 @@
     snprintf(path, sizeof(path), "%s/state", nodename);
     err = xenbus_switch_state(xbt, path, XenbusStateInitialised);
     if (err) {
-        message = "switching state";
+        printk("error writing pci initialized: %s\n", err);
         goto abort_transaction;
     }
 
@@ -237,7 +237,7 @@
 
     {
         char path[strlen(dev->backend) + 1 + 5 + 1];
-        char frontpath[strlen(nodename) + 1 + 5 + 1];
+        char frontpath[strlen(nodename) + 1 + 6 + 1];
         XenbusState state;
         snprintf(path, sizeof(path), "%s/state", dev->backend);
 
@@ -254,9 +254,9 @@
         }
 
         snprintf(frontpath, sizeof(frontpath), "%s/state", nodename);
-        if ((err = xenbus_switch_state(XBT_NIL, frontpath, XenbusStateConnected))
+        if((err = xenbus_switch_state(XBT_NIL, frontpath, XenbusStateConnected))
             != NULL) {
-            printk("error switching state %s\n", err);
+            printk("error switching state: %s\n", err);
             xenbus_unwatch_path_token(XBT_NIL, path, path);
             goto error;
         }
@@ -271,6 +271,7 @@
     return dev;
 
 error:
+    free(msg);
     free(err);
     free_pcifront(dev);
     return NULL;

Re: [PATCH] mini-os: Fix xenbus initialisation

[Jeremy Fitzhardinge]

On 04/05/2010 04:27 PM, Samuel Thibault wrote:
> mini-os: Fix xenbus initialisation
>
> This fixes xenbus initialization of blkfront, netfront and pcifront
> by uniformizing with fbfront: after writing parameters, set state to
> initialised, then wait for backend to switch to connect state, and then
> only read its parameter and switch to the connect state.
>   

This change causes a regression for stub domains.  When this is applied,
the domain and stub come up, but the main domain just sits there - not
running, not blocked.  The vnc window remains black.

sh-4.0# xm destroy debhv32 && sleep 2; xm create debhv32
Using config file "./debhv32".
Started domain debhv32 (id=19)
sh-4.0# xm list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0  2140     4     r-----   4945.7
debhv32                                     19   512     1     ------      0.0
debhv32-dm                                  20    32     1     -b----      0.0

The Xen console shows:

blktap_sysfs_create: adding attributes for dev ffff8800a1a07c00
blktap_control_allocate_tap: allocated tap ffff8800bbcb8800
blktap_ring_open: opening device blktap0
blktap_ring_open: opened device 0
blktap_ring_mmap: blktap: mapping pid is 402
blktap_validate_params: aio:/dev/vg_lilith-raid/xen-debian-32: capacity: 20971520, sector-size: 512
blktap_validate_params: aio:/dev/vg_lilith-raid/xen-debian-32: capacity: 20971520, sector-size: 512
blktap_device_create: minor 0 sectors 20971520 sector-size 512
device vif19.0 entered promiscuous mode
eth0: port 2(vif19.0) entering forwarding state
device vif20.0 entered promiscuous mode
eth0: port 3(vif20.0) entering forwarding state
Bootstrapping...
Xen Minimal OS!
  start_info: 0x563000(VA)
    nr_pages: 0x2000
  shared_inf: 0xb81b1000(MA)
     pt_base: 0x566000(VA)
nr_pt_frames: 0x7
    mfn_list: 0x553000(VA)
   mod_start: 0x0(VA)
     mod_len: 0
       flags: 0x0
    cmd_line:  -d 19
  stack:      0x511700-0x531700
MM: Init
      _text: 0x0(VA)
     _etext: 0xffa73(VA)
   _erodata: 0x150000(VA)
     _edata: 0x158cc8(VA)
stack start: 0x511700(VA)
       _end: 0x552010(VA)
  start_pfn: 570
    max_pfn: 2000
Mapping memory range 0x800000 - 0x2000000
setting 0x0-0x150000 readonly
skipped 0x1000
MM: Initialise page allocator for 57c000(57c000)-2000000(2000000)
MM: done
Demand map pfns at 2001000-2002001000.
Heap resides at 2002002000-4002002000.
Initialising timer interface
Initialising console ... done.
gnttab_table mapped at 0x2001000.
Initialising scheduler
Thread "Idle": pointer: 0x2002002050, stack: 0x5a0000
Initialising xenbus
Thread "xenstore": pointer: 0x2002002800, stack: 0x5b0000
Dummy main: start_info=0x531800
Thread "main": pointer: 0x2002002fb0, stack: 0x5c0000
Initing FS frontend(s).
No FS import
Thread "pcifront": pointer: 0x2002003760, stack: 0x5d0000
pcifront_watches: waiting for backend path to appear device/pci/0/backend
dom vm is at /vm/f2249d1c-5862-444d-a57a-c3c8a6243d1f
************************ NETFRONT for device/vif/0 **********


net TX ring size 256
net RX ring size 256
backend at /local/domain/0/backend/vif/20/0
mac is 00:16:3e:24:3d:1f
vif19.0: no IPv6 routers present
(XEN) HVM19: HVM Loader
(XEN) HVM19: Detected Xen v4.1-unstable
(XEN) HVM19: CPU speed is 2400 MHz
vif20.0: no IPv6 routers present


Reverting this change (b20f897d6010) makes hvm+stubdom work again.

Thanks,
	J


> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
>
> diff -r ebd84be3420a extras/mini-os/blkfront.c
> --- a/extras/mini-os/blkfront.c	Tue Mar 30 18:31:39 2010 +0100
> +++ b/extras/mini-os/blkfront.c	Tue Apr 06 01:24:28 2010 +0200
> @@ -152,13 +152,12 @@
>      }
>  
>      snprintf(path, sizeof(path), "%s/state", nodename);
> -    err = xenbus_switch_state(xbt, path, XenbusStateConnected);
> +    err = xenbus_switch_state(xbt, path, XenbusStateInitialised);
>      if (err) {
> -        message = "switching state";
> +        printk("error writing blk initialized on %s: %s\n", path, err);
>          goto abort_transaction;
>      }
>  
> -
>      err = xenbus_transaction_end(xbt, 0, &retry);
>      if (err) free(err);
>      if (retry) {
> @@ -189,6 +188,7 @@
>      {
>          XenbusState state;
>          char path[strlen(dev->backend) + 1 + 19 + 1];
> +        char frontpath[strlen(nodename) + 1 + 6 + 1];
>          snprintf(path, sizeof(path), "%s/mode", dev->backend);
>          msg = xenbus_read(XBT_NIL, path, &c);
>          if (msg) {
> @@ -232,7 +232,18 @@
>          dev->info.flush = xenbus_read_integer(path);
>  
>          *info = dev->info;
> +
> +        printk("%s connected\n", dev->backend);
> +
> +        snprintf(frontpath, sizeof(frontpath), "%s/state", nodename);
> +        if((err = xenbus_switch_state(XBT_NIL, frontpath, XenbusStateConnected))
> +            != NULL) {
> +            printk("error switching state: %s\n", err);
> +            xenbus_unwatch_path_token(XBT_NIL, path, path);
> +            goto error;
> +        }
>      }
> +
>      unmask_evtchn(dev->evtchn);
>  
>      printk("%u sectors of %u bytes\n", dev->info.sectors, dev->info.sector_size);
> diff -r ebd84be3420a extras/mini-os/fbfront.c
> --- a/extras/mini-os/fbfront.c	Tue Mar 30 18:31:39 2010 +0100
> +++ b/extras/mini-os/fbfront.c	Tue Apr 06 01:24:28 2010 +0200
> @@ -126,8 +126,8 @@
>      snprintf(path, sizeof(path), "%s/state", nodename);
>      err = xenbus_switch_state(xbt, path, XenbusStateInitialised);
>      if (err) {
> -        printk("error writing initialized: %s\n", err);
> -        free(err);
> +        printk("error writing fb initialized: %s\n", err);
> +        goto abort_transaction;
>      }
>  
>      err = xenbus_transaction_end(xbt, 0, &retry);
> @@ -189,6 +189,7 @@
>      printk("************************** KBDFRONT\n");
>  
>      return dev;
> +
>  error:
>      free(msg);
>      free(err);
> diff -r ebd84be3420a extras/mini-os/netfront.c
> --- a/extras/mini-os/netfront.c	Tue Mar 30 18:31:39 2010 +0100
> +++ b/extras/mini-os/netfront.c	Tue Apr 06 01:24:28 2010 +0200
> @@ -407,9 +407,9 @@
>      }
>  
>      snprintf(path, sizeof(path), "%s/state", nodename);
> -    err = xenbus_switch_state(xbt, path, XenbusStateConnected);
> +    err = xenbus_switch_state(xbt, path, XenbusStateInitialised);
>      if (err) {
> -        message = "switching state";
> +        printk("error writing net initialized: %s\n", err);
>          goto abort_transaction;
>      }
>  
> @@ -445,6 +445,7 @@
>      {
>          XenbusState state;
>          char path[strlen(dev->backend) + 1 + 5 + 1];
> +        char frontpath[strlen(nodename) + 1 + 6 + 1];
>          snprintf(path, sizeof(path), "%s/state", dev->backend);
>  
>          xenbus_watch_path_token(XBT_NIL, path, path, &dev->events);
> @@ -463,6 +464,16 @@
>              snprintf(path, sizeof(path), "%s/ip", dev->backend);
>              xenbus_read(XBT_NIL, path, ip);
>          }
> +
> +        printk("%s connected\n", dev->backend);
> +
> +        snprintf(frontpath, sizeof(frontpath), "%s/state", nodename);
> +        if((err = xenbus_switch_state(XBT_NIL, frontpath, XenbusStateConnected))
> +            != NULL) {
> +            printk("error switching state: %s\n", err);
> +            xenbus_unwatch_path_token(XBT_NIL, path, path);
> +            goto error;
> +        }
>      }
>  
>      printk("**************************\n");
> @@ -481,6 +492,7 @@
>              &rawmac[5]);
>  
>      return dev;
> +
>  error:
>      free(msg);
>      free(err);
> diff -r ebd84be3420a extras/mini-os/pcifront.c
> --- a/extras/mini-os/pcifront.c	Tue Mar 30 18:31:39 2010 +0100
> +++ b/extras/mini-os/pcifront.c	Tue Apr 06 01:24:28 2010 +0200
> @@ -143,7 +143,7 @@
>      char* err;
>      char* message=NULL;
>      int retry=0;
> -    char* msg;
> +    char* msg = NULL;
>      char* nodename = _nodename ? _nodename : "device/pci/0";
>      int dom;
>  
> @@ -206,7 +206,7 @@
>      snprintf(path, sizeof(path), "%s/state", nodename);
>      err = xenbus_switch_state(xbt, path, XenbusStateInitialised);
>      if (err) {
> -        message = "switching state";
> +        printk("error writing pci initialized: %s\n", err);
>          goto abort_transaction;
>      }
>  
> @@ -237,7 +237,7 @@
>  
>      {
>          char path[strlen(dev->backend) + 1 + 5 + 1];
> -        char frontpath[strlen(nodename) + 1 + 5 + 1];
> +        char frontpath[strlen(nodename) + 1 + 6 + 1];
>          XenbusState state;
>          snprintf(path, sizeof(path), "%s/state", dev->backend);
>  
> @@ -254,9 +254,9 @@
>          }
>  
>          snprintf(frontpath, sizeof(frontpath), "%s/state", nodename);
> -        if ((err = xenbus_switch_state(XBT_NIL, frontpath, XenbusStateConnected))
> +        if((err = xenbus_switch_state(XBT_NIL, frontpath, XenbusStateConnected))
>              != NULL) {
> -            printk("error switching state %s\n", err);
> +            printk("error switching state: %s\n", err);
>              xenbus_unwatch_path_token(XBT_NIL, path, path);
>              goto error;
>          }
> @@ -271,6 +271,7 @@
>      return dev;
>  
>  error:
> +    free(msg);
>      free(err);
>      free_pcifront(dev);
>      return NULL;
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
>   

Re: [PATCH] mini-os: Fix xenbus initialisation

[Samuel Thibault]

Jeremy Fitzhardinge, le Thu 08 Apr 2010 12:13:41 -0700, a écrit :
> Reverting this change (b20f897d6010) makes hvm+stubdom work again.

Could you try to revert only parts of them?

Samuel

Re: [PATCH] mini-os: Fix xenbus initialisation

[Jeremy Fitzhardinge]

On 04/08/2010 02:25 PM, Samuel Thibault wrote:
> Jeremy Fitzhardinge, le Thu 08 Apr 2010 12:13:41 -0700, a écrit :
>   
>> Reverting this change (b20f897d6010) makes hvm+stubdom work again.
>>     
> Could you try to revert only parts of them?
>   

I narrowed it down to the first hunk of netfront part of the patch;
applying everything but this works, adding it causes the failure.

    J

diff -r e98537246fe5 extras/mini-os/netfront.c
--- a/extras/mini-os/netfront.c	Thu Apr 08 15:23:00 2010 -0700
+++ b/extras/mini-os/netfront.c	Thu Apr 08 15:23:39 2010 -0700
@@ -407,9 +407,9 @@
     }
 
     snprintf(path, sizeof(path), "%s/state", nodename);
-    err = xenbus_switch_state(xbt, path, XenbusStateConnected);
+    err = xenbus_switch_state(xbt, path, XenbusStateInitialised);
     if (err) {
-        message = "switching state";
+        printk("error writing net initialized: %s\n", err);
         goto abort_transaction;
     }
 

Re: [PATCH] mini-os: Fix xenbus initialisation

[Jeremy Fitzhardinge]

On 04/08/2010 03:39 PM, Jeremy Fitzhardinge wrote:
> On 04/08/2010 02:25 PM, Samuel Thibault wrote:
>   
>> Jeremy Fitzhardinge, le Thu 08 Apr 2010 12:13:41 -0700, a écrit :
>>   
>>     
>>> Reverting this change (b20f897d6010) makes hvm+stubdom work again.
>>>     
>>>       
>> Could you try to revert only parts of them?
>>   
>>     
> I narrowed it down to the first hunk of netfront part of the patch;
> applying everything but this works, adding it causes the failure.
>   

Ping?

Keir, can you revert this at least for now?

    J

>     J
>
> diff -r e98537246fe5 extras/mini-os/netfront.c
> --- a/extras/mini-os/netfront.c	Thu Apr 08 15:23:00 2010 -0700
> +++ b/extras/mini-os/netfront.c	Thu Apr 08 15:23:39 2010 -0700
> @@ -407,9 +407,9 @@
>      }
>  
>      snprintf(path, sizeof(path), "%s/state", nodename);
> -    err = xenbus_switch_state(xbt, path, XenbusStateConnected);
> +    err = xenbus_switch_state(xbt, path, XenbusStateInitialised);
>      if (err) {
> -        message = "switching state";
> +        printk("error writing net initialized: %s\n", err);
>          goto abort_transaction;
>      }
>  
>
>
>   

Re: [PATCH] mini-os: Fix xenbus initialisation

[Samuel Thibault]

Jeremy Fitzhardinge, le Thu 29 Apr 2010 13:36:57 -0700, a écrit :
> On 04/08/2010 03:39 PM, Jeremy Fitzhardinge wrote:
> > On 04/08/2010 02:25 PM, Samuel Thibault wrote:
> >> Jeremy Fitzhardinge, le Thu 08 Apr 2010 12:13:41 -0700, a écrit :
> >>     
> >>> Reverting this change (b20f897d6010) makes hvm+stubdom work again.
> >>>       
> >> Could you try to revert only parts of them?
> >>     
> > I narrowed it down to the first hunk of netfront part of the patch;
> > applying everything but this works, adding it causes the failure.
> >   
> 
> Ping?

Just a "keepalive" answer on this: yes, it's still in my mbox, under
loads of stuff I need to handle in priority :/

The short answer is: I do not understand how that hook could be a
culprit, unless the netback driver itself is bogus. I haven't had the
time to investigate, and will unfortunately probably not have it in the
near future.

Samuel