| Message ID | 20220127160133.11608-3-jane.malalane@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: Report and use hardware APIC virtualization capabilities | expand |
The changes for the OCaml bindings are minimal and administrative. This looks good to me. Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>> On 27 Jan 2022, at 16:01, Jane Malalane <jane.malalane@citrix.com<mailto:jane.malalane@citrix.com>> 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@citrix.com<mailto:jane.malalane@citrix.com>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com<mailto:andrew.cooper3@citrix.com>> --- CC: Wei Liu <wl@xen.org<mailto:wl@xen.org>> CC: Anthony PERARD <anthony.perard@citrix.com<mailto:anthony.perard@citrix.com>> CC: Juergen Gross <jgross@suse.com<mailto:jgross@suse.com>> CC: Andrew Cooper <andrew.cooper3@citrix.com<mailto:andrew.cooper3@citrix.com>> CC: George Dunlap <george.dunlap@citrix.com<mailto:george.dunlap@citrix.com>> CC: Jan Beulich <jbeulich@suse.com<mailto:jbeulich@suse.com>> CC: Julien Grall <julien@xen.org<mailto:julien@xen.org>> CC: Stefano Stabellini <sstabellini@kernel.org<mailto:sstabellini@kernel.org>> CC: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>> CC: David Scott <dave@recoil.org<mailto:dave@recoil.org>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com<mailto:Volodymyr_Babchuk@epam.com>> CC: "Roger Pau Monné" <roger.pau@citrix.com<mailto:roger.pau@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)>. + =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. + =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; + } + + rc = 0; +out: + return rc; + } 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; 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); + 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", + assisted_xapic && !assisted_xapic_available ? "" : "2"); + return -EINVAL; + } + return 0; } @@ -863,6 +882,12 @@ int arch_domain_create(struct domain *d, d->arch.msr_relaxed = config->arch.misc_flags & XEN_X86_MSR_RELAXED; + d->arch.hvm.assisted_xapic = + config->arch.misc_flags & XEN_X86_ASSISTED_XAPIC; + + d->arch.hvm.assisted_x2apic = + config->arch.misc_flags & XEN_X86_ASSISTED_X2APIC; + return 0; fail: diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index 7691db0fdd..cb2d9759c9 100644 --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -1154,6 +1154,10 @@ static int construct_vmcs(struct vcpu *v) __vmwrite(PLE_WINDOW, ple_window); } + if ( !v->domain->arch.hvm.assisted_xapic ) + v->arch.hvm.vmx.secondary_exec_control &= + ~SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES; + if ( cpu_has_vmx_secondary_exec_control ) __vmwrite(SECONDARY_VM_EXEC_CONTROL, v->arch.hvm.vmx.secondary_exec_control); diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index c44cf8f5d4..6501fb1a08 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -3342,16 +3342,19 @@ static void vmx_install_vlapic_mapping(struct vcpu *v) void vmx_vlapic_msr_changed(struct vcpu *v) { - int virtualize_x2apic_mode; + int virtualize_xapic_mode, virtualize_x2apic_mode; struct vlapic *vlapic = vcpu_vlapic(v); unsigned int msr; + virtualize_xapic_mode = ( cpu_has_vmx_virtualize_apic_accesses && + v->domain->arch.hvm.assisted_xapic ); + virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtual_intr_delivery) && - cpu_has_vmx_virtualize_x2apic_mode ); + cpu_has_vmx_virtualize_x2apic_mode && + v->domain->arch.hvm.assisted_x2apic ); - if ( !cpu_has_vmx_virtualize_apic_accesses && - !virtualize_x2apic_mode ) + if ( !virtualize_xapic_mode && !virtualize_x2apic_mode ) return; vmx_vmcs_enter(v); @@ -3382,7 +3385,7 @@ void vmx_vlapic_msr_changed(struct vcpu *v) vmx_clear_msr_intercept(v, MSR_X2APIC_SELF, VMX_MSR_W); } } - else + else if ( virtualize_xapic_mode ) v->arch.hvm.vmx.secondary_exec_control |= SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES; } diff --git a/xen/arch/x86/include/asm/hvm/domain.h b/xen/arch/x86/include/asm/hvm/domain.h index 698455444e..0e888cbeba 100644 --- a/xen/arch/x86/include/asm/hvm/domain.h +++ b/xen/arch/x86/include/asm/hvm/domain.h @@ -117,6 +117,12 @@ struct hvm_domain { bool is_s3_suspended; + /* xAPIC hardware assisted emulation. */ + bool assisted_xapic; + + /* x2APIC hardware assisted emulation. */ + bool assisted_x2apic; + /* hypervisor intercepted msix table */ struct list_head msixtbl_list; diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 485bd66971..c215605814 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -1115,7 +1115,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, if ( !is_hvm_domain(d) || subleaf != 0 ) break; - if ( cpu_has_vmx_apic_reg_virt ) + if ( cpu_has_vmx_apic_reg_virt && + v->domain->arch.hvm.assisted_xapic ) res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT; /* @@ -1126,7 +1127,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, */ if ( cpu_has_vmx_virtualize_x2apic_mode && cpu_has_vmx_apic_reg_virt && - cpu_has_vmx_virtual_intr_delivery ) + cpu_has_vmx_virtual_intr_delivery && + v->domain->arch.hvm.assisted_x2apic ) res->a |= XEN_HVM_CPUID_X2APIC_VIRT; /* diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h index f7b17e0f98..3c63c027a8 100644 --- a/xen/include/public/arch-x86/xen.h +++ b/xen/include/public/arch-x86/xen.h @@ -317,6 +317,8 @@ struct xen_arch_domainconfig { * doesn't allow the guest to read or write to the underlying MSR. */ #define XEN_X86_MSR_RELAXED (1u << 0) +#define XEN_X86_ASSISTED_XAPIC (1u << 1) +#define XEN_X86_ASSISTED_X2APIC (1u << 2) uint32_t misc_flags; }; -- 2.11.0
On 27.01.2022 17:01, 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. This is odd to read for a patch which makes it possible to _turn off_ acceleration. Instead it would be interesting to know what problems you have encountered making it desirable to have a way to turn this off. > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -3342,16 +3342,19 @@ static void vmx_install_vlapic_mapping(struct vcpu *v) > > void vmx_vlapic_msr_changed(struct vcpu *v) > { > - int virtualize_x2apic_mode; > + int virtualize_xapic_mode, virtualize_x2apic_mode; Please switch to bool as you touch and extend this. > struct vlapic *vlapic = vcpu_vlapic(v); > unsigned int msr; > > + virtualize_xapic_mode = ( cpu_has_vmx_virtualize_apic_accesses && > + v->domain->arch.hvm.assisted_xapic ); Please don't clone the bad use of blanks immediately inside parentheses here; instead, ... > virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt || > cpu_has_vmx_virtual_intr_delivery) && > - cpu_has_vmx_virtualize_x2apic_mode ); > + cpu_has_vmx_virtualize_x2apic_mode && > + v->domain->arch.hvm.assisted_x2apic ); ... since you're touching this anyway, please consider correcting the style violation here. However - do you need these expressions anymore? The per-domain fields can only be set if the respective CPU capabilities exit. > --- a/xen/arch/x86/include/asm/hvm/domain.h > +++ b/xen/arch/x86/include/asm/hvm/domain.h > @@ -117,6 +117,12 @@ struct hvm_domain { > > bool is_s3_suspended; > > + /* xAPIC hardware assisted emulation. */ > + bool assisted_xapic; > + > + /* x2APIC hardware assisted emulation. */ > + bool assisted_x2apic; > + > /* hypervisor intercepted msix table */ > struct list_head msixtbl_list; Please follow how adjacent code pads types / names here. > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -1115,7 +1115,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, > if ( !is_hvm_domain(d) || subleaf != 0 ) > break; > > - if ( cpu_has_vmx_apic_reg_virt ) > + if ( cpu_has_vmx_apic_reg_virt && > + v->domain->arch.hvm.assisted_xapic ) > res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT; > > /* > @@ -1126,7 +1127,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, > */ > if ( cpu_has_vmx_virtualize_x2apic_mode && > cpu_has_vmx_apic_reg_virt && > - cpu_has_vmx_virtual_intr_delivery ) > + cpu_has_vmx_virtual_intr_delivery && > + v->domain->arch.hvm.assisted_x2apic ) > res->a |= XEN_HVM_CPUID_X2APIC_VIRT; Same remark as above - can't you now use _just_ the per-domain field? In this latter of the two cases this would then also mean bringing the CPU feature checks in line with what vmx_vlapic_msr_changed() does (as also pointed out for patch 1). Albeit it might be best to have a prereq patch fixing the issue, which could then be backported. Jan
Hi, On 31/01/2022 12:05, Jan Beulich wrote: > On 27.01.2022 17:01, 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. > > This is odd to read for a patch which makes it possible to _turn off_ > acceleration. Instead it would be interesting to know what problems > you have encountered making it desirable to have a way to turn this off. Makes sense. > >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -3342,16 +3342,19 @@ static void vmx_install_vlapic_mapping(struct vcpu *v) >> >> void vmx_vlapic_msr_changed(struct vcpu *v) >> { >> - int virtualize_x2apic_mode; >> + int virtualize_xapic_mode, virtualize_x2apic_mode; > > Please switch to bool as you touch and extend this. > >> struct vlapic *vlapic = vcpu_vlapic(v); >> unsigned int msr; >> >> + virtualize_xapic_mode = ( cpu_has_vmx_virtualize_apic_accesses && >> + v->domain->arch.hvm.assisted_xapic ); > > Please don't clone the bad use of blanks immediately inside parentheses > here; instead, ... > >> virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt || >> cpu_has_vmx_virtual_intr_delivery) && >> - cpu_has_vmx_virtualize_x2apic_mode ); >> + cpu_has_vmx_virtualize_x2apic_mode && >> + v->domain->arch.hvm.assisted_x2apic ); > > ... since you're touching this anyway, please consider correcting > the style violation here. > > However - do you need these expressions anymore? The per-domain fields > can only be set if the respective CPU capabilities exit. > >> --- a/xen/arch/x86/include/asm/hvm/domain.h >> +++ b/xen/arch/x86/include/asm/hvm/domain.h >> @@ -117,6 +117,12 @@ struct hvm_domain { >> >> bool is_s3_suspended; >> >> + /* xAPIC hardware assisted emulation. */ >> + bool assisted_xapic; >> + >> + /* x2APIC hardware assisted emulation. */ >> + bool assisted_x2apic; >> + >> /* hypervisor intercepted msix table */ >> struct list_head msixtbl_list; > > Please follow how adjacent code pads types / names here. > >> --- a/xen/arch/x86/traps.c >> +++ b/xen/arch/x86/traps.c >> @@ -1115,7 +1115,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, >> if ( !is_hvm_domain(d) || subleaf != 0 ) >> break; >> >> - if ( cpu_has_vmx_apic_reg_virt ) >> + if ( cpu_has_vmx_apic_reg_virt && >> + v->domain->arch.hvm.assisted_xapic ) >> res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT; >> >> /* >> @@ -1126,7 +1127,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, >> */ >> if ( cpu_has_vmx_virtualize_x2apic_mode && >> cpu_has_vmx_apic_reg_virt && >> - cpu_has_vmx_virtual_intr_delivery ) >> + cpu_has_vmx_virtual_intr_delivery && >> + v->domain->arch.hvm.assisted_x2apic ) >> res->a |= XEN_HVM_CPUID_X2APIC_VIRT; > > Same remark as above - can't you now use _just_ the per-domain field? > In this latter of the two cases this would then also mean bringing > the CPU feature checks in line with what vmx_vlapic_msr_changed() > does (as also pointed out for patch 1). Albeit it might be best to > have a prereq patch fixing the issue, which could then be backported. Indeed, that would be better. Here I think the choice has been to only announce harwdare assisted x2apic support in CPUID if both read and write accesses are virtualized. I could either keep the x2apic_available check in Patch 1 as is and use the per-domain field here, and keep vmx_vlapic_msr_changed as is or vv. > > Jan > > Thank you, Jane.
On Thu, Jan 27, 2022 at 04:01:33PM +0000, Jane Malalane wrote: > 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. > + > =item B<vif.default.script="PATH"> > > Configures the default hotplug script used by virtual network devices. > 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); It seems there's a missing 'return 0' in this function now ;-). > 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); I don't think moving this call later is a good idea. It looks like it's going to break on ARM at least. Instead, calling libxl_get_physinfo() earlier should be fine I think. > 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), Could you add some LIBXL_HAVE_* macro in libxl.h about those new fields like you did in the previous patch? > ])), > # 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 > @@ -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; > + } Would it make sens to enable assisted_xapic and assisted_x2apic by default based on hardware support? That way users of libxl could benefit from the upgrade without having to enable it. Or maybe that could causes issues, like maybe on migration? > + > + rc = 0; > +out: > + return rc; > + > } > > int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc, > 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 seems to contradict the manual which says "Default: enabled". I think you need one more change in ocaml bindings, its ABI checker fails. I found the following change to work, hopefully that the right thing to do: diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c index 5b4fe72c8d..0aa957d379 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 */ (VAL_MISC_FLAGS); #undef VAL_MISC_FLAGS Thanks,
On 31.01.2022 17:05, Anthony PERARD wrote: > On Thu, Jan 27, 2022 at 04:01:33PM +0000, Jane Malalane wrote: >> --- a/tools/libs/light/libxl_x86.c >> +++ b/tools/libs/light/libxl_x86.c >> @@ -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; >> + } > > Would it make sens to enable assisted_xapic and assisted_x2apic by > default based on hardware support? That way users of libxl could benefit > from the upgrade without having to enable it. I think that's the only sensible way - disabling by default would result in perceived performance regressions, I suppose. Jan
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@citrix.com> > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Wei Liu <wl@xen.org> > CC: Anthony PERARD <anthony.perard@citrix.com> > CC: Juergen Gross <jgross@suse.com> > CC: Andrew Cooper <andrew.cooper3@citrix.com> > CC: George Dunlap <george.dunlap@citrix.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Julien Grall <julien@xen.org> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Christian Lindig <christian.lindig@citrix.com> > CC: David Scott <dave@recoil.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: "Roger Pau Monné" <roger.pau@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.
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@citrix.com> >> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Wei Liu <wl@xen.org> >> CC: Anthony PERARD <anthony.perard@citrix.com> >> CC: Juergen Gross <jgross@suse.com> >> CC: Andrew Cooper <andrew.cooper3@citrix.com> >> CC: George Dunlap <george.dunlap@citrix.com> >> CC: Jan Beulich <jbeulich@suse.com> >> CC: Julien Grall <julien@xen.org> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: Christian Lindig <christian.lindig@citrix.com> >> CC: David Scott <dave@recoil.org> >> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> CC: "Roger Pau Monné" <roger.pau@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. 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.
On Wed, Feb 02, 2022 at 03:19:13PM +0000, Jane Malalane wrote: > On 01/02/2022 10:02, Roger Pau Monné wrote: > > On Thu, Jan 27, 2022 at 04:01:33PM +0000, Jane Malalane wrote: > >> 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 > >> @@ -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); I think you actively need to set assisted_x{2}apic if they are using default values, or else a later call to libxl_defbool_val will cause an assert to trigger. libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, physinfo->cap_assisted_xapic); assisted_x{2}apic need to either resolve to true or false past this point, but must not be left using it's default (uninitialized) value. > } > > 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. I would prefer if we actively rejected options that don't make sense. It's wrong to try to set assisted_x{2}apic for PV because there's no APIC at all in that case. I will defer to the maintainer, but I would prefer if an error was reported in that case. I know we are not consistent in that regard, so I'm not going to block what you propose. Thanks, Roger.
On 31/01/2022 12:05, Jan Beulich wrote: > On 27.01.2022 17:01, 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. > > This is odd to read for a patch which makes it possible to _turn off_ > acceleration. Instead it would be interesting to know what problems > you have encountered making it desirable to have a way to turn this off. Hi Jan, After speaking to Andrew he told me of a performance regression that was reported some time ago when enabling apicv related to the pass-through LAPIC timer of a HVM guest causing Xen to intercept the LAPIC timer MSR, making anything that uses the LAPIC timer end up slower than it was before. So, adressing your comment here, other than mentioning how being able to disable acceleration for apicv can be useful when testing and debugging, do you think it's worth mentioning (in more detail) that this perf problem exists, in the commit message. Thanks, Jane. > >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -3342,16 +3342,19 @@ static void vmx_install_vlapic_mapping(struct vcpu *v) >> >> void vmx_vlapic_msr_changed(struct vcpu *v) >> { >> - int virtualize_x2apic_mode; >> + int virtualize_xapic_mode, virtualize_x2apic_mode; > > Please switch to bool as you touch and extend this. > >> struct vlapic *vlapic = vcpu_vlapic(v); >> unsigned int msr; >> >> + virtualize_xapic_mode = ( cpu_has_vmx_virtualize_apic_accesses && >> + v->domain->arch.hvm.assisted_xapic ); > > Please don't clone the bad use of blanks immediately inside parentheses > here; instead, ... > >> virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt || >> cpu_has_vmx_virtual_intr_delivery) && >> - cpu_has_vmx_virtualize_x2apic_mode ); >> + cpu_has_vmx_virtualize_x2apic_mode && >> + v->domain->arch.hvm.assisted_x2apic ); > > ... since you're touching this anyway, please consider correcting > the style violation here. > > However - do you need these expressions anymore? The per-domain fields > can only be set if the respective CPU capabilities exit. > >> --- a/xen/arch/x86/include/asm/hvm/domain.h >> +++ b/xen/arch/x86/include/asm/hvm/domain.h >> @@ -117,6 +117,12 @@ struct hvm_domain { >> >> bool is_s3_suspended; >> >> + /* xAPIC hardware assisted emulation. */ >> + bool assisted_xapic; >> + >> + /* x2APIC hardware assisted emulation. */ >> + bool assisted_x2apic; >> + >> /* hypervisor intercepted msix table */ >> struct list_head msixtbl_list; > > Please follow how adjacent code pads types / names here. > >> --- a/xen/arch/x86/traps.c >> +++ b/xen/arch/x86/traps.c >> @@ -1115,7 +1115,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, >> if ( !is_hvm_domain(d) || subleaf != 0 ) >> break; >> >> - if ( cpu_has_vmx_apic_reg_virt ) >> + if ( cpu_has_vmx_apic_reg_virt && >> + v->domain->arch.hvm.assisted_xapic ) >> res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT; >> >> /* >> @@ -1126,7 +1127,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, >> */ >> if ( cpu_has_vmx_virtualize_x2apic_mode && >> cpu_has_vmx_apic_reg_virt && >> - cpu_has_vmx_virtual_intr_delivery ) >> + cpu_has_vmx_virtual_intr_delivery && >> + v->domain->arch.hvm.assisted_x2apic ) >> res->a |= XEN_HVM_CPUID_X2APIC_VIRT; > > Same remark as above - can't you now use _just_ the per-domain field? > In this latter of the two cases this would then also mean bringing > the CPU feature checks in line with what vmx_vlapic_msr_changed() > does (as also pointed out for patch 1). Albeit it might be best to > have a prereq patch fixing the issue, which could then be backported. > > Jan > >
On 08.02.2022 14:27, Jane Malalane wrote: > On 31/01/2022 12:05, Jan Beulich wrote: >> On 27.01.2022 17:01, 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. >> >> This is odd to read for a patch which makes it possible to _turn off_ >> acceleration. Instead it would be interesting to know what problems >> you have encountered making it desirable to have a way to turn this off. > > After speaking to Andrew he told me of a performance regression that was > reported some time ago when enabling apicv related to the pass-through > LAPIC timer of a HVM guest causing Xen to intercept the LAPIC timer MSR, > making anything that uses the LAPIC timer end up slower than it was > before. So, adressing your comment here, other than mentioning how being > able to disable acceleration for apicv can be useful when testing and > debugging, do you think it's worth mentioning (in more detail) that this > perf problem exists, in the commit message. Yes, I think it would be worth mentioning, as then the purpose of this change is also to be a workaround there, not just testing/debugging. In fact this workaround may then be viewed as the primary purpose. Jan
On 08/02/2022 13:52, Jan Beulich wrote: > [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe. > > On 08.02.2022 14:27, Jane Malalane wrote: >> On 31/01/2022 12:05, Jan Beulich wrote: >>> On 27.01.2022 17:01, 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. >>> >>> This is odd to read for a patch which makes it possible to _turn off_ >>> acceleration. Instead it would be interesting to know what problems >>> you have encountered making it desirable to have a way to turn this off. >> >> After speaking to Andrew he told me of a performance regression that was >> reported some time ago when enabling apicv related to the pass-through >> LAPIC timer of a HVM guest causing Xen to intercept the LAPIC timer MSR, >> making anything that uses the LAPIC timer end up slower than it was >> before. So, adressing your comment here, other than mentioning how being >> able to disable acceleration for apicv can be useful when testing and >> debugging, do you think it's worth mentioning (in more detail) that this >> perf problem exists, in the commit message. > > Yes, I think it would be worth mentioning, as then the purpose of this > change is also to be a workaround there, not just testing/debugging. In > fact this workaround may then be viewed as the primary purpose. > Okay I will add this in a v3 (alongside other changes I'll have to make). Thank you, Jane.
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)>. + =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. + =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; + } + + rc = 0; +out: + return rc; + } 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; 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); + 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", + assisted_xapic && !assisted_xapic_available ? "" : "2"); + return -EINVAL; + } + return 0; } @@ -863,6 +882,12 @@ int arch_domain_create(struct domain *d, d->arch.msr_relaxed = config->arch.misc_flags & XEN_X86_MSR_RELAXED; + d->arch.hvm.assisted_xapic = + config->arch.misc_flags & XEN_X86_ASSISTED_XAPIC; + + d->arch.hvm.assisted_x2apic = + config->arch.misc_flags & XEN_X86_ASSISTED_X2APIC; + return 0; fail: diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index 7691db0fdd..cb2d9759c9 100644 --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -1154,6 +1154,10 @@ static int construct_vmcs(struct vcpu *v) __vmwrite(PLE_WINDOW, ple_window); } + if ( !v->domain->arch.hvm.assisted_xapic ) + v->arch.hvm.vmx.secondary_exec_control &= + ~SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES; + if ( cpu_has_vmx_secondary_exec_control ) __vmwrite(SECONDARY_VM_EXEC_CONTROL, v->arch.hvm.vmx.secondary_exec_control); diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index c44cf8f5d4..6501fb1a08 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -3342,16 +3342,19 @@ static void vmx_install_vlapic_mapping(struct vcpu *v) void vmx_vlapic_msr_changed(struct vcpu *v) { - int virtualize_x2apic_mode; + int virtualize_xapic_mode, virtualize_x2apic_mode; struct vlapic *vlapic = vcpu_vlapic(v); unsigned int msr; + virtualize_xapic_mode = ( cpu_has_vmx_virtualize_apic_accesses && + v->domain->arch.hvm.assisted_xapic ); + virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt || cpu_has_vmx_virtual_intr_delivery) && - cpu_has_vmx_virtualize_x2apic_mode ); + cpu_has_vmx_virtualize_x2apic_mode && + v->domain->arch.hvm.assisted_x2apic ); - if ( !cpu_has_vmx_virtualize_apic_accesses && - !virtualize_x2apic_mode ) + if ( !virtualize_xapic_mode && !virtualize_x2apic_mode ) return; vmx_vmcs_enter(v); @@ -3382,7 +3385,7 @@ void vmx_vlapic_msr_changed(struct vcpu *v) vmx_clear_msr_intercept(v, MSR_X2APIC_SELF, VMX_MSR_W); } } - else + else if ( virtualize_xapic_mode ) v->arch.hvm.vmx.secondary_exec_control |= SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES; } diff --git a/xen/arch/x86/include/asm/hvm/domain.h b/xen/arch/x86/include/asm/hvm/domain.h index 698455444e..0e888cbeba 100644 --- a/xen/arch/x86/include/asm/hvm/domain.h +++ b/xen/arch/x86/include/asm/hvm/domain.h @@ -117,6 +117,12 @@ struct hvm_domain { bool is_s3_suspended; + /* xAPIC hardware assisted emulation. */ + bool assisted_xapic; + + /* x2APIC hardware assisted emulation. */ + bool assisted_x2apic; + /* hypervisor intercepted msix table */ struct list_head msixtbl_list; diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 485bd66971..c215605814 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -1115,7 +1115,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, if ( !is_hvm_domain(d) || subleaf != 0 ) break; - if ( cpu_has_vmx_apic_reg_virt ) + if ( cpu_has_vmx_apic_reg_virt && + v->domain->arch.hvm.assisted_xapic ) res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT; /* @@ -1126,7 +1127,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, */ if ( cpu_has_vmx_virtualize_x2apic_mode && cpu_has_vmx_apic_reg_virt && - cpu_has_vmx_virtual_intr_delivery ) + cpu_has_vmx_virtual_intr_delivery && + v->domain->arch.hvm.assisted_x2apic ) res->a |= XEN_HVM_CPUID_X2APIC_VIRT; /* diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h index f7b17e0f98..3c63c027a8 100644 --- a/xen/include/public/arch-x86/xen.h +++ b/xen/include/public/arch-x86/xen.h @@ -317,6 +317,8 @@ struct xen_arch_domainconfig { * doesn't allow the guest to read or write to the underlying MSR. */ #define XEN_X86_MSR_RELAXED (1u << 0) +#define XEN_X86_ASSISTED_XAPIC (1u << 1) +#define XEN_X86_ASSISTED_X2APIC (1u << 2) uint32_t misc_flags; };
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@citrix.com> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Wei Liu <wl@xen.org> CC: Anthony PERARD <anthony.perard@citrix.com> CC: Juergen Gross <jgross@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: George Dunlap <george.dunlap@citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Julien Grall <julien@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Christian Lindig <christian.lindig@citrix.com> CC: David Scott <dave@recoil.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: "Roger Pau Monné" <roger.pau@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(-)