Xen devel - Dec 2011 - [PATCH 2 of 2] mini-os: do not wait for pci backend in pcifront_scan

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1323432077 0
# Node ID d50241faddf372f2262627b9b2071d2a077e8151
# Parent  9c1b223e152eaaa3861f9b6132590de0b4f6cb7e
mini-os: do not wait for pci backend in pcifront_scan

This blocks the main thread indefinitely if there is no PCI backend present in
xenstore.

Even in the case where there are passthrough devices configured libxl creates
the stubdom and waits for it to startup _before_ adding the backend. Since the
stub domains main thread is blocked before it can write the "running"
state to
xenstore the toolstack eventually times out and kills everything.

There is already a separate pcifront thread which waits for the backend to
appear and calls init_pcifront at the appropriate time should a backend ever
appear.

Unfortunately I don''t have any free test boxes with VT-d so I
haven''t been able
to test the cases where PCI deivces are passed through but I obviously have
tested that I can now start an HVM domain with stub qemu without PCI devices
passed through which I couldn''t do before so this is an improvement.
This stuff
is a bit like pushing the lump around the carpet :-/

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 9c1b223e152e -r d50241faddf3 extras/mini-os/kernel.c
--- a/extras/mini-os/kernel.c	Fri Dec 09 12:01:16 2011 +0000
+++ b/extras/mini-os/kernel.c	Fri Dec 09 12:01:17 2011 +0000
@@ -448,6 +448,7 @@ static void print_pcidev(unsigned int do
 
 static void pcifront_thread(void *p)
 {
+    pcifront_watches(NULL);
     pci_dev = init_pcifront(NULL);
     if (!pci_dev)
         return;
diff -r 9c1b223e152e -r d50241faddf3 extras/mini-os/pcifront.c
--- a/extras/mini-os/pcifront.c	Fri Dec 09 12:01:16 2011 +0000
+++ b/extras/mini-os/pcifront.c	Fri Dec 09 12:01:17 2011 +0000
@@ -280,23 +280,14 @@ void pcifront_scan(struct pcifront_dev *
 {
     char *path;
     int i, n, len;
-    char *s, *msg = NULL, *err = NULL;
+    char *s, *msg = NULL;
     unsigned int domain, bus, slot, fun;
 
     if (!dev)
         dev = pcidev;
     if (!dev) {
-        xenbus_event_queue events = NULL;
-        char *fe_state = "device/pci/0/state";
-        xenbus_watch_path_token(XBT_NIL, fe_state, fe_state, &events);
-        while ((err = xenbus_read(XBT_NIL, fe_state, &msg)) != NULL ||
msg[0] != ''4'') {
-            free(msg);
-            free(err);
-            printk("pcifront_scan: waiting for pcifront to become
ready\n");
-            xenbus_wait_for_watch(&events);
-        }
-        xenbus_unwatch_path_token(XBT_NIL, fe_state, fe_state);
-        dev = pcidev;
+	printk("pcifront_scan: device or bus\n");
+	return;
     }
 
     len = strlen(dev->backend) + 1 + 5 + 10 + 1;

Ian Campbell writes ("[PATCH 2 of 2] mini-os: do not wait for pci backend
in pcifront_scan"):
> Even in the case where there are passthrough devices configured libxl
creates
> the stubdom and waits for it to startup _before_ adding the backend. Since
the
> stub domains main thread is blocked before it can write the
"running" state to
> xenstore the toolstack eventually times out and kills everything.
> 
> There is already a separate pcifront thread which waits for the backend to
> appear and calls init_pcifront at the appropriate time should a backend
ever
> appear.
> 
> Unfortunately I don''t have any free test boxes with VT-d so I
> haven''t been able to test the cases where PCI deivces are passed
> through but I obviously have tested that I can now start an HVM
> domain with stub qemu without PCI devices passed through which I
> couldn''t do before so this is an improvement. This stuff is a bit
> like pushing the lump around the carpet :-/
Right.  The worry would be, surely, that this somehow breaks by
unpausing the guest before everything has been set up by the stubdom.

But I''m happy to ack this patch on the basis that it seemed to improve
things for you and should be harmless for the non-stubdom case.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

(Not applying it yet as the patch floodgate is still closed pending a
test pass.)

Ian.

On Fri, 2011-12-09 at 16:56 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH 2 of 2] mini-os: do not wait for pci
backend in pcifront_scan"):
> > Even in the case where there are passthrough devices configured libxl
creates
> > the stubdom and waits for it to startup _before_ adding the backend.
Since the
> > stub domains main thread is blocked before it can write the
"running" state to
> > xenstore the toolstack eventually times out and kills everything.
> > 
> > There is already a separate pcifront thread which waits for the
backend to
> > appear and calls init_pcifront at the appropriate time should a
backend ever
> > appear.
> > 
> > Unfortunately I don''t have any free test boxes with VT-d so I
> > haven''t been able to test the cases where PCI deivces are
passed
> > through but I obviously have tested that I can now start an HVM
> > domain with stub qemu without PCI devices passed through which I
> > couldn''t do before so this is an improvement. This stuff is a
bit
> > like pushing the lump around the carpet :-/
> 
> Right.  The worry would be, surely, that this somehow breaks by
> unpausing the guest before everything has been set up by the stubdom.
Yes. I was a bit unclear how this ever worked though...

We could potentially add something to libxl__create_pci_backend which
interlocks against the stubdom pcifront but without a system with an
IOMMU I''m not really in a position to test such a patch.
> But I''m happy to ack this patch on the basis that it seemed to
improve
> things for you and should be harmless for the non-stubdom case.
Thanks.
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> (Not applying it yet as the patch floodgate is still closed pending a
> test pass.)
Sure.

Ian.