[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
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? > > > > 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_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. > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ])), > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ("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. > 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. > 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? 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? Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |