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

[Ian Campbell <ian.campbell@citrix.com>]

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.

> 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.

> 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...

>                                         ])),
>                   ("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).

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