| 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
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.
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.
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),
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(-)