From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v4 2/3] arm: Allow the user to specify the GIC version Date: Tue, 7 Jul 2015 15:39:52 +0100 Message-ID: <1436279992.25646.226.camel@citrix.com> References: <1436278137-18892-1-git-send-email-julien.grall@citrix.com> <1436278137-18892-3-git-send-email-julien.grall@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZCU39-0005Zt-SZ for xen-devel@lists.xenproject.org; Tue, 07 Jul 2015 14:40:44 +0000 In-Reply-To: <1436278137-18892-3-git-send-email-julien.grall@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: Julien Grall Cc: xen-devel@lists.xenproject.org, stefano.stabellini@citrix.com, Ian Jackson , Wei Liu List-Id: xen-devel@lists.xenproject.org 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..." > > Signed-off-by: Julien Grall > Cc: Ian Jackson > Cc: Wei Liu > > --- > The hypervisor will check if the GIC is able to virtualize the > version specified by the user (via the DOMCTL createdomain). > If it's not compatible an error will be send on the Xen console > which will make the error not obvious for user. > > I left aside a user error reporting for a follow-up as I'm not > sure how to notify the user which GIC versions are available. May be > by a new mechanism similar to xen_get_caps? > > It may be possible to rework the libxl code to restrict the scope of > xc_config in libxl_domain_make. This can be done in a follow-up if > we figure what to do the frequency field. > > Changes in v4: > - Update the documentation to specify the default behavior > - Update the domain configuration with the GIC version returned > by the hypervisor. > > Changes in v3: > - Rename GIC_VERSION define in LIBXL_GIC_VERSION_Vn. > - Change the value of each define > - Use libxl_gic_version_from_string rather than custom if/else > - Rename LIBXL_HAVE_BUILDINFO_GIC_VERSION into > LIBXL_HAVE_BUILDINFO_ARM_GIC_VERSION and update the comment > - Update doc with Ian's suggestion > - Typoes > > Changes in v2: > - Introduce arch_arm in libxl_domain_build_info to store ARM > specific field > - Add docs > - Remove code that is not necessary with the new version > --- > docs/man/xl.cfg.pod.5 | 34 +++++++++++++++++++++++++++++++++ > tools/libxl/libxl.h | 5 +++++ > tools/libxl/libxl_arch.h | 6 ++++++ > tools/libxl/libxl_arm.c | 35 +++++++++++++++++++++++++++++++++- > tools/libxl/libxl_create.c | 7 +++++++ > tools/libxl/libxl_types.idl | 11 +++++++++++ > tools/libxl/libxl_x86.c | 7 +++++++ > tools/libxl/xl_cmdimpl.c | 12 ++++++++++++ > xen/arch/arm/domain.c | 45 ++++++++++++++++++++++++++------------------ > xen/arch/arm/vgic.c | 4 ++-- > xen/include/asm-arm/domain.h | 2 ++ > 11 files changed, 147 insertions(+), 21 deletions(-) > > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5 > index a3e0e2e..12765d5 100644 > --- a/docs/man/xl.cfg.pod.5 > +++ b/docs/man/xl.cfg.pod.5 > @@ -1688,6 +1688,40 @@ The default is B. > > See L for more information. > > +=head2 Architecture Specific options > + > +=head3 ARM > + > +=over 4 > + > +=item B > + > +Version of the GIC emulated for the guest. Currently, the following > +versions are supported: > + > +=over 4 > + > +=item B > + > +Emulate a GICv2 hardware "Emulate the GICv2 hardware" or just "Emulate a GICv2" (without the h/w). > + > +=item B > + > +Emulate a GICv3 hardware. As above. > Note that the emulated GIC does not support the > +GICv2 compatibility mode. > + > +=item B > + > +Emulate the same version as the native GIC hardware used by host where the > +the domain has been created. "... where the domain was created". > 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? > 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. > + rc = ERROR_FAIL; libxl__arch_domain_save_config already returns a libxl ERROR_*, so it should be stored into rc directly and propagated. > + 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. Ian.