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

From: Ian Campbell <ian.campbell@citrix.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>, xen-devel@lists.xenproject.org
Cc: Wei Liu <wei.liu2@citrix.com>, Ian Jackson <ian.jackson@eu.citrix.com>
Subject: Re: [PATCH v3 5/5] libxl: add options to enable/disable emulated devices
Date: Thu, 21 Jan 2016 09:39:40 +0000	[thread overview]
Message-ID: <1453369180.26343.181.camel@citrix.com> (raw)
In-Reply-To: <569FD316.3090202@citrix.com>

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@citrix.com>
> > > ---
> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > > Cc: Ian Campbell <ian.campbell@citrix.com>
> > > Cc: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > >  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@lists.xen.org
http://lists.xen.org/xen-devel