On Wed, Mar 16, 2022 at 09:13:15AM +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 virtualization, 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 decoding the
> APIC access and providing a VM exit with a more specific exit reason
> than a regular EPT fault or by altogether avoiding a VM exit.
>
> On the other hand, being able to disable x{2}APIC hardware assisted
> virtualization can be useful for testing and debugging purposes.
>
> Note: vmx_install_vlapic_mapping doesn't require modifications
> regardless of whether the guest has "Virtualize APIC accesses" enabled
> or not, i.e., setting the APIC_ACCESS_ADDR VMCS field is fine so long
> as virtualize_apic_accesses is supported by the CPU.
Have you tested migration of guests with this patch applied?
We need to be careful so that a guest that doesn't have
assisted_x{2}apic set in the config file can be migrated between hosts
that have different support for hardware assisted x{2}APIC
virtualization.
Ie: we need to make sure the selection of arch_x86.assisted_x{2}apic
is only present in the migration stream when explicitly set in the
configuration file.
> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> index e0a06ecfe3..46d4de22d1 100644
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -23,6 +23,15 @@ 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 (d_config->c_info.type != LIBXL_DOMAIN_TYPE_PV)
> + {
> + 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 +828,26 @@ 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)
> {
> libxl_defbool_setdefault(&b_info->acpi, true);
> libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false);
> +
> + if (b_info->type != LIBXL_DOMAIN_TYPE_PV) {
> + 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);
Nit: the split lines would better be adjusted to match the
indentation of the first parameter.
libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic,
physinfo->cap_assisted_xapic);
> + }
> + else if (!libxl_defbool_is_default(b_info->arch_x86.assisted_xapic) ||
> + !libxl_defbool_is_default(b_info->arch_x86.assisted_x2apic)) {
> + LOG(ERROR, "Interrupt Controller Virtualization not supported for
> PV");
> + return ERROR_INVAL;
> + }
> +
> + return 0;
> }
>
> 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 712456e098..32f3028828 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 b034434f68..d0fcbc8866 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/ocaml/libs/xc/xenctrl_stubs.c
> b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index 7e9c32ad1b..5df8aaa58f 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -239,7 +239,7 @@ CAMLprim value stub_xc_domain_create(value xch, value
> wanted_domid, value config
>
> cfg.arch.misc_flags = ocaml_list_to_c_bitmap
> /* ! x86_arch_misc_flags X86_ none */
> - /* ! XEN_X86_ XEN_X86_MSR_RELAXED all */
> + /* ! XEN_X86_ XEN_X86_ASSISTED_X2APIC max */
In order to simplify adding new options we would generally introduce
a:
#define XEN_X86_MISC_FLAGS_MAX XEN_X86_ASSISTED_X2APIC
In include/public/arch-x86/xen.h so that adding new flags doesn't
require changing the Ocaml code here. This wasn't done before because
there was a single flag.
> (VAL_MISC_FLAGS);
>
> #undef VAL_MISC_FLAGS
> diff --git a/tools/xl/xl.c b/tools/xl/xl.c
> index 2d1ec18ea3..31eb223309 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 = -1;
> +int assisted_x2apic = -1;
>
> 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..f118dc7e97 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -2761,6 +2761,24 @@ skip_usbdev:
>
> xlu_cfg_get_defbool(config, "vpmu", &b_info->vpmu, 0);
>
> + if (b_info->type != LIBXL_DOMAIN_TYPE_PV) {
> + e = xlu_cfg_get_long(config, "assisted_xapic", &l , 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)
> + libxl_defbool_set(&b_info->arch_x86.assisted_xapic, l);
> + else
> + exit(1);
> +
> + e = xlu_cfg_get_long(config, "assisted_x2apic", &l, 0);
> + if ((e == ESRCH && assisted_x2apic != -1)) /* use global default if
> present */
> + libxl_defbool_set(&b_info->arch_x86.assisted_x2apic,
> assisted_x2apic);
> + else if (!e)
> + libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, l);
> + else
> + exit(1);
Indentation seems wrong in the line above.
Thanks, Roger.
