From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v4 2/3] arm: Allow the user to specify the GIC version Date: Tue, 7 Jul 2015 16:18:43 +0100 Message-ID: <559BEDD3.5060702@citrix.com> References: <1436278137-18892-1-git-send-email-julien.grall@citrix.com> <1436278137-18892-3-git-send-email-julien.grall@citrix.com> <1436279992.25646.226.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZCUen-0002vV-63 for xen-devel@lists.xenproject.org; Tue, 07 Jul 2015 15:19:37 +0000 In-Reply-To: <1436279992.25646.226.camel@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: xen-devel@lists.xenproject.org, stefano.stabellini@citrix.com, Ian Jackson , Wei Liu List-Id: xen-devel@lists.xenproject.org Hi Ian, On 07/07/15 15:39, Ian Campbell wrote: > On Tue, 2015-07-07 at 15:08 +0100, Julien Grall wrote: >> A platform may have a GIC compatible with previous version of the >> device. > > "...a GIC which is compatible with a previous version..." > >> >> This is allow to virtualize an unmodified OS on new hardware if the GIC >> is compatible with older version. > > "This is to allow virtualization of an unmodified..." > > "...with the older version". > >> When a guest is created, the vGIC will emulate same version as the >> hardware. Although, the user can specify in the configuration file the >> preferred version (currently only GICv2 and GICv3 are supported). > > I think "Unless the user has specified in the configuration file..." That sounds better. [..] >> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c >> index 03a9205..8ac90c4 100644 >> --- a/tools/libxl/libxl_arm.c >> +++ b/tools/libxl/libxl_arm.c >> @@ -61,7 +61,40 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, >> xc_config->nr_spis = nr_spis; >> LOG(DEBUG, " - Allocate %u SPIs", nr_spis); >> >> - xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; >> + switch (d_config->b_info.arch_arm.gic_version) { >> + case LIBXL_GIC_VERSION_DEFAULT: >> + xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE; >> + break; >> + case LIBXL_GIC_VERSION_V2: >> + xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_V2; >> + break; >> + case LIBXL_GIC_VERSION_V3: >> + xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_V3; >> + break; >> + default: >> + LOG(ERROR, "Unknown GIC version %s\n", >> + libxl_gic_version_to_string(d_config->b_info.arch_arm.gic_version)); > > libxl_gic_version_to_string will return NULL for a truly unknown value. > Perhaps print %d version too/instead? Will do. > >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c >> index f799081..2b8a506 100644 >> --- a/tools/libxl/libxl_create.c >> +++ b/tools/libxl/libxl_create.c >> @@ -579,6 +579,13 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config, >> goto out; >> } >> >> + ret = libxl__arch_domain_save_config(gc, d_config, xc_config); > > >> + if (ret < 0) { >> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "fail to save domain config"); > > Please use the shorter LOG* macros, and you don't need/want to log errno > here since the function doesn't touch it. > > Actually, the function logs on failure itself, no need to repeat it I > think, just document "logs on failure" next to the prototype. Ok. > >> + rc = ERROR_FAIL; > > libxl__arch_domain_save_config already returns a libxl ERROR_*, so it > should be stored into rc directly and propagated. Ok. > >> + goto out; >> + } >> + >> ret = xc_cpupool_movedomain(ctx->xch, info->poolid, *domid); >> if (ret < 0) { >> LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "domain move fail"); >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c >> index 1be3f8b..e825315 100644 >> --- a/tools/libxl/xl_cmdimpl.c >> +++ b/tools/libxl/xl_cmdimpl.c >> @@ -2250,6 +2250,18 @@ skip_vfb: >> } >> } >> >> + if (!xlu_cfg_get_string (config, "gic_version", &buf, 1)) { >> + libxl_gic_version v; >> + >> + e = libxl_gic_version_from_string(buf, &v); >> + if (e) { >> + fprintf(stderr, >> + "Unknown gic_version \"%s\" specified\n", buf); >> + exit(-ERROR_FAIL); >> + } >> + b_info->arch_arm.gic_version = v; > > You can just pass &b_info....gic_version straight to the from_string > function. I was following some other example in xl_cmdimpl.c (see vendor_device). Anyway, I will do the change. Regards, -- Julien Grall