diff mbox

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

Message ID 56A0AC87.8060405@citrix.com
State New, archived
Headers show

Commit Message

Roger Pau Monné Jan. 21, 2016, 10:01 a.m. UTC
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.





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

Comments

Ian Campbell Jan. 21, 2016, 10:29 a.m. UTC | #1
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.
Roger Pau Monné Jan. 21, 2016, 11:10 a.m. UTC | #2
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.
Wei Liu Jan. 21, 2016, 11:31 a.m. UTC | #3
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.
Roger Pau Monné Jan. 21, 2016, 3:55 p.m. UTC | #4
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 mbox

Patch

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");