From patchwork Fri Nov 29 14:32:21 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Paul Durrant X-Patchwork-Id: 11267211 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 3AFF815AB for ; Fri, 29 Nov 2019 14:33:54 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0C2A721736 for ; Fri, 29 Nov 2019 14:33:53 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=amazon.com header.i=@amazon.com header.b="XLRes0U7" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0C2A721736 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=amazon.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iahK8-0002Ww-1m; Fri, 29 Nov 2019 14:32:44 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iahK6-0002Wo-Oq for xen-devel@lists.xenproject.org; Fri, 29 Nov 2019 14:32:42 +0000 X-Inumbo-ID: 19b8ea80-12b5-11ea-9db0-bc764e2007e4 Received: from smtp-fw-9101.amazon.com (unknown [207.171.184.25]) by us1-rack-iad1.inumbo.com (Halon) with ESMTPS id 19b8ea80-12b5-11ea-9db0-bc764e2007e4; Fri, 29 Nov 2019 14:32:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amazon.com; i=@amazon.com; q=dns/txt; s=amazon201209; t=1575037963; x=1606573963; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=ygAHswZ1xcqJSzp4WCJ6AaoZbuYZiSUXn7cDbJPgkdk=; b=XLRes0U7QbQe4dtwNeCWnIGCs815GG8Hrl57Y+9icXuHFLi907j9yR2G xmEWJOpmyiaWZfzTw5CC1VVFqEIm4z2fNMISjrsq/c3s8YHrTEttbZIrS C6D5jtkd1Yo9WRjA43t2YPZDX3It2BcuCv9ILuAvhk9vPAYd2yoa5rU9P 8=; IronPort-SDR: z9W/7SYVNE4M4SBwnFXNTDwOZOVK1uStkYhV1PvKOOGI+4Jkrc9lWNph+rsM+k3wcKVIqjSmom 2QxV7ed4pvOA== X-IronPort-AV: E=Sophos;i="5.69,257,1571702400"; d="scan'208";a="2180830" Received: from sea32-co-svc-lb4-vlan3.sea.corp.amazon.com (HELO email-inbound-relay-2c-168cbb73.us-west-2.amazon.com) ([10.47.23.38]) by smtp-border-fw-out-9101.sea19.amazon.com with ESMTP; 29 Nov 2019 14:32:31 +0000 Received: from EX13MTAUEA001.ant.amazon.com (pdx4-ws-svc-p6-lb7-vlan2.pdx.amazon.com [10.170.41.162]) by email-inbound-relay-2c-168cbb73.us-west-2.amazon.com (Postfix) with ESMTPS id 741A0A1C24; Fri, 29 Nov 2019 14:32:29 +0000 (UTC) Received: from EX13D32EUB004.ant.amazon.com (10.43.166.212) by EX13MTAUEA001.ant.amazon.com (10.43.61.243) with Microsoft SMTP Server (TLS) id 15.0.1367.3; Fri, 29 Nov 2019 14:32:28 +0000 Received: from EX13MTAUWC001.ant.amazon.com (10.43.162.135) by EX13D32EUB004.ant.amazon.com (10.43.166.212) with Microsoft SMTP Server (TLS) id 15.0.1367.3; Fri, 29 Nov 2019 14:32:27 +0000 Received: from u2f063a87eabd5f.cbg10.amazon.com (10.125.106.135) by mail-relay.amazon.com (10.43.162.232) with Microsoft SMTP Server id 15.0.1367.3 via Frontend Transport; Fri, 29 Nov 2019 14:32:23 +0000 From: Paul Durrant To: Date: Fri, 29 Nov 2019 14:32:21 +0000 Message-ID: <20191129143221.31850-1-pdurrant@amazon.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 Precedence: Bulk Subject: [Xen-devel] [PATCH-for-4.13 v6] Rationalize max_grant_frames and max_maptrack_frames handling X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Juergen Gross , Stefano Stabellini , Julien Grall , Wei Liu , Konrad Rzeszutek Wilk , George Dunlap , Andrew Cooper , Paul Durrant , Ian Jackson , George Dunlap , =?utf-8?q?Marek_Marczykowski-G?= =?utf-8?q?=C3=B3recki?= , Jan Beulich , Anthony PERARD , Volodymyr Babchuk , =?utf-8?q?Roger_Pau_Monn?= =?utf-8?q?=C3=A9?= Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" From: George Dunlap Xen used to have single, system-wide limits for the number of grant frames and maptrack frames a guest was allowed to create. Increasing or decreasing this single limit on the Xen command-line would change the limit for all guests on the system. Later, per-domain limits for these values was created. The system-wide limits became strict limits: domains could not be created with higher limits, but could be created with lower limits. However, that change also introduced a range of different "default" values into various places in the toolstack: - The python libxc bindings hard-coded these values to 32 and 1024, respectively - The libxl default values are 32 and 1024 respectively. - xl will use the libxl default for maptrack, but does its own default calculation for grant frames: either 32 or 64, based on the max possible mfn. These defaults interact poorly with the hypervisor command-line limit: - The hypervisor command-line limit cannot be used to raise the limit for all guests anymore, as the default in the toolstack will effectively override this. - If you use the hypervisor command-line limit to *reduce* the limit, then the "default" values generated by the toolstack are too high, and all guest creations will fail. In other words, the toolstack defaults require any change to be effected by having the admin explicitly specify a new value in every guest. In order to address this, have grant_table_init treat negative values for max_grant_frames and max_maptrack_frames as instructions to use the system-wide default, and have all the above toolstacks default to passing -1 unless a different value is explicitly configured. This restores the old behavior in that changing the hypervisor command-line option can change the behavior for all guests, while retaining the ability to set per-guest values. It also removes the bug that reducing the system-wide max will cause all domains without explicit limits to fail. NOTE: - The Ocaml bindings require the caller to always specify a value, and the code to start a xenstored stubdomain hard-codes these to 4 and 128 respectively; this behavour will not be modified. Signed-off-by: George Dunlap Signed-off-by: Paul Durrant Reviewed-by: Jan Beulich Release-acked-by: Juergen Gross --- Cc: Ian Jackson Cc: Wei Liu Cc: Andrew Cooper Cc: George Dunlap Cc: Julien Grall Cc: Konrad Rzeszutek Wilk Cc: Stefano Stabellini Cc: Anthony PERARD Cc: "Marek Marczykowski-Górecki" Cc: Volodymyr Babchuk Cc: "Roger Pau Monné" v6: - Add missing assignment in parse_gnttab_limit() - Deal with EINVAL return from xlu_cfg_get_bounded_long() - Add LIBXL_HAVE_BUILDINFO_SIGNED_GRANT_LIMITS v5: - Remove erroneous __init annotations - Fail out of range command line values with ERANGE - Make opt_max_maptrack_frames static v4: - Add missing braces in xlu_cfg_get_bounded_long() v3: - Make sure that specified values cannot be negative or overflow a signed int v2: - re-worked George's original commit massage a little - fixed the text in xl.conf.5.pod - use -1 as the sentinel value for 'default' and < 0 for checking it --- docs/man/xl.conf.5.pod | 6 ++-- tools/libxl/libxl.h | 11 ++++++-- tools/libxl/libxl_types.idl | 4 +-- tools/libxl/libxlu_cfg.c | 26 +++++++++++++++-- tools/libxl/libxlutil.h | 2 ++ tools/python/xen/lowlevel/xc/xc.c | 4 +-- tools/xl/xl.c | 22 ++++++++------- tools/xl/xl_parse.c | 19 +++++++++---- xen/arch/arm/setup.c | 2 +- xen/arch/x86/setup.c | 4 +-- xen/common/grant_table.c | 47 +++++++++++++++++++++++++++---- xen/include/public/domctl.h | 10 ++++--- xen/include/xen/grant_table.h | 10 +++---- 13 files changed, 124 insertions(+), 43 deletions(-) diff --git a/docs/man/xl.conf.5.pod b/docs/man/xl.conf.5.pod index 962144e38e..207ab3e77a 100644 --- a/docs/man/xl.conf.5.pod +++ b/docs/man/xl.conf.5.pod @@ -81,13 +81,15 @@ Default: C Sets the default value for the C domain config value. -Default: C<32> on hosts up to 16TB of memory, C<64> on hosts larger than 16TB +Default: value of Xen command line B parameter (or its +default value if unspecified). =item B Sets the default value for the C domain config value. -Default: C<1024> +Default: value of Xen command line B +parameter (or its default value if unspecified). =item B diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 49b56fa1a3..b829c1bbce 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -364,8 +364,15 @@ */ #define LIBXL_HAVE_BUILDINFO_GRANT_LIMITS 1 -#define LIBXL_MAX_GRANT_FRAMES_DEFAULT 32 -#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT 1024 +/* + * LIBXL_HAVE_BUILDINFO_SIGNED_GRANT_LIMITS indicates that the + * signed max_grant_frames and max_maptrack_frames fields in + * libxl_domain_build_info are signed quantities. + */ +#define LIBXL_HAVE_BUILDINFO_SIGNED_GRANT_LIMITS 1 + +#define LIBXL_MAX_GRANT_FRAMES_DEFAULT -1 +#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT -1 /* * LIBXL_HAVE_BUILDINFO_* indicates that libxl_domain_build_info has diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index 0546d7865a..63e29bb2fb 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -511,8 +511,8 @@ libxl_domain_build_info = Struct("domain_build_info",[ ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")), - ("max_grant_frames", uint32, {'init_val': 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}), - ("max_maptrack_frames", uint32, {'init_val': 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}), + ("max_grant_frames", integer, {'init_val': 'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}), + ("max_maptrack_frames", integer, {'init_val': 'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}), ("device_model_version", libxl_device_model_version), ("device_model_stubdomain", libxl_defbool), diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c index 72815d25dd..cafc632fc1 100644 --- a/tools/libxl/libxlu_cfg.c +++ b/tools/libxl/libxlu_cfg.c @@ -268,8 +268,9 @@ int xlu_cfg_replace_string(const XLU_Config *cfg, const char *n, return 0; } -int xlu_cfg_get_long(const XLU_Config *cfg, const char *n, - long *value_r, int dont_warn) { +int xlu_cfg_get_bounded_long(const XLU_Config *cfg, const char *n, + long min, long max, long *value_r, + int dont_warn) { long l; XLU_ConfigSetting *set; int e; @@ -303,10 +304,31 @@ int xlu_cfg_get_long(const XLU_Config *cfg, const char *n, cfg->config_source, set->lineno, n); return EINVAL; } + if (l < min) { + if (!dont_warn) + fprintf(cfg->report, + "%s:%d: warning: value `%ld' is smaller than minimum bound '%ld'\n", + cfg->config_source, set->lineno, l, min); + return EINVAL; + } + if (l > max) { + if (!dont_warn) + fprintf(cfg->report, + "%s:%d: warning: value `%ld' is greater than maximum bound '%ld'\n", + cfg->config_source, set->lineno, l, max); + return EINVAL; + } + *value_r= l; return 0; } +int xlu_cfg_get_long(const XLU_Config *cfg, const char *n, + long *value_r, int dont_warn) { + return xlu_cfg_get_bounded_long(cfg, n, LONG_MIN, LONG_MAX, value_r, + dont_warn); +} + int xlu_cfg_get_defbool(const XLU_Config *cfg, const char *n, libxl_defbool *b, int dont_warn) { diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h index 057cc25cb2..92e35c5462 100644 --- a/tools/libxl/libxlutil.h +++ b/tools/libxl/libxlutil.h @@ -63,6 +63,8 @@ int xlu_cfg_replace_string(const XLU_Config *cfg, const char *n, char **value_r, int dont_warn); int xlu_cfg_get_long(const XLU_Config*, const char *n, long *value_r, int dont_warn); +int xlu_cfg_get_bounded_long(const XLU_Config*, const char *n, long min, + long max, long *value_r, int dont_warn); int xlu_cfg_get_defbool(const XLU_Config*, const char *n, libxl_defbool *b, int dont_warn); diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c index 44d3606141..a751e85910 100644 --- a/tools/python/xen/lowlevel/xc/xc.c +++ b/tools/python/xen/lowlevel/xc/xc.c @@ -127,8 +127,8 @@ static PyObject *pyxc_domain_create(XcObject *self, }, .max_vcpus = 1, .max_evtchn_port = -1, /* No limit. */ - .max_grant_frames = 32, - .max_maptrack_frames = 1024, + .max_grant_frames = -1, + .max_maptrack_frames = -1, }; static char *kwd_list[] = { "domid", "ssidref", "handle", "flags", diff --git a/tools/xl/xl.c b/tools/xl/xl.c index ddd29b3f1b..3d4390a46d 100644 --- a/tools/xl/xl.c +++ b/tools/xl/xl.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -96,7 +97,6 @@ static void parse_global_config(const char *configfile, XLU_Config *config; int e; const char *buf; - libxl_physinfo physinfo; config = xlu_cfg_init(stderr, configfile); if (!config) { @@ -197,17 +197,19 @@ static void parse_global_config(const char *configfile, xlu_cfg_replace_string (config, "colo.default.proxyscript", &default_colo_proxy_script, 0); - if (!xlu_cfg_get_long (config, "max_grant_frames", &l, 0)) + e = xlu_cfg_get_bounded_long (config, "max_grant_frames", 0, INT_MAX, + &l, 1); + if (!e) max_grant_frames = l; - else { - libxl_physinfo_init(&physinfo); - max_grant_frames = (libxl_get_physinfo(ctx, &physinfo) != 0 || - !(physinfo.max_possible_mfn >> 32)) - ? 32 : 64; - libxl_physinfo_dispose(&physinfo); - } - if (!xlu_cfg_get_long (config, "max_maptrack_frames", &l, 0)) + else if (e != ESRCH) + exit(1); + + e = xlu_cfg_get_bounded_long (config, "max_maptrack_frames", 0, + INT_MAX, &l, 1); + if (!e) max_maptrack_frames = l; + else if (e != ESRCH) + exit(1); libxl_cpu_bitmap_alloc(ctx, &global_vm_affinity_mask, 0); libxl_cpu_bitmap_alloc(ctx, &global_hvm_affinity_mask, 0); diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index 112f8ee026..b881184804 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -1411,14 +1411,23 @@ void parse_config_data(const char *config_source, !xlu_cfg_get_string (config, "cpus_soft", &buf, 0)) parse_vcpu_affinity(b_info, cpus, buf, num_cpus, false); - if (!xlu_cfg_get_long (config, "max_grant_frames", &l, 0)) + e = xlu_cfg_get_bounded_long (config, "max_grant_frames", 0, INT_MAX, + &l, 1); + if (e == ESRCH) /* not specified */ + b_info->max_grant_frames = max_grant_frames; + else if (!e) b_info->max_grant_frames = l; else - b_info->max_grant_frames = max_grant_frames; - if (!xlu_cfg_get_long (config, "max_maptrack_frames", &l, 0)) - b_info->max_maptrack_frames = l; - else if (max_maptrack_frames != -1) + exit(1); + + e = xlu_cfg_get_bounded_long (config, "max_maptrack_frames", 0, + INT_MAX, &l, 1); + if (e == ESRCH) /* not specified */ b_info->max_maptrack_frames = max_maptrack_frames; + else if (!e) + b_info->max_maptrack_frames = l; + else + exit(1); libxl_defbool_set(&b_info->claim_mode, claim_mode); diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 51d32106b7..3c899cd4a0 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -789,7 +789,7 @@ void __init start_xen(unsigned long boot_phys_offset, .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap, .max_evtchn_port = -1, .max_grant_frames = gnttab_dom0_frames(), - .max_maptrack_frames = opt_max_maptrack_frames, + .max_maptrack_frames = -1, }; int rc; diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 00ee87bde5..7d27f36053 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) struct xen_domctl_createdomain dom0_cfg = { .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0, .max_evtchn_port = -1, - .max_grant_frames = opt_max_grant_frames, - .max_maptrack_frames = opt_max_maptrack_frames, + .max_grant_frames = -1, + .max_maptrack_frames = -1, }; /* Critical region without IDT or TSS. Any fault is deadly! */ diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index b34d520f6d..729f362ea8 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -84,11 +84,42 @@ struct grant_table { struct grant_table_arch arch; }; +static int parse_gnttab_limit(const char *param, const char *arg, + unsigned int *valp) +{ + const char *e; + unsigned long val; + + val = simple_strtoul(arg, &e, 0); + if ( *e ) + return -EINVAL; + + if ( val > INT_MAX ) + return -ERANGE; + + *valp = val; + + return 0; +} + unsigned int __read_mostly opt_max_grant_frames = 64; -integer_runtime_param("gnttab_max_frames", opt_max_grant_frames); -unsigned int __read_mostly opt_max_maptrack_frames = 1024; -integer_runtime_param("gnttab_max_maptrack_frames", opt_max_maptrack_frames); +static int parse_gnttab_max_frames(const char *arg) +{ + return parse_gnttab_limit("gnttab_max_frames", arg, + &opt_max_grant_frames); +} +custom_runtime_param("gnttab_max_frames", parse_gnttab_max_frames); + +static unsigned int __read_mostly opt_max_maptrack_frames = 1024; + +static int parse_gnttab_max_maptrack_frames(const char *arg) +{ + return parse_gnttab_limit("gnttab_max_maptrack_frames", arg, + &opt_max_maptrack_frames); +} +custom_runtime_param("gnttab_max_maptrack_frames", + parse_gnttab_max_maptrack_frames); #ifndef GNTTAB_MAX_VERSION #define GNTTAB_MAX_VERSION 2 @@ -1837,12 +1868,18 @@ active_alloc_failed: return -ENOMEM; } -int grant_table_init(struct domain *d, unsigned int max_grant_frames, - unsigned int max_maptrack_frames) +int grant_table_init(struct domain *d, int max_grant_frames, + int max_maptrack_frames) { struct grant_table *gt; int ret = -ENOMEM; + /* Default to maximum value if no value was specified */ + if ( max_grant_frames < 0 ) + max_grant_frames = opt_max_grant_frames; + if ( max_maptrack_frames < 0 ) + max_maptrack_frames = opt_max_maptrack_frames; + if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES || max_grant_frames > opt_max_grant_frames || max_maptrack_frames > opt_max_maptrack_frames ) diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 9f2cfd602c..e313da499f 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -82,13 +82,15 @@ struct xen_domctl_createdomain { uint32_t iommu_opts; /* - * Various domain limits, which impact the quantity of resources (global - * mapping space, xenheap, etc) a guest may consume. + * Various domain limits, which impact the quantity of resources + * (global mapping space, xenheap, etc) a guest may consume. For + * max_grant_frames and max_maptrack_frames, < 0 means "use the + * default maximum value in the hypervisor". */ uint32_t max_vcpus; uint32_t max_evtchn_port; - uint32_t max_grant_frames; - uint32_t max_maptrack_frames; + int32_t max_grant_frames; + int32_t max_maptrack_frames; struct xen_arch_domainconfig arch; }; diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h index 6f9345d9ef..98603604b8 100644 --- a/xen/include/xen/grant_table.h +++ b/xen/include/xen/grant_table.h @@ -33,11 +33,10 @@ struct grant_table; extern unsigned int opt_max_grant_frames; -extern unsigned int opt_max_maptrack_frames; /* Create/destroy per-domain grant table context. */ -int grant_table_init(struct domain *d, unsigned int max_grant_frames, - unsigned int max_maptrack_frames); +int grant_table_init(struct domain *d, int max_grant_frames, + int max_maptrack_frames); void grant_table_destroy( struct domain *d); void grant_table_init_vcpu(struct vcpu *v); @@ -65,11 +64,10 @@ int gnttab_get_status_frame(struct domain *d, unsigned long idx, #else #define opt_max_grant_frames 0 -#define opt_max_maptrack_frames 0 static inline int grant_table_init(struct domain *d, - unsigned int max_grant_frames, - unsigned int max_maptrack_frames) + int max_grant_frames, + int max_maptrack_frames) { return 0; }