On Thu, Jan 27, 2022 at 04:01:33PM +0000, Jane Malalane wrote:
> Introduce a new per-domain creation x86 specific flag to
> select whether hardware assisted virtualization should be used for
> x{2}APIC.
>
> A per-domain option is added to xl in order to select the usage of
> x{2}APIC hardware assisted vitualization, as well as a global
> configuration option.
>
> Having all APIC interaction exit to Xen for emulation is slow and can
> induce much overhead. Hardware can speed up x{2}APIC by running APIC
> read/write accesses without taking a VM exit.
>
> Signed-off-by: Jane Malalane <jane.malal...@citrix.com>
> Suggested-by: Andrew Cooper <andrew.coop...@citrix.com>
> ---
> CC: Wei Liu <w...@xen.org>
> CC: Anthony PERARD <anthony.per...@citrix.com>
> CC: Juergen Gross <jgr...@suse.com>
> CC: Andrew Cooper <andrew.coop...@citrix.com>
> CC: George Dunlap <george.dun...@citrix.com>
> CC: Jan Beulich <jbeul...@suse.com>
> CC: Julien Grall <jul...@xen.org>
> CC: Stefano Stabellini <sstabell...@kernel.org>
> CC: Christian Lindig <christian.lin...@citrix.com>
> CC: David Scott <d...@recoil.org>
> CC: Volodymyr Babchuk <volodymyr_babc...@epam.com>
> CC: "Roger Pau Monné" <roger....@citrix.com>
> ---
> docs/man/xl.cfg.5.pod.in | 10 ++++++++
> docs/man/xl.conf.5.pod.in | 12 ++++++++++
> tools/golang/xenlight/helpers.gen.go | 12 ++++++++++
> tools/libs/light/libxl_arch.h | 5 ++--
> tools/libs/light/libxl_arm.c | 5 ++--
> tools/libs/light/libxl_create.c | 21 ++++++++++-------
> tools/libs/light/libxl_types.idl | 2 ++
> tools/libs/light/libxl_x86.c | 43
> +++++++++++++++++++++++++++++++++--
> tools/ocaml/libs/xc/xenctrl.ml | 2 ++
> tools/ocaml/libs/xc/xenctrl.mli | 2 ++
> tools/xl/xl.c | 8 +++++++
> tools/xl/xl.h | 2 ++
> tools/xl/xl_parse.c | 14 ++++++++++++
> xen/arch/x86/domain.c | 27 +++++++++++++++++++++-
> xen/arch/x86/hvm/vmx/vmcs.c | 4 ++++
> xen/arch/x86/hvm/vmx/vmx.c | 13 +++++++----
> xen/arch/x86/include/asm/hvm/domain.h | 6 +++++
> xen/arch/x86/traps.c | 6 +++--
> xen/include/public/arch-x86/xen.h | 2 ++
> 19 files changed, 174 insertions(+), 22 deletions(-)
>
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index b98d161398..974fe7d2d8 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -1862,6 +1862,16 @@ firmware tables when using certain older guest
> Operating
> Systems. These tables have been superseded by newer constructs within
> the ACPI tables.
>
> +=item B<assisted_xapic=BOOLEAN>
> +B<(x86 only)> Enables or disables hardware assisted virtualization for xapic.
> +This allows accessing APIC registers without a VM-exit.
> +The default is settable via L<xl.conf(5)>.
> +
> +=item B<assisted_x2apic=BOOLEAN>
> +B<(x86 only)> Enables or disables hardware assisted virtualization for
> x2apic.
> +This allows accessing APIC registers without a VM-exit.
> +The default is settable via L<xl.conf(5)>.
Like you do below I would capitalize xAPIC and x2APIC in the option
text.
> +
> =item B<nx=BOOLEAN>
>
> B<(x86 only)> Hides or exposes the No-eXecute capability. This allows a guest
> diff --git a/docs/man/xl.conf.5.pod.in b/docs/man/xl.conf.5.pod.in
> index df20c08137..2d0a59d019 100644
> --- a/docs/man/xl.conf.5.pod.in
> +++ b/docs/man/xl.conf.5.pod.in
> @@ -107,6 +107,18 @@ Sets the default value for the C<max_grant_version>
> domain config value.
>
> Default: maximum grant version supported by the hypervisor.
>
> +=item B<assisted_xapic=BOOLEAN>
> +
> +If enabled, domains will use xAPIC hardware assisted emulation by default.
> +
> +Default: enabled.
> +
> +=item B<assisted_x2apic=BOOLEAN>
> +
> +If enabled, domains will use x2APIC hardware assisted emulation by default.
> +
> +Default: enabled.
I think for both options this should be:
Default: enabled if supported.
> +
> =item B<vif.default.script="PATH">
>
> Configures the default hotplug script used by virtual network devices.
> diff --git a/tools/golang/xenlight/helpers.gen.go
> b/tools/golang/xenlight/helpers.gen.go
> index dd4e6c9f14..90e7b9b205 100644
> --- a/tools/golang/xenlight/helpers.gen.go
> +++ b/tools/golang/xenlight/helpers.gen.go
> @@ -636,6 +636,12 @@ x.Passthrough = Passthrough(xc.passthrough)
> if err :=
> x.XendSuspendEvtchnCompat.fromC(&xc.xend_suspend_evtchn_compat);err != nil {
> return fmt.Errorf("converting field XendSuspendEvtchnCompat: %v", err)
> }
> +if err := x.ArchX86.AssistedXapic.fromC(&xc.arch_x86.assisted_xapic);err !=
> nil {
> +return fmt.Errorf("converting field ArchX86.AssistedXapic: %v", err)
> +}
> +if err := x.ArchX86.AssistedX2Apic.fromC(&xc.arch_x86.assisted_x2apic);err
> != nil {
> +return fmt.Errorf("converting field ArchX86.AssistedX2Apic: %v", err)
> +}
>
> return nil}
>
> @@ -679,6 +685,12 @@ xc.passthrough = C.libxl_passthrough(x.Passthrough)
> if err := x.XendSuspendEvtchnCompat.toC(&xc.xend_suspend_evtchn_compat); err
> != nil {
> return fmt.Errorf("converting field XendSuspendEvtchnCompat: %v", err)
> }
> +if err := x.ArchX86.AssistedXapic.toC(&xc.arch_x86.assisted_xapic); err !=
> nil {
> +return fmt.Errorf("converting field ArchX86.AssistedXapic: %v", err)
> +}
> +if err := x.ArchX86.AssistedX2Apic.toC(&xc.arch_x86.assisted_x2apic); err !=
> nil {
> +return fmt.Errorf("converting field ArchX86.AssistedX2Apic: %v", err)
> +}
>
> return nil
> }
> diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h
> index 00cc50394d..2eaff45526 100644
> --- a/tools/libs/light/libxl_arch.h
> +++ b/tools/libs/light/libxl_arch.h
> @@ -71,8 +71,9 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc
> *gc,
> libxl_domain_create_info
> *c_info);
>
> _hidden
> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> - libxl_domain_build_info
> *b_info);
> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> + libxl_domain_build_info *b_info,
> + const libxl_physinfo *physinfo);
>
> _hidden
> int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 52f2545498..4d422bef96 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -1384,8 +1384,9 @@ void
> libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
> }
> }
>
> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> - libxl_domain_build_info
> *b_info)
> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> + libxl_domain_build_info *b_info,
> + const libxl_physinfo *physinfo)
> {
> /* ACPI is disabled by default */
> libxl_defbool_setdefault(&b_info->acpi, false);
> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index d7a40d7550..2bae6fef62 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -264,7 +264,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> if (!b_info->event_channels)
> b_info->event_channels = 1023;
>
> - libxl__arch_domain_build_info_setdefault(gc, b_info);
> libxl_defbool_setdefault(&b_info->dm_restrict, false);
>
> if (b_info->iommu_memkb == LIBXL_MEMKB_DEFAULT)
> @@ -456,15 +455,21 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> libxl_defbool_setdefault(&b_info->nested_hvm, false);
> }
>
> - if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
> - libxl_physinfo info;
> + libxl_physinfo info;
>
> - rc = libxl_get_physinfo(CTX, &info);
> - if (rc) {
> - LOG(ERROR, "failed to get hypervisor info");
> - return rc;
> - }
> + rc = libxl_get_physinfo(CTX, &info);
> + if (rc) {
> + LOG(ERROR, "failed to get hypervisor info");
> + return rc;
> + }
>
> + rc = libxl__arch_domain_build_info_setdefault(gc, b_info, &info);
> + if (rc) {
> + LOG(ERROR, "unable to set domain arch build info defaults");
> + return rc;
> + }
> +
> + if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
> if (info.cap_gnttab_v2)
> b_info->max_grant_version = 2;
> else if (info.cap_gnttab_v1)
> diff --git a/tools/libs/light/libxl_types.idl
> b/tools/libs/light/libxl_types.idl
> index 42ac6c357b..db5eb0a0b3 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -648,6 +648,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
> ("vuart", libxl_vuart_type),
> ])),
> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> + ("assisted_xapic", libxl_defbool),
> + ("assisted_x2apic", libxl_defbool),
> ])),
> # Alternate p2m is not bound to any architecture or guest type, as it is
> # supported by x86 HVM and ARM support is planned.
> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> index 33da51fe89..b257fca756 100644
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -23,6 +23,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> if (libxl_defbool_val(d_config->b_info.arch_x86.msr_relaxed))
> config->arch.misc_flags |= XEN_X86_MSR_RELAXED;
>
> + if(libxl_defbool_val(d_config->b_info.arch_x86.assisted_xapic))
> + config->arch.misc_flags |= XEN_X86_ASSISTED_XAPIC;
> +
> + if(libxl_defbool_val(d_config->b_info.arch_x86.assisted_x2apic))
> + config->arch.misc_flags |= XEN_X86_ASSISTED_X2APIC;
> +
> return 0;
> }
>
> @@ -819,11 +825,44 @@ void
> libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
> {
> }
>
> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> - libxl_domain_build_info
> *b_info)
> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> + libxl_domain_build_info *b_info,
> + const libxl_physinfo *physinfo)
> {
> + int rc;
> + bool assisted_xapic;
> + bool assisted_x2apic;
> +
> libxl_defbool_setdefault(&b_info->acpi, true);
> libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false);
> +
> + libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, false);
> + libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, false);
> +
> + assisted_xapic = libxl_defbool_val(b_info->arch_x86.assisted_xapic);
> + assisted_x2apic = libxl_defbool_val(b_info->arch_x86.assisted_x2apic);
> +
> + if ((assisted_xapic || assisted_x2apic) &&
> + b_info->type == LIBXL_DOMAIN_TYPE_PV)
> + {
> + LOG(ERROR, "Interrupt Controller Virtualization not supported for
> PV");
> + rc = ERROR_INVAL;
> + goto out;
> + }
> +
> + if ((assisted_xapic && !physinfo->cap_assisted_xapic) ||
> + (assisted_x2apic && !physinfo->cap_assisted_x2apic))
> + {
> + LOG(ERROR, "x%sAPIC hardware supported emulation not available",
> + assisted_xapic && !physinfo->cap_assisted_xapic ? "" : "2");
> + rc = ERROR_INVAL;
> + goto out;
> + }
I think the logic here is slightly wrong, as you are setting the
default value of assisted_x{2}apic to false, and we would instead like
to set it to the current value supported by the hardware in order to
keep current behavior.
Also the options are HVM/PVH only, so having them set for PV should
result in an error regardless of the set value, ie:
if (b_info->type == LIBXL_DOMAIN_TYPE_PV &&
(!libxl_defbool_is_default(&b_info->arch_x86.assisted_xapic) ||
!libxl_defbool_is_default(&b_info->arch_x86.assisted_x2apic)))
ERROR
libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic,
physinfo->cap_assisted_xapic);
libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic,
physinfo->cap_assisted_x2apic);
I don't think you need the local assisted_x{2}apic variables.
> +
> + rc = 0;
> +out:
> + return rc;
The out label is not really needed here and makes the code longer.
Just 'return ERROR_INVAL' in the error paths or 0 at the end of the
function. You can then also drop the local rc variable.
> +
> }
>
> int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> index 7ce832d605..cce30d8731 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -50,6 +50,8 @@ type x86_arch_emulation_flags =
>
> type x86_arch_misc_flags =
> | X86_MSR_RELAXED
> + | X86_ASSISTED_XAPIC
> + | X86_ASSISTED_X2APIC
>
> type xen_x86_arch_domainconfig =
> {
> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
> index a2b15130ee..67a22ec15c 100644
> --- a/tools/ocaml/libs/xc/xenctrl.mli
> +++ b/tools/ocaml/libs/xc/xenctrl.mli
> @@ -44,6 +44,8 @@ type x86_arch_emulation_flags =
>
> type x86_arch_misc_flags =
> | X86_MSR_RELAXED
> + | X86_ASSISTED_XAPIC
> + | X86_ASSISTED_X2APIC
>
> type xen_x86_arch_domainconfig = {
> emulation_flags: x86_arch_emulation_flags list;
> diff --git a/tools/xl/xl.c b/tools/xl/xl.c
> index 2d1ec18ea3..b97e491c9c 100644
> --- a/tools/xl/xl.c
> +++ b/tools/xl/xl.c
> @@ -57,6 +57,8 @@ int max_grant_frames = -1;
> int max_maptrack_frames = -1;
> int max_grant_version = LIBXL_MAX_GRANT_DEFAULT;
> libxl_domid domid_policy = INVALID_DOMID;
> +int assisted_xapic = 0;
> +int assisted_x2apic = 0;
This should be initialized to -1, in order to denote the values are
unset...
>
> xentoollog_level minmsglevel = minmsglevel_default;
>
> @@ -201,6 +203,12 @@ static void parse_global_config(const char *configfile,
> if (!xlu_cfg_get_long (config, "claim_mode", &l, 0))
> claim_mode = l;
>
> + if (!xlu_cfg_get_long (config, "assisted_xapic", &l, 0))
> + assisted_xapic = l;
> +
> + if (!xlu_cfg_get_long (config, "assisted_x2apic", &l, 0))
> + assisted_x2apic = l;
> +
> xlu_cfg_replace_string (config, "remus.default.netbufscript",
> &default_remus_netbufscript, 0);
> xlu_cfg_replace_string (config, "colo.default.proxyscript",
> diff --git a/tools/xl/xl.h b/tools/xl/xl.h
> index c5c4bedbdd..528deb3feb 100644
> --- a/tools/xl/xl.h
> +++ b/tools/xl/xl.h
> @@ -286,6 +286,8 @@ extern libxl_bitmap global_vm_affinity_mask;
> extern libxl_bitmap global_hvm_affinity_mask;
> extern libxl_bitmap global_pv_affinity_mask;
> extern libxl_domid domid_policy;
> +extern int assisted_xapic;
> +extern int assisted_x2apic;
>
> enum output_format {
> OUTPUT_FORMAT_JSON,
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 117fcdcb2b..16ff9e76bc 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1681,6 +1681,20 @@ void parse_config_data(const char *config_source,
> xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align,
> 0);
> xlu_cfg_get_defbool(config, "apic", &b_info->apic, 0);
>
> + e = xlu_cfg_get_defbool(config, "assisted_xapic",
> + &b_info->arch_x86.assisted_xapic, 0);
> + if (e == ESRCH) /* not specified */
> + libxl_defbool_set(&b_info->arch_x86.assisted_xapic,
> assisted_xapic);
...because here you only want to use the global values if they have
actually been set by the user (assisted_x{2}apic != -1):
e = xlu_cfg_get_defbool(config, "assisted_xapic",
&b_info->arch_x86.assisted_xapic, 0);
if (e == ESRCH && assisted_xapic != -1) /* use global default if present */
libxl_defbool_set(&b_info->arch_x86.assisted_xapic, assisted_xapic);
else if (e)
exit(1);
> + else if (e)
> + exit(1);
> +
> + e = xlu_cfg_get_defbool(config, "assisted_x2apic",
> + &b_info->arch_x86.assisted_x2apic, 0);
> + if (e == ESRCH) /* not specified */
> + libxl_defbool_set(&b_info->arch_x86.assisted_x2apic,
> assisted_x2apic);
> + else if (e)
> + exit(1);
> +
> switch (xlu_cfg_get_list(config, "viridian",
> &viridian, &num_viridian, 1))
> {
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index ef1812dc14..d08f51e28b 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -619,6 +619,8 @@ int arch_sanitise_domain_config(struct
> xen_domctl_createdomain *config)
> bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
> bool hap = config->flags & XEN_DOMCTL_CDF_hap;
> bool nested_virt = config->flags & XEN_DOMCTL_CDF_nested_virt;
> + bool assisted_xapic = config->arch.misc_flags & XEN_X86_ASSISTED_XAPIC;
> + bool assisted_x2apic = config->arch.misc_flags & XEN_X86_ASSISTED_X2APIC;
> unsigned int max_vcpus;
>
> if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
> @@ -685,13 +687,30 @@ int arch_sanitise_domain_config(struct
> xen_domctl_createdomain *config)
> }
> }
>
> - if ( config->arch.misc_flags & ~XEN_X86_MSR_RELAXED )
> + if ( config->arch.misc_flags & ~(XEN_X86_MSR_RELAXED |
> + XEN_X86_ASSISTED_XAPIC |
> + XEN_X86_ASSISTED_X2APIC) )
> {
> dprintk(XENLOG_INFO, "Invalid arch misc flags %#x\n",
> config->arch.misc_flags);
> return -EINVAL;
> }
>
> + if ( (assisted_xapic || assisted_x2apic) && !hvm )
> + {
> + dprintk(XENLOG_INFO,
> + "Interrupt Controller Virtualization not supported for
> PV\n");
> + return -EINVAL;
> + }
> +
> + if ( (assisted_xapic && !assisted_xapic_available) ||
> + (assisted_x2apic && !assisted_x2apic_available) )
> + {
> + dprintk(XENLOG_INFO, "x%sAPIC requested but not available\n",
This should be a little bit more concise, as Xen does always offer
a fully software virtualized x{2}APIC.
"hardware assisted x%sAPIC requested but not available\n"
Thanks, Roger.
