Re: [Xen-devel] [PATCH v3 5/5] libxl: add options to enable/disable emulated devices

[Roger Pau Monnà <roger.pau@xxxxxxxxxx>]

El 20/01/16 a les 14.01, Ian Campbell ha escrit:
> On Wed, 2016-01-20 at 12:57 +0100, Roger Pau Monne wrote:
>> Allow enabling or disabling emulated devices from the libxl domain
>> configuration file. For HVM guests with a device model all the emulated
>> devices are enabled. For HVM guests without a device model no devices are
>> enabled by default, although they can be enabled using the options
>> provided.
>> The arbiter of whether a combination is posible or not is always Xen,
>> libxl
>> doesn't do any kind of check.
>>
>> This set of options is also propagated inside of the libxl migration record
>> as part of the contents of the libxl_domain_build_info struct.
> 
> ... and this is the real motivation for this change, not actually allowing
> users to control all this AIUI.
> 
> Did you check that the fields updated using libxl_defbool_setdefault are
> actually updated in the JSON copy and therefore propagated to the other
> side of a migration as specific values and not as "pick a default"? I think
> we don't want these changing on migration. I think/hope all this was
> automatically handled by the work Wei did in the last release cycle.

No, values populated by the {build/create}_info_setdefault functions are
not propagated, OTOH values manually set by the user in the config file
are indeed propagated. Do we have any guarantee that _setdefault is
always going to behave in the same way?

If we don't have that guarantee I think this is already a bug, and we
should call _setdefault before sending the domain info to the other end.
In fact I have a patch that does exactly that, but I'm unsure if it's
needed because I don't know the policy regarding default values in libxl.

>> Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
>> ---
>> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
>> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
>> ---
>>  docs/man/xl.cfg.pod.5       | 39 +++++++++++++++++++++++++++++++++++++++
>>  tools/libxl/libxl.h         | 11 +++++++++++
>>  tools/libxl/libxl_create.c  | 21 ++++++++++++++++++++-
>>  tools/libxl/libxl_types.idl |  6 ++++++
>>  tools/libxl/libxl_x86.c     | 33 ++++++++++++++++++++++++++++-----
>>  tools/libxl/xl_cmdimpl.c    |  7 +++++++
>>  6 files changed, 111 insertions(+), 6 deletions(-)
>>
>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
>> index 8899f75..46d4529 100644
>> --- a/docs/man/xl.cfg.pod.5
>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -1762,6 +1762,45 @@ See F<docs/misc/pci-device-reservations.txt> for
>> more information.
>>  
>>  =back
>>  
>> +=head3 HVM without a device model options
>> +
>> +This options can be used to change the set of emulated devices provided
> 
> "These..."
> 
>> +to guests without a device model. Note that Xen might not support all
>> +possible combinations. By default HVM guests without a device model
>> +don't have any of them enabled.
> 
> ... and for those with a device model? The title and text suggest these
> options are invalid/ignored in that case, but the code does actually honour
> what the user specified in this case.

Right, I've clarified this by adding the following paragraph:

"It is important to notice that these options (except the hpet one) are
not available to HVM guests with a device model, and trying to set them
will cause xl to exit with an error."

I've also fixed up the code in libxl__domain_build_info_setdefault to
actually error out if a HVM guest with device model tries to set any of
them.

>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 92c95e5..8a21cda 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -519,6 +519,12 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>                                         ("serial_list",      
>> libxl_string_list),
>>                                         ("rdm", libxl_rdm_reserve),
>>                                         ("rdm_mem_boundary_memkb", MemKB),
>> +                                       ("lapic",            libxl_defbool),
>> +                                       ("ioapic",           libxl_defbool),
>> +                                       ("rtc",              libxl_defbool),
>> +                                       ("power_management", libxl_defbool),
>> +                                       ("pic",              libxl_defbool),
>> +                                       ("pit",              libxl_defbool),
> 
> I wonder if these should go in a sub-struct. Although you might reasonably
> argue that this is already such a dumping ground it doesn't matter...

Right, TBH I saw that ARM added an arch_arm sub-struct, which sounds
fine and should have been done earlier. Now the hvm sub-struct is
already so x86 specific that, as you said, I don't think it matters much.

>>                                         ])),
>>                   ("pv", Struct(None, [("kernel", string),
>>                                        ("slack_memkb", MemKB),
>> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
>> index 46cfafb..92f25fd 100644
>> --- a/tools/libxl/libxl_x86.c
>> +++ b/tools/libxl/libxl_x86.c
>> @@ -7,15 +7,38 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>                                        libxl_domain_config *d_config,
>>                                        xc_domain_configuration_t
>> *xc_config)
>>  {
>> +    struct libxl_domain_build_info *info = &d_config->b_info;
>>  
>> -    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
>> -        d_config->b_info.device_model_version !=
>> -        LIBXL_DEVICE_MODEL_VERSION_NONE) {
>> -        /* HVM domains with a device model. */
> 
> So, I'm not 100% clear on why this check and the corresponding logic to set
> xc_config->emulation_flags is not also sufficient for after migration.
> Could you explain (and likely eventually add the rationale to the commit
> message).

As I understand this, we want to avoid having two different places where
the policy (ie: the set of enabled devices) is enforced.

With the current code, libxl basically limits the set of allowed masks
to what it knows. After the change libxl just becomes a proxy for
transmitting what the user has selected to Xen, and Xen either accepts
or refuses it, basically making Xen the only arbiter that decides which
emulated devices get enabled or not. This means that if we want to make
more emulated devices available to the guest in the future no libxl
changes will be required.

It also means that HVMlite guests created with current Xen will be
capable of migrating to newer version of Xen, that might have a
different default policy. For example in the future we might want to
enable the lapic by default, so if a guest is created with the current
Xen version it doesn't get a lapic at all, and then when migrated to
newer versions a lapic would magically appear after the migration, which
is not desired.

Roger.


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