Re: [Xen-devel] [PATCH v2 4/5] libxl: change xs path for pv qemu

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 9 Jun 2015, Stefano Stabellini wrote:
> On Tue, 9 Jun 2015, Wei Liu wrote:
> > On Mon, Jun 08, 2015 at 04:27:26PM +0100, Stefano Stabellini wrote:
> > > On Mon, 8 Jun 2015, Wei Liu wrote:
> > > > On Thu, Jun 04, 2015 at 12:28:18PM +0100, Stefano Stabellini wrote:
> > > > > If QEMU is ran just to provide PV backends, change the xenstore path 
> > > > > to
> > > > > /local/domain/0/device-model/$DOMID/pv.
> > > > > 
> > > > > Add a parameter to libxl__device_model_xs_path to distinguish the 
> > > > > device
> > > > > model from the pv backends provider.
> > > > > 
> > > > > Store the device model binary path under
> > > > > /local/domain/$DOMID/device-model on xenstore, so that we can fetch it
> > > > > later and retrieve the list of supported options from
> > > > > /local/domain/0/libxl/$device_model_binary, introduce in the previous
> > > > > path.
> > > > > 
> > > > 
> > > > TBH this protocol works, but it is not very extensible.
> > > > 
> > > > I envisaged we need to assign $emulator_id to different device models
> > > > when I fixed stubdom, but I never got to that since there weren't need
> > > > for multiple emulators.
> > > > 
> > > > That is, as an example:
> > > > 
> > > > /local/domain/$backend_domid/device-model/$domid/$emulator_id/xxx
> > > > 
> > > > That way we can:
> > > > 
> > > > 1. Have something like multidev in libxl to wait for several device
> > > >    models to be ready without writing tedious code for every single one.
> > > > 2. Fit into libxl migration stream, which naturally uses $emulator_id to
> > > >    distinguish different emulators. 
> > > > 
> > > > The downside of this is we need to add an extra option to QEMU to accept
> > > > the emulator id assigned by toolstack.
> > > > 
> > > > Just my two cents.
> > > 
> > > Actually QEMU already retrieves the ioservid by calling
> > > xc_hvm_create_ioreq_server. I could easily use that as id. I am not sure
> > > how I could get that from libxl though, except by assuming that is going
> > > to be 0, because libxl only knows how to create one QEMU emulator.
> > > 
> > 
> > I don't think it's good to use that id directly because as you said
> > there is no way to allocate and get that from libxl. Libxl needs to
> > arrange xenstore directory before QEMU starts up, i.e. before QEMU is
> > able to allocate such id from hypervisor.
> > 
> > Assigning an emulator id would be easy in libxl.
> 
> Adding another option to QEMU is easy. However it is hard to figure out
> which one is the right path from libxl__device_model_xs_path, see:
> 
> char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
>                                           uint32_t domid, const char
>                                           *format,  ...)
> 
> As you can see from this patch, libxl__device_model_xs_path is called in
> many places. Some of them have very little knowledge about QEMU (see for
> example libxl__toolstack_save and libxl_domain_unpause), so it would be
> difficult to find the right emulator_id, unless we assume that the
> device model is emulator_id=0 and pv qemu emulator_id=1. 

To be clear, changing the xenstore protocol to what you suggested is a
good idea and I am happy to do so. Changing the libxl internals to
support multiple emulator_ids is difficult. Unless we do something hacky
like the following:

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index b11297b..7b0c989 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -106,6 +106,9 @@
 #define TAP_DEVICE_SUFFIX "-emu"
 #define DISABLE_UDEV_PATH "libxl/disable_udev"
 #define DOMID_XS_PATH "domid"
+/* For the moment assume only 2 QEMU are present */
+#define QEMU_XEN_DEVICE_MODEL_ID 0
+#define QEMU_XEN_PV_QEMU_ID 1
 
 #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index c2c67bd..2780fd8 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -567,14 +567,25 @@ void libxl__update_domain_configuration(libxl__gc *gc,
     dst->b_info.video_memkb = src->b_info.video_memkb;
 }
 
-char *libxl__device_model_xs_path(libxl__gc *gc, uint32_t dm_domid,
+char *libxl__device_model_xs_path(libxl__gc *gc, bool pvqemu, uint32_t 
dm_domid,
                                   uint32_t domid, const char *format,  ...)
 {
     char *s, *fmt;
     va_list ap;
-
-    fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
-                    domid, format);
+    char *dm;
+    int emulator_id = 0;
+
+    dm = libxl__xs_read(gc, XBT_NULL, 
GCSPRINTF("/local/domain/%u/device-model", domid));
+    if (dm)
+        emulator_id = libxl__check_qemu_supported(gc, dm, "emulator_id");
+
+    if (!emulator_id) {
+        fmt = GCSPRINTF("/local/domain/%u/device-model/%u%s", dm_domid,
+                domid, format);
+    } else {
+        fmt = GCSPRINTF("/local/domain/%u/device-model/%u/%u%s", dm_domid,
+                domid, pvqemu ? QEMU_XEN_PV_QEMU_ID : 
QEMU_XEN_DEVICE_MODEL_ID, format);
+    }
 
     va_start(ap, format);
     s = libxl__vsprintf(gc, fmt, ap); 

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