From patchwork Thu Jan 21 10:01:43 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Roger_Pau_Monn=C3=A9?= X-Patchwork-Id: 8079571 Return-Path: X-Original-To: patchwork-xen-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 7C196BEEE5 for ; Thu, 21 Jan 2016 10:05:43 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 23C382054B for ; Thu, 21 Jan 2016 10:05:42 +0000 (UTC) Received: from lists.xen.org (lists.xenproject.org [50.57.142.19]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 276DB20528 for ; Thu, 21 Jan 2016 10:05:40 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xen.org) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aMC4y-0007aG-3U; Thu, 21 Jan 2016 10:03:00 +0000 Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aMC4x-0007a8-Bl for xen-devel@lists.xenproject.org; Thu, 21 Jan 2016 10:02:59 +0000 Received: from [193.109.254.147] by server-13.bemta-14.messagelabs.com id E0/30-08347-2DCA0A65; Thu, 21 Jan 2016 10:02:58 +0000 X-Env-Sender: prvs=8211775ec=roger.pau@citrix.com X-Msg-Ref: server-10.tower-27.messagelabs.com!1453370576!18391093!1 X-Originating-IP: [66.165.176.89] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogNjYuMTY1LjE3Ni44OSA9PiAyMDMwMDc=\n, received_headers: No Received headers X-StarScan-Received: X-StarScan-Version: 7.35.1; banners=-,-,- X-VirusChecked: Checked Received: (qmail 48949 invoked from network); 21 Jan 2016 10:02:57 -0000 Received: from smtp.citrix.com (HELO SMTP.CITRIX.COM) (66.165.176.89) by server-10.tower-27.messagelabs.com with RC4-SHA encrypted SMTP; 21 Jan 2016 10:02:57 -0000 X-IronPort-AV: E=Sophos;i="5.22,324,1449532800"; d="scan'208";a="326691707" To: Ian Campbell , References: <1453291044-83976-1-git-send-email-roger.pau@citrix.com> <1453291044-83976-6-git-send-email-roger.pau@citrix.com> <1453294881.26343.112.camel@citrix.com> <569FD316.3090202@citrix.com> <1453369180.26343.181.camel@citrix.com> From: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= X-Enigmail-Draft-Status: N1110 Message-ID: <56A0AC87.8060405@citrix.com> Date: Thu, 21 Jan 2016 11:01:43 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <1453369180.26343.181.camel@citrix.com> X-DLP: MIA2 Cc: Wei Liu , Ian Jackson Subject: Re: [Xen-devel] [PATCH v3 5/5] libxl: add options to enable/disable emulated devices X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 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é --- --- 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é >>>> --- >>>> Cc: Ian Jackson >>>> Cc: Ian Campbell >>>> Cc: Wei Liu >>>> --- >>>> 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. 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");