Re: [Qemu-devel] [Xen-devel] [XEN][RFC PATCH 12/15] xl: Add interface to handle multiple device models

[Julien Grall <julien.grall@citrix.com>]

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.