* [PATCH v2 0/2] xen: Report and use hardware APIC virtualization capabilities
@ 2022-02-07 18:20 Jane Malalane
2022-02-07 18:21 ` [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 Jane Malalane
2022-02-07 18:21 ` [PATCH v2 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC Jane Malalane
0 siblings, 2 replies; 28+ messages in thread
From: Jane Malalane @ 2022-02-07 18:20 UTC (permalink / raw)
To: Xen-devel; +Cc: Jane Malalane
Jane Malalane (2):
xen+tools: Report Interrupt Controller Virtualization capabilities on
x86
x86/xen: Allow per-domain usage of hardware virtualized APIC
docs/man/xl.cfg.5.pod.in | 10 +++++++++
docs/man/xl.conf.5.pod.in | 12 ++++++++++
tools/golang/xenlight/helpers.gen.go | 16 +++++++++++++
tools/golang/xenlight/types.gen.go | 6 +++++
tools/include/libxl.h | 14 ++++++++++++
tools/libs/light/libxl.c | 3 +++
tools/libs/light/libxl_arch.h | 9 ++++++--
tools/libs/light/libxl_arm.c | 12 ++++++++--
tools/libs/light/libxl_create.c | 23 +++++++++++--------
tools/libs/light/libxl_types.idl | 4 ++++
tools/libs/light/libxl_x86.c | 42 +++++++++++++++++++++++++++++++++--
tools/ocaml/libs/xc/xenctrl.ml | 7 ++++++
tools/ocaml/libs/xc/xenctrl.mli | 7 ++++++
tools/ocaml/libs/xc/xenctrl_stubs.c | 2 +-
tools/xl/xl.c | 8 +++++++
tools/xl/xl.h | 2 ++
tools/xl/xl_info.c | 6 +++--
tools/xl/xl_parse.c | 16 +++++++++++++
xen/arch/x86/domain.c | 28 ++++++++++++++++++++++-
xen/arch/x86/hvm/vmx/vmcs.c | 13 +++++++++++
xen/arch/x86/hvm/vmx/vmx.c | 14 +++++-------
xen/arch/x86/include/asm/domain.h | 3 +++
xen/arch/x86/include/asm/hvm/domain.h | 6 +++++
xen/arch/x86/sysctl.c | 7 ++++++
xen/arch/x86/traps.c | 8 +++----
xen/include/public/arch-x86/xen.h | 2 ++
xen/include/public/sysctl.h | 8 ++++++-
27 files changed, 255 insertions(+), 33 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 28+ messages in thread* [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-07 18:20 [PATCH v2 0/2] xen: Report and use hardware APIC virtualization capabilities Jane Malalane @ 2022-02-07 18:21 ` Jane Malalane 2022-02-08 15:26 ` Roger Pau Monné 2022-02-10 10:03 ` Roger Pau Monné 2022-02-07 18:21 ` [PATCH v2 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC Jane Malalane 1 sibling, 2 replies; 28+ messages in thread From: Jane Malalane @ 2022-02-07 18:21 UTC (permalink / raw) To: Xen-devel Cc: Jane Malalane, Wei Liu, Anthony PERARD, Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian, Roger Pau Monné Add XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_xapic and XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_x2apic to report accelerated xapic and x2apic, on x86 hardware. No such features are currently implemented on AMD hardware. For that purpose, also add an arch-specific "capabilities" parameter to struct xen_sysctl_physinfo. 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: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Jun Nakajima <jun.nakajima@intel.com> CC: Kevin Tian <kevin.tian@intel.com> CC: "Roger Pau Monné" <roger.pau@citrix.com> v2: * Use one macro LIBXL_HAVE_PHYSINFO_ASSISTED_APIC instead of two * Pass xcpyshinfo as a pointer in libxl__arch_get_physinfo * Set assisted_x{2}apic_available to be conditional upon "bsp" and annotate it with __ro_after_init * Change XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_X{2}APIC to .._X86_ASSISTED_X{2}APIC * Keep XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X{2}APIC contained within sysctl.h * Fix padding introduced in struct xen_sysctl_physinfo and bump XEN_SYSCTL_INTERFACE_VERSION --- tools/golang/xenlight/helpers.gen.go | 4 ++++ tools/golang/xenlight/types.gen.go | 6 ++++++ tools/include/libxl.h | 7 +++++++ tools/libs/light/libxl.c | 3 +++ tools/libs/light/libxl_arch.h | 4 ++++ tools/libs/light/libxl_arm.c | 5 +++++ tools/libs/light/libxl_types.idl | 2 ++ tools/libs/light/libxl_x86.c | 11 +++++++++++ tools/ocaml/libs/xc/xenctrl.ml | 5 +++++ tools/ocaml/libs/xc/xenctrl.mli | 5 +++++ tools/xl/xl_info.c | 6 ++++-- xen/arch/x86/hvm/vmx/vmcs.c | 9 +++++++++ xen/arch/x86/include/asm/domain.h | 3 +++ xen/arch/x86/sysctl.c | 7 +++++++ xen/include/public/sysctl.h | 8 +++++++- 15 files changed, 82 insertions(+), 3 deletions(-) diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go index b746ff1081..dd4e6c9f14 100644 --- a/tools/golang/xenlight/helpers.gen.go +++ b/tools/golang/xenlight/helpers.gen.go @@ -3373,6 +3373,8 @@ x.CapVmtrace = bool(xc.cap_vmtrace) x.CapVpmu = bool(xc.cap_vpmu) x.CapGnttabV1 = bool(xc.cap_gnttab_v1) x.CapGnttabV2 = bool(xc.cap_gnttab_v2) +x.CapAssistedXapic = bool(xc.cap_assisted_xapic) +x.CapAssistedX2Apic = bool(xc.cap_assisted_x2apic) return nil} @@ -3407,6 +3409,8 @@ xc.cap_vmtrace = C.bool(x.CapVmtrace) xc.cap_vpmu = C.bool(x.CapVpmu) xc.cap_gnttab_v1 = C.bool(x.CapGnttabV1) xc.cap_gnttab_v2 = C.bool(x.CapGnttabV2) +xc.cap_assisted_xapic = C.bool(x.CapAssistedXapic) +xc.cap_assisted_x2apic = C.bool(x.CapAssistedX2Apic) return nil } diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go index b1e84d5258..5f384b767c 100644 --- a/tools/golang/xenlight/types.gen.go +++ b/tools/golang/xenlight/types.gen.go @@ -389,6 +389,10 @@ RunHotplugScripts Defbool DriverDomain Defbool Passthrough Passthrough XendSuspendEvtchnCompat Defbool +ArchX86 struct { +AssistedXapic Defbool +AssistedX2Apic Defbool +} } type DomainRestoreParams struct { @@ -1014,6 +1018,8 @@ CapVmtrace bool CapVpmu bool CapGnttabV1 bool CapGnttabV2 bool +CapAssistedXApic bool +CapAssistedX2apic bool } type Connectorinfo struct { diff --git a/tools/include/libxl.h b/tools/include/libxl.h index 2bbbd21f0b..924e142628 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -528,6 +528,13 @@ #define LIBXL_HAVE_MAX_GRANT_VERSION 1 /* + * LIBXL_HAVE_PHYSINFO_ASSISTED_APIC indicates that libxl_physinfo has + * cap_assisted_x{2}apic fields, which indicates the availability of x{2}APIC + * hardware assisted virtualization. + */ +#define LIBXL_HAVE_PHYSINFO_ASSISTED_APIC 1 + +/* * libxl ABI compatibility * * The only guarantee which libxl makes regarding ABI compatibility diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c index 667ae6409b..fabb474221 100644 --- a/tools/libs/light/libxl.c +++ b/tools/libs/light/libxl.c @@ -15,6 +15,7 @@ #include "libxl_osdeps.h" #include "libxl_internal.h" +#include "libxl_arch.h" int libxl_ctx_alloc(libxl_ctx **pctx, int version, unsigned flags, xentoollog_logger * lg) @@ -410,6 +411,8 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo) physinfo->cap_gnttab_v2 = !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_gnttab_v2); + libxl__arch_get_physinfo(physinfo, &xcphysinfo); + GC_FREE; return 0; } diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h index 1522ecb97f..207ceac6a1 100644 --- a/tools/libs/light/libxl_arch.h +++ b/tools/libs/light/libxl_arch.h @@ -86,6 +86,10 @@ int libxl__arch_extra_memory(libxl__gc *gc, uint64_t *out); _hidden +void libxl__arch_get_physinfo(libxl_physinfo *physinfo, + const xc_physinfo_t *xcphysinfo); + +_hidden void libxl__arch_update_domain_config(libxl__gc *gc, libxl_domain_config *dst, const libxl_domain_config *src); diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index eef1de0939..39fdca1b49 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -1431,6 +1431,11 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc, return rc; } +void libxl__arch_get_physinfo(libxl_physinfo *physinfo, + const xc_physinfo_t *xcphysinfo) +{ +} + void libxl__arch_update_domain_config(libxl__gc *gc, libxl_domain_config *dst, const libxl_domain_config *src) diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl index 2a42da2f7d..42ac6c357b 100644 --- a/tools/libs/light/libxl_types.idl +++ b/tools/libs/light/libxl_types.idl @@ -1068,6 +1068,8 @@ libxl_physinfo = Struct("physinfo", [ ("cap_vpmu", bool), ("cap_gnttab_v1", bool), ("cap_gnttab_v2", bool), + ("cap_assisted_xapic", bool), + ("cap_assisted_x2apic", bool), ], dir=DIR_OUT) libxl_connectorinfo = Struct("connectorinfo", [ diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c index 1feadebb18..e0a06ecfe3 100644 --- a/tools/libs/light/libxl_x86.c +++ b/tools/libs/light/libxl_x86.c @@ -866,6 +866,17 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc, return rc; } +void libxl__arch_get_physinfo(libxl_physinfo *physinfo, + const xc_physinfo_t *xcphysinfo) +{ + physinfo->cap_assisted_xapic = + !!(xcphysinfo->arch_capabilities & + XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC); + physinfo->cap_assisted_x2apic = + !!(xcphysinfo->arch_capabilities & + XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC); +} + void libxl__arch_update_domain_config(libxl__gc *gc, libxl_domain_config *dst, const libxl_domain_config *src) diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml index 7503031d8f..7ce832d605 100644 --- a/tools/ocaml/libs/xc/xenctrl.ml +++ b/tools/ocaml/libs/xc/xenctrl.ml @@ -127,6 +127,10 @@ type physinfo_cap_flag = | CAP_Gnttab_v1 | CAP_Gnttab_v2 +type physinfo_cap_arch_flag = + | CAP_ARCH_ASSISTED_XAPIC + | CAP_ARCH_ASSISTED_X2APIC + type physinfo = { threads_per_core : int; @@ -139,6 +143,7 @@ type physinfo = scrub_pages : nativeint; (* XXX hw_cap *) capabilities : physinfo_cap_flag list; + arch_capabilities : physinfo_cap_arch_flag list; max_nr_cpus : int; } diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli index d1d9c9247a..a2b15130ee 100644 --- a/tools/ocaml/libs/xc/xenctrl.mli +++ b/tools/ocaml/libs/xc/xenctrl.mli @@ -112,6 +112,10 @@ type physinfo_cap_flag = | CAP_Gnttab_v1 | CAP_Gnttab_v2 +type physinfo_cap_arch_flag = + | CAP_ARCH_ASSISTED_XAPIC + | CAP_ARCH_ASSISTED_X2APIC + type physinfo = { threads_per_core : int; cores_per_socket : int; @@ -122,6 +126,7 @@ type physinfo = { free_pages : nativeint; scrub_pages : nativeint; capabilities : physinfo_cap_flag list; + arch_capabilities : physinfo_cap_arch_flag list; max_nr_cpus : int; (** compile-time max possible number of nr_cpus *) } type version = { major : int; minor : int; extra : string; } diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c index 712b7638b0..3205270754 100644 --- a/tools/xl/xl_info.c +++ b/tools/xl/xl_info.c @@ -210,7 +210,7 @@ static void output_physinfo(void) info.hw_cap[4], info.hw_cap[5], info.hw_cap[6], info.hw_cap[7] ); - maybe_printf("virt_caps :%s%s%s%s%s%s%s%s%s%s%s\n", + maybe_printf("virt_caps :%s%s%s%s%s%s%s%s%s%s%s%s%s\n", info.cap_pv ? " pv" : "", info.cap_hvm ? " hvm" : "", info.cap_hvm && info.cap_hvm_directio ? " hvm_directio" : "", @@ -221,7 +221,9 @@ static void output_physinfo(void) info.cap_vmtrace ? " vmtrace" : "", info.cap_vpmu ? " vpmu" : "", info.cap_gnttab_v1 ? " gnttab-v1" : "", - info.cap_gnttab_v2 ? " gnttab-v2" : "" + info.cap_gnttab_v2 ? " gnttab-v2" : "", + info.cap_assisted_xapic ? " assisted_xapic" : "", + info.cap_assisted_x2apic ? " assisted_x2apic" : "" ); vinfo = libxl_get_version_info(ctx); diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index 7ab15e07a0..4060aef1bd 100644 --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); } + /* Check whether hardware supports accelerated xapic and x2apic. */ + if ( bsp ) + { + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || + cpu_has_vmx_virtual_intr_delivery) && + cpu_has_vmx_virtualize_x2apic_mode; + } + /* The IA32_VMX_EPT_VPID_CAP MSR exists only when EPT or VPID available */ if ( _vmx_secondary_exec_control & (SECONDARY_EXEC_ENABLE_EPT | SECONDARY_EXEC_ENABLE_VPID) ) diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h index e62e109598..72431df26d 100644 --- a/xen/arch/x86/include/asm/domain.h +++ b/xen/arch/x86/include/asm/domain.h @@ -756,6 +756,9 @@ static inline void pv_inject_sw_interrupt(unsigned int vector) : is_pv_32bit_domain(d) ? PV32_VM_ASSIST_MASK \ : PV64_VM_ASSIST_MASK) +extern bool assisted_xapic_available; +extern bool assisted_x2apic_available; + #endif /* __ASM_DOMAIN_H__ */ /* diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c index aff52a13f3..642cc96985 100644 --- a/xen/arch/x86/sysctl.c +++ b/xen/arch/x86/sysctl.c @@ -69,6 +69,9 @@ struct l3_cache_info { unsigned long size; }; +bool __ro_after_init assisted_xapic_available; +bool __ro_after_init assisted_x2apic_available; + static void l3_cache_get(void *arg) { struct cpuid4_info info; @@ -135,6 +138,10 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi) pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap; if ( IS_ENABLED(CONFIG_SHADOW_PAGING) ) pi->capabilities |= XEN_SYSCTL_PHYSCAP_shadow; + if ( assisted_xapic_available ) + pi->arch_capabilities |= XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC; + if ( assisted_x2apic_available ) + pi->arch_capabilities |= XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC; } long arch_do_sysctl( diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index 55252e97f2..11328bbf78 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -35,7 +35,7 @@ #include "domctl.h" #include "physdev.h" -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000014 +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000015 /* * Read console content from Xen buffer ring. @@ -111,6 +111,10 @@ struct xen_sysctl_tbuf_op { /* Max XEN_SYSCTL_PHYSCAP_* constant. Used for ABI checking. */ #define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_gnttab_v2 +/* The platform supports x{2}apic hardware assisted emulation. */ +#define XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC (1u << 0) +#define XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC (1u << 1) + struct xen_sysctl_physinfo { uint32_t threads_per_core; uint32_t cores_per_socket; @@ -120,6 +124,8 @@ struct xen_sysctl_physinfo { uint32_t max_node_id; /* Largest possible node ID on this host */ uint32_t cpu_khz; uint32_t capabilities;/* XEN_SYSCTL_PHYSCAP_??? */ + uint32_t arch_capabilities;/* XEN_SYSCTL_PHYSCAP_X86{ARM}_??? */ + uint32_t pad; /* Must be zero. */ uint64_aligned_t total_pages; uint64_aligned_t free_pages; uint64_aligned_t scrub_pages; -- 2.11.0 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-07 18:21 ` [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 Jane Malalane @ 2022-02-08 15:26 ` Roger Pau Monné 2022-02-09 12:26 ` Jane Malalane 2022-02-10 10:03 ` Roger Pau Monné 1 sibling, 1 reply; 28+ messages in thread From: Roger Pau Monné @ 2022-02-08 15:26 UTC (permalink / raw) To: Jane Malalane Cc: Xen-devel, Wei Liu, Anthony PERARD, Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: > Add XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_xapic and > XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_x2apic to report accelerated xapic > and x2apic, on x86 hardware. > No such features are currently implemented on AMD hardware. > > For that purpose, also add an arch-specific "capabilities" parameter > to struct xen_sysctl_physinfo. > > Signed-off-by: Jane Malalane <jane.malalane@citrix.com> > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Tag order should be inverted, first Suggested-by, then SoB. > --- > 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: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis <bertrand.marquis@arm.com> > CC: Jun Nakajima <jun.nakajima@intel.com> > CC: Kevin Tian <kevin.tian@intel.com> > CC: "Roger Pau Monné" <roger.pau@citrix.com> > > v2: > * Use one macro LIBXL_HAVE_PHYSINFO_ASSISTED_APIC instead of two > * Pass xcpyshinfo as a pointer in libxl__arch_get_physinfo > * Set assisted_x{2}apic_available to be conditional upon "bsp" and > annotate it with __ro_after_init > * Change XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_X{2}APIC to > .._X86_ASSISTED_X{2}APIC > * Keep XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X{2}APIC contained within > sysctl.h > * Fix padding introduced in struct xen_sysctl_physinfo and bump > XEN_SYSCTL_INTERFACE_VERSION > --- > tools/golang/xenlight/helpers.gen.go | 4 ++++ > tools/golang/xenlight/types.gen.go | 6 ++++++ > tools/include/libxl.h | 7 +++++++ > tools/libs/light/libxl.c | 3 +++ > tools/libs/light/libxl_arch.h | 4 ++++ > tools/libs/light/libxl_arm.c | 5 +++++ > tools/libs/light/libxl_types.idl | 2 ++ > tools/libs/light/libxl_x86.c | 11 +++++++++++ > tools/ocaml/libs/xc/xenctrl.ml | 5 +++++ > tools/ocaml/libs/xc/xenctrl.mli | 5 +++++ > tools/xl/xl_info.c | 6 ++++-- > xen/arch/x86/hvm/vmx/vmcs.c | 9 +++++++++ > xen/arch/x86/include/asm/domain.h | 3 +++ > xen/arch/x86/sysctl.c | 7 +++++++ > xen/include/public/sysctl.h | 8 +++++++- > 15 files changed, 82 insertions(+), 3 deletions(-) > > diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go > index b746ff1081..dd4e6c9f14 100644 > --- a/tools/golang/xenlight/helpers.gen.go > +++ b/tools/golang/xenlight/helpers.gen.go > @@ -3373,6 +3373,8 @@ x.CapVmtrace = bool(xc.cap_vmtrace) > x.CapVpmu = bool(xc.cap_vpmu) > x.CapGnttabV1 = bool(xc.cap_gnttab_v1) > x.CapGnttabV2 = bool(xc.cap_gnttab_v2) > +x.CapAssistedXapic = bool(xc.cap_assisted_xapic) > +x.CapAssistedX2Apic = bool(xc.cap_assisted_x2apic) > > return nil} > > @@ -3407,6 +3409,8 @@ xc.cap_vmtrace = C.bool(x.CapVmtrace) > xc.cap_vpmu = C.bool(x.CapVpmu) > xc.cap_gnttab_v1 = C.bool(x.CapGnttabV1) > xc.cap_gnttab_v2 = C.bool(x.CapGnttabV2) > +xc.cap_assisted_xapic = C.bool(x.CapAssistedXapic) > +xc.cap_assisted_x2apic = C.bool(x.CapAssistedX2Apic) > > return nil > } > diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go > index b1e84d5258..5f384b767c 100644 > --- a/tools/golang/xenlight/types.gen.go > +++ b/tools/golang/xenlight/types.gen.go > @@ -389,6 +389,10 @@ RunHotplugScripts Defbool > DriverDomain Defbool > Passthrough Passthrough > XendSuspendEvtchnCompat Defbool > +ArchX86 struct { > +AssistedXapic Defbool > +AssistedX2Apic Defbool Don't you need some indentation here? Also name would better be Assistedx{2}APIC IMO if possible. Having a capital 'X' and lowercase 'apic' looks really strange. > +} > } > > type DomainRestoreParams struct { > @@ -1014,6 +1018,8 @@ CapVmtrace bool > CapVpmu bool > CapGnttabV1 bool > CapGnttabV2 bool > +CapAssistedXApic bool > +CapAssistedX2apic bool > } > > type Connectorinfo struct { > diff --git a/tools/include/libxl.h b/tools/include/libxl.h > index 2bbbd21f0b..924e142628 100644 > --- a/tools/include/libxl.h > +++ b/tools/include/libxl.h > @@ -528,6 +528,13 @@ > #define LIBXL_HAVE_MAX_GRANT_VERSION 1 > > /* > + * LIBXL_HAVE_PHYSINFO_ASSISTED_APIC indicates that libxl_physinfo has > + * cap_assisted_x{2}apic fields, which indicates the availability of x{2}APIC > + * hardware assisted virtualization. > + */ > +#define LIBXL_HAVE_PHYSINFO_ASSISTED_APIC 1 > + > +/* > * libxl ABI compatibility > * > * The only guarantee which libxl makes regarding ABI compatibility > diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c > index 667ae6409b..fabb474221 100644 > --- a/tools/libs/light/libxl.c > +++ b/tools/libs/light/libxl.c > @@ -15,6 +15,7 @@ > #include "libxl_osdeps.h" > > #include "libxl_internal.h" > +#include "libxl_arch.h" > > int libxl_ctx_alloc(libxl_ctx **pctx, int version, > unsigned flags, xentoollog_logger * lg) > @@ -410,6 +411,8 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo) > physinfo->cap_gnttab_v2 = > !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_gnttab_v2); > > + libxl__arch_get_physinfo(physinfo, &xcphysinfo); > + > GC_FREE; > return 0; > } > diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h > index 1522ecb97f..207ceac6a1 100644 > --- a/tools/libs/light/libxl_arch.h > +++ b/tools/libs/light/libxl_arch.h > @@ -86,6 +86,10 @@ int libxl__arch_extra_memory(libxl__gc *gc, > uint64_t *out); > > _hidden > +void libxl__arch_get_physinfo(libxl_physinfo *physinfo, > + const xc_physinfo_t *xcphysinfo); > + > +_hidden > void libxl__arch_update_domain_config(libxl__gc *gc, > libxl_domain_config *dst, > const libxl_domain_config *src); > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c > index eef1de0939..39fdca1b49 100644 > --- a/tools/libs/light/libxl_arm.c > +++ b/tools/libs/light/libxl_arm.c > @@ -1431,6 +1431,11 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc, > return rc; > } > > +void libxl__arch_get_physinfo(libxl_physinfo *physinfo, > + const xc_physinfo_t *xcphysinfo) > +{ > +} > + > void libxl__arch_update_domain_config(libxl__gc *gc, > libxl_domain_config *dst, > const libxl_domain_config *src) > diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl > index 2a42da2f7d..42ac6c357b 100644 > --- a/tools/libs/light/libxl_types.idl > +++ b/tools/libs/light/libxl_types.idl > @@ -1068,6 +1068,8 @@ libxl_physinfo = Struct("physinfo", [ > ("cap_vpmu", bool), > ("cap_gnttab_v1", bool), > ("cap_gnttab_v2", bool), > + ("cap_assisted_xapic", bool), > + ("cap_assisted_x2apic", bool), > ], dir=DIR_OUT) > > libxl_connectorinfo = Struct("connectorinfo", [ > diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c > index 1feadebb18..e0a06ecfe3 100644 > --- a/tools/libs/light/libxl_x86.c > +++ b/tools/libs/light/libxl_x86.c > @@ -866,6 +866,17 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc, > return rc; > } > > +void libxl__arch_get_physinfo(libxl_physinfo *physinfo, > + const xc_physinfo_t *xcphysinfo) > +{ > + physinfo->cap_assisted_xapic = > + !!(xcphysinfo->arch_capabilities & > + XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC); > + physinfo->cap_assisted_x2apic = > + !!(xcphysinfo->arch_capabilities & > + XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC); > +} > + > void libxl__arch_update_domain_config(libxl__gc *gc, > libxl_domain_config *dst, > const libxl_domain_config *src) > diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml > index 7503031d8f..7ce832d605 100644 > --- a/tools/ocaml/libs/xc/xenctrl.ml > +++ b/tools/ocaml/libs/xc/xenctrl.ml > @@ -127,6 +127,10 @@ type physinfo_cap_flag = > | CAP_Gnttab_v1 > | CAP_Gnttab_v2 > > +type physinfo_cap_arch_flag = > + | CAP_ARCH_ASSISTED_XAPIC > + | CAP_ARCH_ASSISTED_X2APIC > + > type physinfo = > { > threads_per_core : int; > @@ -139,6 +143,7 @@ type physinfo = > scrub_pages : nativeint; > (* XXX hw_cap *) > capabilities : physinfo_cap_flag list; > + arch_capabilities : physinfo_cap_arch_flag list; I know very little about Ocaml, but I think you are not setting this field anywhere? I would expect a call to ocaml_list_to_c_bitmap and then you will likely need to define XEN_SYSCTL_PHYSCAP_X86_MAX so you can check the options. See XEN_SYSCTL_PHYSCAP_MAX for example. > max_nr_cpus : int; > } > > diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli > index d1d9c9247a..a2b15130ee 100644 > --- a/tools/ocaml/libs/xc/xenctrl.mli > +++ b/tools/ocaml/libs/xc/xenctrl.mli > @@ -112,6 +112,10 @@ type physinfo_cap_flag = > | CAP_Gnttab_v1 > | CAP_Gnttab_v2 > > +type physinfo_cap_arch_flag = > + | CAP_ARCH_ASSISTED_XAPIC > + | CAP_ARCH_ASSISTED_X2APIC > + > type physinfo = { > threads_per_core : int; > cores_per_socket : int; > @@ -122,6 +126,7 @@ type physinfo = { > free_pages : nativeint; > scrub_pages : nativeint; > capabilities : physinfo_cap_flag list; > + arch_capabilities : physinfo_cap_arch_flag list; > max_nr_cpus : int; (** compile-time max possible number of nr_cpus *) > } > type version = { major : int; minor : int; extra : string; } > diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c > index 712b7638b0..3205270754 100644 > --- a/tools/xl/xl_info.c > +++ b/tools/xl/xl_info.c > @@ -210,7 +210,7 @@ static void output_physinfo(void) > info.hw_cap[4], info.hw_cap[5], info.hw_cap[6], info.hw_cap[7] > ); > > - maybe_printf("virt_caps :%s%s%s%s%s%s%s%s%s%s%s\n", > + maybe_printf("virt_caps :%s%s%s%s%s%s%s%s%s%s%s%s%s\n", > info.cap_pv ? " pv" : "", > info.cap_hvm ? " hvm" : "", > info.cap_hvm && info.cap_hvm_directio ? " hvm_directio" : "", > @@ -221,7 +221,9 @@ static void output_physinfo(void) > info.cap_vmtrace ? " vmtrace" : "", > info.cap_vpmu ? " vpmu" : "", > info.cap_gnttab_v1 ? " gnttab-v1" : "", > - info.cap_gnttab_v2 ? " gnttab-v2" : "" > + info.cap_gnttab_v2 ? " gnttab-v2" : "", > + info.cap_assisted_xapic ? " assisted_xapic" : "", > + info.cap_assisted_x2apic ? " assisted_x2apic" : "" > ); > > vinfo = libxl_get_version_info(ctx); > diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c > index 7ab15e07a0..4060aef1bd 100644 > --- a/xen/arch/x86/hvm/vmx/vmcs.c > +++ b/xen/arch/x86/hvm/vmx/vmcs.c > @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) > MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); > } > > + /* Check whether hardware supports accelerated xapic and x2apic. */ > + if ( bsp ) > + { > + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; > + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || > + cpu_has_vmx_virtual_intr_delivery) && > + cpu_has_vmx_virtualize_x2apic_mode; > + } > + > /* The IA32_VMX_EPT_VPID_CAP MSR exists only when EPT or VPID available */ > if ( _vmx_secondary_exec_control & (SECONDARY_EXEC_ENABLE_EPT | > SECONDARY_EXEC_ENABLE_VPID) ) > diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h > index e62e109598..72431df26d 100644 > --- a/xen/arch/x86/include/asm/domain.h > +++ b/xen/arch/x86/include/asm/domain.h > @@ -756,6 +756,9 @@ static inline void pv_inject_sw_interrupt(unsigned int vector) > : is_pv_32bit_domain(d) ? PV32_VM_ASSIST_MASK \ > : PV64_VM_ASSIST_MASK) > > +extern bool assisted_xapic_available; > +extern bool assisted_x2apic_available; > + > #endif /* __ASM_DOMAIN_H__ */ > > /* > diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c > index aff52a13f3..642cc96985 100644 > --- a/xen/arch/x86/sysctl.c > +++ b/xen/arch/x86/sysctl.c > @@ -69,6 +69,9 @@ struct l3_cache_info { > unsigned long size; > }; > > +bool __ro_after_init assisted_xapic_available; > +bool __ro_after_init assisted_x2apic_available; You could likely shorten this by dropping the _available suffix. Thanks, Roger. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-08 15:26 ` Roger Pau Monné @ 2022-02-09 12:26 ` Jane Malalane 2022-02-09 13:48 ` Anthony PERARD 0 siblings, 1 reply; 28+ messages in thread From: Jane Malalane @ 2022-02-09 12:26 UTC (permalink / raw) To: Roger Pau Monne Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian On 08/02/2022 15:26, Roger Pau Monné wrote: > On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: >> Add XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_xapic and >> XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_x2apic to report accelerated xapic >> and x2apic, on x86 hardware. >> No such features are currently implemented on AMD hardware. >> >> For that purpose, also add an arch-specific "capabilities" parameter >> to struct xen_sysctl_physinfo. >> >> Signed-off-by: Jane Malalane <jane.malalane@citrix.com> >> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Tag order should be inverted, first Suggested-by, then SoB. > >> --- >> 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: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> CC: Bertrand Marquis <bertrand.marquis@arm.com> >> CC: Jun Nakajima <jun.nakajima@intel.com> >> CC: Kevin Tian <kevin.tian@intel.com> >> CC: "Roger Pau Monné" <roger.pau@citrix.com> >> >> v2: >> * Use one macro LIBXL_HAVE_PHYSINFO_ASSISTED_APIC instead of two >> * Pass xcpyshinfo as a pointer in libxl__arch_get_physinfo >> * Set assisted_x{2}apic_available to be conditional upon "bsp" and >> annotate it with __ro_after_init >> * Change XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_X{2}APIC to >> .._X86_ASSISTED_X{2}APIC >> * Keep XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X{2}APIC contained within >> sysctl.h >> * Fix padding introduced in struct xen_sysctl_physinfo and bump >> XEN_SYSCTL_INTERFACE_VERSION >> --- >> tools/golang/xenlight/helpers.gen.go | 4 ++++ >> tools/golang/xenlight/types.gen.go | 6 ++++++ >> tools/include/libxl.h | 7 +++++++ >> tools/libs/light/libxl.c | 3 +++ >> tools/libs/light/libxl_arch.h | 4 ++++ >> tools/libs/light/libxl_arm.c | 5 +++++ >> tools/libs/light/libxl_types.idl | 2 ++ >> tools/libs/light/libxl_x86.c | 11 +++++++++++ >> tools/ocaml/libs/xc/xenctrl.ml | 5 +++++ >> tools/ocaml/libs/xc/xenctrl.mli | 5 +++++ >> tools/xl/xl_info.c | 6 ++++-- >> xen/arch/x86/hvm/vmx/vmcs.c | 9 +++++++++ >> xen/arch/x86/include/asm/domain.h | 3 +++ >> xen/arch/x86/sysctl.c | 7 +++++++ >> xen/include/public/sysctl.h | 8 +++++++- >> 15 files changed, 82 insertions(+), 3 deletions(-) >> >> diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go >> index b746ff1081..dd4e6c9f14 100644 >> --- a/tools/golang/xenlight/helpers.gen.go >> +++ b/tools/golang/xenlight/helpers.gen.go >> @@ -3373,6 +3373,8 @@ x.CapVmtrace = bool(xc.cap_vmtrace) >> x.CapVpmu = bool(xc.cap_vpmu) >> x.CapGnttabV1 = bool(xc.cap_gnttab_v1) >> x.CapGnttabV2 = bool(xc.cap_gnttab_v2) >> +x.CapAssistedXapic = bool(xc.cap_assisted_xapic) >> +x.CapAssistedX2Apic = bool(xc.cap_assisted_x2apic) >> >> return nil} >> >> @@ -3407,6 +3409,8 @@ xc.cap_vmtrace = C.bool(x.CapVmtrace) >> xc.cap_vpmu = C.bool(x.CapVpmu) >> xc.cap_gnttab_v1 = C.bool(x.CapGnttabV1) >> xc.cap_gnttab_v2 = C.bool(x.CapGnttabV2) >> +xc.cap_assisted_xapic = C.bool(x.CapAssistedXapic) >> +xc.cap_assisted_x2apic = C.bool(x.CapAssistedX2Apic) >> >> return nil >> } >> diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go >> index b1e84d5258..5f384b767c 100644 >> --- a/tools/golang/xenlight/types.gen.go >> +++ b/tools/golang/xenlight/types.gen.go >> @@ -389,6 +389,10 @@ RunHotplugScripts Defbool >> DriverDomain Defbool >> Passthrough Passthrough >> XendSuspendEvtchnCompat Defbool >> +ArchX86 struct { >> +AssistedXapic Defbool >> +AssistedX2Apic Defbool > > Don't you need some indentation here? I hadn't realized it appeared like this here (and the same happens for other parts of my code as I'm seeing now) because the git output is correct. I will fix it. > > Also name would better be Assistedx{2}APIC IMO if possible. Having a > capital 'X' and lowercase 'apic' looks really strange. Okay. > >> +} >> } >> >> type DomainRestoreParams struct { >> @@ -1014,6 +1018,8 @@ CapVmtrace bool >> CapVpmu bool >> CapGnttabV1 bool >> CapGnttabV2 bool >> +CapAssistedXApic bool >> +CapAssistedX2apic bool >> } >> >> type Connectorinfo struct { >> diff --git a/tools/include/libxl.h b/tools/include/libxl.h >> index 2bbbd21f0b..924e142628 100644 >> --- a/tools/include/libxl.h >> +++ b/tools/include/libxl.h >> @@ -528,6 +528,13 @@ >> #define LIBXL_HAVE_MAX_GRANT_VERSION 1 >> >> /* >> + * LIBXL_HAVE_PHYSINFO_ASSISTED_APIC indicates that libxl_physinfo has >> + * cap_assisted_x{2}apic fields, which indicates the availability of x{2}APIC >> + * hardware assisted virtualization. >> + */ >> +#define LIBXL_HAVE_PHYSINFO_ASSISTED_APIC 1 >> + >> +/* >> * libxl ABI compatibility >> * >> * The only guarantee which libxl makes regarding ABI compatibility >> diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c >> index 667ae6409b..fabb474221 100644 >> --- a/tools/libs/light/libxl.c >> +++ b/tools/libs/light/libxl.c >> @@ -15,6 +15,7 @@ >> #include "libxl_osdeps.h" >> >> #include "libxl_internal.h" >> +#include "libxl_arch.h" >> >> int libxl_ctx_alloc(libxl_ctx **pctx, int version, >> unsigned flags, xentoollog_logger * lg) >> @@ -410,6 +411,8 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo) >> physinfo->cap_gnttab_v2 = >> !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_gnttab_v2); >> >> + libxl__arch_get_physinfo(physinfo, &xcphysinfo); >> + >> GC_FREE; >> return 0; >> } >> diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h >> index 1522ecb97f..207ceac6a1 100644 >> --- a/tools/libs/light/libxl_arch.h >> +++ b/tools/libs/light/libxl_arch.h >> @@ -86,6 +86,10 @@ int libxl__arch_extra_memory(libxl__gc *gc, >> uint64_t *out); >> >> _hidden >> +void libxl__arch_get_physinfo(libxl_physinfo *physinfo, >> + const xc_physinfo_t *xcphysinfo); >> + >> +_hidden >> void libxl__arch_update_domain_config(libxl__gc *gc, >> libxl_domain_config *dst, >> const libxl_domain_config *src); >> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c >> index eef1de0939..39fdca1b49 100644 >> --- a/tools/libs/light/libxl_arm.c >> +++ b/tools/libs/light/libxl_arm.c >> @@ -1431,6 +1431,11 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc, >> return rc; >> } >> >> +void libxl__arch_get_physinfo(libxl_physinfo *physinfo, >> + const xc_physinfo_t *xcphysinfo) >> +{ >> +} >> + >> void libxl__arch_update_domain_config(libxl__gc *gc, >> libxl_domain_config *dst, >> const libxl_domain_config *src) >> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl >> index 2a42da2f7d..42ac6c357b 100644 >> --- a/tools/libs/light/libxl_types.idl >> +++ b/tools/libs/light/libxl_types.idl >> @@ -1068,6 +1068,8 @@ libxl_physinfo = Struct("physinfo", [ >> ("cap_vpmu", bool), >> ("cap_gnttab_v1", bool), >> ("cap_gnttab_v2", bool), >> + ("cap_assisted_xapic", bool), >> + ("cap_assisted_x2apic", bool), >> ], dir=DIR_OUT) >> >> libxl_connectorinfo = Struct("connectorinfo", [ >> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c >> index 1feadebb18..e0a06ecfe3 100644 >> --- a/tools/libs/light/libxl_x86.c >> +++ b/tools/libs/light/libxl_x86.c >> @@ -866,6 +866,17 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc, >> return rc; >> } >> >> +void libxl__arch_get_physinfo(libxl_physinfo *physinfo, >> + const xc_physinfo_t *xcphysinfo) >> +{ >> + physinfo->cap_assisted_xapic = >> + !!(xcphysinfo->arch_capabilities & >> + XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC); >> + physinfo->cap_assisted_x2apic = >> + !!(xcphysinfo->arch_capabilities & >> + XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC); >> +} >> + >> void libxl__arch_update_domain_config(libxl__gc *gc, >> libxl_domain_config *dst, >> const libxl_domain_config *src) >> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml >> index 7503031d8f..7ce832d605 100644 >> --- a/tools/ocaml/libs/xc/xenctrl.ml >> +++ b/tools/ocaml/libs/xc/xenctrl.ml >> @@ -127,6 +127,10 @@ type physinfo_cap_flag = >> | CAP_Gnttab_v1 >> | CAP_Gnttab_v2 >> >> +type physinfo_cap_arch_flag = >> + | CAP_ARCH_ASSISTED_XAPIC >> + | CAP_ARCH_ASSISTED_X2APIC >> + >> type physinfo = >> { >> threads_per_core : int; >> @@ -139,6 +143,7 @@ type physinfo = >> scrub_pages : nativeint; >> (* XXX hw_cap *) >> capabilities : physinfo_cap_flag list; >> + arch_capabilities : physinfo_cap_arch_flag list; > > I know very little about Ocaml, but I think you are not setting this > field anywhere? I would expect a call to ocaml_list_to_c_bitmap and > then you will likely need to define XEN_SYSCTL_PHYSCAP_X86_MAX so you > can check the options. See XEN_SYSCTL_PHYSCAP_MAX for example. Yes, you're right, I will add that in the v3. > >> max_nr_cpus : int; >> } >> >> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli >> index d1d9c9247a..a2b15130ee 100644 >> --- a/tools/ocaml/libs/xc/xenctrl.mli >> +++ b/tools/ocaml/libs/xc/xenctrl.mli >> @@ -112,6 +112,10 @@ type physinfo_cap_flag = >> | CAP_Gnttab_v1 >> | CAP_Gnttab_v2 >> >> +type physinfo_cap_arch_flag = >> + | CAP_ARCH_ASSISTED_XAPIC >> + | CAP_ARCH_ASSISTED_X2APIC >> + >> type physinfo = { >> threads_per_core : int; >> cores_per_socket : int; >> @@ -122,6 +126,7 @@ type physinfo = { >> free_pages : nativeint; >> scrub_pages : nativeint; >> capabilities : physinfo_cap_flag list; >> + arch_capabilities : physinfo_cap_arch_flag list; >> max_nr_cpus : int; (** compile-time max possible number of nr_cpus *) >> } >> type version = { major : int; minor : int; extra : string; } >> diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c >> index 712b7638b0..3205270754 100644 >> --- a/tools/xl/xl_info.c >> +++ b/tools/xl/xl_info.c >> @@ -210,7 +210,7 @@ static void output_physinfo(void) >> info.hw_cap[4], info.hw_cap[5], info.hw_cap[6], info.hw_cap[7] >> ); >> >> - maybe_printf("virt_caps :%s%s%s%s%s%s%s%s%s%s%s\n", >> + maybe_printf("virt_caps :%s%s%s%s%s%s%s%s%s%s%s%s%s\n", >> info.cap_pv ? " pv" : "", >> info.cap_hvm ? " hvm" : "", >> info.cap_hvm && info.cap_hvm_directio ? " hvm_directio" : "", >> @@ -221,7 +221,9 @@ static void output_physinfo(void) >> info.cap_vmtrace ? " vmtrace" : "", >> info.cap_vpmu ? " vpmu" : "", >> info.cap_gnttab_v1 ? " gnttab-v1" : "", >> - info.cap_gnttab_v2 ? " gnttab-v2" : "" >> + info.cap_gnttab_v2 ? " gnttab-v2" : "", >> + info.cap_assisted_xapic ? " assisted_xapic" : "", >> + info.cap_assisted_x2apic ? " assisted_x2apic" : "" >> ); >> >> vinfo = libxl_get_version_info(ctx); >> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >> index 7ab15e07a0..4060aef1bd 100644 >> --- a/xen/arch/x86/hvm/vmx/vmcs.c >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) >> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); >> } >> >> + /* Check whether hardware supports accelerated xapic and x2apic. */ >> + if ( bsp ) >> + { >> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; >> + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || >> + cpu_has_vmx_virtual_intr_delivery) && >> + cpu_has_vmx_virtualize_x2apic_mode; >> + } >> + >> /* The IA32_VMX_EPT_VPID_CAP MSR exists only when EPT or VPID available */ >> if ( _vmx_secondary_exec_control & (SECONDARY_EXEC_ENABLE_EPT | >> SECONDARY_EXEC_ENABLE_VPID) ) >> diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h >> index e62e109598..72431df26d 100644 >> --- a/xen/arch/x86/include/asm/domain.h >> +++ b/xen/arch/x86/include/asm/domain.h >> @@ -756,6 +756,9 @@ static inline void pv_inject_sw_interrupt(unsigned int vector) >> : is_pv_32bit_domain(d) ? PV32_VM_ASSIST_MASK \ >> : PV64_VM_ASSIST_MASK) >> >> +extern bool assisted_xapic_available; >> +extern bool assisted_x2apic_available; >> + >> #endif /* __ASM_DOMAIN_H__ */ >> >> /* >> diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c >> index aff52a13f3..642cc96985 100644 >> --- a/xen/arch/x86/sysctl.c >> +++ b/xen/arch/x86/sysctl.c >> @@ -69,6 +69,9 @@ struct l3_cache_info { >> unsigned long size; >> }; >> >> +bool __ro_after_init assisted_xapic_available; >> +bool __ro_after_init assisted_x2apic_available; > > You could likely shorten this by dropping the _available suffix. Okay. Thanks, Jane. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-09 12:26 ` Jane Malalane @ 2022-02-09 13:48 ` Anthony PERARD 2022-02-09 15:28 ` Jane Malalane 0 siblings, 1 reply; 28+ messages in thread From: Anthony PERARD @ 2022-02-09 13:48 UTC (permalink / raw) To: Jane Malalane Cc: Roger Pau Monne, Xen-devel, Wei Liu, Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian On Wed, Feb 09, 2022 at 12:26:05PM +0000, Jane Malalane wrote: > On 08/02/2022 15:26, Roger Pau Monné wrote: > > On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: > >> diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go > >> index b1e84d5258..5f384b767c 100644 > >> --- a/tools/golang/xenlight/types.gen.go > >> +++ b/tools/golang/xenlight/types.gen.go > >> @@ -389,6 +389,10 @@ RunHotplugScripts Defbool > >> DriverDomain Defbool > >> Passthrough Passthrough > >> XendSuspendEvtchnCompat Defbool > >> +ArchX86 struct { > >> +AssistedXapic Defbool > >> +AssistedX2Apic Defbool > > > > Don't you need some indentation here? > I hadn't realized it appeared like this here (and the same happens for > other parts of my code as I'm seeing now) because the git output is > correct. I will fix it. > > > > Also name would better be Assistedx{2}APIC IMO if possible. Having a > > capital 'X' and lowercase 'apic' looks really strange. > Okay. This is a generated file, you can't change indentation or fields names. It would be rebuilt automatically if you had golang installed and where rebuilding all the tools. There's two ways to generate it, you could install golang and build all the tools. Or without golang: just `cd tools/golang/xenlight; make types.gen.go`. Both should regenerate both "helpers.gen.go" "types.gen.go" files. There's an even easier way, tell the committer to regen the files when committing :-). Cheers, -- Anthony PERARD ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-09 13:48 ` Anthony PERARD @ 2022-02-09 15:28 ` Jane Malalane 0 siblings, 0 replies; 28+ messages in thread From: Jane Malalane @ 2022-02-09 15:28 UTC (permalink / raw) To: Anthony Perard Cc: Roger Pau Monne, Xen-devel, Wei Liu, Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian On 09/02/2022 13:48, Anthony PERARD wrote: > On Wed, Feb 09, 2022 at 12:26:05PM +0000, Jane Malalane wrote: >> On 08/02/2022 15:26, Roger Pau Monné wrote: >>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: >>>> diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go >>>> index b1e84d5258..5f384b767c 100644 >>>> --- a/tools/golang/xenlight/types.gen.go >>>> +++ b/tools/golang/xenlight/types.gen.go >>>> @@ -389,6 +389,10 @@ RunHotplugScripts Defbool >>>> DriverDomain Defbool >>>> Passthrough Passthrough >>>> XendSuspendEvtchnCompat Defbool >>>> +ArchX86 struct { >>>> +AssistedXapic Defbool >>>> +AssistedX2Apic Defbool >>> >>> Don't you need some indentation here? >> I hadn't realized it appeared like this here (and the same happens for >> other parts of my code as I'm seeing now) because the git output is >> correct. I will fix it. >>> >>> Also name would better be Assistedx{2}APIC IMO if possible. Having a >>> capital 'X' and lowercase 'apic' looks really strange. >> Okay. > > > This is a generated file, you can't change indentation or fields names. > It would be rebuilt automatically if you had golang installed and where > rebuilding all the tools. > > There's two ways to generate it, you could install golang and build all > the tools. Or without golang: just > `cd tools/golang/xenlight; make types.gen.go`. Both should regenerate > both "helpers.gen.go" "types.gen.go" files. > > There's an even easier way, tell the committer to regen the files when > committing :-). Oh, yeah I forgot they were automatically generated. Right, thank you. Jane. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-07 18:21 ` [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 Jane Malalane 2022-02-08 15:26 ` Roger Pau Monné @ 2022-02-10 10:03 ` Roger Pau Monné 2022-02-11 10:06 ` Jane Malalane 1 sibling, 1 reply; 28+ messages in thread From: Roger Pau Monné @ 2022-02-10 10:03 UTC (permalink / raw) To: Jane Malalane Cc: Xen-devel, Wei Liu, Anthony PERARD, Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: > diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c > index 7ab15e07a0..4060aef1bd 100644 > --- a/xen/arch/x86/hvm/vmx/vmcs.c > +++ b/xen/arch/x86/hvm/vmx/vmcs.c > @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) > MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); > } > > + /* Check whether hardware supports accelerated xapic and x2apic. */ > + if ( bsp ) > + { > + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; > + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || > + cpu_has_vmx_virtual_intr_delivery) && > + cpu_has_vmx_virtualize_x2apic_mode; I've been think about this, and it seems kind of asymmetric that for xAPIC mode we report hw assisted support only with virtualize_apic_accesses available, while for x2APIC we require virtualize_x2apic_mode plus either apic_reg_virt or virtual_intr_delivery. I think we likely need to be more consistent here, and report hw assisted x2APIC support as long as virtualize_x2apic_mode is available. This will likely have some effect on patch 2 also, as you will have to adjust vmx_vlapic_msr_changed. Thanks, Roger. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-10 10:03 ` Roger Pau Monné @ 2022-02-11 10:06 ` Jane Malalane 2022-02-11 11:29 ` Roger Pau Monné 0 siblings, 1 reply; 28+ messages in thread From: Jane Malalane @ 2022-02-11 10:06 UTC (permalink / raw) To: Roger Pau Monne, Jan Beulich, Andrew Cooper Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian On 10/02/2022 10:03, Roger Pau Monné wrote: > On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: >> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >> index 7ab15e07a0..4060aef1bd 100644 >> --- a/xen/arch/x86/hvm/vmx/vmcs.c >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) >> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); >> } >> >> + /* Check whether hardware supports accelerated xapic and x2apic. */ >> + if ( bsp ) >> + { >> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; >> + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || >> + cpu_has_vmx_virtual_intr_delivery) && >> + cpu_has_vmx_virtualize_x2apic_mode; > > I've been think about this, and it seems kind of asymmetric that for > xAPIC mode we report hw assisted support only with > virtualize_apic_accesses available, while for x2APIC we require > virtualize_x2apic_mode plus either apic_reg_virt or > virtual_intr_delivery. > > I think we likely need to be more consistent here, and report hw > assisted x2APIC support as long as virtualize_x2apic_mode is > available. > > This will likely have some effect on patch 2 also, as you will have to > adjust vmx_vlapic_msr_changed. > > Thanks, Roger. Any other thoughts on this? As on one hand it is asymmetric but also there isn't much assistance with only virtualize_x2apic_mode set as, in this case, a VM exit will be avoided only when trying to access the TPR register. Thanks, Jane. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-11 10:06 ` Jane Malalane @ 2022-02-11 11:29 ` Roger Pau Monné 2022-02-11 11:46 ` Jan Beulich 0 siblings, 1 reply; 28+ messages in thread From: Roger Pau Monné @ 2022-02-11 11:29 UTC (permalink / raw) To: Jane Malalane, Jan Beulich, Andrew Cooper Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, George Dunlap, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote: > On 10/02/2022 10:03, Roger Pau Monné wrote: > > On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: > >> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c > >> index 7ab15e07a0..4060aef1bd 100644 > >> --- a/xen/arch/x86/hvm/vmx/vmcs.c > >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c > >> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) > >> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); > >> } > >> > >> + /* Check whether hardware supports accelerated xapic and x2apic. */ > >> + if ( bsp ) > >> + { > >> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; > >> + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || > >> + cpu_has_vmx_virtual_intr_delivery) && > >> + cpu_has_vmx_virtualize_x2apic_mode; > > > > I've been think about this, and it seems kind of asymmetric that for > > xAPIC mode we report hw assisted support only with > > virtualize_apic_accesses available, while for x2APIC we require > > virtualize_x2apic_mode plus either apic_reg_virt or > > virtual_intr_delivery. > > > > I think we likely need to be more consistent here, and report hw > > assisted x2APIC support as long as virtualize_x2apic_mode is > > available. > > > > This will likely have some effect on patch 2 also, as you will have to > > adjust vmx_vlapic_msr_changed. > > > > Thanks, Roger. > > Any other thoughts on this? As on one hand it is asymmetric but also > there isn't much assistance with only virtualize_x2apic_mode set as, in > this case, a VM exit will be avoided only when trying to access the TPR > register. I've been thinking about this, and reporting hardware assisted x{2}APIC virtualization with just SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While those provide some assistance to the VMM in order to handle APIC accesses, it will still require a trap into the hypervisor to handle most of the accesses. So maybe we should only report hardware assisted support when the mentioned features are present together with SECONDARY_EXEC_APIC_REGISTER_VIRT? Thanks, Roger. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-11 11:29 ` Roger Pau Monné @ 2022-02-11 11:46 ` Jan Beulich 2022-02-14 13:11 ` Jane Malalane 0 siblings, 1 reply; 28+ messages in thread From: Jan Beulich @ 2022-02-11 11:46 UTC (permalink / raw) To: Roger Pau Monné Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, George Dunlap, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian, Jane Malalane, Andrew Cooper On 11.02.2022 12:29, Roger Pau Monné wrote: > On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote: >> On 10/02/2022 10:03, Roger Pau Monné wrote: >>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: >>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >>>> index 7ab15e07a0..4060aef1bd 100644 >>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) >>>> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); >>>> } >>>> >>>> + /* Check whether hardware supports accelerated xapic and x2apic. */ >>>> + if ( bsp ) >>>> + { >>>> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; >>>> + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || >>>> + cpu_has_vmx_virtual_intr_delivery) && >>>> + cpu_has_vmx_virtualize_x2apic_mode; >>> >>> I've been think about this, and it seems kind of asymmetric that for >>> xAPIC mode we report hw assisted support only with >>> virtualize_apic_accesses available, while for x2APIC we require >>> virtualize_x2apic_mode plus either apic_reg_virt or >>> virtual_intr_delivery. >>> >>> I think we likely need to be more consistent here, and report hw >>> assisted x2APIC support as long as virtualize_x2apic_mode is >>> available. >>> >>> This will likely have some effect on patch 2 also, as you will have to >>> adjust vmx_vlapic_msr_changed. >>> >>> Thanks, Roger. >> >> Any other thoughts on this? As on one hand it is asymmetric but also >> there isn't much assistance with only virtualize_x2apic_mode set as, in >> this case, a VM exit will be avoided only when trying to access the TPR >> register. > > I've been thinking about this, and reporting hardware assisted > x{2}APIC virtualization with just > SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or > SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While > those provide some assistance to the VMM in order to handle APIC > accesses, it will still require a trap into the hypervisor to handle > most of the accesses. > > So maybe we should only report hardware assisted support when the > mentioned features are present together with > SECONDARY_EXEC_APIC_REGISTER_VIRT? Not sure - "some assistance" seems still a little better than none at all. Which route to go depends on what exactly we intend the bit to be used for. Jan ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-11 11:46 ` Jan Beulich @ 2022-02-14 13:11 ` Jane Malalane 2022-02-14 13:18 ` Jan Beulich 0 siblings, 1 reply; 28+ messages in thread From: Jane Malalane @ 2022-02-14 13:11 UTC (permalink / raw) To: Jan Beulich, Roger Pau Monne Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, George Dunlap, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian, Andrew Cooper On 11/02/2022 11:46, 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 11.02.2022 12:29, Roger Pau Monné wrote: >> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote: >>> On 10/02/2022 10:03, Roger Pau Monné wrote: >>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: >>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >>>>> index 7ab15e07a0..4060aef1bd 100644 >>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) >>>>> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); >>>>> } >>>>> >>>>> + /* Check whether hardware supports accelerated xapic and x2apic. */ >>>>> + if ( bsp ) >>>>> + { >>>>> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; >>>>> + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || >>>>> + cpu_has_vmx_virtual_intr_delivery) && >>>>> + cpu_has_vmx_virtualize_x2apic_mode; >>>> >>>> I've been think about this, and it seems kind of asymmetric that for >>>> xAPIC mode we report hw assisted support only with >>>> virtualize_apic_accesses available, while for x2APIC we require >>>> virtualize_x2apic_mode plus either apic_reg_virt or >>>> virtual_intr_delivery. >>>> >>>> I think we likely need to be more consistent here, and report hw >>>> assisted x2APIC support as long as virtualize_x2apic_mode is >>>> available. >>>> >>>> This will likely have some effect on patch 2 also, as you will have to >>>> adjust vmx_vlapic_msr_changed. >>>> >>>> Thanks, Roger. >>> >>> Any other thoughts on this? As on one hand it is asymmetric but also >>> there isn't much assistance with only virtualize_x2apic_mode set as, in >>> this case, a VM exit will be avoided only when trying to access the TPR >>> register. >> >> I've been thinking about this, and reporting hardware assisted >> x{2}APIC virtualization with just >> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or >> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While >> those provide some assistance to the VMM in order to handle APIC >> accesses, it will still require a trap into the hypervisor to handle >> most of the accesses. >> >> So maybe we should only report hardware assisted support when the >> mentioned features are present together with >> SECONDARY_EXEC_APIC_REGISTER_VIRT? > > Not sure - "some assistance" seems still a little better than none at all. > Which route to go depends on what exactly we intend the bit to be used for. > > Jan > True. I intended this bit to be specifically for enabling assisted_x{2}apic. So, would it be inconsistent to report hardware assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE but still claim that x{2}apic is virtualized if no MSR accesses are intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you say, the guest gets at least "some assistance" instead of none but we still claim x{2}apic virtualization when it is actually complete? Maybe I could also add a comment alluding to this in the xl documentation. Thanks, Jane. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-14 13:11 ` Jane Malalane @ 2022-02-14 13:18 ` Jan Beulich 2022-02-14 17:09 ` Jane Malalane 0 siblings, 1 reply; 28+ messages in thread From: Jan Beulich @ 2022-02-14 13:18 UTC (permalink / raw) To: Jane Malalane Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, George Dunlap, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian, Andrew Cooper, Roger Pau Monne On 14.02.2022 14:11, Jane Malalane wrote: > On 11/02/2022 11:46, 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 11.02.2022 12:29, Roger Pau Monné wrote: >>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote: >>>> On 10/02/2022 10:03, Roger Pau Monné wrote: >>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: >>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>> index 7ab15e07a0..4060aef1bd 100644 >>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) >>>>>> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); >>>>>> } >>>>>> >>>>>> + /* Check whether hardware supports accelerated xapic and x2apic. */ >>>>>> + if ( bsp ) >>>>>> + { >>>>>> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; >>>>>> + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || >>>>>> + cpu_has_vmx_virtual_intr_delivery) && >>>>>> + cpu_has_vmx_virtualize_x2apic_mode; >>>>> >>>>> I've been think about this, and it seems kind of asymmetric that for >>>>> xAPIC mode we report hw assisted support only with >>>>> virtualize_apic_accesses available, while for x2APIC we require >>>>> virtualize_x2apic_mode plus either apic_reg_virt or >>>>> virtual_intr_delivery. >>>>> >>>>> I think we likely need to be more consistent here, and report hw >>>>> assisted x2APIC support as long as virtualize_x2apic_mode is >>>>> available. >>>>> >>>>> This will likely have some effect on patch 2 also, as you will have to >>>>> adjust vmx_vlapic_msr_changed. >>>>> >>>>> Thanks, Roger. >>>> >>>> Any other thoughts on this? As on one hand it is asymmetric but also >>>> there isn't much assistance with only virtualize_x2apic_mode set as, in >>>> this case, a VM exit will be avoided only when trying to access the TPR >>>> register. >>> >>> I've been thinking about this, and reporting hardware assisted >>> x{2}APIC virtualization with just >>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or >>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While >>> those provide some assistance to the VMM in order to handle APIC >>> accesses, it will still require a trap into the hypervisor to handle >>> most of the accesses. >>> >>> So maybe we should only report hardware assisted support when the >>> mentioned features are present together with >>> SECONDARY_EXEC_APIC_REGISTER_VIRT? >> >> Not sure - "some assistance" seems still a little better than none at all. >> Which route to go depends on what exactly we intend the bit to be used for. >> > True. I intended this bit to be specifically for enabling > assisted_x{2}apic. So, would it be inconsistent to report hardware > assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE > but still claim that x{2}apic is virtualized if no MSR accesses are > intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you > say, the guest gets at least "some assistance" instead of none but we > still claim x{2}apic virtualization when it is actually complete? Maybe > I could also add a comment alluding to this in the xl documentation. To rephrase my earlier point: Which kind of decisions are the consumer(s) of us reporting hardware assistance going to take? In how far is there a risk that "some assistance" is overall going to lead to a loss of performance? I guess I'd need to see comment and actual code all in one place ... Jan ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-14 13:18 ` Jan Beulich @ 2022-02-14 17:09 ` Jane Malalane 2022-02-15 7:09 ` Jan Beulich 0 siblings, 1 reply; 28+ messages in thread From: Jane Malalane @ 2022-02-14 17:09 UTC (permalink / raw) To: Jan Beulich, Roger Pau Monne, Andrew Cooper Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, George Dunlap, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian, Andrew Cooper, Roger Pau Monne On 14/02/2022 13:18, 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 14.02.2022 14:11, Jane Malalane wrote: >> On 11/02/2022 11:46, 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 11.02.2022 12:29, Roger Pau Monné wrote: >>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote: >>>>> On 10/02/2022 10:03, Roger Pau Monné wrote: >>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: >>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>> index 7ab15e07a0..4060aef1bd 100644 >>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) >>>>>>> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); >>>>>>> } >>>>>>> >>>>>>> + /* Check whether hardware supports accelerated xapic and x2apic. */ >>>>>>> + if ( bsp ) >>>>>>> + { >>>>>>> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; >>>>>>> + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || >>>>>>> + cpu_has_vmx_virtual_intr_delivery) && >>>>>>> + cpu_has_vmx_virtualize_x2apic_mode; >>>>>> >>>>>> I've been think about this, and it seems kind of asymmetric that for >>>>>> xAPIC mode we report hw assisted support only with >>>>>> virtualize_apic_accesses available, while for x2APIC we require >>>>>> virtualize_x2apic_mode plus either apic_reg_virt or >>>>>> virtual_intr_delivery. >>>>>> >>>>>> I think we likely need to be more consistent here, and report hw >>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is >>>>>> available. >>>>>> >>>>>> This will likely have some effect on patch 2 also, as you will have to >>>>>> adjust vmx_vlapic_msr_changed. >>>>>> >>>>>> Thanks, Roger. >>>>> >>>>> Any other thoughts on this? As on one hand it is asymmetric but also >>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in >>>>> this case, a VM exit will be avoided only when trying to access the TPR >>>>> register. >>>> >>>> I've been thinking about this, and reporting hardware assisted >>>> x{2}APIC virtualization with just >>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or >>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While >>>> those provide some assistance to the VMM in order to handle APIC >>>> accesses, it will still require a trap into the hypervisor to handle >>>> most of the accesses. >>>> >>>> So maybe we should only report hardware assisted support when the >>>> mentioned features are present together with >>>> SECONDARY_EXEC_APIC_REGISTER_VIRT? >>> >>> Not sure - "some assistance" seems still a little better than none at all. >>> Which route to go depends on what exactly we intend the bit to be used for. >>> >> True. I intended this bit to be specifically for enabling >> assisted_x{2}apic. So, would it be inconsistent to report hardware >> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE >> but still claim that x{2}apic is virtualized if no MSR accesses are >> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you >> say, the guest gets at least "some assistance" instead of none but we >> still claim x{2}apic virtualization when it is actually complete? Maybe >> I could also add a comment alluding to this in the xl documentation. > > To rephrase my earlier point: Which kind of decisions are the consumer(s) > of us reporting hardware assistance going to take? In how far is there a > risk that "some assistance" is overall going to lead to a loss of > performance? I guess I'd need to see comment and actual code all in one > place ... > So, I was thinking of adding something along the lines of: +=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. Notice enabling +this does not guarantee full virtualization for xAPIC, as this can +only be achieved if hardware supports “APIC-register virtualization” +and “virtual-interrupt delivery”. The default is settable via +L<xl.conf(5)>. and going for assisted_x2apic_available = cpu_has_vmx_virtualize_x2apic_mode. This would prevent the customer from expecting full acceleration when apic_register_virt and/or virtual_intr_delivery aren't available whilst still offering some if they are not available as Xen currently does. In a future patch, we could also expose and add config options for these controls if we wanted to. Thank you for your help, Jane. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-14 17:09 ` Jane Malalane @ 2022-02-15 7:09 ` Jan Beulich 2022-02-15 10:14 ` Jane Malalane 0 siblings, 1 reply; 28+ messages in thread From: Jan Beulich @ 2022-02-15 7:09 UTC (permalink / raw) To: Jane Malalane Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, George Dunlap, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian, Roger Pau Monne, Andrew Cooper On 14.02.2022 18:09, Jane Malalane wrote: > On 14/02/2022 13:18, 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 14.02.2022 14:11, Jane Malalane wrote: >>> On 11/02/2022 11:46, 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 11.02.2022 12:29, Roger Pau Monné wrote: >>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote: >>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote: >>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: >>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>> index 7ab15e07a0..4060aef1bd 100644 >>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) >>>>>>>> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); >>>>>>>> } >>>>>>>> >>>>>>>> + /* Check whether hardware supports accelerated xapic and x2apic. */ >>>>>>>> + if ( bsp ) >>>>>>>> + { >>>>>>>> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; >>>>>>>> + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || >>>>>>>> + cpu_has_vmx_virtual_intr_delivery) && >>>>>>>> + cpu_has_vmx_virtualize_x2apic_mode; >>>>>>> >>>>>>> I've been think about this, and it seems kind of asymmetric that for >>>>>>> xAPIC mode we report hw assisted support only with >>>>>>> virtualize_apic_accesses available, while for x2APIC we require >>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or >>>>>>> virtual_intr_delivery. >>>>>>> >>>>>>> I think we likely need to be more consistent here, and report hw >>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is >>>>>>> available. >>>>>>> >>>>>>> This will likely have some effect on patch 2 also, as you will have to >>>>>>> adjust vmx_vlapic_msr_changed. >>>>>>> >>>>>>> Thanks, Roger. >>>>>> >>>>>> Any other thoughts on this? As on one hand it is asymmetric but also >>>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in >>>>>> this case, a VM exit will be avoided only when trying to access the TPR >>>>>> register. >>>>> >>>>> I've been thinking about this, and reporting hardware assisted >>>>> x{2}APIC virtualization with just >>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or >>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While >>>>> those provide some assistance to the VMM in order to handle APIC >>>>> accesses, it will still require a trap into the hypervisor to handle >>>>> most of the accesses. >>>>> >>>>> So maybe we should only report hardware assisted support when the >>>>> mentioned features are present together with >>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT? >>>> >>>> Not sure - "some assistance" seems still a little better than none at all. >>>> Which route to go depends on what exactly we intend the bit to be used for. >>>> >>> True. I intended this bit to be specifically for enabling >>> assisted_x{2}apic. So, would it be inconsistent to report hardware >>> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE >>> but still claim that x{2}apic is virtualized if no MSR accesses are >>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you >>> say, the guest gets at least "some assistance" instead of none but we >>> still claim x{2}apic virtualization when it is actually complete? Maybe >>> I could also add a comment alluding to this in the xl documentation. >> >> To rephrase my earlier point: Which kind of decisions are the consumer(s) >> of us reporting hardware assistance going to take? In how far is there a >> risk that "some assistance" is overall going to lead to a loss of >> performance? I guess I'd need to see comment and actual code all in one >> place ... >> > So, I was thinking of adding something along the lines of: > > +=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. Notice enabling > +this does not guarantee full virtualization for xAPIC, as this can > +only be achieved if hardware supports “APIC-register virtualization” > +and “virtual-interrupt delivery”. The default is settable via > +L<xl.conf(5)>. But isn't this contradictory? Doesn't lack of APIC-register virtualization mean VM exits upon (most) accesses? Jan > and going for assisted_x2apic_available = > cpu_has_vmx_virtualize_x2apic_mode. > > This would prevent the customer from expecting full acceleration when > apic_register_virt and/or virtual_intr_delivery aren't available whilst > still offering some if they are not available as Xen currently does. In > a future patch, we could also expose and add config options for these > controls if we wanted to. > > Thank you for your help, > > Jane. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-15 7:09 ` Jan Beulich @ 2022-02-15 10:14 ` Jane Malalane 2022-02-15 10:19 ` Jan Beulich 0 siblings, 1 reply; 28+ messages in thread From: Jane Malalane @ 2022-02-15 10:14 UTC (permalink / raw) To: Jan Beulich Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, George Dunlap, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian, Roger Pau Monne, Andrew Cooper On 15/02/2022 07:09, 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 14.02.2022 18:09, Jane Malalane wrote: >> On 14/02/2022 13:18, 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 14.02.2022 14:11, Jane Malalane wrote: >>>> On 11/02/2022 11:46, 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 11.02.2022 12:29, Roger Pau Monné wrote: >>>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote: >>>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote: >>>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: >>>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>>> index 7ab15e07a0..4060aef1bd 100644 >>>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) >>>>>>>>> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); >>>>>>>>> } >>>>>>>>> >>>>>>>>> + /* Check whether hardware supports accelerated xapic and x2apic. */ >>>>>>>>> + if ( bsp ) >>>>>>>>> + { >>>>>>>>> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; >>>>>>>>> + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || >>>>>>>>> + cpu_has_vmx_virtual_intr_delivery) && >>>>>>>>> + cpu_has_vmx_virtualize_x2apic_mode; >>>>>>>> >>>>>>>> I've been think about this, and it seems kind of asymmetric that for >>>>>>>> xAPIC mode we report hw assisted support only with >>>>>>>> virtualize_apic_accesses available, while for x2APIC we require >>>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or >>>>>>>> virtual_intr_delivery. >>>>>>>> >>>>>>>> I think we likely need to be more consistent here, and report hw >>>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is >>>>>>>> available. >>>>>>>> >>>>>>>> This will likely have some effect on patch 2 also, as you will have to >>>>>>>> adjust vmx_vlapic_msr_changed. >>>>>>>> >>>>>>>> Thanks, Roger. >>>>>>> >>>>>>> Any other thoughts on this? As on one hand it is asymmetric but also >>>>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in >>>>>>> this case, a VM exit will be avoided only when trying to access the TPR >>>>>>> register. >>>>>> >>>>>> I've been thinking about this, and reporting hardware assisted >>>>>> x{2}APIC virtualization with just >>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or >>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While >>>>>> those provide some assistance to the VMM in order to handle APIC >>>>>> accesses, it will still require a trap into the hypervisor to handle >>>>>> most of the accesses. >>>>>> >>>>>> So maybe we should only report hardware assisted support when the >>>>>> mentioned features are present together with >>>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT? >>>>> >>>>> Not sure - "some assistance" seems still a little better than none at all. >>>>> Which route to go depends on what exactly we intend the bit to be used for. >>>>> >>>> True. I intended this bit to be specifically for enabling >>>> assisted_x{2}apic. So, would it be inconsistent to report hardware >>>> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE >>>> but still claim that x{2}apic is virtualized if no MSR accesses are >>>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you >>>> say, the guest gets at least "some assistance" instead of none but we >>>> still claim x{2}apic virtualization when it is actually complete? Maybe >>>> I could also add a comment alluding to this in the xl documentation. >>> >>> To rephrase my earlier point: Which kind of decisions are the consumer(s) >>> of us reporting hardware assistance going to take? In how far is there a >>> risk that "some assistance" is overall going to lead to a loss of >>> performance? I guess I'd need to see comment and actual code all in one >>> place ... >>> >> So, I was thinking of adding something along the lines of: >> >> +=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. Notice enabling >> +this does not guarantee full virtualization for xAPIC, as this can >> +only be achieved if hardware supports “APIC-register virtualization” >> +and “virtual-interrupt delivery”. The default is settable via >> +L<xl.conf(5)>. > > But isn't this contradictory? Doesn't lack of APIC-register virtualization > mean VM exits upon (most) accesses? Yes, it does mean. I guess the alternative wouuld be then to require APIC-register virtualization for enabling xAPIC. But also, although this doesn't provide much acceleration, even getting a VM exit is some assistance if compared to instead getting an EPT fault and having to decode the access. Thanks, Jane. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-15 10:14 ` Jane Malalane @ 2022-02-15 10:19 ` Jan Beulich 2022-02-15 15:10 ` Jane Malalane 0 siblings, 1 reply; 28+ messages in thread From: Jan Beulich @ 2022-02-15 10:19 UTC (permalink / raw) To: Jane Malalane Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, George Dunlap, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian, Roger Pau Monne, Andrew Cooper On 15.02.2022 11:14, Jane Malalane wrote: > On 15/02/2022 07:09, 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 14.02.2022 18:09, Jane Malalane wrote: >>> On 14/02/2022 13:18, 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 14.02.2022 14:11, Jane Malalane wrote: >>>>> On 11/02/2022 11:46, 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 11.02.2022 12:29, Roger Pau Monné wrote: >>>>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote: >>>>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote: >>>>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: >>>>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>>>> index 7ab15e07a0..4060aef1bd 100644 >>>>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) >>>>>>>>>> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> + /* Check whether hardware supports accelerated xapic and x2apic. */ >>>>>>>>>> + if ( bsp ) >>>>>>>>>> + { >>>>>>>>>> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; >>>>>>>>>> + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || >>>>>>>>>> + cpu_has_vmx_virtual_intr_delivery) && >>>>>>>>>> + cpu_has_vmx_virtualize_x2apic_mode; >>>>>>>>> >>>>>>>>> I've been think about this, and it seems kind of asymmetric that for >>>>>>>>> xAPIC mode we report hw assisted support only with >>>>>>>>> virtualize_apic_accesses available, while for x2APIC we require >>>>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or >>>>>>>>> virtual_intr_delivery. >>>>>>>>> >>>>>>>>> I think we likely need to be more consistent here, and report hw >>>>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is >>>>>>>>> available. >>>>>>>>> >>>>>>>>> This will likely have some effect on patch 2 also, as you will have to >>>>>>>>> adjust vmx_vlapic_msr_changed. >>>>>>>>> >>>>>>>>> Thanks, Roger. >>>>>>>> >>>>>>>> Any other thoughts on this? As on one hand it is asymmetric but also >>>>>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in >>>>>>>> this case, a VM exit will be avoided only when trying to access the TPR >>>>>>>> register. >>>>>>> >>>>>>> I've been thinking about this, and reporting hardware assisted >>>>>>> x{2}APIC virtualization with just >>>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or >>>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While >>>>>>> those provide some assistance to the VMM in order to handle APIC >>>>>>> accesses, it will still require a trap into the hypervisor to handle >>>>>>> most of the accesses. >>>>>>> >>>>>>> So maybe we should only report hardware assisted support when the >>>>>>> mentioned features are present together with >>>>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT? >>>>>> >>>>>> Not sure - "some assistance" seems still a little better than none at all. >>>>>> Which route to go depends on what exactly we intend the bit to be used for. >>>>>> >>>>> True. I intended this bit to be specifically for enabling >>>>> assisted_x{2}apic. So, would it be inconsistent to report hardware >>>>> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE >>>>> but still claim that x{2}apic is virtualized if no MSR accesses are >>>>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you >>>>> say, the guest gets at least "some assistance" instead of none but we >>>>> still claim x{2}apic virtualization when it is actually complete? Maybe >>>>> I could also add a comment alluding to this in the xl documentation. >>>> >>>> To rephrase my earlier point: Which kind of decisions are the consumer(s) >>>> of us reporting hardware assistance going to take? In how far is there a >>>> risk that "some assistance" is overall going to lead to a loss of >>>> performance? I guess I'd need to see comment and actual code all in one >>>> place ... >>>> >>> So, I was thinking of adding something along the lines of: >>> >>> +=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. Notice enabling >>> +this does not guarantee full virtualization for xAPIC, as this can >>> +only be achieved if hardware supports “APIC-register virtualization” >>> +and “virtual-interrupt delivery”. The default is settable via >>> +L<xl.conf(5)>. >> >> But isn't this contradictory? Doesn't lack of APIC-register virtualization >> mean VM exits upon (most) accesses? > > Yes, it does mean. I guess the alternative wouuld be then to require > APIC-register virtualization for enabling xAPIC. But also, although this > doesn't provide much acceleration, even getting a VM exit is some > assistance if compared to instead getting an EPT fault and having to > decode the access. I agree here, albeit I'd like to mention that EPT faults are also VM exits. All my earlier comment was about is that this piece of doc wants to express reality, whichever way it is that things end up being implemented. Jan ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-15 10:19 ` Jan Beulich @ 2022-02-15 15:10 ` Jane Malalane 2022-02-15 15:21 ` Jan Beulich 0 siblings, 1 reply; 28+ messages in thread From: Jane Malalane @ 2022-02-15 15:10 UTC (permalink / raw) To: Jan Beulich, Roger Pau Monne, Andrew Cooper Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, George Dunlap, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian, Roger Pau Monne, Andrew Cooper On 15/02/2022 10:19, 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 15.02.2022 11:14, Jane Malalane wrote: >> On 15/02/2022 07:09, 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 14.02.2022 18:09, Jane Malalane wrote: >>>> On 14/02/2022 13:18, 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 14.02.2022 14:11, Jane Malalane wrote: >>>>>> On 11/02/2022 11:46, 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 11.02.2022 12:29, Roger Pau Monné wrote: >>>>>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote: >>>>>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote: >>>>>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: >>>>>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>>>>> index 7ab15e07a0..4060aef1bd 100644 >>>>>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) >>>>>>>>>>> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> + /* Check whether hardware supports accelerated xapic and x2apic. */ >>>>>>>>>>> + if ( bsp ) >>>>>>>>>>> + { >>>>>>>>>>> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; >>>>>>>>>>> + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || >>>>>>>>>>> + cpu_has_vmx_virtual_intr_delivery) && >>>>>>>>>>> + cpu_has_vmx_virtualize_x2apic_mode; >>>>>>>>>> >>>>>>>>>> I've been think about this, and it seems kind of asymmetric that for >>>>>>>>>> xAPIC mode we report hw assisted support only with >>>>>>>>>> virtualize_apic_accesses available, while for x2APIC we require >>>>>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or >>>>>>>>>> virtual_intr_delivery. >>>>>>>>>> >>>>>>>>>> I think we likely need to be more consistent here, and report hw >>>>>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is >>>>>>>>>> available. >>>>>>>>>> >>>>>>>>>> This will likely have some effect on patch 2 also, as you will have to >>>>>>>>>> adjust vmx_vlapic_msr_changed. >>>>>>>>>> >>>>>>>>>> Thanks, Roger. >>>>>>>>> >>>>>>>>> Any other thoughts on this? As on one hand it is asymmetric but also >>>>>>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in >>>>>>>>> this case, a VM exit will be avoided only when trying to access the TPR >>>>>>>>> register. >>>>>>>> >>>>>>>> I've been thinking about this, and reporting hardware assisted >>>>>>>> x{2}APIC virtualization with just >>>>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or >>>>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While >>>>>>>> those provide some assistance to the VMM in order to handle APIC >>>>>>>> accesses, it will still require a trap into the hypervisor to handle >>>>>>>> most of the accesses. >>>>>>>> >>>>>>>> So maybe we should only report hardware assisted support when the >>>>>>>> mentioned features are present together with >>>>>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT? >>>>>>> >>>>>>> Not sure - "some assistance" seems still a little better than none at all. >>>>>>> Which route to go depends on what exactly we intend the bit to be used for. >>>>>>> >>>>>> True. I intended this bit to be specifically for enabling >>>>>> assisted_x{2}apic. So, would it be inconsistent to report hardware >>>>>> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE >>>>>> but still claim that x{2}apic is virtualized if no MSR accesses are >>>>>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you >>>>>> say, the guest gets at least "some assistance" instead of none but we >>>>>> still claim x{2}apic virtualization when it is actually complete? Maybe >>>>>> I could also add a comment alluding to this in the xl documentation. >>>>> >>>>> To rephrase my earlier point: Which kind of decisions are the consumer(s) >>>>> of us reporting hardware assistance going to take? In how far is there a >>>>> risk that "some assistance" is overall going to lead to a loss of >>>>> performance? I guess I'd need to see comment and actual code all in one >>>>> place ... >>>>> >>>> So, I was thinking of adding something along the lines of: >>>> >>>> +=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. Notice enabling >>>> +this does not guarantee full virtualization for xAPIC, as this can >>>> +only be achieved if hardware supports “APIC-register virtualization” >>>> +and “virtual-interrupt delivery”. The default is settable via >>>> +L<xl.conf(5)>. >>> >>> But isn't this contradictory? Doesn't lack of APIC-register virtualization >>> mean VM exits upon (most) accesses? >> >> Yes, it does mean. I guess the alternative wouuld be then to require >> APIC-register virtualization for enabling xAPIC. But also, although this >> doesn't provide much acceleration, even getting a VM exit is some >> assistance if compared to instead getting an EPT fault and having to >> decode the access. > > I agree here, albeit I'd like to mention that EPT faults are also VM > exits. All my earlier comment was about is that this piece of doc > wants to express reality, whichever way it is that things end up > being implemented. Oh yes. Right, I see how this info could be misleading. How about this?... +=item B<assisted_xapic=BOOLEAN> B<(x86 only)> + +B<(x86 only)> Enables or disables hardware assisted virtualization for +xAPIC. With this option enabled, a memory-mapped APIC access will be +decoded by hardware and either issue a VM exit with an exit reason +instead of an EPT fault or altogether avoid a VM exit. Notice +full virtualization for xAPIC can only be achieved if hardware +supports “APIC-register virtualization” and “virtual-interrupt +delivery”. 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. With this option enabled, an MSR-Based APIC access will either +issue a VM exit or altogether avoid one. Notice full virtualization +for x2APIC can only be achieved if hardware supports “APIC-register +virtualization” and “virtual-interrupt delivery”. The default is +settable via L<xl.conf(5)>. ...because with only VIRTUALIZE_APIC_ACCESSES enabled, hardware decodes accesses to the xAPIC page and the VM exit gives an exit reason. And if VIRTUALIZE_X2APIC_MODE is set, although no assistance is provided w.r.t. to decoding x2APIC accesses as the MSR that the VM tried to access is already part of the vmexit information, VM exits for accesses to the TPR MSR are avoided, regardless of whether shadow TPR is set or not for e.g. I hope this makes sense but I welcome any other suggestions/corrections. Thank you, Jane. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-15 15:10 ` Jane Malalane @ 2022-02-15 15:21 ` Jan Beulich 2022-02-15 16:33 ` Jane Malalane 0 siblings, 1 reply; 28+ messages in thread From: Jan Beulich @ 2022-02-15 15:21 UTC (permalink / raw) To: Jane Malalane Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, George Dunlap, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian, Roger Pau Monne, Andrew Cooper On 15.02.2022 16:10, Jane Malalane wrote: > On 15/02/2022 10:19, 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 15.02.2022 11:14, Jane Malalane wrote: >>> On 15/02/2022 07:09, 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 14.02.2022 18:09, Jane Malalane wrote: >>>>> On 14/02/2022 13:18, 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 14.02.2022 14:11, Jane Malalane wrote: >>>>>>> On 11/02/2022 11:46, 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 11.02.2022 12:29, Roger Pau Monné wrote: >>>>>>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote: >>>>>>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote: >>>>>>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: >>>>>>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>>>>>> index 7ab15e07a0..4060aef1bd 100644 >>>>>>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) >>>>>>>>>>>> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); >>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>>> + /* Check whether hardware supports accelerated xapic and x2apic. */ >>>>>>>>>>>> + if ( bsp ) >>>>>>>>>>>> + { >>>>>>>>>>>> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; >>>>>>>>>>>> + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || >>>>>>>>>>>> + cpu_has_vmx_virtual_intr_delivery) && >>>>>>>>>>>> + cpu_has_vmx_virtualize_x2apic_mode; >>>>>>>>>>> >>>>>>>>>>> I've been think about this, and it seems kind of asymmetric that for >>>>>>>>>>> xAPIC mode we report hw assisted support only with >>>>>>>>>>> virtualize_apic_accesses available, while for x2APIC we require >>>>>>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or >>>>>>>>>>> virtual_intr_delivery. >>>>>>>>>>> >>>>>>>>>>> I think we likely need to be more consistent here, and report hw >>>>>>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is >>>>>>>>>>> available. >>>>>>>>>>> >>>>>>>>>>> This will likely have some effect on patch 2 also, as you will have to >>>>>>>>>>> adjust vmx_vlapic_msr_changed. >>>>>>>>>>> >>>>>>>>>>> Thanks, Roger. >>>>>>>>>> >>>>>>>>>> Any other thoughts on this? As on one hand it is asymmetric but also >>>>>>>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in >>>>>>>>>> this case, a VM exit will be avoided only when trying to access the TPR >>>>>>>>>> register. >>>>>>>>> >>>>>>>>> I've been thinking about this, and reporting hardware assisted >>>>>>>>> x{2}APIC virtualization with just >>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or >>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While >>>>>>>>> those provide some assistance to the VMM in order to handle APIC >>>>>>>>> accesses, it will still require a trap into the hypervisor to handle >>>>>>>>> most of the accesses. >>>>>>>>> >>>>>>>>> So maybe we should only report hardware assisted support when the >>>>>>>>> mentioned features are present together with >>>>>>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT? >>>>>>>> >>>>>>>> Not sure - "some assistance" seems still a little better than none at all. >>>>>>>> Which route to go depends on what exactly we intend the bit to be used for. >>>>>>>> >>>>>>> True. I intended this bit to be specifically for enabling >>>>>>> assisted_x{2}apic. So, would it be inconsistent to report hardware >>>>>>> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE >>>>>>> but still claim that x{2}apic is virtualized if no MSR accesses are >>>>>>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you >>>>>>> say, the guest gets at least "some assistance" instead of none but we >>>>>>> still claim x{2}apic virtualization when it is actually complete? Maybe >>>>>>> I could also add a comment alluding to this in the xl documentation. >>>>>> >>>>>> To rephrase my earlier point: Which kind of decisions are the consumer(s) >>>>>> of us reporting hardware assistance going to take? In how far is there a >>>>>> risk that "some assistance" is overall going to lead to a loss of >>>>>> performance? I guess I'd need to see comment and actual code all in one >>>>>> place ... >>>>>> >>>>> So, I was thinking of adding something along the lines of: >>>>> >>>>> +=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. Notice enabling >>>>> +this does not guarantee full virtualization for xAPIC, as this can >>>>> +only be achieved if hardware supports “APIC-register virtualization” >>>>> +and “virtual-interrupt delivery”. The default is settable via >>>>> +L<xl.conf(5)>. >>>> >>>> But isn't this contradictory? Doesn't lack of APIC-register virtualization >>>> mean VM exits upon (most) accesses? >>> >>> Yes, it does mean. I guess the alternative wouuld be then to require >>> APIC-register virtualization for enabling xAPIC. But also, although this >>> doesn't provide much acceleration, even getting a VM exit is some >>> assistance if compared to instead getting an EPT fault and having to >>> decode the access. >> >> I agree here, albeit I'd like to mention that EPT faults are also VM >> exits. All my earlier comment was about is that this piece of doc >> wants to express reality, whichever way it is that things end up >> being implemented. > > Oh yes. Right, I see how this info could be misleading. > > How about this?... Getting close. The thing I can't judge is whether this level of technical detail is suitable for this doc. Just one further remark: > +=item B<assisted_xapic=BOOLEAN> B<(x86 only)> > + > +B<(x86 only)> Enables or disables hardware assisted virtualization for > +xAPIC. With this option enabled, a memory-mapped APIC access will be > +decoded by hardware and either issue a VM exit with an exit reason > +instead of an EPT fault or altogether avoid a VM exit. Notice As said before, EPT faults also are VM exits and also provide an exit reason. Therefore maybe "... and either issue a VM exit with a more specific exit reason than an EPT fault would provide, or altogether avoid a VM exit" or "... and either issue a more specific VM exit than just an EPT fault, or altogether avoid a VM exit"? Jan > +full virtualization for xAPIC can only be achieved if hardware > +supports “APIC-register virtualization” and “virtual-interrupt > +delivery”. 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. With this option enabled, an MSR-Based APIC access will either > +issue a VM exit or altogether avoid one. Notice full virtualization > +for x2APIC can only be achieved if hardware supports “APIC-register > +virtualization” and “virtual-interrupt delivery”. The default is > +settable via L<xl.conf(5)>. > > > ...because with only VIRTUALIZE_APIC_ACCESSES enabled, hardware decodes > accesses to the xAPIC page and the VM exit gives an exit reason. > And if VIRTUALIZE_X2APIC_MODE is set, although no assistance is provided > w.r.t. to decoding x2APIC accesses as the MSR that the VM tried to > access is already part of the vmexit information, VM exits for accesses > to the TPR MSR are avoided, regardless of whether shadow TPR is set or > not for e.g. > > I hope this makes sense but I welcome any other suggestions/corrections. > > Thank you, > > Jane. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-15 15:21 ` Jan Beulich @ 2022-02-15 16:33 ` Jane Malalane 2022-03-09 13:47 ` Roger Pau Monné 0 siblings, 1 reply; 28+ messages in thread From: Jane Malalane @ 2022-02-15 16:33 UTC (permalink / raw) To: Jan Beulich, Roger Pau Monne Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, George Dunlap, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian, Roger Pau Monne, Andrew Cooper On 15/02/2022 15:21, 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 15.02.2022 16:10, Jane Malalane wrote: >> On 15/02/2022 10:19, 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 15.02.2022 11:14, Jane Malalane wrote: >>>> On 15/02/2022 07:09, 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 14.02.2022 18:09, Jane Malalane wrote: >>>>>> On 14/02/2022 13:18, 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 14.02.2022 14:11, Jane Malalane wrote: >>>>>>>> On 11/02/2022 11:46, 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 11.02.2022 12:29, Roger Pau Monné wrote: >>>>>>>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote: >>>>>>>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote: >>>>>>>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: >>>>>>>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>>>>>>> index 7ab15e07a0..4060aef1bd 100644 >>>>>>>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>>>>>>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) >>>>>>>>>>>>> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); >>>>>>>>>>>>> } >>>>>>>>>>>>> >>>>>>>>>>>>> + /* Check whether hardware supports accelerated xapic and x2apic. */ >>>>>>>>>>>>> + if ( bsp ) >>>>>>>>>>>>> + { >>>>>>>>>>>>> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; >>>>>>>>>>>>> + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || >>>>>>>>>>>>> + cpu_has_vmx_virtual_intr_delivery) && >>>>>>>>>>>>> + cpu_has_vmx_virtualize_x2apic_mode; >>>>>>>>>>>> >>>>>>>>>>>> I've been think about this, and it seems kind of asymmetric that for >>>>>>>>>>>> xAPIC mode we report hw assisted support only with >>>>>>>>>>>> virtualize_apic_accesses available, while for x2APIC we require >>>>>>>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or >>>>>>>>>>>> virtual_intr_delivery. >>>>>>>>>>>> >>>>>>>>>>>> I think we likely need to be more consistent here, and report hw >>>>>>>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is >>>>>>>>>>>> available. >>>>>>>>>>>> >>>>>>>>>>>> This will likely have some effect on patch 2 also, as you will have to >>>>>>>>>>>> adjust vmx_vlapic_msr_changed. >>>>>>>>>>>> >>>>>>>>>>>> Thanks, Roger. >>>>>>>>>>> >>>>>>>>>>> Any other thoughts on this? As on one hand it is asymmetric but also >>>>>>>>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in >>>>>>>>>>> this case, a VM exit will be avoided only when trying to access the TPR >>>>>>>>>>> register. >>>>>>>>>> >>>>>>>>>> I've been thinking about this, and reporting hardware assisted >>>>>>>>>> x{2}APIC virtualization with just >>>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or >>>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While >>>>>>>>>> those provide some assistance to the VMM in order to handle APIC >>>>>>>>>> accesses, it will still require a trap into the hypervisor to handle >>>>>>>>>> most of the accesses. >>>>>>>>>> >>>>>>>>>> So maybe we should only report hardware assisted support when the >>>>>>>>>> mentioned features are present together with >>>>>>>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT? >>>>>>>>> >>>>>>>>> Not sure - "some assistance" seems still a little better than none at all. >>>>>>>>> Which route to go depends on what exactly we intend the bit to be used for. >>>>>>>>> >>>>>>>> True. I intended this bit to be specifically for enabling >>>>>>>> assisted_x{2}apic. So, would it be inconsistent to report hardware >>>>>>>> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE >>>>>>>> but still claim that x{2}apic is virtualized if no MSR accesses are >>>>>>>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you >>>>>>>> say, the guest gets at least "some assistance" instead of none but we >>>>>>>> still claim x{2}apic virtualization when it is actually complete? Maybe >>>>>>>> I could also add a comment alluding to this in the xl documentation. >>>>>>> >>>>>>> To rephrase my earlier point: Which kind of decisions are the consumer(s) >>>>>>> of us reporting hardware assistance going to take? In how far is there a >>>>>>> risk that "some assistance" is overall going to lead to a loss of >>>>>>> performance? I guess I'd need to see comment and actual code all in one >>>>>>> place ... >>>>>>> >>>>>> So, I was thinking of adding something along the lines of: >>>>>> >>>>>> +=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. Notice enabling >>>>>> +this does not guarantee full virtualization for xAPIC, as this can >>>>>> +only be achieved if hardware supports “APIC-register virtualization” >>>>>> +and “virtual-interrupt delivery”. The default is settable via >>>>>> +L<xl.conf(5)>. >>>>> >>>>> But isn't this contradictory? Doesn't lack of APIC-register virtualization >>>>> mean VM exits upon (most) accesses? >>>> >>>> Yes, it does mean. I guess the alternative wouuld be then to require >>>> APIC-register virtualization for enabling xAPIC. But also, although this >>>> doesn't provide much acceleration, even getting a VM exit is some >>>> assistance if compared to instead getting an EPT fault and having to >>>> decode the access. >>> >>> I agree here, albeit I'd like to mention that EPT faults are also VM >>> exits. All my earlier comment was about is that this piece of doc >>> wants to express reality, whichever way it is that things end up >>> being implemented. >> >> Oh yes. Right, I see how this info could be misleading. >> >> How about this?... > > Getting close. The thing I can't judge is whether this level of technical > detail is suitable for this doc. Just one further remark: Unsure too. >> +=item B<assisted_xapic=BOOLEAN> B<(x86 only)> >> + >> +B<(x86 only)> Enables or disables hardware assisted virtualization for >> +xAPIC. With this option enabled, a memory-mapped APIC access will be >> +decoded by hardware and either issue a VM exit with an exit reason >> +instead of an EPT fault or altogether avoid a VM exit. Notice > > As said before, EPT faults also are VM exits and also provide an exit > reason. Therefore maybe "... and either issue a VM exit with a more > specific exit reason than an EPT fault would provide, or altogether > avoid a VM exit" or "... and either issue a more specific VM exit than > just an EPT fault, or altogether avoid a VM exit"? Yes, that's better. Thank you, Jane. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 2022-02-15 16:33 ` Jane Malalane @ 2022-03-09 13:47 ` Roger Pau Monné 0 siblings, 0 replies; 28+ messages in thread From: Roger Pau Monné @ 2022-03-09 13:47 UTC (permalink / raw) To: Jane Malalane Cc: Jan Beulich, Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, George Dunlap, Julien Grall, Stefano Stabellini, Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian, Andrew Cooper On Tue, Feb 15, 2022 at 04:33:15PM +0000, Jane Malalane wrote: > On 15/02/2022 15:21, 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 15.02.2022 16:10, Jane Malalane wrote: > >> On 15/02/2022 10:19, 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 15.02.2022 11:14, Jane Malalane wrote: > >>>> On 15/02/2022 07:09, 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 14.02.2022 18:09, Jane Malalane wrote: > >>>>>> On 14/02/2022 13:18, 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 14.02.2022 14:11, Jane Malalane wrote: > >>>>>>>> On 11/02/2022 11:46, 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 11.02.2022 12:29, Roger Pau Monné wrote: > >>>>>>>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote: > >>>>>>>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote: > >>>>>>>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote: > >>>>>>>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c > >>>>>>>>>>>>> index 7ab15e07a0..4060aef1bd 100644 > >>>>>>>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c > >>>>>>>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c > >>>>>>>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp) > >>>>>>>>>>>>> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch); > >>>>>>>>>>>>> } > >>>>>>>>>>>>> > >>>>>>>>>>>>> + /* Check whether hardware supports accelerated xapic and x2apic. */ > >>>>>>>>>>>>> + if ( bsp ) > >>>>>>>>>>>>> + { > >>>>>>>>>>>>> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses; > >>>>>>>>>>>>> + assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt || > >>>>>>>>>>>>> + cpu_has_vmx_virtual_intr_delivery) && > >>>>>>>>>>>>> + cpu_has_vmx_virtualize_x2apic_mode; > >>>>>>>>>>>> > >>>>>>>>>>>> I've been think about this, and it seems kind of asymmetric that for > >>>>>>>>>>>> xAPIC mode we report hw assisted support only with > >>>>>>>>>>>> virtualize_apic_accesses available, while for x2APIC we require > >>>>>>>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or > >>>>>>>>>>>> virtual_intr_delivery. > >>>>>>>>>>>> > >>>>>>>>>>>> I think we likely need to be more consistent here, and report hw > >>>>>>>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is > >>>>>>>>>>>> available. > >>>>>>>>>>>> > >>>>>>>>>>>> This will likely have some effect on patch 2 also, as you will have to > >>>>>>>>>>>> adjust vmx_vlapic_msr_changed. > >>>>>>>>>>>> > >>>>>>>>>>>> Thanks, Roger. > >>>>>>>>>>> > >>>>>>>>>>> Any other thoughts on this? As on one hand it is asymmetric but also > >>>>>>>>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in > >>>>>>>>>>> this case, a VM exit will be avoided only when trying to access the TPR > >>>>>>>>>>> register. > >>>>>>>>>> > >>>>>>>>>> I've been thinking about this, and reporting hardware assisted > >>>>>>>>>> x{2}APIC virtualization with just > >>>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or > >>>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While > >>>>>>>>>> those provide some assistance to the VMM in order to handle APIC > >>>>>>>>>> accesses, it will still require a trap into the hypervisor to handle > >>>>>>>>>> most of the accesses. > >>>>>>>>>> > >>>>>>>>>> So maybe we should only report hardware assisted support when the > >>>>>>>>>> mentioned features are present together with > >>>>>>>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT? > >>>>>>>>> > >>>>>>>>> Not sure - "some assistance" seems still a little better than none at all. > >>>>>>>>> Which route to go depends on what exactly we intend the bit to be used for. > >>>>>>>>> > >>>>>>>> True. I intended this bit to be specifically for enabling > >>>>>>>> assisted_x{2}apic. So, would it be inconsistent to report hardware > >>>>>>>> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE > >>>>>>>> but still claim that x{2}apic is virtualized if no MSR accesses are > >>>>>>>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you > >>>>>>>> say, the guest gets at least "some assistance" instead of none but we > >>>>>>>> still claim x{2}apic virtualization when it is actually complete? Maybe > >>>>>>>> I could also add a comment alluding to this in the xl documentation. > >>>>>>> > >>>>>>> To rephrase my earlier point: Which kind of decisions are the consumer(s) > >>>>>>> of us reporting hardware assistance going to take? In how far is there a > >>>>>>> risk that "some assistance" is overall going to lead to a loss of > >>>>>>> performance? I guess I'd need to see comment and actual code all in one > >>>>>>> place ... > >>>>>>> > >>>>>> So, I was thinking of adding something along the lines of: > >>>>>> > >>>>>> +=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. Notice enabling > >>>>>> +this does not guarantee full virtualization for xAPIC, as this can > >>>>>> +only be achieved if hardware supports “APIC-register virtualization” > >>>>>> +and “virtual-interrupt delivery”. The default is settable via > >>>>>> +L<xl.conf(5)>. > >>>>> > >>>>> But isn't this contradictory? Doesn't lack of APIC-register virtualization > >>>>> mean VM exits upon (most) accesses? > >>>> > >>>> Yes, it does mean. I guess the alternative wouuld be then to require > >>>> APIC-register virtualization for enabling xAPIC. But also, although this > >>>> doesn't provide much acceleration, even getting a VM exit is some > >>>> assistance if compared to instead getting an EPT fault and having to > >>>> decode the access. > >>> > >>> I agree here, albeit I'd like to mention that EPT faults are also VM > >>> exits. All my earlier comment was about is that this piece of doc > >>> wants to express reality, whichever way it is that things end up > >>> being implemented. > >> > >> Oh yes. Right, I see how this info could be misleading. > >> > >> How about this?... > > > > Getting close. The thing I can't judge is whether this level of technical > > detail is suitable for this doc. Just one further remark: > > Unsure too. > > >> +=item B<assisted_xapic=BOOLEAN> B<(x86 only)> > >> + > >> +B<(x86 only)> Enables or disables hardware assisted virtualization for > >> +xAPIC. With this option enabled, a memory-mapped APIC access will be > >> +decoded by hardware and either issue a VM exit with an exit reason > >> +instead of an EPT fault or altogether avoid a VM exit. Notice > > > > As said before, EPT faults also are VM exits and also provide an exit > > reason. Therefore maybe "... and either issue a VM exit with a more > > specific exit reason than an EPT fault would provide, or altogether > > avoid a VM exit" or "... and either issue a more specific VM exit than > > just an EPT fault, or altogether avoid a VM exit"? > > Yes, that's better. I would avoid mentioning EPT, as that's an Intel specific technology. Could we instead use 'p2m fault'? Thanks, Roger. ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC 2022-02-07 18:20 [PATCH v2 0/2] xen: Report and use hardware APIC virtualization capabilities Jane Malalane 2022-02-07 18:21 ` [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 Jane Malalane @ 2022-02-07 18:21 ` Jane Malalane 2022-02-08 8:58 ` Christian Lindig ` (2 more replies) 1 sibling, 3 replies; 28+ messages in thread From: Jane Malalane @ 2022-02-07 18:21 UTC (permalink / raw) To: Xen-devel Cc: Jane Malalane, Wei Liu, Anthony PERARD, Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, Christian Lindig, David Scott, Volodymyr Babchuk, Roger Pau Monné 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. Being able to disable x{2}APIC hardware assisted vitualization can be useful for testing and debugging purposes. 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> v2: * Add a LIBXL_HAVE_ASSISTED_APIC macro * Pass xcpyshinfo as a pointer in libxl__arch_get_physinfo * Add a return statement in now "int" libxl__arch_domain_build_info_setdefault * Preserve libxl__arch_domain_build_info_setdefault 's location in libxl_create.c * Correct x{2}apic default setting logic in libxl__arch_domain_prepare_config * Correct logic for parsing assisted_x{2}apic host/guest options in xl_parse.c and initialize them to -1 in xl.c * Use guest options directly in vmx_vlapic_msr_changed * Fix indentation of bool assisted_x{2}apic in struct hvm_domain * Add a change in xenctrl_stubs.c to pass xenctrl ABI checks --- docs/man/xl.cfg.5.pod.in | 10 ++++++++++ docs/man/xl.conf.5.pod.in | 12 ++++++++++++ tools/golang/xenlight/helpers.gen.go | 12 ++++++++++++ tools/include/libxl.h | 7 +++++++ tools/libs/light/libxl_arch.h | 5 +++-- tools/libs/light/libxl_arm.c | 7 +++++-- tools/libs/light/libxl_create.c | 23 ++++++++++++++--------- tools/libs/light/libxl_types.idl | 2 ++ tools/libs/light/libxl_x86.c | 31 +++++++++++++++++++++++++++++-- tools/ocaml/libs/xc/xenctrl.ml | 2 ++ tools/ocaml/libs/xc/xenctrl.mli | 2 ++ tools/ocaml/libs/xc/xenctrl_stubs.c | 2 +- tools/xl/xl.c | 8 ++++++++ tools/xl/xl.h | 2 ++ tools/xl/xl_parse.c | 16 ++++++++++++++++ xen/arch/x86/domain.c | 28 +++++++++++++++++++++++++++- xen/arch/x86/hvm/vmx/vmcs.c | 4 ++++ xen/arch/x86/hvm/vmx/vmx.c | 14 +++++--------- xen/arch/x86/include/asm/hvm/domain.h | 6 ++++++ xen/arch/x86/traps.c | 8 ++++---- xen/include/public/arch-x86/xen.h | 2 ++ 21 files changed, 173 insertions(+), 30 deletions(-) diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index b98d161398..1d98bbd182 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..30993827e5 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 virtualization by default. + +Default: enabled if supported. + +=item B<assisted_x2APIC=BOOLEAN> + +If enabled, domains will use x2APIC hardware assisted virtualization by default. + +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/include/libxl.h b/tools/include/libxl.h index 924e142628..83944c17ae 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -535,6 +535,13 @@ #define LIBXL_HAVE_PHYSINFO_ASSISTED_APIC 1 /* + * LIBXL_HAVE_ASSISTED_APIC indicates that libxl_domain_build_info has + * assisted_x{2}apic fields, for enabling hardware assisted virtualization for + * x{2}apic per domain. + */ +#define LIBXL_HAVE_ASSISTED_APIC 1 + +/* * libxl ABI compatibility * * The only guarantee which libxl makes regarding ABI compatibility diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h index 207ceac6a1..03b89929e6 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 39fdca1b49..ba5b8f433f 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); @@ -1399,6 +1400,8 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc, memset(&b_info->u, '\0', sizeof(b_info->u)); b_info->type = LIBXL_DOMAIN_TYPE_INVALID; libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PVH); + + return 0; } int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc, diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c index d7a40d7550..843e523df9 100644 --- a/tools/libs/light/libxl_create.c +++ b/tools/libs/light/libxl_create.c @@ -264,7 +264,20 @@ 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_physinfo info; + + 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; + } + libxl_defbool_setdefault(&b_info->dm_restrict, false); if (b_info->iommu_memkb == LIBXL_MEMKB_DEFAULT) @@ -457,14 +470,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc, } if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) { - libxl_physinfo info; - - rc = libxl_get_physinfo(CTX, &info); - if (rc) { - LOG(ERROR, "failed to get hypervisor info"); - return rc; - } - 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 e0a06ecfe3..f0fa0ceea2 100644 --- a/tools/libs/light/libxl_x86.c +++ b/tools/libs/light/libxl_x86.c @@ -23,6 +23,14 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, if (libxl_defbool_val(d_config->b_info.arch_x86.msr_relaxed)) config->arch.misc_flags |= XEN_X86_MSR_RELAXED; + if (d_config->c_info.type != LIBXL_DOMAIN_TYPE_PV) + { + if (libxl_defbool_val(d_config->b_info.arch_x86.assisted_xapic)) + config->arch.misc_flags |= XEN_X86_ASSISTED_XAPIC; + + if (libxl_defbool_val(d_config->b_info.arch_x86.assisted_x2apic)) + config->arch.misc_flags |= XEN_X86_ASSISTED_X2APIC; + } return 0; } @@ -819,11 +827,30 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc, { } -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc, - libxl_domain_build_info *b_info) +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc, + libxl_domain_build_info *b_info, + const libxl_physinfo *physinfo) { libxl_defbool_setdefault(&b_info->acpi, true); libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false); + + if (b_info->type != LIBXL_DOMAIN_TYPE_PV) + { + libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, + physinfo->cap_assisted_xapic); + libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, + physinfo->cap_assisted_x2apic); + } + + 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))) + { + LOG(ERROR, "Interrupt Controller Virtualization not supported for PV"); + return ERROR_INVAL; + } + + return 0; } int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc, diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml index 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/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 diff --git a/tools/xl/xl.c b/tools/xl/xl.c index 2d1ec18ea3..31eb223309 100644 --- a/tools/xl/xl.c +++ b/tools/xl/xl.c @@ -57,6 +57,8 @@ int max_grant_frames = -1; int max_maptrack_frames = -1; int max_grant_version = LIBXL_MAX_GRANT_DEFAULT; libxl_domid domid_policy = INVALID_DOMID; +int assisted_xapic = -1; +int assisted_x2apic = -1; xentoollog_level minmsglevel = minmsglevel_default; @@ -201,6 +203,12 @@ static void parse_global_config(const char *configfile, if (!xlu_cfg_get_long (config, "claim_mode", &l, 0)) claim_mode = l; + if (!xlu_cfg_get_long (config, "assisted_xapic", &l, 0)) + assisted_xapic = l; + + if (!xlu_cfg_get_long (config, "assisted_x2apic", &l, 0)) + assisted_x2apic = l; + xlu_cfg_replace_string (config, "remus.default.netbufscript", &default_remus_netbufscript, 0); xlu_cfg_replace_string (config, "colo.default.proxyscript", diff --git a/tools/xl/xl.h b/tools/xl/xl.h index c5c4bedbdd..528deb3feb 100644 --- a/tools/xl/xl.h +++ b/tools/xl/xl.h @@ -286,6 +286,8 @@ extern libxl_bitmap global_vm_affinity_mask; extern libxl_bitmap global_hvm_affinity_mask; extern libxl_bitmap global_pv_affinity_mask; extern libxl_domid domid_policy; +extern int assisted_xapic; +extern int assisted_x2apic; enum output_format { OUTPUT_FORMAT_JSON, diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index 117fcdcb2b..0ab9b145fe 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -1681,6 +1681,22 @@ 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_long(config, "assisted_xapic", &l , 0); + if ((e == ESRCH && assisted_xapic != -1)) /* use global default if present */ + libxl_defbool_set(&b_info->arch_x86.assisted_xapic, assisted_xapic); + else if (!e) + libxl_defbool_set(&b_info->arch_x86.assisted_xapic, l); + else + exit(1); + + e = xlu_cfg_get_long(config, "assisted_x2apic", &l, 0); + if ((e == ESRCH && assisted_x2apic != -1)) /* use global default if present */ + libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, assisted_x2apic); + else if (!e) + libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, l); + else + exit(1); + 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..9033a0e181 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,31 @@ 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, + "Hardware assisted x%sAPIC requested but not available\n", + assisted_xapic && !assisted_xapic_available ? "" : "2"); + return -EINVAL; + } + return 0; } @@ -863,6 +883,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 4060aef1bd..614db5c4a4 100644 --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -1157,6 +1157,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 36c8a12cfe..3c9ff60154 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -3333,16 +3333,11 @@ static void vmx_install_vlapic_mapping(struct vcpu *v) void vmx_vlapic_msr_changed(struct vcpu *v) { - int virtualize_x2apic_mode; struct vlapic *vlapic = vcpu_vlapic(v); unsigned int msr; - virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt || - cpu_has_vmx_virtual_intr_delivery) && - cpu_has_vmx_virtualize_x2apic_mode ); - - if ( !cpu_has_vmx_virtualize_apic_accesses && - !virtualize_x2apic_mode ) + if ( ! v->domain->arch.hvm.assisted_xapic && + ! v->domain->arch.hvm.assisted_x2apic ) return; vmx_vmcs_enter(v); @@ -3352,7 +3347,8 @@ void vmx_vlapic_msr_changed(struct vcpu *v) if ( !vlapic_hw_disabled(vlapic) && (vlapic_base_address(vlapic) == APIC_DEFAULT_PHYS_BASE) ) { - if ( virtualize_x2apic_mode && vlapic_x2apic_mode(vlapic) ) + if ( v->domain->arch.hvm.assisted_x2apic + && vlapic_x2apic_mode(vlapic) ) { v->arch.hvm.vmx.secondary_exec_control |= SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE; @@ -3373,7 +3369,7 @@ void vmx_vlapic_msr_changed(struct vcpu *v) vmx_clear_msr_intercept(v, MSR_X2APIC_SELF, VMX_MSR_W); } } - else + else if ( v->domain->arch.hvm.assisted_xapic ) 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..92bf53483c 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 virtualization. */ + bool assisted_xapic; + + /* x2APIC hardware assisted virtualization. */ + 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..33694acc99 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; /* @@ -1124,9 +1125,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, * and wrmsr in the guest will run without VMEXITs (see * vmx_vlapic_msr_changed()). */ - if ( cpu_has_vmx_virtualize_x2apic_mode && - cpu_has_vmx_apic_reg_virt && - cpu_has_vmx_virtual_intr_delivery ) + if ( (cpu_has_vmx_apic_reg_virt && 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 7acd94c8eb..9da32c6239 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC 2022-02-07 18:21 ` [PATCH v2 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC Jane Malalane @ 2022-02-08 8:58 ` Christian Lindig 2022-02-08 16:17 ` Roger Pau Monné 2022-02-10 10:09 ` Roger Pau Monné 2 siblings, 0 replies; 28+ messages in thread From: Christian Lindig @ 2022-02-08 8:58 UTC (permalink / raw) To: Jane Malalane Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, David Scott, Volodymyr Babchuk, Roger Pau Monne > On 7 Feb 2022, at 18:21, Jane Malalane <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. > > --- > docs/man/xl.cfg.5.pod.in | 10 ++++++++++ > docs/man/xl.conf.5.pod.in | 12 ++++++++++++ > tools/golang/xenlight/helpers.gen.go | 12 ++++++++++++ > tools/include/libxl.h | 7 +++++++ > tools/libs/light/libxl_arch.h | 5 +++-- > tools/libs/light/libxl_arm.c | 7 +++++-- > tools/libs/light/libxl_create.c | 23 ++++++++++++++--------- > tools/libs/light/libxl_types.idl | 2 ++ > tools/libs/light/libxl_x86.c | 31 +++++++++++++++++++++++++++++-- > tools/ocaml/libs/xc/xenctrl.ml | 2 ++ > tools/ocaml/libs/xc/xenctrl.mli | 2 ++ > tools/ocaml/libs/xc/xenctrl_stubs.c | 2 +- > tools/xl/xl.c | 8 ++++++++ > tools/xl/xl.h | 2 ++ > tools/xl/xl_parse.c | 16 ++++++++++++++++ > xen/arch/x86/domain.c | 28 +++++++++++++++++++++++++++- > xen/arch/x86/hvm/vmx/vmcs.c | 4 ++++ > xen/arch/x86/hvm/vmx/vmx.c | 14 +++++--------- > xen/arch/x86/include/asm/hvm/domain.h | 6 ++++++ > xen/arch/x86/traps.c | 8 ++++---- > xen/include/public/arch-x86/xen.h | 2 ++ > 21 files changed, 173 insertions(+), 30 deletions(-) The changes to the OCaml part are minimal. Acked-by: Christian Lindig <christian.lindig@citrix.com> — C ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC 2022-02-07 18:21 ` [PATCH v2 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC Jane Malalane 2022-02-08 8:58 ` Christian Lindig @ 2022-02-08 16:17 ` Roger Pau Monné 2022-02-09 10:57 ` Jane Malalane 2022-02-14 14:29 ` Jan Beulich 2022-02-10 10:09 ` Roger Pau Monné 2 siblings, 2 replies; 28+ messages in thread From: Roger Pau Monné @ 2022-02-08 16:17 UTC (permalink / raw) To: Jane Malalane Cc: Xen-devel, Wei Liu, Anthony PERARD, Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, Christian Lindig, David Scott, Volodymyr Babchuk On Mon, Feb 07, 2022 at 06:21:01PM +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. > > Being able to disable x{2}APIC hardware assisted vitualization can be > useful for testing and debugging purposes. I think you have agreed with Jan some changes to the description regarding the purpose of the commit. > > 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> > > v2: > * Add a LIBXL_HAVE_ASSISTED_APIC macro > * Pass xcpyshinfo as a pointer in libxl__arch_get_physinfo > * Add a return statement in now "int" > libxl__arch_domain_build_info_setdefault > * Preserve libxl__arch_domain_build_info_setdefault 's location in > libxl_create.c > * Correct x{2}apic default setting logic in > libxl__arch_domain_prepare_config > * Correct logic for parsing assisted_x{2}apic host/guest options in > xl_parse.c and initialize them to -1 in xl.c > * Use guest options directly in vmx_vlapic_msr_changed > * Fix indentation of bool assisted_x{2}apic in struct hvm_domain > * Add a change in xenctrl_stubs.c to pass xenctrl ABI checks > --- > docs/man/xl.cfg.5.pod.in | 10 ++++++++++ > docs/man/xl.conf.5.pod.in | 12 ++++++++++++ > tools/golang/xenlight/helpers.gen.go | 12 ++++++++++++ > tools/include/libxl.h | 7 +++++++ > tools/libs/light/libxl_arch.h | 5 +++-- > tools/libs/light/libxl_arm.c | 7 +++++-- > tools/libs/light/libxl_create.c | 23 ++++++++++++++--------- > tools/libs/light/libxl_types.idl | 2 ++ > tools/libs/light/libxl_x86.c | 31 +++++++++++++++++++++++++++++-- > tools/ocaml/libs/xc/xenctrl.ml | 2 ++ > tools/ocaml/libs/xc/xenctrl.mli | 2 ++ > tools/ocaml/libs/xc/xenctrl_stubs.c | 2 +- > tools/xl/xl.c | 8 ++++++++ > tools/xl/xl.h | 2 ++ > tools/xl/xl_parse.c | 16 ++++++++++++++++ > xen/arch/x86/domain.c | 28 +++++++++++++++++++++++++++- > xen/arch/x86/hvm/vmx/vmcs.c | 4 ++++ > xen/arch/x86/hvm/vmx/vmx.c | 14 +++++--------- > xen/arch/x86/include/asm/hvm/domain.h | 6 ++++++ > xen/arch/x86/traps.c | 8 ++++---- > xen/include/public/arch-x86/xen.h | 2 ++ > 21 files changed, 173 insertions(+), 30 deletions(-) > > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in > index b98d161398..1d98bbd182 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..30993827e5 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 virtualization by default. > + > +Default: enabled if supported. > + > +=item B<assisted_x2APIC=BOOLEAN> > + > +If enabled, domains will use x2APIC hardware assisted virtualization by default. > + > +Default: enabled if supported. We don't capitalize xl options, so I would suggest to lowercase APIC for all the option names. > + > =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/include/libxl.h b/tools/include/libxl.h > index 924e142628..83944c17ae 100644 > --- a/tools/include/libxl.h > +++ b/tools/include/libxl.h > @@ -535,6 +535,13 @@ > #define LIBXL_HAVE_PHYSINFO_ASSISTED_APIC 1 > > /* > + * LIBXL_HAVE_ASSISTED_APIC indicates that libxl_domain_build_info has > + * assisted_x{2}apic fields, for enabling hardware assisted virtualization for > + * x{2}apic per domain. > + */ > +#define LIBXL_HAVE_ASSISTED_APIC 1 > + > +/* > * libxl ABI compatibility > * > * The only guarantee which libxl makes regarding ABI compatibility > diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h > index 207ceac6a1..03b89929e6 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 39fdca1b49..ba5b8f433f 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); > @@ -1399,6 +1400,8 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc, > memset(&b_info->u, '\0', sizeof(b_info->u)); > b_info->type = LIBXL_DOMAIN_TYPE_INVALID; > libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PVH); > + > + return 0; > } > > int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc, > diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c > index d7a40d7550..843e523df9 100644 > --- a/tools/libs/light/libxl_create.c > +++ b/tools/libs/light/libxl_create.c > @@ -264,7 +264,20 @@ 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_physinfo info; The definition of info needs to be at the top of the function, together with the rest of the variable definitions. > + > + 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; > + } > + > libxl_defbool_setdefault(&b_info->dm_restrict, false); > > if (b_info->iommu_memkb == LIBXL_MEMKB_DEFAULT) > @@ -457,14 +470,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc, > } > > if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) { > - libxl_physinfo info; > - > - rc = libxl_get_physinfo(CTX, &info); > - if (rc) { > - LOG(ERROR, "failed to get hypervisor info"); > - return rc; > - } > - > 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 e0a06ecfe3..f0fa0ceea2 100644 > --- a/tools/libs/light/libxl_x86.c > +++ b/tools/libs/light/libxl_x86.c > @@ -23,6 +23,14 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, > if (libxl_defbool_val(d_config->b_info.arch_x86.msr_relaxed)) > config->arch.misc_flags |= XEN_X86_MSR_RELAXED; > > + if (d_config->c_info.type != LIBXL_DOMAIN_TYPE_PV) > + { Coding style for libxl is to place the bracket in the same line as the if. > + 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 +827,30 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc, > { > } > > -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc, > - libxl_domain_build_info *b_info) > +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc, > + libxl_domain_build_info *b_info, > + const libxl_physinfo *physinfo) > { > libxl_defbool_setdefault(&b_info->acpi, true); > libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false); > + > + if (b_info->type != LIBXL_DOMAIN_TYPE_PV) > + { > + libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, > + physinfo->cap_assisted_xapic); > + libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, > + physinfo->cap_assisted_x2apic); > + } > + > + 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))) You could just do: if (b_info->type != LIBXL_DOMAIN_TYPE_PV) { libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, physinfo->cap_assisted_xapic); libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, physinfo->cap_assisted_x2apic); } else if (!libxl_defbool_is_default(b_info->arch_x86.assisted_xapic) || !libxl_defbool_is_default(b_info->arch_x86.assisted_x2apic)) /* ERROR */ > + { > + LOG(ERROR, "Interrupt Controller Virtualization not supported for PV"); > + return ERROR_INVAL; > + } > + > + return 0; > } > > int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc, > diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml > index 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/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 */ We would usually define an XEN_X86_MISC_MAX that would point to XEN_X86_ASSISTED_X2APIC currently. > (VAL_MISC_FLAGS); > > #undef VAL_MISC_FLAGS > diff --git a/tools/xl/xl.c b/tools/xl/xl.c > index 2d1ec18ea3..31eb223309 100644 > --- a/tools/xl/xl.c > +++ b/tools/xl/xl.c > @@ -57,6 +57,8 @@ int max_grant_frames = -1; > int max_maptrack_frames = -1; > int max_grant_version = LIBXL_MAX_GRANT_DEFAULT; > libxl_domid domid_policy = INVALID_DOMID; > +int assisted_xapic = -1; > +int assisted_x2apic = -1; > > xentoollog_level minmsglevel = minmsglevel_default; > > @@ -201,6 +203,12 @@ static void parse_global_config(const char *configfile, > if (!xlu_cfg_get_long (config, "claim_mode", &l, 0)) > claim_mode = l; > > + if (!xlu_cfg_get_long (config, "assisted_xapic", &l, 0)) > + assisted_xapic = l; > + > + if (!xlu_cfg_get_long (config, "assisted_x2apic", &l, 0)) > + assisted_x2apic = l; > + > xlu_cfg_replace_string (config, "remus.default.netbufscript", > &default_remus_netbufscript, 0); > xlu_cfg_replace_string (config, "colo.default.proxyscript", > diff --git a/tools/xl/xl.h b/tools/xl/xl.h > index c5c4bedbdd..528deb3feb 100644 > --- a/tools/xl/xl.h > +++ b/tools/xl/xl.h > @@ -286,6 +286,8 @@ extern libxl_bitmap global_vm_affinity_mask; > extern libxl_bitmap global_hvm_affinity_mask; > extern libxl_bitmap global_pv_affinity_mask; > extern libxl_domid domid_policy; > +extern int assisted_xapic; > +extern int assisted_x2apic; > > enum output_format { > OUTPUT_FORMAT_JSON, > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c > index 117fcdcb2b..0ab9b145fe 100644 > --- a/tools/xl/xl_parse.c > +++ b/tools/xl/xl_parse.c > @@ -1681,6 +1681,22 @@ 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_long(config, "assisted_xapic", &l , 0); > + if ((e == ESRCH && assisted_xapic != -1)) /* use global default if present */ ^ no need for the extra parentheses here and below. > + libxl_defbool_set(&b_info->arch_x86.assisted_xapic, assisted_xapic); > + else if (!e) > + libxl_defbool_set(&b_info->arch_x86.assisted_xapic, l); > + else > + exit(1); > + > + e = xlu_cfg_get_long(config, "assisted_x2apic", &l, 0); > + if ((e == ESRCH && assisted_x2apic != -1)) /* use global default if present */ > + libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, assisted_x2apic); > + else if (!e) > + libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, l); > + else > + exit(1); > + > 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..9033a0e181 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,31 @@ 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, > + "Hardware assisted x%sAPIC requested but not available\n", > + assisted_xapic && !assisted_xapic_available ? "" : "2"); > + return -EINVAL; > + } > + > return 0; > } > > @@ -863,6 +883,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 4060aef1bd..614db5c4a4 100644 > --- a/xen/arch/x86/hvm/vmx/vmcs.c > +++ b/xen/arch/x86/hvm/vmx/vmcs.c > @@ -1157,6 +1157,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 36c8a12cfe..3c9ff60154 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -3333,16 +3333,11 @@ static void vmx_install_vlapic_mapping(struct vcpu *v) > > void vmx_vlapic_msr_changed(struct vcpu *v) > { > - int virtualize_x2apic_mode; > struct vlapic *vlapic = vcpu_vlapic(v); > unsigned int msr; > > - virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt || > - cpu_has_vmx_virtual_intr_delivery) && > - cpu_has_vmx_virtualize_x2apic_mode ); > - > - if ( !cpu_has_vmx_virtualize_apic_accesses && > - !virtualize_x2apic_mode ) > + if ( ! v->domain->arch.hvm.assisted_xapic && > + ! v->domain->arch.hvm.assisted_x2apic ) ^ extra space. > return; > > vmx_vmcs_enter(v); > @@ -3352,7 +3347,8 @@ void vmx_vlapic_msr_changed(struct vcpu *v) > if ( !vlapic_hw_disabled(vlapic) && > (vlapic_base_address(vlapic) == APIC_DEFAULT_PHYS_BASE) ) > { > - if ( virtualize_x2apic_mode && vlapic_x2apic_mode(vlapic) ) > + if ( v->domain->arch.hvm.assisted_x2apic > + && vlapic_x2apic_mode(vlapic) ) > { > v->arch.hvm.vmx.secondary_exec_control |= > SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE; > @@ -3373,7 +3369,7 @@ void vmx_vlapic_msr_changed(struct vcpu *v) > vmx_clear_msr_intercept(v, MSR_X2APIC_SELF, VMX_MSR_W); > } > } > - else > + else if ( v->domain->arch.hvm.assisted_xapic ) > 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..92bf53483c 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 virtualization. */ > + bool assisted_xapic; > + > + /* x2APIC hardware assisted virtualization. */ > + 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..33694acc99 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 && You can drop the cpu_has_vmx_apic_reg_virt check here, if cpu_has_vmx_apic_reg_virt is false assisted_xapic won't be set to true. > + v->domain->arch.hvm.assisted_xapic ) > res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT; > > /* > @@ -1124,9 +1125,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, > * and wrmsr in the guest will run without VMEXITs (see > * vmx_vlapic_msr_changed()). > */ > - if ( cpu_has_vmx_virtualize_x2apic_mode && > - cpu_has_vmx_apic_reg_virt && > - cpu_has_vmx_virtual_intr_delivery ) > + if ( (cpu_has_vmx_apic_reg_virt && cpu_has_vmx_virtual_intr_delivery) && ^ unneeded parentheses Thanks, Roger. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC 2022-02-08 16:17 ` Roger Pau Monné @ 2022-02-09 10:57 ` Jane Malalane 2022-02-10 9:37 ` Roger Pau Monné 2022-02-14 14:29 ` Jan Beulich 1 sibling, 1 reply; 28+ messages in thread From: Jane Malalane @ 2022-02-09 10:57 UTC (permalink / raw) To: Roger Pau Monne Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, Christian Lindig, David Scott, Volodymyr Babchuk On 08/02/2022 16:17, Roger Pau Monné wrote: > On Mon, Feb 07, 2022 at 06:21:01PM +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. >> >> Being able to disable x{2}APIC hardware assisted vitualization can be >> useful for testing and debugging purposes. > > I think you have agreed with Jan some changes to the description > regarding the purpose of the commit. Yes. > >> >> Signed-off-by: Jane Malalane <jane.malalane@citrix.com> >> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Will swap the ordering, forgot to do this previously, apologies. >> --- >> 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> >> >> v2: >> * Add a LIBXL_HAVE_ASSISTED_APIC macro >> * Pass xcpyshinfo as a pointer in libxl__arch_get_physinfo >> * Add a return statement in now "int" >> libxl__arch_domain_build_info_setdefault >> * Preserve libxl__arch_domain_build_info_setdefault 's location in >> libxl_create.c >> * Correct x{2}apic default setting logic in >> libxl__arch_domain_prepare_config >> * Correct logic for parsing assisted_x{2}apic host/guest options in >> xl_parse.c and initialize them to -1 in xl.c >> * Use guest options directly in vmx_vlapic_msr_changed >> * Fix indentation of bool assisted_x{2}apic in struct hvm_domain >> * Add a change in xenctrl_stubs.c to pass xenctrl ABI checks >> --- >> docs/man/xl.cfg.5.pod.in | 10 ++++++++++ >> docs/man/xl.conf.5.pod.in | 12 ++++++++++++ >> tools/golang/xenlight/helpers.gen.go | 12 ++++++++++++ >> tools/include/libxl.h | 7 +++++++ >> tools/libs/light/libxl_arch.h | 5 +++-- >> tools/libs/light/libxl_arm.c | 7 +++++-- >> tools/libs/light/libxl_create.c | 23 ++++++++++++++--------- >> tools/libs/light/libxl_types.idl | 2 ++ >> tools/libs/light/libxl_x86.c | 31 +++++++++++++++++++++++++++++-- >> tools/ocaml/libs/xc/xenctrl.ml | 2 ++ >> tools/ocaml/libs/xc/xenctrl.mli | 2 ++ >> tools/ocaml/libs/xc/xenctrl_stubs.c | 2 +- >> tools/xl/xl.c | 8 ++++++++ >> tools/xl/xl.h | 2 ++ >> tools/xl/xl_parse.c | 16 ++++++++++++++++ >> xen/arch/x86/domain.c | 28 +++++++++++++++++++++++++++- >> xen/arch/x86/hvm/vmx/vmcs.c | 4 ++++ >> xen/arch/x86/hvm/vmx/vmx.c | 14 +++++--------- >> xen/arch/x86/include/asm/hvm/domain.h | 6 ++++++ >> xen/arch/x86/traps.c | 8 ++++---- >> xen/include/public/arch-x86/xen.h | 2 ++ >> 21 files changed, 173 insertions(+), 30 deletions(-) >> >> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in >> index b98d161398..1d98bbd182 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..30993827e5 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 virtualization by default. >> + >> +Default: enabled if supported. >> + >> +=item B<assisted_x2APIC=BOOLEAN> >> + >> +If enabled, domains will use x2APIC hardware assisted virtualization by default. >> + >> +Default: enabled if supported. > > We don't capitalize xl options, so I would suggest to lowercase APIC > for all the option names. Okay. > >> + >> =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/include/libxl.h b/tools/include/libxl.h >> index 924e142628..83944c17ae 100644 >> --- a/tools/include/libxl.h >> +++ b/tools/include/libxl.h >> @@ -535,6 +535,13 @@ >> #define LIBXL_HAVE_PHYSINFO_ASSISTED_APIC 1 >> >> /* >> + * LIBXL_HAVE_ASSISTED_APIC indicates that libxl_domain_build_info has >> + * assisted_x{2}apic fields, for enabling hardware assisted virtualization for >> + * x{2}apic per domain. >> + */ >> +#define LIBXL_HAVE_ASSISTED_APIC 1 >> + >> +/* >> * libxl ABI compatibility >> * >> * The only guarantee which libxl makes regarding ABI compatibility >> diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h >> index 207ceac6a1..03b89929e6 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 39fdca1b49..ba5b8f433f 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); >> @@ -1399,6 +1400,8 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc, >> memset(&b_info->u, '\0', sizeof(b_info->u)); >> b_info->type = LIBXL_DOMAIN_TYPE_INVALID; >> libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PVH); >> + >> + return 0; >> } >> >> int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc, >> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c >> index d7a40d7550..843e523df9 100644 >> --- a/tools/libs/light/libxl_create.c >> +++ b/tools/libs/light/libxl_create.c >> @@ -264,7 +264,20 @@ 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_physinfo info; > > The definition of info needs to be at the top of the function, > together with the rest of the variable definitions. Okay. > >> + >> + 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; >> + } >> + >> libxl_defbool_setdefault(&b_info->dm_restrict, false); >> >> if (b_info->iommu_memkb == LIBXL_MEMKB_DEFAULT) >> @@ -457,14 +470,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc, >> } >> >> if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) { >> - libxl_physinfo info; >> - >> - rc = libxl_get_physinfo(CTX, &info); >> - if (rc) { >> - LOG(ERROR, "failed to get hypervisor info"); >> - return rc; >> - } >> - >> 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 e0a06ecfe3..f0fa0ceea2 100644 >> --- a/tools/libs/light/libxl_x86.c >> +++ b/tools/libs/light/libxl_x86.c >> @@ -23,6 +23,14 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, >> if (libxl_defbool_val(d_config->b_info.arch_x86.msr_relaxed)) >> config->arch.misc_flags |= XEN_X86_MSR_RELAXED; >> >> + if (d_config->c_info.type != LIBXL_DOMAIN_TYPE_PV) >> + { > > Coding style for libxl is to place the bracket in the same line as the > if. Okay. > >> + 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 +827,30 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc, >> { >> } >> >> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc, >> - libxl_domain_build_info *b_info) >> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc, >> + libxl_domain_build_info *b_info, >> + const libxl_physinfo *physinfo) >> { >> libxl_defbool_setdefault(&b_info->acpi, true); >> libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false); >> + >> + if (b_info->type != LIBXL_DOMAIN_TYPE_PV) >> + { >> + libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, >> + physinfo->cap_assisted_xapic); >> + libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, >> + physinfo->cap_assisted_x2apic); >> + } >> + >> + 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))) > > You could just do: > > if (b_info->type != LIBXL_DOMAIN_TYPE_PV) { > libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, > physinfo->cap_assisted_xapic); > libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, > physinfo->cap_assisted_x2apic); > } else if (!libxl_defbool_is_default(b_info->arch_x86.assisted_xapic) || > !libxl_defbool_is_default(b_info->arch_x86.assisted_x2apic)) > /* ERROR */ > >> + { >> + LOG(ERROR, "Interrupt Controller Virtualization not supported for PV"); >> + return ERROR_INVAL; >> + } >> + >> + return 0; True. >> } >> >> 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/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 */ > > We would usually define an XEN_X86_MISC_MAX that would point to > XEN_X86_ASSISTED_X2APIC currently. > >> (VAL_MISC_FLAGS); >> >> #undef VAL_MISC_FLAGS >> diff --git a/tools/xl/xl.c b/tools/xl/xl.c >> index 2d1ec18ea3..31eb223309 100644 >> --- a/tools/xl/xl.c >> +++ b/tools/xl/xl.c >> @@ -57,6 +57,8 @@ int max_grant_frames = -1; >> int max_maptrack_frames = -1; >> int max_grant_version = LIBXL_MAX_GRANT_DEFAULT; >> libxl_domid domid_policy = INVALID_DOMID; >> +int assisted_xapic = -1; >> +int assisted_x2apic = -1; >> >> xentoollog_level minmsglevel = minmsglevel_default; >> >> @@ -201,6 +203,12 @@ static void parse_global_config(const char *configfile, >> if (!xlu_cfg_get_long (config, "claim_mode", &l, 0)) >> claim_mode = l; >> >> + if (!xlu_cfg_get_long (config, "assisted_xapic", &l, 0)) >> + assisted_xapic = l; >> + >> + if (!xlu_cfg_get_long (config, "assisted_x2apic", &l, 0)) >> + assisted_x2apic = l; >> + >> xlu_cfg_replace_string (config, "remus.default.netbufscript", >> &default_remus_netbufscript, 0); >> xlu_cfg_replace_string (config, "colo.default.proxyscript", >> diff --git a/tools/xl/xl.h b/tools/xl/xl.h >> index c5c4bedbdd..528deb3feb 100644 >> --- a/tools/xl/xl.h >> +++ b/tools/xl/xl.h >> @@ -286,6 +286,8 @@ extern libxl_bitmap global_vm_affinity_mask; >> extern libxl_bitmap global_hvm_affinity_mask; >> extern libxl_bitmap global_pv_affinity_mask; >> extern libxl_domid domid_policy; >> +extern int assisted_xapic; >> +extern int assisted_x2apic; >> >> enum output_format { >> OUTPUT_FORMAT_JSON, >> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c >> index 117fcdcb2b..0ab9b145fe 100644 >> --- a/tools/xl/xl_parse.c >> +++ b/tools/xl/xl_parse.c >> @@ -1681,6 +1681,22 @@ 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_long(config, "assisted_xapic", &l , 0); >> + if ((e == ESRCH && assisted_xapic != -1)) /* use global default if present */ > ^ no need for the extra parentheses here and below. > >> + libxl_defbool_set(&b_info->arch_x86.assisted_xapic, assisted_xapic); >> + else if (!e) >> + libxl_defbool_set(&b_info->arch_x86.assisted_xapic, l); >> + else >> + exit(1); >> + >> + e = xlu_cfg_get_long(config, "assisted_x2apic", &l, 0); >> + if ((e == ESRCH && assisted_x2apic != -1)) /* use global default if present */ >> + libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, assisted_x2apic); >> + else if (!e) >> + libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, l); >> + else >> + exit(1); >> + >> 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..9033a0e181 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,31 @@ 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, >> + "Hardware assisted x%sAPIC requested but not available\n", >> + assisted_xapic && !assisted_xapic_available ? "" : "2"); >> + return -EINVAL; >> + } >> + >> return 0; >> } >> >> @@ -863,6 +883,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 4060aef1bd..614db5c4a4 100644 >> --- a/xen/arch/x86/hvm/vmx/vmcs.c >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >> @@ -1157,6 +1157,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 36c8a12cfe..3c9ff60154 100644 >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -3333,16 +3333,11 @@ static void vmx_install_vlapic_mapping(struct vcpu *v) >> >> void vmx_vlapic_msr_changed(struct vcpu *v) >> { >> - int virtualize_x2apic_mode; >> struct vlapic *vlapic = vcpu_vlapic(v); >> unsigned int msr; >> >> - virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt || >> - cpu_has_vmx_virtual_intr_delivery) && >> - cpu_has_vmx_virtualize_x2apic_mode ); >> - >> - if ( !cpu_has_vmx_virtualize_apic_accesses && >> - !virtualize_x2apic_mode ) >> + if ( ! v->domain->arch.hvm.assisted_xapic && >> + ! v->domain->arch.hvm.assisted_x2apic ) > ^ extra space. >> return; >> >> vmx_vmcs_enter(v); >> @@ -3352,7 +3347,8 @@ void vmx_vlapic_msr_changed(struct vcpu *v) >> if ( !vlapic_hw_disabled(vlapic) && >> (vlapic_base_address(vlapic) == APIC_DEFAULT_PHYS_BASE) ) >> { >> - if ( virtualize_x2apic_mode && vlapic_x2apic_mode(vlapic) ) >> + if ( v->domain->arch.hvm.assisted_x2apic >> + && vlapic_x2apic_mode(vlapic) ) >> { >> v->arch.hvm.vmx.secondary_exec_control |= >> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE; >> @@ -3373,7 +3369,7 @@ void vmx_vlapic_msr_changed(struct vcpu *v) >> vmx_clear_msr_intercept(v, MSR_X2APIC_SELF, VMX_MSR_W); >> } >> } >> - else >> + else if ( v->domain->arch.hvm.assisted_xapic ) >> 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..92bf53483c 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 virtualization. */ >> + bool assisted_xapic; >> + >> + /* x2APIC hardware assisted virtualization. */ >> + 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..33694acc99 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 && > > You can drop the cpu_has_vmx_apic_reg_virt check here, if > cpu_has_vmx_apic_reg_virt is false assisted_xapic won't be set to true. Oh, but assisted_xapic_available is only set to depend on cpu_has_vmx_virtualize_apic_accesses, unless I should correct this, but I might be missing something... > >> + v->domain->arch.hvm.assisted_xapic ) >> res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT; >> >> /* >> @@ -1124,9 +1125,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, >> * and wrmsr in the guest will run without VMEXITs (see >> * vmx_vlapic_msr_changed()). >> */ >> - if ( cpu_has_vmx_virtualize_x2apic_mode && >> - cpu_has_vmx_apic_reg_virt && >> - cpu_has_vmx_virtual_intr_delivery ) >> + if ( (cpu_has_vmx_apic_reg_virt && cpu_has_vmx_virtual_intr_delivery) && > ^ unneeded parentheses > Okay. > Thanks, Roger. Thank you, Jane. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC 2022-02-09 10:57 ` Jane Malalane @ 2022-02-10 9:37 ` Roger Pau Monné 0 siblings, 0 replies; 28+ messages in thread From: Roger Pau Monné @ 2022-02-10 9:37 UTC (permalink / raw) To: Jane Malalane Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, Christian Lindig, David Scott, Volodymyr Babchuk On Wed, Feb 09, 2022 at 10:57:28AM +0000, Jane Malalane wrote: > On 08/02/2022 16:17, Roger Pau Monné wrote: > > On Mon, Feb 07, 2022 at 06:21:01PM +0000, Jane Malalane wrote: > >> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c > >> index 485bd66971..33694acc99 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 && > > > > You can drop the cpu_has_vmx_apic_reg_virt check here, if > > cpu_has_vmx_apic_reg_virt is false assisted_xapic won't be set to true. > Oh, but assisted_xapic_available is only set to depend on > cpu_has_vmx_virtualize_apic_accesses, unless I should correct this, but > I might be missing something... No, you are right. We only report hw virtualized xAPIC to guests if both apic_reg_virt and virtualize_apic_accesses are available. Thanks, Roger. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC 2022-02-08 16:17 ` Roger Pau Monné 2022-02-09 10:57 ` Jane Malalane @ 2022-02-14 14:29 ` Jan Beulich 1 sibling, 0 replies; 28+ messages in thread From: Jan Beulich @ 2022-02-14 14:29 UTC (permalink / raw) To: Roger Pau Monné, Jane Malalane Cc: Xen-devel, Wei Liu, Anthony PERARD, Juergen Gross, Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini, Christian Lindig, David Scott, Volodymyr Babchuk On 08.02.2022 17:17, Roger Pau Monné wrote: > On Mon, Feb 07, 2022 at 06:21:01PM +0000, Jane Malalane wrote: >> --- 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 && > > You can drop the cpu_has_vmx_apic_reg_virt check here, if > cpu_has_vmx_apic_reg_virt is false assisted_xapic won't be set to true. Along these lines ... >> + v->domain->arch.hvm.assisted_xapic ) >> res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT; >> >> /* >> @@ -1124,9 +1125,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf, >> * and wrmsr in the guest will run without VMEXITs (see >> * vmx_vlapic_msr_changed()). >> */ >> - if ( cpu_has_vmx_virtualize_x2apic_mode && >> - cpu_has_vmx_apic_reg_virt && >> - cpu_has_vmx_virtual_intr_delivery ) >> + if ( (cpu_has_vmx_apic_reg_virt && cpu_has_vmx_virtual_intr_delivery) && > ^ unneeded parentheses ... this also wants simplifying to just v->domain->arch.hvm.assisted_x2apic: The apparently stray parentheses were, I think, added in reply to me pointing out that the check here isn't in line with that put in place by patch 1 in vmx_init_vmcs_config(). I.e. the inner && really was meant to be || as it looks. Yet once the two are in line, the same simplification as above is possible. Jan ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC 2022-02-07 18:21 ` [PATCH v2 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC Jane Malalane 2022-02-08 8:58 ` Christian Lindig 2022-02-08 16:17 ` Roger Pau Monné @ 2022-02-10 10:09 ` Roger Pau Monné 2022-02-10 16:44 ` Jane Malalane 2 siblings, 1 reply; 28+ messages in thread From: Roger Pau Monné @ 2022-02-10 10:09 UTC (permalink / raw) To: Jane Malalane Cc: Xen-devel, Wei Liu, Anthony PERARD, Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, Christian Lindig, David Scott, Volodymyr Babchuk On Mon, Feb 07, 2022 at 06:21:01PM +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. > > Being able to disable x{2}APIC hardware assisted vitualization can be > useful for testing and debugging purposes. Might be worth adding a note to the commit log in order to note that vmx_install_vlapic_mapping doesn't require modifications regardless of whether the guest has virtualize_apic_accesses enabled or not. Setting the APIC_ACCESS_ADDR VMCS field is fine even if virtualize_apic_accesses is not enabled for the guest: as long as the feature is supported by the CPU the field will exist. Thanks, Roger. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC 2022-02-10 10:09 ` Roger Pau Monné @ 2022-02-10 16:44 ` Jane Malalane 0 siblings, 0 replies; 28+ messages in thread From: Jane Malalane @ 2022-02-10 16:44 UTC (permalink / raw) To: Roger Pau Monne Cc: Xen-devel, Wei Liu, Anthony Perard, Juergen Gross, Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini, Christian Lindig, David Scott, Volodymyr Babchuk On 10/02/2022 10:09, Roger Pau Monné wrote: > On Mon, Feb 07, 2022 at 06:21:01PM +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. >> >> Being able to disable x{2}APIC hardware assisted vitualization can be >> useful for testing and debugging purposes. > > Might be worth adding a note to the commit log in order to note that > vmx_install_vlapic_mapping doesn't require modifications regardless of > whether the guest has virtualize_apic_accesses enabled or not. > > Setting the APIC_ACCESS_ADDR VMCS field is fine even if > virtualize_apic_accesses is not enabled for the guest: as long as the > feature is supported by the CPU the field will exist. Oh right. Will add these two points. Thanks, Jane. ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2022-03-09 13:47 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-07 18:20 [PATCH v2 0/2] xen: Report and use hardware APIC virtualization capabilities Jane Malalane 2022-02-07 18:21 ` [PATCH v2 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 Jane Malalane 2022-02-08 15:26 ` Roger Pau Monné 2022-02-09 12:26 ` Jane Malalane 2022-02-09 13:48 ` Anthony PERARD 2022-02-09 15:28 ` Jane Malalane 2022-02-10 10:03 ` Roger Pau Monné 2022-02-11 10:06 ` Jane Malalane 2022-02-11 11:29 ` Roger Pau Monné 2022-02-11 11:46 ` Jan Beulich 2022-02-14 13:11 ` Jane Malalane 2022-02-14 13:18 ` Jan Beulich 2022-02-14 17:09 ` Jane Malalane 2022-02-15 7:09 ` Jan Beulich 2022-02-15 10:14 ` Jane Malalane 2022-02-15 10:19 ` Jan Beulich 2022-02-15 15:10 ` Jane Malalane 2022-02-15 15:21 ` Jan Beulich 2022-02-15 16:33 ` Jane Malalane 2022-03-09 13:47 ` Roger Pau Monné 2022-02-07 18:21 ` [PATCH v2 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC Jane Malalane 2022-02-08 8:58 ` Christian Lindig 2022-02-08 16:17 ` Roger Pau Monné 2022-02-09 10:57 ` Jane Malalane 2022-02-10 9:37 ` Roger Pau Monné 2022-02-14 14:29 ` Jan Beulich 2022-02-10 10:09 ` Roger Pau Monné 2022-02-10 16:44 ` Jane Malalane
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.