| Message ID | 56A0AC87.8060405@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2016-01-21 at 11:01 +0100, Roger Pau Monné wrote: > El 21/01/16 a les 10.39, Ian Campbell ha escrit: > > > 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. Correct. > 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: I'll let Wei comment on why this isn't already done. > > > 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. Which would then require changing the defbool_set_default in libxl after this change, so you do still need to change libxl. > > > 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. This seems like the right approach to me, but I appreciate you not wanting to tackle this here and now. Would it be possible to set the set of "potential" emulated devices at create time and only "commit" to it after the save state is loaded? That would essentially mean init-all, load state, de-init those which didn't get any state loaded? Not as nice as doing it with the split of hypercall availability, but might be more achievable, since you already have the de- init code for domain teardown time. In any case, whatever is chosen as the solution the commit message needs to go into a fair amount of detail as to why we picked that way of doing things, particularly if it is a compromise vs doing it properly. This is important so we can answer "why did we do it this way" in 2 years time. > > 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. Grand. Ian.
El 21/01/16 a les 11.29, Ian Campbell ha escrit: > On Thu, 2016-01-21 at 11:01 +0100, Roger Pau Monné wrote: >> El 21/01/16 a les 10.39, Ian Campbell ha escrit: >>>> 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. > > This seems like the right approach to me, but I appreciate you not wanting > to tackle this here and now. > > Would it be possible to set the set of "potential" emulated devices at > create time and only "commit" to it after the save state is loaded? That > would essentially mean init-all, load state, de-init those which didn't get > any state loaded? Not as nice as doing it with the split of hypercall > availability, but might be more achievable, since you already have the de- > init code for domain teardown time. Sadly there are devices that AFAICT don't restore any state (like the VGA), which means a more complex mechanism is needed and we cannot rely exclusively on restores in order to decide if a device is present or not. IMHO the current approach is not that bad, and I think we should be able to migrate to a better one without problems. If in the future we want to do something like what you describe above (setting the set of emulated devices based on the Xen context restored), the extra information in the libxl JSON stream is certainly not going to hurt us. At worst we could always check that the libxl JSON information matches with what the hypervisor context contains for extra safety. > In any case, whatever is chosen as the solution the commit message needs to > go into a fair amount of detail as to why we picked that way of doing > things, particularly if it is a compromise vs doing it properly. > > This is important so we can answer "why did we do it this way" in 2 years > time. Right, thanks, I will update the commit message with the outcome of this discussion. Roger.
On Thu, Jan 21, 2016 at 11:01:43AM +0100, Roger Pau Monné wrote: > 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: The requirement of recording decision made in libxl and pass that to the receiving end is not new. We had the same problem for uuid, disk and some other things. The first way of doing it is to update JSON before it is sent -- see libxl.c:libxl_retrieve_domain_configuration. It uses the stashed JSON file as template and pull in various bits from hypervisor and xenstore. Your need of recording what emulated devices are available fits here. You just need to provide a way to retrieve those bits in that function. Another way of doing it is to update the stashed JSON template before it is saved. See libxl_internal.c:libxl__update_domain_configuration. I think this might be easier than the first way of doing it. You should not export the _setdefault function to xl because it is a layer violation. Hope this clarify things. Wei.
El 21/01/16 a les 12.31, Wei Liu ha escrit: > On Thu, Jan 21, 2016 at 11:01:43AM +0100, Roger Pau Monné wrote: >> 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: > > The requirement of recording decision made in libxl and pass that to the > receiving end is not new. We had the same problem for uuid, disk and > some other things. > > The first way of doing it is to update JSON before it is sent -- see > libxl.c:libxl_retrieve_domain_configuration. It uses the stashed JSON > file as template and pull in various bits from hypervisor and xenstore. > Your need of recording what emulated devices are available fits here. > You just need to provide a way to retrieve those bits in that function. > > Another way of doing it is to update the stashed JSON template before it > is saved. See libxl_internal.c:libxl__update_domain_configuration. I > think this might be easier than the first way of doing it. > > You should not export the _setdefault function to xl because it is a > layer violation. > > Hope this clarify things. Hello, Yes, thank you very much, it has indeed clarified things. I've implemented it inside of libxl__update_domain_configuration without issues. Roger.
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");