Patchwork [XEN,RFC,12/15] xl: Add interface to handle multiple device models

login
register
mail settings
Submitter Julien Grall
Date March 22, 2012, 3:59 p.m.
Message ID <b3914fdf2377f7acbbba73662104f952a59cb6b4.1332430811.git.julien.grall@citrix.com>
Download mbox | patch
Permalink /patch/148323/
State New
Headers show

Comments

Julien Grall - March 22, 2012, 3:59 p.m.
This patch add a structure with contain all informations about
a device model.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 tools/libxl/libxl.h          |    4 ++--
 tools/libxl/libxl_internal.h |    1 +
 tools/libxl/libxl_types.idl  |   11 +++++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)
Ian Campbell - March 23, 2012, 11:47 a.m.
On Thu, 2012-03-22 at 15:59 +0000, Julien Grall wrote:
> This patch add a structure with contain all informations about
> a device model.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> ---
>  tools/libxl/libxl.h          |    4 ++--
>  tools/libxl/libxl_internal.h |    1 +
>  tools/libxl/libxl_types.idl  |   11 +++++++++++
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 6b69030..a347a34 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -357,9 +357,9 @@ typedef struct {
>  typedef struct {
>      libxl_domain_create_info c_info;
>      libxl_domain_build_info b_info;
> -
>      int num_disks, num_vifs, num_pcidevs, num_vfbs, num_vkbs;
> -
> +    int num_dms;
> +    libxl_dm *dms;
>      libxl_device_disk *disks;
>      libxl_device_nic *vifs;
>      libxl_device_pci *pcidevs;
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index e0a1070..247bdb9 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -767,6 +767,7 @@ typedef struct {
>      char *dom_path; /* from libxl_malloc, only for libxl_spawner_record_pid */
>      const char *pid_path; /* only for libxl_spawner_record_pid */
>      int domid;
> +    uint32_t dmid;
>      libxl__spawn_starting *for_spawn;
>  } libxl__spawner_starting;
>  
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 413a1a6..7e48817 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -37,6 +37,7 @@ libxl_domain_type = Enumeration("domain_type", [
>  libxl_device_model_version = Enumeration("device_model_version", [
>      (1, "QEMU_XEN_TRADITIONAL"), # Historical qemu-xen device model (qemu-dm)
>      (2, "QEMU_XEN"),             # Upstream based qemu-xen device model
> +    (3, "MULTIPLE_QEMU_XEN"),    # Handle multiple dm

Isn't this implicit in the provision or otherwise of num_dms?

>      ])
>  
>  libxl_console_type = Enumeration("console_type", [
> @@ -224,6 +225,15 @@ libxl_domain_create_info = Struct("domain_create_info",[
>  
>  MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
>  
> +libxl_dm = Struct("dm", [
> +    ("id",           uint32),
> +    ("name",         string),
> +    ("path",         string),
> +    ("pcis",         libxl_string_list),
> +    ("mmios",        libxl_string_list),
> +    ("pios",         libxl_string_list),
> +    ])

Why does the user of libxl need to know the id? can't that be internal
to the library?

What are name and path? I guess path is something to do with xenstore
but isn't that also internal to the libxl<->dm interface not something
the caller of libxl need be aware of?

I'm not sure what syntax "pcis", "mmios" and "pios" are going to have
but I expect that this would be better represent as actual
datastructures rather than encoding it as a string.

How are toolstack supposed to know the values for e.g. pcis?

All in all this seems like a very raw/low-level interface. Can libxl not
expose something a bit more meaningful to toolstack authors? For example
if we consider emulated disk controllers then perhaps the options are
      * Handled by the "primary" dm
      * Handled by a single disaggregated dm
      * Handled by multiple disaggregated dm's (one per disk controller)
Similarly for other classes or emulated device. Or maybe this should be
a flag on those actual devices (e.g. in libxl_device_FOO)?

> +
>  # Instances of libxl_file_reference contained in this struct which
>  # have been mapped (with libxl_file_reference_map) will be unmapped
>  # by libxl_domain_build/restore. If either of these are never called
> @@ -289,6 +299,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                         ("usbdevice",        string),
>                                         ("soundhw",          string),
>                                         ("xen_platform_pci", libxl_defbool),
> +                                       ("max_servers",     integer),

As a toolstack author how do I decide what number to use here?

Ian.
Julien Grall - March 23, 2012, 1:06 p.m.
On 03/23/2012 11:47 AM, Ian Campbell wrote:
> On Thu, 2012-03-22 at 15:59 +0000, Julien Grall wrote:
>    
>> This patch add a structure with contain all informations about
>> a device model.
>>
>> Signed-off-by: Julien Grall<julien.grall@citrix.com>
>> ---
>>   tools/libxl/libxl.h          |    4 ++--
>>   tools/libxl/libxl_internal.h |    1 +
>>   tools/libxl/libxl_types.idl  |   11 +++++++++++
>>   3 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>> index 6b69030..a347a34 100644
>> --- a/tools/libxl/libxl.h
>> +++ b/tools/libxl/libxl.h
>> @@ -357,9 +357,9 @@ typedef struct {
>>   typedef struct {
>>       libxl_domain_create_info c_info;
>>       libxl_domain_build_info b_info;
>> -
>>       int num_disks, num_vifs, num_pcidevs, num_vfbs, num_vkbs;
>> -
>> +    int num_dms;
>> +    libxl_dm *dms;
>>       libxl_device_disk *disks;
>>       libxl_device_nic *vifs;
>>       libxl_device_pci *pcidevs;
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index e0a1070..247bdb9 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -767,6 +767,7 @@ typedef struct {
>>       char *dom_path; /* from libxl_malloc, only for libxl_spawner_record_pid */
>>       const char *pid_path; /* only for libxl_spawner_record_pid */
>>       int domid;
>> +    uint32_t dmid;
>>       libxl__spawn_starting *for_spawn;
>>   } libxl__spawner_starting;
>>
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 413a1a6..7e48817 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -37,6 +37,7 @@ libxl_domain_type = Enumeration("domain_type", [
>>   libxl_device_model_version = Enumeration("device_model_version", [
>>       (1, "QEMU_XEN_TRADITIONAL"), # Historical qemu-xen device model (qemu-dm)
>>       (2, "QEMU_XEN"),             # Upstream based qemu-xen device model
>> +    (3, "MULTIPLE_QEMU_XEN"),    # Handle multiple dm
>>      
> Isn't this implicit in the provision or otherwise of num_dms?
>    
Sorry but I don't understand the question.

>>       ])
>>
>>   libxl_console_type = Enumeration("console_type", [
>> @@ -224,6 +225,15 @@ libxl_domain_create_info = Struct("domain_create_info",[
>>
>>   MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
>>
>> +libxl_dm = Struct("dm", [
>> +    ("id",           uint32),
>> +    ("name",         string),
>> +    ("path",         string),
>> +    ("pcis",         libxl_string_list),
>> +    ("mmios",        libxl_string_list),
>> +    ("pios",         libxl_string_list),
>> +    ])
>>      
> Why does the user of libxl need to know the id? can't that be internal
> to the library?
>    
Indeed, I will remove that.

> What are name and path? I guess path is something to do with xenstore
> but isn't that also internal to the libxl<->dm interface not something
> the caller of libxl need be aware of?
>    
The path is the binary path and the name is just a string append
to the log filename.

> I'm not sure what syntax "pcis", "mmios" and "pios" are going to have
> but I expect that this would be better represent as actual
> datastructures rather than encoding it as a string.
>
> How are toolstack supposed to know the values for e.g. pcis?
>    
For the moment pcis are describe as bdf for instance: 00:01.1
and mmios/pios as range (0x10-0x20 or just 0x19).
> All in all this seems like a very raw/low-level interface. Can libxl not
> expose something a bit more meaningful to toolstack authors? For example
> if we consider emulated disk controllers then perhaps the options are
>        * Handled by the "primary" dm
>        * Handled by a single disaggregated dm
>        * Handled by multiple disaggregated dm's (one per disk controller)
> Similarly for other classes or emulated device. Or maybe this should be
> a flag on those actual devices (e.g. in libxl_device_FOO)?
>    

I have not really ideas about that. The first solution that comes
in my mind is to bind each device with the device model name.

But in this case, we need to modify QEMU to launch only a subset
of hardware and find a way to said which QEMU will emulate the
"basic" hardware (keyboard, mouse, piix, ...).
Moreover, who will allocate the bdf ? The toolstack ?

>> +
>>   # Instances of libxl_file_reference contained in this struct which
>>   # have been mapped (with libxl_file_reference_map) will be unmapped
>>   # by libxl_domain_build/restore. If either of these are never called
>> @@ -289,6 +299,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>                                          ("usbdevice",        string),
>>                                          ("soundhw",          string),
>>                                          ("xen_platform_pci", libxl_defbool),
>> +                                       ("max_servers",     integer),
>>      
> As a toolstack author how do I decide what number to use here?
>    
The max_servers variables is used to compute the additionnal special pages.
So the max_servers indicates the maximum servers that we want to spawn.
Ian Campbell - March 23, 2012, 1:55 p.m.
On Fri, 2012-03-23 at 13:06 +0000, Julien Grall wrote:
> >> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> >> index 413a1a6..7e48817 100644
> >> --- a/tools/libxl/libxl_types.idl
> >> +++ b/tools/libxl/libxl_types.idl
> >> @@ -37,6 +37,7 @@ libxl_domain_type = Enumeration("domain_type", [
> >>   libxl_device_model_version = Enumeration("device_model_version", [
> >>       (1, "QEMU_XEN_TRADITIONAL"), # Historical qemu-xen device model (qemu-dm)
> >>       (2, "QEMU_XEN"),             # Upstream based qemu-xen device model
> >> +    (3, "MULTIPLE_QEMU_XEN"),    # Handle multiple dm
> >>      
> > Isn't this implicit in the provision or otherwise of num_dms?
> >    
> Sorry but I don't understand the question.

What I mean is can't you tell that the user wants multiple qemus from
the fact that domain_config.num_dms is non-zero (or >1, whichever it
is)?

> > What are name and path? I guess path is something to do with xenstore
> > but isn't that also internal to the libxl<->dm interface not something
> > the caller of libxl need be aware of?
> >    
> The path is the binary path

Are these supposed to supersede libxl_domain_build_info.device_model ?

> and the name is just a string append to the log filename.

This could be internal to libxl I expect.

> > I'm not sure what syntax "pcis", "mmios" and "pios" are going to have
> > but I expect that this would be better represent as actual
> > datastructures rather than encoding it as a string.
> >
> > How are toolstack supposed to know the values for e.g. pcis?
> >    
> For the moment pcis are describe as bdf for instance: 00:01.1
> and mmios/pios as range (0x10-0x20 or just 0x19).

And how does the toolstack know what values to use? How does it decide
that BDF 00:01.1 is something it wants to move into a different DM?

> > All in all this seems like a very raw/low-level interface. Can libxl not
> > expose something a bit more meaningful to toolstack authors? For example
> > if we consider emulated disk controllers then perhaps the options are
> >        * Handled by the "primary" dm
> >        * Handled by a single disaggregated dm
> >        * Handled by multiple disaggregated dm's (one per disk controller)
> > Similarly for other classes or emulated device. Or maybe this should be
> > a flag on those actual devices (e.g. in libxl_device_FOO)?
> >    
> 
> I have not really ideas about that. The first solution that comes
> in my mind is to bind each device with the device model name.
> 
> But in this case, we need to modify QEMU to launch only a subset
> of hardware and find a way to said which QEMU will emulate the
> "basic" hardware (keyboard, mouse, piix, ...).
> Moreover, who will allocate the bdf ? The toolstack ?

I don't know. It's (currently) not the toolstack AFAIK, I guess it is
either the lower level libraries (libxl, libxc) or the device model
itself.

I think this needs figuring out before we can decide what a sensible
libxl interface for for describing which device is emulated by which dm
looks like.

What I want to avoid is the situation where there is something deep in
the bowels of Xen which defines (or even hardcodes) the property that
BDF X:Y.Z corresponds the some device or other and then require that the
upper level tool stack (or, worse, the user) then has to encode that
magic knowledge as a string in the configuration.

> >> +
> >>   # Instances of libxl_file_reference contained in this struct which
> >>   # have been mapped (with libxl_file_reference_map) will be unmapped
> >>   # by libxl_domain_build/restore. If either of these are never called
> >> @@ -289,6 +299,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >>                                          ("usbdevice",        string),
> >>                                          ("soundhw",          string),
> >>                                          ("xen_platform_pci", libxl_defbool),
> >> +                                       ("max_servers",     integer),
> >>      
> > As a toolstack author how do I decide what number to use here?
> >    
> The max_servers variables is used to compute the additionnal special pages.
> So the max_servers indicates the maximum servers that we want to spawn.

We already know this from num_dms, don't we?

Ian.

Patch

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6b69030..a347a34 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -357,9 +357,9 @@  typedef struct {
 typedef struct {
     libxl_domain_create_info c_info;
     libxl_domain_build_info b_info;
-
     int num_disks, num_vifs, num_pcidevs, num_vfbs, num_vkbs;
-
+    int num_dms;
+    libxl_dm *dms;
     libxl_device_disk *disks;
     libxl_device_nic *vifs;
     libxl_device_pci *pcidevs;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e0a1070..247bdb9 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -767,6 +767,7 @@  typedef struct {
     char *dom_path; /* from libxl_malloc, only for libxl_spawner_record_pid */
     const char *pid_path; /* only for libxl_spawner_record_pid */
     int domid;
+    uint32_t dmid;
     libxl__spawn_starting *for_spawn;
 } libxl__spawner_starting;
 
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 413a1a6..7e48817 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -37,6 +37,7 @@  libxl_domain_type = Enumeration("domain_type", [
 libxl_device_model_version = Enumeration("device_model_version", [
     (1, "QEMU_XEN_TRADITIONAL"), # Historical qemu-xen device model (qemu-dm)
     (2, "QEMU_XEN"),             # Upstream based qemu-xen device model
+    (3, "MULTIPLE_QEMU_XEN"),    # Handle multiple dm
     ])
 
 libxl_console_type = Enumeration("console_type", [
@@ -224,6 +225,15 @@  libxl_domain_create_info = Struct("domain_create_info",[
 
 MemKB = UInt(64, init_val = "LIBXL_MEMKB_DEFAULT")
 
+libxl_dm = Struct("dm", [
+    ("id",           uint32),
+    ("name",         string),
+    ("path",         string),
+    ("pcis",         libxl_string_list),
+    ("mmios",        libxl_string_list),
+    ("pios",         libxl_string_list),
+    ])
+
 # Instances of libxl_file_reference contained in this struct which
 # have been mapped (with libxl_file_reference_map) will be unmapped
 # by libxl_domain_build/restore. If either of these are never called
@@ -289,6 +299,7 @@  libxl_domain_build_info = Struct("domain_build_info",[
                                        ("usbdevice",        string),
                                        ("soundhw",          string),
                                        ("xen_platform_pci", libxl_defbool),
+                                       ("max_servers",     integer),
                                        ])),
                  ("pv", Struct(None, [("kernel", libxl_file_reference),
                                       ("slack_memkb", MemKB),