|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 5/5] libxl: add options to enable/disable emulated devices
El 21/01/16 a les 10.39, Ian Campbell ha escrit:
> On Wed, 2016-01-20 at 19:33 +0100, Roger Pau Monnà wrote:
>> 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?
>
> No, part of the purpose of defbool and the other "do the default" values is
> that we can evolve things over time.
>
>> 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.
>
> Wei, isn't this (turning the defaults into concrete values) supposed to be
> taken care of by the JSON mangling which you added?
Heh, I think you mean the JSON mangling added by Wei. In order to
propagate the values filled by default in libxl_domain_config I had to
add the following patch, which basically calls the _setdefault
functions before converting the domain_config into JSON. I'm planning
to make it part of this series in the next iteration:
---
commit b1b2cea4b61ce9bd05797d3dc5ff0f5fffccfd05
Author: Roger Pau Monne <roger.pau@xxxxxxxxxx>
Date: Wed Jan 20 19:06:50 2016 +0100
libxl: introduce libxl_domain_info_setdefault to the public API
The newly introduced function populates the libxl_domain_config with their
default values, just like it's done during domain creation.
This is needed so the libxl_domain_config sent to the restore side during
migration is accurate, since default values might change between libxl
versions.
Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
---
---
NB: I'm unsure whether this is a bug or not. IMHO I think it is, because
there's no guarantee that the default values will stay the same between
libxl versions, so a domain created with an old libxl version might see
differences when migrated to a newer libxl version.
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 157f07c..70bb6e1 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -886,6 +886,15 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst,
libxl_mac *src);
*/
#define LIBXL_HAVE_VGA_INTERFACE_TYPE_UNKNOWN 1
+/*
+ * LIBXL_HAVE_DOMAIN_INFO_SETDEFAULT
+ *
+ * In the case that LIBXL_HAVE_DOMAIN_INFO_SETDEFAULT is set libxl
+ * provides the libxl_domain_info_setdefault function that can be used
+ * to set the libxl_domain_config fields to their default values.
+ */
+#define LIBXL_HAVE_DOMAIN_INFO_SETDEFAULT 1
+
typedef char **libxl_string_list;
void libxl_string_list_dispose(libxl_string_list *sl);
int libxl_string_list_length(const libxl_string_list *sl);
@@ -1202,6 +1211,9 @@ int libxl_domain_soft_reset(libxl_ctx *ctx,
void libxl_domain_config_init(libxl_domain_config *d_config);
void libxl_domain_config_dispose(libxl_domain_config *d_config);
+/* Fill the libxl_domain_config struct with their default values. */
+int libxl_domain_info_setdefault(libxl_ctx *ctx, libxl_domain_config
*d_config);
+
/*
* Retrieve domain configuration and filled it in d_config. The
* returned configuration can be used to rebuild a domain. It only
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index c7700a7..c988c2e 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -886,17 +886,10 @@ static void initiate_domain_create(libxl__egc *egc,
goto error_out;
}
- ret = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
- if (ret) {
- LOG(ERROR, "Unable to set domain create info defaults");
- goto error_out;
- }
-
- ret = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
- if (ret) {
- LOG(ERROR, "Unable to set domain build info defaults");
+ ret = libxl_domain_info_setdefault(CTX, d_config);
+ if (ret)
+ /* libxl_domain_info_setdefault already logs errors. */
goto error_out;
- }
ret = libxl__domain_make(gc, d_config, &domid, &state->config);
if (ret) {
@@ -1804,6 +1797,26 @@ int libxl_domain_soft_reset(libxl_ctx *ctx,
aop_console_how);
}
+int libxl_domain_info_setdefault(libxl_ctx *ctx, libxl_domain_config *d_config)
+{
+ GC_INIT(ctx);
+ int rc;
+
+ rc = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
+ if (rc) {
+ LOG(ERROR, "Unable to set domain create info defaults");
+ return rc;
+ }
+ rc = libxl__domain_build_info_setdefault(gc, &d_config->b_info);
+ if (rc) {
+ LOG(ERROR, "Unable to set domain build info defaults");
+ return rc;
+ }
+
+ GC_FREE;
+ return 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 25507c7..0454efa 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4044,6 +4044,14 @@ static void save_domain_core_begin(uint32_t domid,
}
}
+#ifdef LIBXL_HAVE_DOMAIN_INFO_SETDEFAULT
+ rc = libxl_domain_info_setdefault(ctx, &d_config);
+ if (rc) {
+ fprintf(stderr, "unable to set default configuration values\n");
+ exit(2);
+ }
+#endif
+
config_c = libxl_domain_config_to_json(ctx, &d_config);
if (!config_c) {
fprintf(stderr, "unable to convert config file to JSON\n");
>>
>>>> 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/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_st
>>>> ring_list),
>>>> ("rdm", libxl_rdm_reserve),
>>>> ("rdm_mem_boundary_memkb",
>>>> MemKB),
>>>> + ("lapic", libxl_de
>>>> fbool),
>>>> + ("ioapic", libxl_de
>>>> fbool),
>>>> + ("rtc", libxl_de
>>>> fbool),
>>>> + ("power_management",
>>>> libxl_defbool),
>>>> + ("pic", libxl_de
>>>> fbool),
>>>> + ("pit", libxl_de
>>>> fbool),
>>>
>>> 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.
>
> I meant a substruct of hvm (i.e. vhm.emul_opts), but your point is also
> valid.
I would probably place them in the hvm struct, since it already
contains a hpet defbool.
>>>> ])),
>>>> ("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.
>
> But it must _always_ be enforced by Xen as the last resort.
Yes, that's already done, Xen always has the last word on what gets
enabled or not.
>
>> 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.
>
> We would need to add a new defbool for the new feature.
Yes, but I was thinking more in the direction of enabling them, rather
than adding new ones.
>
>> 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.
>
> ... and the reason these details can't be propagated via the Xen blob is
> that this emul stuff needs to be set exactly once at domain create time I
> suppose? Changing it to be later binding is considered to be too hard/too
> big a yak?
Right, emulated devices are initialised as part of the
XEN_DOMCTL_createdomain hypercall. Allowing them to be added later on
and introducing a kind of intermediate domain building phase where only
a certain set of hypercalls are valid is a possibility that Andrew
already pointed out, however this seems like a very big project.
> Even with the set of devices set at domain creation time Xen needs to take
> care when reading its blob, and not fall apart (from a security PoV, it's
> allowed to fail the state load) when presented with a save record relating
> to something which is supposedly disabled. Has this been checked?
Yes, trying to load a state into a disable device will result in
-ENODEV.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |