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

["Roger Pau Monné" <roger.pau@citrix.com>]

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@citrix.com>
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@citrix.com>
    ---
    ---
    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@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/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@lists.xen.org
http://lists.xen.org/xen-devel