[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
On 01/02/2022 10:02, Roger Pau Monné wrote: > 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.malalane@xxxxxxxxxx> >> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> >> --- >> CC: Wei Liu <wl@xxxxxxx> >> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx> >> CC: Juergen Gross <jgross@xxxxxxxx> >> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> >> CC: George Dunlap <george.dunlap@xxxxxxxxxx> >> CC: Jan Beulich <jbeulich@xxxxxxxx> >> CC: Julien Grall <julien@xxxxxxx> >> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx> >> CC: Christian Lindig <christian.lindig@xxxxxxxxxx> >> CC: David Scott <dave@xxxxxxxxxx> >> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx> >> CC: "Roger Pau Monné" <roger.pau@xxxxxxxxxx> >> --- >> 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. Makes sense. In that case, could I instead just have this? if (b_info->type != LIBXL_DOMAIN_TYPE_PV) { if (physinfo->cap_assisted_xapic) libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, true); if (physinfo->cap_assisted_x2apic) libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, true); } Or do i still need to also check that assisted_x{2}apic hasn't been set elsewhere for PV domains, in which case, I'm happy to add the code you proposed above with this code I have here too. > >> + >> + 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); Oh right I see. > >> + 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. Thank you for your comments. Jane.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |