* [PATCH 0/2] xen: Report and use hardware APIC virtualization capabilities
@ 2022-01-27 16:01 Jane Malalane
2022-01-27 16:01 ` [PATCH 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 Jane Malalane
2022-01-27 16:01 ` [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC Jane Malalane
0 siblings, 2 replies; 19+ messages in thread
From: Jane Malalane @ 2022-01-27 16:01 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 | 9 ++++--
tools/libs/light/libxl_create.c | 21 ++++++++------
tools/libs/light/libxl_types.idl | 4 +++
tools/libs/light/libxl_x86.c | 54 +++++++++++++++++++++++++++++++++--
tools/ocaml/libs/xc/xenctrl.ml | 7 +++++
tools/ocaml/libs/xc/xenctrl.mli | 7 +++++
tools/xl/xl.c | 8 ++++++
tools/xl/xl.h | 2 ++
tools/xl/xl_info.c | 6 ++--
tools/xl/xl_parse.c | 14 +++++++++
xen/arch/x86/domain.c | 27 +++++++++++++++++-
xen/arch/x86/hvm/vmx/vmcs.c | 10 +++++++
xen/arch/x86/hvm/vmx/vmx.c | 13 +++++----
xen/arch/x86/include/asm/domain.h | 4 +++
xen/arch/x86/include/asm/hvm/domain.h | 6 ++++
xen/arch/x86/sysctl.c | 7 +++++
xen/arch/x86/traps.c | 6 ++--
xen/include/public/arch-x86/xen.h | 6 ++++
xen/include/public/sysctl.h | 1 +
26 files changed, 258 insertions(+), 24 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
2022-01-27 16:01 [PATCH 0/2] xen: Report and use hardware APIC virtualization capabilities Jane Malalane
@ 2022-01-27 16:01 ` Jane Malalane
2022-01-28 17:04 ` Anthony PERARD
` (3 more replies)
2022-01-27 16:01 ` [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC Jane Malalane
1 sibling, 4 replies; 19+ messages in thread
From: Jane Malalane @ 2022-01-27 16:01 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>
---
tools/golang/xenlight/helpers.gen.go | 4 ++++
tools/golang/xenlight/types.gen.go | 6 ++++++
tools/include/libxl.h | 14 ++++++++++++++
tools/libs/light/libxl.c | 3 +++
tools/libs/light/libxl_arch.h | 4 ++++
tools/libs/light/libxl_arm.c | 4 ++++
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 | 6 ++++++
xen/arch/x86/include/asm/domain.h | 4 ++++
xen/arch/x86/sysctl.c | 7 +++++++
xen/include/public/arch-x86/xen.h | 4 ++++
xen/include/public/sysctl.h | 1 +
16 files changed, 84 insertions(+), 2 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..6d51e56704 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -528,6 +528,20 @@
#define LIBXL_HAVE_MAX_GRANT_VERSION 1
/*
+ * LIBXL_HAVE_PHYSINFO_ASSISTED_XAPIC indicates that libxl_physinfo has a
+ * cap_assisted_xapic field, which indicates the availability of xAPIC
+ * hardware assisted emulation.
+ */
+#define LIBXL_HAVE_PHYSINFO_ASSISTED_XAPIC 1
+
+/*
+ * LIBXL_HAVE_PHYSINFO_ASSISTED_X2APIC indicates that libxl_physinfo has a
+ * cap_assisted_x2apic field, which indicates the availability of x2APIC
+ * hardware assisted emulation.
+ */
+#define LIBXL_HAVE_PHYSINFO_ASSISTED_X2APIC 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..1588701d19 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..00cc50394d 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,
+ 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..52f2545498 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -1431,6 +1431,10 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
return rc;
}
+void libxl__arch_get_physinfo(libxl_physinfo *physinfo, 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..33da51fe89 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,
+ xc_physinfo_t xcphysinfo)
+{
+ physinfo->cap_assisted_xapic =
+ !!(xcphysinfo.arch_capabilities &
+ XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_XAPIC);
+ physinfo->cap_assisted_x2apic =
+ !!(xcphysinfo.arch_capabilities &
+ XEN_SYSCTL_PHYSCAP_ARCH_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..7691db0fdd 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -343,6 +343,12 @@ static int vmx_init_vmcs_config(bool bsp)
MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
}
+ /* Check whether hardware supports accelerated xapic and x2apic. */
+ assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
+ assisted_x2apic_available = cpu_has_vmx_virtualize_x2apic_mode &&
+ cpu_has_vmx_apic_reg_virt &&
+ cpu_has_vmx_virtual_intr_delivery;
+
/* 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..d6602795b4 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -756,6 +756,10 @@ 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..d0acf1f584 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 __read_mostly assisted_xapic_available;
+bool __read_mostly 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_ARCH_ASSISTED_XAPIC;
+ if ( assisted_x2apic_available )
+ pi->arch_capabilities |= XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_X2APIC;
}
long arch_do_sysctl(
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index 7acd94c8eb..f7b17e0f98 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -326,6 +326,10 @@ struct xen_arch_domainconfig {
/* GPE0 bit set during CPU hotplug */
#define XEN_ACPI_GPE0_CPUHP_BIT 2
+
+#define XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_XAPIC (1u << 0)
+#define XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_X2APIC (1u << 1)
+
#endif
/*
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 55252e97f2..16bdcab853 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -120,6 +120,7 @@ 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_ARCH_ */
uint64_aligned_t total_pages;
uint64_aligned_t free_pages;
uint64_aligned_t scrub_pages;
--
2.11.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
2022-01-27 16:01 [PATCH 0/2] xen: Report and use hardware APIC virtualization capabilities Jane Malalane
2022-01-27 16:01 ` [PATCH 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 Jane Malalane
@ 2022-01-27 16:01 ` Jane Malalane
2022-01-27 16:21 ` Christian Lindig
` (3 more replies)
1 sibling, 4 replies; 19+ messages in thread
From: Jane Malalane @ 2022-01-27 16:01 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.
Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
CC: Juergen Gross <jgross@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <george.dunlap@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Christian Lindig <christian.lindig@citrix.com>
CC: David Scott <dave@recoil.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
---
docs/man/xl.cfg.5.pod.in | 10 ++++++++
docs/man/xl.conf.5.pod.in | 12 ++++++++++
tools/golang/xenlight/helpers.gen.go | 12 ++++++++++
tools/libs/light/libxl_arch.h | 5 ++--
tools/libs/light/libxl_arm.c | 5 ++--
tools/libs/light/libxl_create.c | 21 ++++++++++-------
tools/libs/light/libxl_types.idl | 2 ++
tools/libs/light/libxl_x86.c | 43 +++++++++++++++++++++++++++++++++--
tools/ocaml/libs/xc/xenctrl.ml | 2 ++
tools/ocaml/libs/xc/xenctrl.mli | 2 ++
tools/xl/xl.c | 8 +++++++
tools/xl/xl.h | 2 ++
tools/xl/xl_parse.c | 14 ++++++++++++
xen/arch/x86/domain.c | 27 +++++++++++++++++++++-
xen/arch/x86/hvm/vmx/vmcs.c | 4 ++++
xen/arch/x86/hvm/vmx/vmx.c | 13 +++++++----
xen/arch/x86/include/asm/hvm/domain.h | 6 +++++
xen/arch/x86/traps.c | 6 +++--
xen/include/public/arch-x86/xen.h | 2 ++
19 files changed, 174 insertions(+), 22 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index b98d161398..974fe7d2d8 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -1862,6 +1862,16 @@ firmware tables when using certain older guest Operating
Systems. These tables have been superseded by newer constructs within
the ACPI tables.
+=item B<assisted_xapic=BOOLEAN>
+B<(x86 only)> Enables or disables hardware assisted virtualization for xapic.
+This allows accessing APIC registers without a VM-exit.
+The default is settable via L<xl.conf(5)>.
+
+=item B<assisted_x2apic=BOOLEAN>
+B<(x86 only)> Enables or disables hardware assisted virtualization for x2apic.
+This allows accessing APIC registers without a VM-exit.
+The default is settable via L<xl.conf(5)>.
+
=item B<nx=BOOLEAN>
B<(x86 only)> Hides or exposes the No-eXecute capability. This allows a guest
diff --git a/docs/man/xl.conf.5.pod.in b/docs/man/xl.conf.5.pod.in
index df20c08137..2d0a59d019 100644
--- a/docs/man/xl.conf.5.pod.in
+++ b/docs/man/xl.conf.5.pod.in
@@ -107,6 +107,18 @@ Sets the default value for the C<max_grant_version> domain config value.
Default: maximum grant version supported by the hypervisor.
+=item B<assisted_xapic=BOOLEAN>
+
+If enabled, domains will use xAPIC hardware assisted emulation by default.
+
+Default: enabled.
+
+=item B<assisted_x2apic=BOOLEAN>
+
+If enabled, domains will use x2APIC hardware assisted emulation by default.
+
+Default: enabled.
+
=item B<vif.default.script="PATH">
Configures the default hotplug script used by virtual network devices.
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index dd4e6c9f14..90e7b9b205 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -636,6 +636,12 @@ x.Passthrough = Passthrough(xc.passthrough)
if err := x.XendSuspendEvtchnCompat.fromC(&xc.xend_suspend_evtchn_compat);err != nil {
return fmt.Errorf("converting field XendSuspendEvtchnCompat: %v", err)
}
+if err := x.ArchX86.AssistedXapic.fromC(&xc.arch_x86.assisted_xapic);err != nil {
+return fmt.Errorf("converting field ArchX86.AssistedXapic: %v", err)
+}
+if err := x.ArchX86.AssistedX2Apic.fromC(&xc.arch_x86.assisted_x2apic);err != nil {
+return fmt.Errorf("converting field ArchX86.AssistedX2Apic: %v", err)
+}
return nil}
@@ -679,6 +685,12 @@ xc.passthrough = C.libxl_passthrough(x.Passthrough)
if err := x.XendSuspendEvtchnCompat.toC(&xc.xend_suspend_evtchn_compat); err != nil {
return fmt.Errorf("converting field XendSuspendEvtchnCompat: %v", err)
}
+if err := x.ArchX86.AssistedXapic.toC(&xc.arch_x86.assisted_xapic); err != nil {
+return fmt.Errorf("converting field ArchX86.AssistedXapic: %v", err)
+}
+if err := x.ArchX86.AssistedX2Apic.toC(&xc.arch_x86.assisted_x2apic); err != nil {
+return fmt.Errorf("converting field ArchX86.AssistedX2Apic: %v", err)
+}
return nil
}
diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h
index 00cc50394d..2eaff45526 100644
--- a/tools/libs/light/libxl_arch.h
+++ b/tools/libs/light/libxl_arch.h
@@ -71,8 +71,9 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
libxl_domain_create_info *c_info);
_hidden
-void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
- libxl_domain_build_info *b_info);
+int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
+ libxl_domain_build_info *b_info,
+ const libxl_physinfo *physinfo);
_hidden
int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 52f2545498..4d422bef96 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -1384,8 +1384,9 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
}
}
-void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
- libxl_domain_build_info *b_info)
+int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
+ libxl_domain_build_info *b_info,
+ const libxl_physinfo *physinfo)
{
/* ACPI is disabled by default */
libxl_defbool_setdefault(&b_info->acpi, false);
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index d7a40d7550..2bae6fef62 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -264,7 +264,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
if (!b_info->event_channels)
b_info->event_channels = 1023;
- libxl__arch_domain_build_info_setdefault(gc, b_info);
libxl_defbool_setdefault(&b_info->dm_restrict, false);
if (b_info->iommu_memkb == LIBXL_MEMKB_DEFAULT)
@@ -456,15 +455,21 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
libxl_defbool_setdefault(&b_info->nested_hvm, false);
}
- if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
- libxl_physinfo info;
+ libxl_physinfo info;
- rc = libxl_get_physinfo(CTX, &info);
- if (rc) {
- LOG(ERROR, "failed to get hypervisor info");
- return rc;
- }
+ rc = libxl_get_physinfo(CTX, &info);
+ if (rc) {
+ LOG(ERROR, "failed to get hypervisor info");
+ return rc;
+ }
+ rc = libxl__arch_domain_build_info_setdefault(gc, b_info, &info);
+ if (rc) {
+ LOG(ERROR, "unable to set domain arch build info defaults");
+ return rc;
+ }
+
+ if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
if (info.cap_gnttab_v2)
b_info->max_grant_version = 2;
else if (info.cap_gnttab_v1)
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 42ac6c357b..db5eb0a0b3 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -648,6 +648,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
("vuart", libxl_vuart_type),
])),
("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
+ ("assisted_xapic", libxl_defbool),
+ ("assisted_x2apic", libxl_defbool),
])),
# Alternate p2m is not bound to any architecture or guest type, as it is
# supported by x86 HVM and ARM support is planned.
diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
index 33da51fe89..b257fca756 100644
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -23,6 +23,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
if (libxl_defbool_val(d_config->b_info.arch_x86.msr_relaxed))
config->arch.misc_flags |= XEN_X86_MSR_RELAXED;
+ if(libxl_defbool_val(d_config->b_info.arch_x86.assisted_xapic))
+ config->arch.misc_flags |= XEN_X86_ASSISTED_XAPIC;
+
+ if(libxl_defbool_val(d_config->b_info.arch_x86.assisted_x2apic))
+ config->arch.misc_flags |= XEN_X86_ASSISTED_X2APIC;
+
return 0;
}
@@ -819,11 +825,44 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
{
}
-void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
- libxl_domain_build_info *b_info)
+int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
+ libxl_domain_build_info *b_info,
+ const libxl_physinfo *physinfo)
{
+ int rc;
+ bool assisted_xapic;
+ bool assisted_x2apic;
+
libxl_defbool_setdefault(&b_info->acpi, true);
libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false);
+
+ libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, false);
+ libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, false);
+
+ assisted_xapic = libxl_defbool_val(b_info->arch_x86.assisted_xapic);
+ assisted_x2apic = libxl_defbool_val(b_info->arch_x86.assisted_x2apic);
+
+ if ((assisted_xapic || assisted_x2apic) &&
+ b_info->type == LIBXL_DOMAIN_TYPE_PV)
+ {
+ LOG(ERROR, "Interrupt Controller Virtualization not supported for PV");
+ rc = ERROR_INVAL;
+ goto out;
+ }
+
+ if ((assisted_xapic && !physinfo->cap_assisted_xapic) ||
+ (assisted_x2apic && !physinfo->cap_assisted_x2apic))
+ {
+ LOG(ERROR, "x%sAPIC hardware supported emulation not available",
+ assisted_xapic && !physinfo->cap_assisted_xapic ? "" : "2");
+ rc = ERROR_INVAL;
+ goto out;
+ }
+
+ rc = 0;
+out:
+ return rc;
+
}
int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 7ce832d605..cce30d8731 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -50,6 +50,8 @@ type x86_arch_emulation_flags =
type x86_arch_misc_flags =
| X86_MSR_RELAXED
+ | X86_ASSISTED_XAPIC
+ | X86_ASSISTED_X2APIC
type xen_x86_arch_domainconfig =
{
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index a2b15130ee..67a22ec15c 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -44,6 +44,8 @@ type x86_arch_emulation_flags =
type x86_arch_misc_flags =
| X86_MSR_RELAXED
+ | X86_ASSISTED_XAPIC
+ | X86_ASSISTED_X2APIC
type xen_x86_arch_domainconfig = {
emulation_flags: x86_arch_emulation_flags list;
diff --git a/tools/xl/xl.c b/tools/xl/xl.c
index 2d1ec18ea3..b97e491c9c 100644
--- a/tools/xl/xl.c
+++ b/tools/xl/xl.c
@@ -57,6 +57,8 @@ int max_grant_frames = -1;
int max_maptrack_frames = -1;
int max_grant_version = LIBXL_MAX_GRANT_DEFAULT;
libxl_domid domid_policy = INVALID_DOMID;
+int assisted_xapic = 0;
+int assisted_x2apic = 0;
xentoollog_level minmsglevel = minmsglevel_default;
@@ -201,6 +203,12 @@ static void parse_global_config(const char *configfile,
if (!xlu_cfg_get_long (config, "claim_mode", &l, 0))
claim_mode = l;
+ if (!xlu_cfg_get_long (config, "assisted_xapic", &l, 0))
+ assisted_xapic = l;
+
+ if (!xlu_cfg_get_long (config, "assisted_x2apic", &l, 0))
+ assisted_x2apic = l;
+
xlu_cfg_replace_string (config, "remus.default.netbufscript",
&default_remus_netbufscript, 0);
xlu_cfg_replace_string (config, "colo.default.proxyscript",
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index c5c4bedbdd..528deb3feb 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -286,6 +286,8 @@ extern libxl_bitmap global_vm_affinity_mask;
extern libxl_bitmap global_hvm_affinity_mask;
extern libxl_bitmap global_pv_affinity_mask;
extern libxl_domid domid_policy;
+extern int assisted_xapic;
+extern int assisted_x2apic;
enum output_format {
OUTPUT_FORMAT_JSON,
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 117fcdcb2b..16ff9e76bc 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1681,6 +1681,20 @@ void parse_config_data(const char *config_source,
xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align, 0);
xlu_cfg_get_defbool(config, "apic", &b_info->apic, 0);
+ e = xlu_cfg_get_defbool(config, "assisted_xapic",
+ &b_info->arch_x86.assisted_xapic, 0);
+ if (e == ESRCH) /* not specified */
+ libxl_defbool_set(&b_info->arch_x86.assisted_xapic, assisted_xapic);
+ else if (e)
+ exit(1);
+
+ e = xlu_cfg_get_defbool(config, "assisted_x2apic",
+ &b_info->arch_x86.assisted_x2apic, 0);
+ if (e == ESRCH) /* not specified */
+ libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, assisted_x2apic);
+ else if (e)
+ exit(1);
+
switch (xlu_cfg_get_list(config, "viridian",
&viridian, &num_viridian, 1))
{
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ef1812dc14..d08f51e28b 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -619,6 +619,8 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
bool hap = config->flags & XEN_DOMCTL_CDF_hap;
bool nested_virt = config->flags & XEN_DOMCTL_CDF_nested_virt;
+ bool assisted_xapic = config->arch.misc_flags & XEN_X86_ASSISTED_XAPIC;
+ bool assisted_x2apic = config->arch.misc_flags & XEN_X86_ASSISTED_X2APIC;
unsigned int max_vcpus;
if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
@@ -685,13 +687,30 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
}
}
- if ( config->arch.misc_flags & ~XEN_X86_MSR_RELAXED )
+ if ( config->arch.misc_flags & ~(XEN_X86_MSR_RELAXED |
+ XEN_X86_ASSISTED_XAPIC |
+ XEN_X86_ASSISTED_X2APIC) )
{
dprintk(XENLOG_INFO, "Invalid arch misc flags %#x\n",
config->arch.misc_flags);
return -EINVAL;
}
+ if ( (assisted_xapic || assisted_x2apic) && !hvm )
+ {
+ dprintk(XENLOG_INFO,
+ "Interrupt Controller Virtualization not supported for PV\n");
+ return -EINVAL;
+ }
+
+ if ( (assisted_xapic && !assisted_xapic_available) ||
+ (assisted_x2apic && !assisted_x2apic_available) )
+ {
+ dprintk(XENLOG_INFO, "x%sAPIC requested but not available\n",
+ assisted_xapic && !assisted_xapic_available ? "" : "2");
+ return -EINVAL;
+ }
+
return 0;
}
@@ -863,6 +882,12 @@ int arch_domain_create(struct domain *d,
d->arch.msr_relaxed = config->arch.misc_flags & XEN_X86_MSR_RELAXED;
+ d->arch.hvm.assisted_xapic =
+ config->arch.misc_flags & XEN_X86_ASSISTED_XAPIC;
+
+ d->arch.hvm.assisted_x2apic =
+ config->arch.misc_flags & XEN_X86_ASSISTED_X2APIC;
+
return 0;
fail:
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 7691db0fdd..cb2d9759c9 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1154,6 +1154,10 @@ static int construct_vmcs(struct vcpu *v)
__vmwrite(PLE_WINDOW, ple_window);
}
+ if ( !v->domain->arch.hvm.assisted_xapic )
+ v->arch.hvm.vmx.secondary_exec_control &=
+ ~SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
+
if ( cpu_has_vmx_secondary_exec_control )
__vmwrite(SECONDARY_VM_EXEC_CONTROL,
v->arch.hvm.vmx.secondary_exec_control);
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index c44cf8f5d4..6501fb1a08 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3342,16 +3342,19 @@ static void vmx_install_vlapic_mapping(struct vcpu *v)
void vmx_vlapic_msr_changed(struct vcpu *v)
{
- int virtualize_x2apic_mode;
+ int virtualize_xapic_mode, virtualize_x2apic_mode;
struct vlapic *vlapic = vcpu_vlapic(v);
unsigned int msr;
+ virtualize_xapic_mode = ( cpu_has_vmx_virtualize_apic_accesses &&
+ v->domain->arch.hvm.assisted_xapic );
+
virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt ||
cpu_has_vmx_virtual_intr_delivery) &&
- cpu_has_vmx_virtualize_x2apic_mode );
+ cpu_has_vmx_virtualize_x2apic_mode &&
+ v->domain->arch.hvm.assisted_x2apic );
- if ( !cpu_has_vmx_virtualize_apic_accesses &&
- !virtualize_x2apic_mode )
+ if ( !virtualize_xapic_mode && !virtualize_x2apic_mode )
return;
vmx_vmcs_enter(v);
@@ -3382,7 +3385,7 @@ void vmx_vlapic_msr_changed(struct vcpu *v)
vmx_clear_msr_intercept(v, MSR_X2APIC_SELF, VMX_MSR_W);
}
}
- else
+ else if ( virtualize_xapic_mode )
v->arch.hvm.vmx.secondary_exec_control |=
SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
}
diff --git a/xen/arch/x86/include/asm/hvm/domain.h b/xen/arch/x86/include/asm/hvm/domain.h
index 698455444e..0e888cbeba 100644
--- a/xen/arch/x86/include/asm/hvm/domain.h
+++ b/xen/arch/x86/include/asm/hvm/domain.h
@@ -117,6 +117,12 @@ struct hvm_domain {
bool is_s3_suspended;
+ /* xAPIC hardware assisted emulation. */
+ bool assisted_xapic;
+
+ /* x2APIC hardware assisted emulation. */
+ bool assisted_x2apic;
+
/* hypervisor intercepted msix table */
struct list_head msixtbl_list;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 485bd66971..c215605814 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1115,7 +1115,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
if ( !is_hvm_domain(d) || subleaf != 0 )
break;
- if ( cpu_has_vmx_apic_reg_virt )
+ if ( cpu_has_vmx_apic_reg_virt &&
+ v->domain->arch.hvm.assisted_xapic )
res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT;
/*
@@ -1126,7 +1127,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
*/
if ( cpu_has_vmx_virtualize_x2apic_mode &&
cpu_has_vmx_apic_reg_virt &&
- cpu_has_vmx_virtual_intr_delivery )
+ cpu_has_vmx_virtual_intr_delivery &&
+ v->domain->arch.hvm.assisted_x2apic )
res->a |= XEN_HVM_CPUID_X2APIC_VIRT;
/*
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index f7b17e0f98..3c63c027a8 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -317,6 +317,8 @@ struct xen_arch_domainconfig {
* doesn't allow the guest to read or write to the underlying MSR.
*/
#define XEN_X86_MSR_RELAXED (1u << 0)
+#define XEN_X86_ASSISTED_XAPIC (1u << 1)
+#define XEN_X86_ASSISTED_X2APIC (1u << 2)
uint32_t misc_flags;
};
--
2.11.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
2022-01-27 16:01 ` [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC Jane Malalane
@ 2022-01-27 16:21 ` Christian Lindig
2022-01-31 12:05 ` Jan Beulich
` (2 subsequent siblings)
3 siblings, 0 replies; 19+ messages in thread
From: Christian Lindig @ 2022-01-27 16:21 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
[-- Attachment #1: Type: text/plain, Size: 21464 bytes --]
The changes for the OCaml bindings are minimal and administrative. This looks good to me.
Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>
On 27 Jan 2022, at 16:01, Jane Malalane <jane.malalane@citrix.com<mailto:jane.malalane@citrix.com>> wrote:
Introduce a new per-domain creation x86 specific flag to
select whether hardware assisted virtualization should be used for
x{2}APIC.
A per-domain option is added to xl in order to select the usage of
x{2}APIC hardware assisted vitualization, as well as a global
configuration option.
Having all APIC interaction exit to Xen for emulation is slow and can
induce much overhead. Hardware can speed up x{2}APIC by running APIC
read/write accesses without taking a VM exit.
Signed-off-by: Jane Malalane <jane.malalane@citrix.com<mailto:jane.malalane@citrix.com>>
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com<mailto:andrew.cooper3@citrix.com>>
---
CC: Wei Liu <wl@xen.org<mailto:wl@xen.org>>
CC: Anthony PERARD <anthony.perard@citrix.com<mailto:anthony.perard@citrix.com>>
CC: Juergen Gross <jgross@suse.com<mailto:jgross@suse.com>>
CC: Andrew Cooper <andrew.cooper3@citrix.com<mailto:andrew.cooper3@citrix.com>>
CC: George Dunlap <george.dunlap@citrix.com<mailto:george.dunlap@citrix.com>>
CC: Jan Beulich <jbeulich@suse.com<mailto:jbeulich@suse.com>>
CC: Julien Grall <julien@xen.org<mailto:julien@xen.org>>
CC: Stefano Stabellini <sstabellini@kernel.org<mailto:sstabellini@kernel.org>>
CC: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>
CC: David Scott <dave@recoil.org<mailto:dave@recoil.org>>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com<mailto:Volodymyr_Babchuk@epam.com>>
CC: "Roger Pau Monné" <roger.pau@citrix.com<mailto:roger.pau@citrix.com>>
---
docs/man/xl.cfg.5.pod.in | 10 ++++++++
docs/man/xl.conf.5.pod.in | 12 ++++++++++
tools/golang/xenlight/helpers.gen.go | 12 ++++++++++
tools/libs/light/libxl_arch.h | 5 ++--
tools/libs/light/libxl_arm.c | 5 ++--
tools/libs/light/libxl_create.c | 21 ++++++++++-------
tools/libs/light/libxl_types.idl | 2 ++
tools/libs/light/libxl_x86.c | 43 +++++++++++++++++++++++++++++++++--
tools/ocaml/libs/xc/xenctrl.ml | 2 ++
tools/ocaml/libs/xc/xenctrl.mli | 2 ++
tools/xl/xl.c | 8 +++++++
tools/xl/xl.h | 2 ++
tools/xl/xl_parse.c | 14 ++++++++++++
xen/arch/x86/domain.c | 27 +++++++++++++++++++++-
xen/arch/x86/hvm/vmx/vmcs.c | 4 ++++
xen/arch/x86/hvm/vmx/vmx.c | 13 +++++++----
xen/arch/x86/include/asm/hvm/domain.h | 6 +++++
xen/arch/x86/traps.c | 6 +++--
xen/include/public/arch-x86/xen.h | 2 ++
19 files changed, 174 insertions(+), 22 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index b98d161398..974fe7d2d8 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -1862,6 +1862,16 @@ firmware tables when using certain older guest Operating
Systems. These tables have been superseded by newer constructs within
the ACPI tables.
+=item B<assisted_xapic=BOOLEAN>
+B<(x86 only)> Enables or disables hardware assisted virtualization for xapic.
+This allows accessing APIC registers without a VM-exit.
+The default is settable via L<xl.conf(5)>.
+
+=item B<assisted_x2apic=BOOLEAN>
+B<(x86 only)> Enables or disables hardware assisted virtualization for x2apic.
+This allows accessing APIC registers without a VM-exit.
+The default is settable via L<xl.conf(5)>.
+
=item B<nx=BOOLEAN>
B<(x86 only)> Hides or exposes the No-eXecute capability. This allows a guest
diff --git a/docs/man/xl.conf.5.pod.in b/docs/man/xl.conf.5.pod.in
index df20c08137..2d0a59d019 100644
--- a/docs/man/xl.conf.5.pod.in
+++ b/docs/man/xl.conf.5.pod.in
@@ -107,6 +107,18 @@ Sets the default value for the C<max_grant_version> domain config value.
Default: maximum grant version supported by the hypervisor.
+=item B<assisted_xapic=BOOLEAN>
+
+If enabled, domains will use xAPIC hardware assisted emulation by default.
+
+Default: enabled.
+
+=item B<assisted_x2apic=BOOLEAN>
+
+If enabled, domains will use x2APIC hardware assisted emulation by default.
+
+Default: enabled.
+
=item B<vif.default.script="PATH">
Configures the default hotplug script used by virtual network devices.
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index dd4e6c9f14..90e7b9b205 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -636,6 +636,12 @@ x.Passthrough = Passthrough(xc.passthrough)
if err := x.XendSuspendEvtchnCompat.fromC(&xc.xend_suspend_evtchn_compat);err != nil {
return fmt.Errorf("converting field XendSuspendEvtchnCompat: %v", err)
}
+if err := x.ArchX86.AssistedXapic.fromC(&xc.arch_x86.assisted_xapic);err != nil {
+return fmt.Errorf("converting field ArchX86.AssistedXapic: %v", err)
+}
+if err := x.ArchX86.AssistedX2Apic.fromC(&xc.arch_x86.assisted_x2apic);err != nil {
+return fmt.Errorf("converting field ArchX86.AssistedX2Apic: %v", err)
+}
return nil}
@@ -679,6 +685,12 @@ xc.passthrough = C.libxl_passthrough(x.Passthrough)
if err := x.XendSuspendEvtchnCompat.toC(&xc.xend_suspend_evtchn_compat); err != nil {
return fmt.Errorf("converting field XendSuspendEvtchnCompat: %v", err)
}
+if err := x.ArchX86.AssistedXapic.toC(&xc.arch_x86.assisted_xapic); err != nil {
+return fmt.Errorf("converting field ArchX86.AssistedXapic: %v", err)
+}
+if err := x.ArchX86.AssistedX2Apic.toC(&xc.arch_x86.assisted_x2apic); err != nil {
+return fmt.Errorf("converting field ArchX86.AssistedX2Apic: %v", err)
+}
return nil
}
diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h
index 00cc50394d..2eaff45526 100644
--- a/tools/libs/light/libxl_arch.h
+++ b/tools/libs/light/libxl_arch.h
@@ -71,8 +71,9 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
libxl_domain_create_info *c_info);
_hidden
-void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
- libxl_domain_build_info *b_info);
+int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
+ libxl_domain_build_info *b_info,
+ const libxl_physinfo *physinfo);
_hidden
int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 52f2545498..4d422bef96 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -1384,8 +1384,9 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
}
}
-void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
- libxl_domain_build_info *b_info)
+int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
+ libxl_domain_build_info *b_info,
+ const libxl_physinfo *physinfo)
{
/* ACPI is disabled by default */
libxl_defbool_setdefault(&b_info->acpi, false);
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index d7a40d7550..2bae6fef62 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -264,7 +264,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
if (!b_info->event_channels)
b_info->event_channels = 1023;
- libxl__arch_domain_build_info_setdefault(gc, b_info);
libxl_defbool_setdefault(&b_info->dm_restrict, false);
if (b_info->iommu_memkb == LIBXL_MEMKB_DEFAULT)
@@ -456,15 +455,21 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
libxl_defbool_setdefault(&b_info->nested_hvm, false);
}
- if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
- libxl_physinfo info;
+ libxl_physinfo info;
- rc = libxl_get_physinfo(CTX, &info);
- if (rc) {
- LOG(ERROR, "failed to get hypervisor info");
- return rc;
- }
+ rc = libxl_get_physinfo(CTX, &info);
+ if (rc) {
+ LOG(ERROR, "failed to get hypervisor info");
+ return rc;
+ }
+ rc = libxl__arch_domain_build_info_setdefault(gc, b_info, &info);
+ if (rc) {
+ LOG(ERROR, "unable to set domain arch build info defaults");
+ return rc;
+ }
+
+ if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
if (info.cap_gnttab_v2)
b_info->max_grant_version = 2;
else if (info.cap_gnttab_v1)
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 42ac6c357b..db5eb0a0b3 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -648,6 +648,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
("vuart", libxl_vuart_type),
])),
("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
+ ("assisted_xapic", libxl_defbool),
+ ("assisted_x2apic", libxl_defbool),
])),
# Alternate p2m is not bound to any architecture or guest type, as it is
# supported by x86 HVM and ARM support is planned.
diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
index 33da51fe89..b257fca756 100644
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -23,6 +23,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
if (libxl_defbool_val(d_config->b_info.arch_x86.msr_relaxed))
config->arch.misc_flags |= XEN_X86_MSR_RELAXED;
+ if(libxl_defbool_val(d_config->b_info.arch_x86.assisted_xapic))
+ config->arch.misc_flags |= XEN_X86_ASSISTED_XAPIC;
+
+ if(libxl_defbool_val(d_config->b_info.arch_x86.assisted_x2apic))
+ config->arch.misc_flags |= XEN_X86_ASSISTED_X2APIC;
+
return 0;
}
@@ -819,11 +825,44 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
{
}
-void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
- libxl_domain_build_info *b_info)
+int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
+ libxl_domain_build_info *b_info,
+ const libxl_physinfo *physinfo)
{
+ int rc;
+ bool assisted_xapic;
+ bool assisted_x2apic;
+
libxl_defbool_setdefault(&b_info->acpi, true);
libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false);
+
+ libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, false);
+ libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, false);
+
+ assisted_xapic = libxl_defbool_val(b_info->arch_x86.assisted_xapic);
+ assisted_x2apic = libxl_defbool_val(b_info->arch_x86.assisted_x2apic);
+
+ if ((assisted_xapic || assisted_x2apic) &&
+ b_info->type == LIBXL_DOMAIN_TYPE_PV)
+ {
+ LOG(ERROR, "Interrupt Controller Virtualization not supported for PV");
+ rc = ERROR_INVAL;
+ goto out;
+ }
+
+ if ((assisted_xapic && !physinfo->cap_assisted_xapic) ||
+ (assisted_x2apic && !physinfo->cap_assisted_x2apic))
+ {
+ LOG(ERROR, "x%sAPIC hardware supported emulation not available",
+ assisted_xapic && !physinfo->cap_assisted_xapic ? "" : "2");
+ rc = ERROR_INVAL;
+ goto out;
+ }
+
+ rc = 0;
+out:
+ return rc;
+
}
int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 7ce832d605..cce30d8731 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -50,6 +50,8 @@ type x86_arch_emulation_flags =
type x86_arch_misc_flags =
| X86_MSR_RELAXED
+ | X86_ASSISTED_XAPIC
+ | X86_ASSISTED_X2APIC
type xen_x86_arch_domainconfig =
{
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index a2b15130ee..67a22ec15c 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -44,6 +44,8 @@ type x86_arch_emulation_flags =
type x86_arch_misc_flags =
| X86_MSR_RELAXED
+ | X86_ASSISTED_XAPIC
+ | X86_ASSISTED_X2APIC
type xen_x86_arch_domainconfig = {
emulation_flags: x86_arch_emulation_flags list;
diff --git a/tools/xl/xl.c b/tools/xl/xl.c
index 2d1ec18ea3..b97e491c9c 100644
--- a/tools/xl/xl.c
+++ b/tools/xl/xl.c
@@ -57,6 +57,8 @@ int max_grant_frames = -1;
int max_maptrack_frames = -1;
int max_grant_version = LIBXL_MAX_GRANT_DEFAULT;
libxl_domid domid_policy = INVALID_DOMID;
+int assisted_xapic = 0;
+int assisted_x2apic = 0;
xentoollog_level minmsglevel = minmsglevel_default;
@@ -201,6 +203,12 @@ static void parse_global_config(const char *configfile,
if (!xlu_cfg_get_long (config, "claim_mode", &l, 0))
claim_mode = l;
+ if (!xlu_cfg_get_long (config, "assisted_xapic", &l, 0))
+ assisted_xapic = l;
+
+ if (!xlu_cfg_get_long (config, "assisted_x2apic", &l, 0))
+ assisted_x2apic = l;
+
xlu_cfg_replace_string (config, "remus.default.netbufscript",
&default_remus_netbufscript, 0);
xlu_cfg_replace_string (config, "colo.default.proxyscript",
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index c5c4bedbdd..528deb3feb 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -286,6 +286,8 @@ extern libxl_bitmap global_vm_affinity_mask;
extern libxl_bitmap global_hvm_affinity_mask;
extern libxl_bitmap global_pv_affinity_mask;
extern libxl_domid domid_policy;
+extern int assisted_xapic;
+extern int assisted_x2apic;
enum output_format {
OUTPUT_FORMAT_JSON,
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 117fcdcb2b..16ff9e76bc 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1681,6 +1681,20 @@ void parse_config_data(const char *config_source,
xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align, 0);
xlu_cfg_get_defbool(config, "apic", &b_info->apic, 0);
+ e = xlu_cfg_get_defbool(config, "assisted_xapic",
+ &b_info->arch_x86.assisted_xapic, 0);
+ if (e == ESRCH) /* not specified */
+ libxl_defbool_set(&b_info->arch_x86.assisted_xapic, assisted_xapic);
+ else if (e)
+ exit(1);
+
+ e = xlu_cfg_get_defbool(config, "assisted_x2apic",
+ &b_info->arch_x86.assisted_x2apic, 0);
+ if (e == ESRCH) /* not specified */
+ libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, assisted_x2apic);
+ else if (e)
+ exit(1);
+
switch (xlu_cfg_get_list(config, "viridian",
&viridian, &num_viridian, 1))
{
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ef1812dc14..d08f51e28b 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -619,6 +619,8 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
bool hap = config->flags & XEN_DOMCTL_CDF_hap;
bool nested_virt = config->flags & XEN_DOMCTL_CDF_nested_virt;
+ bool assisted_xapic = config->arch.misc_flags & XEN_X86_ASSISTED_XAPIC;
+ bool assisted_x2apic = config->arch.misc_flags & XEN_X86_ASSISTED_X2APIC;
unsigned int max_vcpus;
if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
@@ -685,13 +687,30 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
}
}
- if ( config->arch.misc_flags & ~XEN_X86_MSR_RELAXED )
+ if ( config->arch.misc_flags & ~(XEN_X86_MSR_RELAXED |
+ XEN_X86_ASSISTED_XAPIC |
+ XEN_X86_ASSISTED_X2APIC) )
{
dprintk(XENLOG_INFO, "Invalid arch misc flags %#x\n",
config->arch.misc_flags);
return -EINVAL;
}
+ if ( (assisted_xapic || assisted_x2apic) && !hvm )
+ {
+ dprintk(XENLOG_INFO,
+ "Interrupt Controller Virtualization not supported for PV\n");
+ return -EINVAL;
+ }
+
+ if ( (assisted_xapic && !assisted_xapic_available) ||
+ (assisted_x2apic && !assisted_x2apic_available) )
+ {
+ dprintk(XENLOG_INFO, "x%sAPIC requested but not available\n",
+ assisted_xapic && !assisted_xapic_available ? "" : "2");
+ return -EINVAL;
+ }
+
return 0;
}
@@ -863,6 +882,12 @@ int arch_domain_create(struct domain *d,
d->arch.msr_relaxed = config->arch.misc_flags & XEN_X86_MSR_RELAXED;
+ d->arch.hvm.assisted_xapic =
+ config->arch.misc_flags & XEN_X86_ASSISTED_XAPIC;
+
+ d->arch.hvm.assisted_x2apic =
+ config->arch.misc_flags & XEN_X86_ASSISTED_X2APIC;
+
return 0;
fail:
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 7691db0fdd..cb2d9759c9 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1154,6 +1154,10 @@ static int construct_vmcs(struct vcpu *v)
__vmwrite(PLE_WINDOW, ple_window);
}
+ if ( !v->domain->arch.hvm.assisted_xapic )
+ v->arch.hvm.vmx.secondary_exec_control &=
+ ~SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
+
if ( cpu_has_vmx_secondary_exec_control )
__vmwrite(SECONDARY_VM_EXEC_CONTROL,
v->arch.hvm.vmx.secondary_exec_control);
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index c44cf8f5d4..6501fb1a08 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3342,16 +3342,19 @@ static void vmx_install_vlapic_mapping(struct vcpu *v)
void vmx_vlapic_msr_changed(struct vcpu *v)
{
- int virtualize_x2apic_mode;
+ int virtualize_xapic_mode, virtualize_x2apic_mode;
struct vlapic *vlapic = vcpu_vlapic(v);
unsigned int msr;
+ virtualize_xapic_mode = ( cpu_has_vmx_virtualize_apic_accesses &&
+ v->domain->arch.hvm.assisted_xapic );
+
virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt ||
cpu_has_vmx_virtual_intr_delivery) &&
- cpu_has_vmx_virtualize_x2apic_mode );
+ cpu_has_vmx_virtualize_x2apic_mode &&
+ v->domain->arch.hvm.assisted_x2apic );
- if ( !cpu_has_vmx_virtualize_apic_accesses &&
- !virtualize_x2apic_mode )
+ if ( !virtualize_xapic_mode && !virtualize_x2apic_mode )
return;
vmx_vmcs_enter(v);
@@ -3382,7 +3385,7 @@ void vmx_vlapic_msr_changed(struct vcpu *v)
vmx_clear_msr_intercept(v, MSR_X2APIC_SELF, VMX_MSR_W);
}
}
- else
+ else if ( virtualize_xapic_mode )
v->arch.hvm.vmx.secondary_exec_control |=
SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES;
}
diff --git a/xen/arch/x86/include/asm/hvm/domain.h b/xen/arch/x86/include/asm/hvm/domain.h
index 698455444e..0e888cbeba 100644
--- a/xen/arch/x86/include/asm/hvm/domain.h
+++ b/xen/arch/x86/include/asm/hvm/domain.h
@@ -117,6 +117,12 @@ struct hvm_domain {
bool is_s3_suspended;
+ /* xAPIC hardware assisted emulation. */
+ bool assisted_xapic;
+
+ /* x2APIC hardware assisted emulation. */
+ bool assisted_x2apic;
+
/* hypervisor intercepted msix table */
struct list_head msixtbl_list;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 485bd66971..c215605814 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1115,7 +1115,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
if ( !is_hvm_domain(d) || subleaf != 0 )
break;
- if ( cpu_has_vmx_apic_reg_virt )
+ if ( cpu_has_vmx_apic_reg_virt &&
+ v->domain->arch.hvm.assisted_xapic )
res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT;
/*
@@ -1126,7 +1127,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
*/
if ( cpu_has_vmx_virtualize_x2apic_mode &&
cpu_has_vmx_apic_reg_virt &&
- cpu_has_vmx_virtual_intr_delivery )
+ cpu_has_vmx_virtual_intr_delivery &&
+ v->domain->arch.hvm.assisted_x2apic )
res->a |= XEN_HVM_CPUID_X2APIC_VIRT;
/*
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index f7b17e0f98..3c63c027a8 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -317,6 +317,8 @@ struct xen_arch_domainconfig {
* doesn't allow the guest to read or write to the underlying MSR.
*/
#define XEN_X86_MSR_RELAXED (1u << 0)
+#define XEN_X86_ASSISTED_XAPIC (1u << 1)
+#define XEN_X86_ASSISTED_X2APIC (1u << 2)
uint32_t misc_flags;
};
--
2.11.0
[-- Attachment #2: Type: text/html, Size: 41725 bytes --]
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
2022-01-27 16:01 ` [PATCH 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 Jane Malalane
@ 2022-01-28 17:04 ` Anthony PERARD
2022-01-31 15:54 ` Jane Malalane
2022-01-31 11:18 ` Jan Beulich
` (2 subsequent siblings)
3 siblings, 1 reply; 19+ messages in thread
From: Anthony PERARD @ 2022-01-28 17:04 UTC (permalink / raw)
To: Jane Malalane
Cc: Xen-devel, Wei Liu, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Volodymyr Babchuk,
Bertrand Marquis, Jun Nakajima, Kevin Tian, Roger Pau Monné
On Thu, Jan 27, 2022 at 04:01:32PM +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>
> ---
> tools/golang/xenlight/helpers.gen.go | 4 ++++
> tools/golang/xenlight/types.gen.go | 6 ++++++
Note for committers: Please regenerate the go bindings, there are
out-of-sync with libxl_types.idl at the moment.
> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> index 1feadebb18..33da51fe89 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,
> + xc_physinfo_t xcphysinfo)
It might be better to pass "xcphysinfo" as a pointer, otherwise I think
a copy of the whole struct is made when calling this function.
In any case, the tool part of the patch looks good:
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
2022-01-27 16:01 ` [PATCH 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 Jane Malalane
2022-01-28 17:04 ` Anthony PERARD
@ 2022-01-31 11:18 ` Jan Beulich
2022-01-31 11:53 ` Jan Beulich
2022-01-31 17:57 ` Roger Pau Monné
3 siblings, 0 replies; 19+ messages in thread
From: Jan Beulich @ 2022-01-31 11:18 UTC (permalink / raw)
To: Jane Malalane
Cc: Wei Liu, Anthony PERARD, Juergen Gross, Andrew Cooper,
George Dunlap, Julien Grall, Stefano Stabellini,
Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian,
Roger Pau Monné,
Xen-devel
On 27.01.2022 17:01, 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>
Nit: Please try to keep tags in chronological order.
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -343,6 +343,12 @@ static int vmx_init_vmcs_config(bool bsp)
> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
> }
>
> + /* Check whether hardware supports accelerated xapic and x2apic. */
> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
> + assisted_x2apic_available = cpu_has_vmx_virtualize_x2apic_mode &&
> + cpu_has_vmx_apic_reg_virt &&
> + cpu_has_vmx_virtual_intr_delivery;
Imo this would better live in .init.text, which would guarantee that
hot-onlined CPUs later cannot corrupt the original setting. IOW at the
very least the setting of the variables wants to be conditional upon
"bsp", but ideally this would move to e.g. vmx_vmcs_init().
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -756,6 +756,10 @@ 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;
May I suggest that you omit the intermediate blank line, just like
you did below?
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -69,6 +69,9 @@ struct l3_cache_info {
> unsigned long size;
> };
>
> +bool __read_mostly assisted_xapic_available;
> +bool __read_mostly assisted_x2apic_available;
With the above this could then be __ro_after_init.
> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -326,6 +326,10 @@ struct xen_arch_domainconfig {
>
> /* GPE0 bit set during CPU hotplug */
> #define XEN_ACPI_GPE0_CPUHP_BIT 2
> +
> +#define XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_XAPIC (1u << 0)
> +#define XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_X2APIC (1u << 1)
> +
> #endif
I have to admit I'm not convinced it is a good idea to put these here,
far away and disconnected from ...
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -120,6 +120,7 @@ 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_ARCH_ */
... this. See e.g. XEN_SYSCTL_get_cpu_levelling_caps which is entirely
x86-specific yet still fully contained in sysctl.h. Furthermore I
think you want "X86" somewhere in the names of the two #define-s.
While xAPIC and x2APIC are sufficiently clearly x86 terms, future
further bits may not as obviously be, yet we will want to have the
names be consistent. Perhaps best to replace "ARCH" by "X86"?
> uint64_aligned_t total_pages;
> uint64_aligned_t free_pages;
> uint64_aligned_t scrub_pages;
With these subsequent fields you're introducing a padding hole.
Please make that explicit (including the setting of it to zero if
that doesn't happen implicitly already). And changing the layout of a
struct here also requires that XEN_SYSCTL_INTERFACE_VERSION be bumped,
at least as long as it hasn't yet in the current dev cycle.
Jan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
2022-01-27 16:01 ` [PATCH 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 Jane Malalane
2022-01-28 17:04 ` Anthony PERARD
2022-01-31 11:18 ` Jan Beulich
@ 2022-01-31 11:53 ` Jan Beulich
2022-01-31 17:57 ` Roger Pau Monné
3 siblings, 0 replies; 19+ messages in thread
From: Jan Beulich @ 2022-01-31 11:53 UTC (permalink / raw)
To: Jane Malalane
Cc: Wei Liu, Anthony PERARD, Juergen Gross, Andrew Cooper,
George Dunlap, Julien Grall, Stefano Stabellini,
Volodymyr Babchuk, Bertrand Marquis, Jun Nakajima, Kevin Tian,
Roger Pau Monné,
Xen-devel
On 27.01.2022 17:01, Jane Malalane wrote:
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -343,6 +343,12 @@ static int vmx_init_vmcs_config(bool bsp)
> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
> }
>
> + /* Check whether hardware supports accelerated xapic and x2apic. */
> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
> + assisted_x2apic_available = cpu_has_vmx_virtualize_x2apic_mode &&
> + cpu_has_vmx_apic_reg_virt &&
> + cpu_has_vmx_virtual_intr_delivery;
In patch 2 you modify this original related construct:
virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt ||
cpu_has_vmx_virtual_intr_delivery) &&
cpu_has_vmx_virtualize_x2apic_mode );
As a result I don't think the expression you use above is correct.
Jan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
2022-01-27 16:01 ` [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC Jane Malalane
2022-01-27 16:21 ` Christian Lindig
@ 2022-01-31 12:05 ` Jan Beulich
2022-01-31 16:02 ` Jane Malalane
2022-02-08 13:27 ` Jane Malalane
2022-01-31 16:05 ` Anthony PERARD
2022-02-01 10:02 ` Roger Pau Monné
3 siblings, 2 replies; 19+ messages in thread
From: Jan Beulich @ 2022-01-31 12:05 UTC (permalink / raw)
To: Jane Malalane
Cc: Wei Liu, Anthony PERARD, Juergen Gross, Andrew Cooper,
George Dunlap, Julien Grall, Stefano Stabellini,
Christian Lindig, David Scott, Volodymyr Babchuk,
Roger Pau Monné,
Xen-devel
On 27.01.2022 17:01, Jane Malalane wrote:
> Introduce a new per-domain creation x86 specific flag to
> select whether hardware assisted virtualization should be used for
> x{2}APIC.
>
> A per-domain option is added to xl in order to select the usage of
> x{2}APIC hardware assisted vitualization, as well as a global
> configuration option.
>
> Having all APIC interaction exit to Xen for emulation is slow and can
> induce much overhead. Hardware can speed up x{2}APIC by running APIC
> read/write accesses without taking a VM exit.
This is odd to read for a patch which makes it possible to _turn off_
acceleration. Instead it would be interesting to know what problems
you have encountered making it desirable to have a way to turn this off.
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3342,16 +3342,19 @@ static void vmx_install_vlapic_mapping(struct vcpu *v)
>
> void vmx_vlapic_msr_changed(struct vcpu *v)
> {
> - int virtualize_x2apic_mode;
> + int virtualize_xapic_mode, virtualize_x2apic_mode;
Please switch to bool as you touch and extend this.
> struct vlapic *vlapic = vcpu_vlapic(v);
> unsigned int msr;
>
> + virtualize_xapic_mode = ( cpu_has_vmx_virtualize_apic_accesses &&
> + v->domain->arch.hvm.assisted_xapic );
Please don't clone the bad use of blanks immediately inside parentheses
here; instead, ...
> virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt ||
> cpu_has_vmx_virtual_intr_delivery) &&
> - cpu_has_vmx_virtualize_x2apic_mode );
> + cpu_has_vmx_virtualize_x2apic_mode &&
> + v->domain->arch.hvm.assisted_x2apic );
... since you're touching this anyway, please consider correcting
the style violation here.
However - do you need these expressions anymore? The per-domain fields
can only be set if the respective CPU capabilities exit.
> --- a/xen/arch/x86/include/asm/hvm/domain.h
> +++ b/xen/arch/x86/include/asm/hvm/domain.h
> @@ -117,6 +117,12 @@ struct hvm_domain {
>
> bool is_s3_suspended;
>
> + /* xAPIC hardware assisted emulation. */
> + bool assisted_xapic;
> +
> + /* x2APIC hardware assisted emulation. */
> + bool assisted_x2apic;
> +
> /* hypervisor intercepted msix table */
> struct list_head msixtbl_list;
Please follow how adjacent code pads types / names here.
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1115,7 +1115,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
> if ( !is_hvm_domain(d) || subleaf != 0 )
> break;
>
> - if ( cpu_has_vmx_apic_reg_virt )
> + if ( cpu_has_vmx_apic_reg_virt &&
> + v->domain->arch.hvm.assisted_xapic )
> res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT;
>
> /*
> @@ -1126,7 +1127,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
> */
> if ( cpu_has_vmx_virtualize_x2apic_mode &&
> cpu_has_vmx_apic_reg_virt &&
> - cpu_has_vmx_virtual_intr_delivery )
> + cpu_has_vmx_virtual_intr_delivery &&
> + v->domain->arch.hvm.assisted_x2apic )
> res->a |= XEN_HVM_CPUID_X2APIC_VIRT;
Same remark as above - can't you now use _just_ the per-domain field?
In this latter of the two cases this would then also mean bringing
the CPU feature checks in line with what vmx_vlapic_msr_changed()
does (as also pointed out for patch 1). Albeit it might be best to
have a prereq patch fixing the issue, which could then be backported.
Jan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
2022-01-28 17:04 ` Anthony PERARD
@ 2022-01-31 15:54 ` Jane Malalane
0 siblings, 0 replies; 19+ messages in thread
From: Jane Malalane @ 2022-01-31 15:54 UTC (permalink / raw)
To: Anthony Perard
Cc: Xen-devel, Wei Liu, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Volodymyr Babchuk,
Bertrand Marquis, Jun Nakajima, Kevin Tian, Roger Pau Monne
On 28/01/2022 17:04, Anthony PERARD wrote:
> On Thu, Jan 27, 2022 at 04:01:32PM +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>
>> ---
>> tools/golang/xenlight/helpers.gen.go | 4 ++++
>> tools/golang/xenlight/types.gen.go | 6 ++++++
>
> Note for committers: Please regenerate the go bindings, there are
> out-of-sync with libxl_types.idl at the moment.
>
>> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
>> index 1feadebb18..33da51fe89 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,
>> + xc_physinfo_t xcphysinfo)
>
> It might be better to pass "xcphysinfo" as a pointer, otherwise I think
> a copy of the whole struct is made when calling this function.
>
Will correct this, thanks.
>
> In any case, the tool part of the patch looks good:
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>
>
> Thanks,
>
Jane.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
2022-01-31 12:05 ` Jan Beulich
@ 2022-01-31 16:02 ` Jane Malalane
2022-02-08 13:27 ` Jane Malalane
1 sibling, 0 replies; 19+ messages in thread
From: Jane Malalane @ 2022-01-31 16:02 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, Anthony Perard, Juergen Gross, Andrew Cooper,
George Dunlap, Julien Grall, Stefano Stabellini,
Christian Lindig, David Scott, Volodymyr Babchuk,
Roger Pau Monne, Xen-devel
Hi,
On 31/01/2022 12:05, Jan Beulich wrote:
> On 27.01.2022 17:01, Jane Malalane wrote:
>> Introduce a new per-domain creation x86 specific flag to
>> select whether hardware assisted virtualization should be used for
>> x{2}APIC.
>>
>> A per-domain option is added to xl in order to select the usage of
>> x{2}APIC hardware assisted vitualization, as well as a global
>> configuration option.
>>
>> Having all APIC interaction exit to Xen for emulation is slow and can
>> induce much overhead. Hardware can speed up x{2}APIC by running APIC
>> read/write accesses without taking a VM exit.
>
> This is odd to read for a patch which makes it possible to _turn off_
> acceleration. Instead it would be interesting to know what problems
> you have encountered making it desirable to have a way to turn this off.
Makes sense.
>
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -3342,16 +3342,19 @@ static void vmx_install_vlapic_mapping(struct vcpu *v)
>>
>> void vmx_vlapic_msr_changed(struct vcpu *v)
>> {
>> - int virtualize_x2apic_mode;
>> + int virtualize_xapic_mode, virtualize_x2apic_mode;
>
> Please switch to bool as you touch and extend this.
>
>> struct vlapic *vlapic = vcpu_vlapic(v);
>> unsigned int msr;
>>
>> + virtualize_xapic_mode = ( cpu_has_vmx_virtualize_apic_accesses &&
>> + v->domain->arch.hvm.assisted_xapic );
>
> Please don't clone the bad use of blanks immediately inside parentheses
> here; instead, ...
>
>> virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt ||
>> cpu_has_vmx_virtual_intr_delivery) &&
>> - cpu_has_vmx_virtualize_x2apic_mode );
>> + cpu_has_vmx_virtualize_x2apic_mode &&
>> + v->domain->arch.hvm.assisted_x2apic );
>
> ... since you're touching this anyway, please consider correcting
> the style violation here.
>
> However - do you need these expressions anymore? The per-domain fields
> can only be set if the respective CPU capabilities exit.
>
>> --- a/xen/arch/x86/include/asm/hvm/domain.h
>> +++ b/xen/arch/x86/include/asm/hvm/domain.h
>> @@ -117,6 +117,12 @@ struct hvm_domain {
>>
>> bool is_s3_suspended;
>>
>> + /* xAPIC hardware assisted emulation. */
>> + bool assisted_xapic;
>> +
>> + /* x2APIC hardware assisted emulation. */
>> + bool assisted_x2apic;
>> +
>> /* hypervisor intercepted msix table */
>> struct list_head msixtbl_list;
>
> Please follow how adjacent code pads types / names here.
>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1115,7 +1115,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
>> if ( !is_hvm_domain(d) || subleaf != 0 )
>> break;
>>
>> - if ( cpu_has_vmx_apic_reg_virt )
>> + if ( cpu_has_vmx_apic_reg_virt &&
>> + v->domain->arch.hvm.assisted_xapic )
>> res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT;
>>
>> /*
>> @@ -1126,7 +1127,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
>> */
>> if ( cpu_has_vmx_virtualize_x2apic_mode &&
>> cpu_has_vmx_apic_reg_virt &&
>> - cpu_has_vmx_virtual_intr_delivery )
>> + cpu_has_vmx_virtual_intr_delivery &&
>> + v->domain->arch.hvm.assisted_x2apic )
>> res->a |= XEN_HVM_CPUID_X2APIC_VIRT;
>
> Same remark as above - can't you now use _just_ the per-domain field?
> In this latter of the two cases this would then also mean bringing
> the CPU feature checks in line with what vmx_vlapic_msr_changed()
> does (as also pointed out for patch 1). Albeit it might be best to
> have a prereq patch fixing the issue, which could then be backported.
Indeed, that would be better.
Here I think the choice has been to only announce harwdare assisted
x2apic support in CPUID if both read and write accesses are virtualized.
I could either keep the x2apic_available check in Patch 1 as is and use
the per-domain field here, and keep vmx_vlapic_msr_changed as is or vv.
>
> Jan
>
>
Thank you,
Jane.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
2022-01-27 16:01 ` [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC Jane Malalane
2022-01-27 16:21 ` Christian Lindig
2022-01-31 12:05 ` Jan Beulich
@ 2022-01-31 16:05 ` Anthony PERARD
2022-01-31 16:13 ` Jan Beulich
2022-02-01 10:02 ` Roger Pau Monné
3 siblings, 1 reply; 19+ messages in thread
From: Anthony PERARD @ 2022-01-31 16:05 UTC (permalink / raw)
To: Jane Malalane
Cc: Xen-devel, Wei Liu, Juergen Gross, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Christian Lindig,
David Scott, Volodymyr Babchuk, Roger Pau Monné
On Thu, Jan 27, 2022 at 04:01:33PM +0000, Jane Malalane wrote:
> diff --git a/docs/man/xl.conf.5.pod.in b/docs/man/xl.conf.5.pod.in
> index df20c08137..2d0a59d019 100644
> --- a/docs/man/xl.conf.5.pod.in
> +++ b/docs/man/xl.conf.5.pod.in
> @@ -107,6 +107,18 @@ Sets the default value for the C<max_grant_version> domain config value.
>
> Default: maximum grant version supported by the hypervisor.
>
> +=item B<assisted_xapic=BOOLEAN>
> +
> +If enabled, domains will use xAPIC hardware assisted emulation by default.
> +
> +Default: enabled.
> +
> +=item B<assisted_x2apic=BOOLEAN>
> +
> +If enabled, domains will use x2APIC hardware assisted emulation by default.
> +
> +Default: enabled.
> +
> =item B<vif.default.script="PATH">
>
> Configures the default hotplug script used by virtual network devices.
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 52f2545498..4d422bef96 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -1384,8 +1384,9 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
> }
> }
>
> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> - libxl_domain_build_info *b_info)
> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> + libxl_domain_build_info *b_info,
> + const libxl_physinfo *physinfo)
> {
> /* ACPI is disabled by default */
> libxl_defbool_setdefault(&b_info->acpi, false);
It seems there's a missing 'return 0' in this function now ;-).
> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index d7a40d7550..2bae6fef62 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -264,7 +264,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> if (!b_info->event_channels)
> b_info->event_channels = 1023;
>
> - libxl__arch_domain_build_info_setdefault(gc, b_info);
I don't think moving this call later is a good idea. It looks like it's
going to break on ARM at least. Instead, calling libxl_get_physinfo()
earlier should be fine I think.
> libxl_defbool_setdefault(&b_info->dm_restrict, false);
>
> if (b_info->iommu_memkb == LIBXL_MEMKB_DEFAULT)
> @@ -456,15 +455,21 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> libxl_defbool_setdefault(&b_info->nested_hvm, false);
> }
>
> - if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
> - libxl_physinfo info;
> + libxl_physinfo info;
>
> - rc = libxl_get_physinfo(CTX, &info);
> - if (rc) {
> - LOG(ERROR, "failed to get hypervisor info");
> - return rc;
> - }
> + rc = libxl_get_physinfo(CTX, &info);
> + if (rc) {
> + LOG(ERROR, "failed to get hypervisor info");
> + return rc;
> + }
>
> + rc = libxl__arch_domain_build_info_setdefault(gc, b_info, &info);
> + if (rc) {
> + LOG(ERROR, "unable to set domain arch build info defaults");
> + return rc;
> + }
> +
> + if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
> if (info.cap_gnttab_v2)
> b_info->max_grant_version = 2;
> else if (info.cap_gnttab_v1)
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index 42ac6c357b..db5eb0a0b3 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -648,6 +648,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
> ("vuart", libxl_vuart_type),
> ])),
> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> + ("assisted_xapic", libxl_defbool),
> + ("assisted_x2apic", libxl_defbool),
Could you add some LIBXL_HAVE_* macro in libxl.h about those new fields
like you did in the previous patch?
> ])),
> # Alternate p2m is not bound to any architecture or guest type, as it is
> # supported by x86 HVM and ARM support is planned.
> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> index 33da51fe89..b257fca756 100644
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -819,11 +825,44 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
> {
> }
>
> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> - libxl_domain_build_info *b_info)
> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> + libxl_domain_build_info *b_info,
> + const libxl_physinfo *physinfo)
> {
> + int rc;
> + bool assisted_xapic;
> + bool assisted_x2apic;
> +
> libxl_defbool_setdefault(&b_info->acpi, true);
> libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false);
> +
> + libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, false);
> + libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, false);
> +
> + assisted_xapic = libxl_defbool_val(b_info->arch_x86.assisted_xapic);
> + assisted_x2apic = libxl_defbool_val(b_info->arch_x86.assisted_x2apic);
> +
> + if ((assisted_xapic || assisted_x2apic) &&
> + b_info->type == LIBXL_DOMAIN_TYPE_PV)
> + {
> + LOG(ERROR, "Interrupt Controller Virtualization not supported for PV");
> + rc = ERROR_INVAL;
> + goto out;
> + }
> +
> + if ((assisted_xapic && !physinfo->cap_assisted_xapic) ||
> + (assisted_x2apic && !physinfo->cap_assisted_x2apic))
> + {
> + LOG(ERROR, "x%sAPIC hardware supported emulation not available",
> + assisted_xapic && !physinfo->cap_assisted_xapic ? "" : "2");
> + rc = ERROR_INVAL;
> + goto out;
> + }
Would it make sens to enable assisted_xapic and assisted_x2apic by
default based on hardware support? That way users of libxl could benefit
from the upgrade without having to enable it. Or maybe that could causes
issues, like maybe on migration?
> +
> + rc = 0;
> +out:
> + return rc;
> +
> }
>
> int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
> diff --git a/tools/xl/xl.c b/tools/xl/xl.c
> index 2d1ec18ea3..b97e491c9c 100644
> --- a/tools/xl/xl.c
> +++ b/tools/xl/xl.c
> @@ -57,6 +57,8 @@ int max_grant_frames = -1;
> int max_maptrack_frames = -1;
> int max_grant_version = LIBXL_MAX_GRANT_DEFAULT;
> libxl_domid domid_policy = INVALID_DOMID;
> +int assisted_xapic = 0;
> +int assisted_x2apic = 0;
This seems to contradict the manual which says "Default: enabled".
I think you need one more change in ocaml bindings, its ABI checker
fails. I found the following change to work, hopefully that the right
thing to do:
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 5b4fe72c8d..0aa957d379 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -239,7 +239,7 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
cfg.arch.misc_flags = ocaml_list_to_c_bitmap
/* ! x86_arch_misc_flags X86_ none */
- /* ! XEN_X86_ XEN_X86_MSR_RELAXED all */
+ /* ! XEN_X86_ XEN_X86_ASSISTED_X2APIC max */
(VAL_MISC_FLAGS);
#undef VAL_MISC_FLAGS
Thanks,
--
Anthony PERARD
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
2022-01-31 16:05 ` Anthony PERARD
@ 2022-01-31 16:13 ` Jan Beulich
0 siblings, 0 replies; 19+ messages in thread
From: Jan Beulich @ 2022-01-31 16:13 UTC (permalink / raw)
To: Anthony PERARD, Jane Malalane
Cc: Xen-devel, Wei Liu, Juergen Gross, Andrew Cooper, George Dunlap,
Julien Grall, Stefano Stabellini, Christian Lindig, David Scott,
Volodymyr Babchuk, Roger Pau Monné
On 31.01.2022 17:05, Anthony PERARD wrote:
> On Thu, Jan 27, 2022 at 04:01:33PM +0000, Jane Malalane wrote:
>> --- a/tools/libs/light/libxl_x86.c
>> +++ b/tools/libs/light/libxl_x86.c
>> @@ -819,11 +825,44 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
>> {
>> }
>>
>> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> - libxl_domain_build_info *b_info)
>> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> + libxl_domain_build_info *b_info,
>> + const libxl_physinfo *physinfo)
>> {
>> + int rc;
>> + bool assisted_xapic;
>> + bool assisted_x2apic;
>> +
>> libxl_defbool_setdefault(&b_info->acpi, true);
>> libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false);
>> +
>> + libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, false);
>> + libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, false);
>> +
>> + assisted_xapic = libxl_defbool_val(b_info->arch_x86.assisted_xapic);
>> + assisted_x2apic = libxl_defbool_val(b_info->arch_x86.assisted_x2apic);
>> +
>> + if ((assisted_xapic || assisted_x2apic) &&
>> + b_info->type == LIBXL_DOMAIN_TYPE_PV)
>> + {
>> + LOG(ERROR, "Interrupt Controller Virtualization not supported for PV");
>> + rc = ERROR_INVAL;
>> + goto out;
>> + }
>> +
>> + if ((assisted_xapic && !physinfo->cap_assisted_xapic) ||
>> + (assisted_x2apic && !physinfo->cap_assisted_x2apic))
>> + {
>> + LOG(ERROR, "x%sAPIC hardware supported emulation not available",
>> + assisted_xapic && !physinfo->cap_assisted_xapic ? "" : "2");
>> + rc = ERROR_INVAL;
>> + goto out;
>> + }
>
> Would it make sens to enable assisted_xapic and assisted_x2apic by
> default based on hardware support? That way users of libxl could benefit
> from the upgrade without having to enable it.
I think that's the only sensible way - disabling by default would result
in perceived performance regressions, I suppose.
Jan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86
2022-01-27 16:01 ` [PATCH 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 Jane Malalane
` (2 preceding siblings ...)
2022-01-31 11:53 ` Jan Beulich
@ 2022-01-31 17:57 ` Roger Pau Monné
3 siblings, 0 replies; 19+ messages in thread
From: Roger Pau Monné @ 2022-01-31 17:57 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 Thu, Jan 27, 2022 at 04:01:32PM +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>
> ---
> 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>
> ---
> tools/golang/xenlight/helpers.gen.go | 4 ++++
> tools/golang/xenlight/types.gen.go | 6 ++++++
> tools/include/libxl.h | 14 ++++++++++++++
> tools/libs/light/libxl.c | 3 +++
> tools/libs/light/libxl_arch.h | 4 ++++
> tools/libs/light/libxl_arm.c | 4 ++++
> 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 | 6 ++++++
> xen/arch/x86/include/asm/domain.h | 4 ++++
> xen/arch/x86/sysctl.c | 7 +++++++
> xen/include/public/arch-x86/xen.h | 4 ++++
> xen/include/public/sysctl.h | 1 +
> 16 files changed, 84 insertions(+), 2 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..6d51e56704 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -528,6 +528,20 @@
> #define LIBXL_HAVE_MAX_GRANT_VERSION 1
>
> /*
> + * LIBXL_HAVE_PHYSINFO_ASSISTED_XAPIC indicates that libxl_physinfo has a
> + * cap_assisted_xapic field, which indicates the availability of xAPIC
> + * hardware assisted emulation.
> + */
> +#define LIBXL_HAVE_PHYSINFO_ASSISTED_XAPIC 1
> +
> +/*
> + * LIBXL_HAVE_PHYSINFO_ASSISTED_X2APIC indicates that libxl_physinfo has a
> + * cap_assisted_x2apic field, which indicates the availability of x2APIC
> + * hardware assisted emulation.
> + */
> +#define LIBXL_HAVE_PHYSINFO_ASSISTED_X2APIC 1
I think you could likely introduce a single define that covers both
features, as they are introduced in the same commit, ie:
LIBXL_HAVE_PHYSINFO_ASSISTED_APIC.
> +
> +/*
> * 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..1588701d19 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..00cc50394d 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,
> + 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..52f2545498 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -1431,6 +1431,10 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
> return rc;
> }
>
> +void libxl__arch_get_physinfo(libxl_physinfo *physinfo, 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..33da51fe89 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,
> + xc_physinfo_t xcphysinfo)
If you make xcphysinfo passed by reference as Anthony suggested also
make it const.
Thanks, Roger.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
2022-01-27 16:01 ` [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC Jane Malalane
` (2 preceding siblings ...)
2022-01-31 16:05 ` Anthony PERARD
@ 2022-02-01 10:02 ` Roger Pau Monné
2022-02-02 15:19 ` Jane Malalane
3 siblings, 1 reply; 19+ messages in thread
From: Roger Pau Monné @ 2022-02-01 10:02 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 Thu, Jan 27, 2022 at 04:01:33PM +0000, Jane Malalane wrote:
> Introduce a new per-domain creation x86 specific flag to
> select whether hardware assisted virtualization should be used for
> x{2}APIC.
>
> A per-domain option is added to xl in order to select the usage of
> x{2}APIC hardware assisted vitualization, as well as a global
> configuration option.
>
> Having all APIC interaction exit to Xen for emulation is slow and can
> induce much overhead. Hardware can speed up x{2}APIC by running APIC
> read/write accesses without taking a VM exit.
>
> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Wei Liu <wl@xen.org>
> CC: Anthony PERARD <anthony.perard@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: George Dunlap <george.dunlap@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Julien Grall <julien@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Christian Lindig <christian.lindig@citrix.com>
> CC: David Scott <dave@recoil.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: "Roger Pau Monné" <roger.pau@citrix.com>
> ---
> docs/man/xl.cfg.5.pod.in | 10 ++++++++
> docs/man/xl.conf.5.pod.in | 12 ++++++++++
> tools/golang/xenlight/helpers.gen.go | 12 ++++++++++
> tools/libs/light/libxl_arch.h | 5 ++--
> tools/libs/light/libxl_arm.c | 5 ++--
> tools/libs/light/libxl_create.c | 21 ++++++++++-------
> tools/libs/light/libxl_types.idl | 2 ++
> tools/libs/light/libxl_x86.c | 43 +++++++++++++++++++++++++++++++++--
> tools/ocaml/libs/xc/xenctrl.ml | 2 ++
> tools/ocaml/libs/xc/xenctrl.mli | 2 ++
> tools/xl/xl.c | 8 +++++++
> tools/xl/xl.h | 2 ++
> tools/xl/xl_parse.c | 14 ++++++++++++
> xen/arch/x86/domain.c | 27 +++++++++++++++++++++-
> xen/arch/x86/hvm/vmx/vmcs.c | 4 ++++
> xen/arch/x86/hvm/vmx/vmx.c | 13 +++++++----
> xen/arch/x86/include/asm/hvm/domain.h | 6 +++++
> xen/arch/x86/traps.c | 6 +++--
> xen/include/public/arch-x86/xen.h | 2 ++
> 19 files changed, 174 insertions(+), 22 deletions(-)
>
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index b98d161398..974fe7d2d8 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -1862,6 +1862,16 @@ firmware tables when using certain older guest Operating
> Systems. These tables have been superseded by newer constructs within
> the ACPI tables.
>
> +=item B<assisted_xapic=BOOLEAN>
> +B<(x86 only)> Enables or disables hardware assisted virtualization for xapic.
> +This allows accessing APIC registers without a VM-exit.
> +The default is settable via L<xl.conf(5)>.
> +
> +=item B<assisted_x2apic=BOOLEAN>
> +B<(x86 only)> Enables or disables hardware assisted virtualization for x2apic.
> +This allows accessing APIC registers without a VM-exit.
> +The default is settable via L<xl.conf(5)>.
Like you do below I would capitalize xAPIC and x2APIC in the option
text.
> +
> =item B<nx=BOOLEAN>
>
> B<(x86 only)> Hides or exposes the No-eXecute capability. This allows a guest
> diff --git a/docs/man/xl.conf.5.pod.in b/docs/man/xl.conf.5.pod.in
> index df20c08137..2d0a59d019 100644
> --- a/docs/man/xl.conf.5.pod.in
> +++ b/docs/man/xl.conf.5.pod.in
> @@ -107,6 +107,18 @@ Sets the default value for the C<max_grant_version> domain config value.
>
> Default: maximum grant version supported by the hypervisor.
>
> +=item B<assisted_xapic=BOOLEAN>
> +
> +If enabled, domains will use xAPIC hardware assisted emulation by default.
> +
> +Default: enabled.
> +
> +=item B<assisted_x2apic=BOOLEAN>
> +
> +If enabled, domains will use x2APIC hardware assisted emulation by default.
> +
> +Default: enabled.
I think for both options this should be:
Default: enabled if supported.
> +
> =item B<vif.default.script="PATH">
>
> Configures the default hotplug script used by virtual network devices.
> diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
> index dd4e6c9f14..90e7b9b205 100644
> --- a/tools/golang/xenlight/helpers.gen.go
> +++ b/tools/golang/xenlight/helpers.gen.go
> @@ -636,6 +636,12 @@ x.Passthrough = Passthrough(xc.passthrough)
> if err := x.XendSuspendEvtchnCompat.fromC(&xc.xend_suspend_evtchn_compat);err != nil {
> return fmt.Errorf("converting field XendSuspendEvtchnCompat: %v", err)
> }
> +if err := x.ArchX86.AssistedXapic.fromC(&xc.arch_x86.assisted_xapic);err != nil {
> +return fmt.Errorf("converting field ArchX86.AssistedXapic: %v", err)
> +}
> +if err := x.ArchX86.AssistedX2Apic.fromC(&xc.arch_x86.assisted_x2apic);err != nil {
> +return fmt.Errorf("converting field ArchX86.AssistedX2Apic: %v", err)
> +}
>
> return nil}
>
> @@ -679,6 +685,12 @@ xc.passthrough = C.libxl_passthrough(x.Passthrough)
> if err := x.XendSuspendEvtchnCompat.toC(&xc.xend_suspend_evtchn_compat); err != nil {
> return fmt.Errorf("converting field XendSuspendEvtchnCompat: %v", err)
> }
> +if err := x.ArchX86.AssistedXapic.toC(&xc.arch_x86.assisted_xapic); err != nil {
> +return fmt.Errorf("converting field ArchX86.AssistedXapic: %v", err)
> +}
> +if err := x.ArchX86.AssistedX2Apic.toC(&xc.arch_x86.assisted_x2apic); err != nil {
> +return fmt.Errorf("converting field ArchX86.AssistedX2Apic: %v", err)
> +}
>
> return nil
> }
> diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h
> index 00cc50394d..2eaff45526 100644
> --- a/tools/libs/light/libxl_arch.h
> +++ b/tools/libs/light/libxl_arch.h
> @@ -71,8 +71,9 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
> libxl_domain_create_info *c_info);
>
> _hidden
> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> - libxl_domain_build_info *b_info);
> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> + libxl_domain_build_info *b_info,
> + const libxl_physinfo *physinfo);
>
> _hidden
> int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 52f2545498..4d422bef96 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -1384,8 +1384,9 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
> }
> }
>
> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> - libxl_domain_build_info *b_info)
> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> + libxl_domain_build_info *b_info,
> + const libxl_physinfo *physinfo)
> {
> /* ACPI is disabled by default */
> libxl_defbool_setdefault(&b_info->acpi, false);
> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index d7a40d7550..2bae6fef62 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -264,7 +264,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> if (!b_info->event_channels)
> b_info->event_channels = 1023;
>
> - libxl__arch_domain_build_info_setdefault(gc, b_info);
> libxl_defbool_setdefault(&b_info->dm_restrict, false);
>
> if (b_info->iommu_memkb == LIBXL_MEMKB_DEFAULT)
> @@ -456,15 +455,21 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> libxl_defbool_setdefault(&b_info->nested_hvm, false);
> }
>
> - if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
> - libxl_physinfo info;
> + libxl_physinfo info;
>
> - rc = libxl_get_physinfo(CTX, &info);
> - if (rc) {
> - LOG(ERROR, "failed to get hypervisor info");
> - return rc;
> - }
> + rc = libxl_get_physinfo(CTX, &info);
> + if (rc) {
> + LOG(ERROR, "failed to get hypervisor info");
> + return rc;
> + }
>
> + rc = libxl__arch_domain_build_info_setdefault(gc, b_info, &info);
> + if (rc) {
> + LOG(ERROR, "unable to set domain arch build info defaults");
> + return rc;
> + }
> +
> + if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
> if (info.cap_gnttab_v2)
> b_info->max_grant_version = 2;
> else if (info.cap_gnttab_v1)
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index 42ac6c357b..db5eb0a0b3 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -648,6 +648,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
> ("vuart", libxl_vuart_type),
> ])),
> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
> + ("assisted_xapic", libxl_defbool),
> + ("assisted_x2apic", libxl_defbool),
> ])),
> # Alternate p2m is not bound to any architecture or guest type, as it is
> # supported by x86 HVM and ARM support is planned.
> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> index 33da51fe89..b257fca756 100644
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -23,6 +23,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> if (libxl_defbool_val(d_config->b_info.arch_x86.msr_relaxed))
> config->arch.misc_flags |= XEN_X86_MSR_RELAXED;
>
> + if(libxl_defbool_val(d_config->b_info.arch_x86.assisted_xapic))
> + config->arch.misc_flags |= XEN_X86_ASSISTED_XAPIC;
> +
> + if(libxl_defbool_val(d_config->b_info.arch_x86.assisted_x2apic))
> + config->arch.misc_flags |= XEN_X86_ASSISTED_X2APIC;
> +
> return 0;
> }
>
> @@ -819,11 +825,44 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
> {
> }
>
> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> - libxl_domain_build_info *b_info)
> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> + libxl_domain_build_info *b_info,
> + const libxl_physinfo *physinfo)
> {
> + int rc;
> + bool assisted_xapic;
> + bool assisted_x2apic;
> +
> libxl_defbool_setdefault(&b_info->acpi, true);
> libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false);
> +
> + libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, false);
> + libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, false);
> +
> + assisted_xapic = libxl_defbool_val(b_info->arch_x86.assisted_xapic);
> + assisted_x2apic = libxl_defbool_val(b_info->arch_x86.assisted_x2apic);
> +
> + if ((assisted_xapic || assisted_x2apic) &&
> + b_info->type == LIBXL_DOMAIN_TYPE_PV)
> + {
> + LOG(ERROR, "Interrupt Controller Virtualization not supported for PV");
> + rc = ERROR_INVAL;
> + goto out;
> + }
> +
> + if ((assisted_xapic && !physinfo->cap_assisted_xapic) ||
> + (assisted_x2apic && !physinfo->cap_assisted_x2apic))
> + {
> + LOG(ERROR, "x%sAPIC hardware supported emulation not available",
> + assisted_xapic && !physinfo->cap_assisted_xapic ? "" : "2");
> + rc = ERROR_INVAL;
> + goto out;
> + }
I think the logic here is slightly wrong, as you are setting the
default value of assisted_x{2}apic to false, and we would instead like
to set it to the current value supported by the hardware in order to
keep current behavior.
Also the options are HVM/PVH only, so having them set for PV should
result in an error regardless of the set value, ie:
if (b_info->type == LIBXL_DOMAIN_TYPE_PV &&
(!libxl_defbool_is_default(&b_info->arch_x86.assisted_xapic) ||
!libxl_defbool_is_default(&b_info->arch_x86.assisted_x2apic)))
ERROR
libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic,
physinfo->cap_assisted_xapic);
libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic,
physinfo->cap_assisted_x2apic);
I don't think you need the local assisted_x{2}apic variables.
> +
> + rc = 0;
> +out:
> + return rc;
The out label is not really needed here and makes the code longer.
Just 'return ERROR_INVAL' in the error paths or 0 at the end of the
function. You can then also drop the local rc variable.
> +
> }
>
> int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> index 7ce832d605..cce30d8731 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -50,6 +50,8 @@ type x86_arch_emulation_flags =
>
> type x86_arch_misc_flags =
> | X86_MSR_RELAXED
> + | X86_ASSISTED_XAPIC
> + | X86_ASSISTED_X2APIC
>
> type xen_x86_arch_domainconfig =
> {
> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
> index a2b15130ee..67a22ec15c 100644
> --- a/tools/ocaml/libs/xc/xenctrl.mli
> +++ b/tools/ocaml/libs/xc/xenctrl.mli
> @@ -44,6 +44,8 @@ type x86_arch_emulation_flags =
>
> type x86_arch_misc_flags =
> | X86_MSR_RELAXED
> + | X86_ASSISTED_XAPIC
> + | X86_ASSISTED_X2APIC
>
> type xen_x86_arch_domainconfig = {
> emulation_flags: x86_arch_emulation_flags list;
> diff --git a/tools/xl/xl.c b/tools/xl/xl.c
> index 2d1ec18ea3..b97e491c9c 100644
> --- a/tools/xl/xl.c
> +++ b/tools/xl/xl.c
> @@ -57,6 +57,8 @@ int max_grant_frames = -1;
> int max_maptrack_frames = -1;
> int max_grant_version = LIBXL_MAX_GRANT_DEFAULT;
> libxl_domid domid_policy = INVALID_DOMID;
> +int assisted_xapic = 0;
> +int assisted_x2apic = 0;
This should be initialized to -1, in order to denote the values are
unset...
>
> xentoollog_level minmsglevel = minmsglevel_default;
>
> @@ -201,6 +203,12 @@ static void parse_global_config(const char *configfile,
> if (!xlu_cfg_get_long (config, "claim_mode", &l, 0))
> claim_mode = l;
>
> + if (!xlu_cfg_get_long (config, "assisted_xapic", &l, 0))
> + assisted_xapic = l;
> +
> + if (!xlu_cfg_get_long (config, "assisted_x2apic", &l, 0))
> + assisted_x2apic = l;
> +
> xlu_cfg_replace_string (config, "remus.default.netbufscript",
> &default_remus_netbufscript, 0);
> xlu_cfg_replace_string (config, "colo.default.proxyscript",
> diff --git a/tools/xl/xl.h b/tools/xl/xl.h
> index c5c4bedbdd..528deb3feb 100644
> --- a/tools/xl/xl.h
> +++ b/tools/xl/xl.h
> @@ -286,6 +286,8 @@ extern libxl_bitmap global_vm_affinity_mask;
> extern libxl_bitmap global_hvm_affinity_mask;
> extern libxl_bitmap global_pv_affinity_mask;
> extern libxl_domid domid_policy;
> +extern int assisted_xapic;
> +extern int assisted_x2apic;
>
> enum output_format {
> OUTPUT_FORMAT_JSON,
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 117fcdcb2b..16ff9e76bc 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1681,6 +1681,20 @@ void parse_config_data(const char *config_source,
> xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align, 0);
> xlu_cfg_get_defbool(config, "apic", &b_info->apic, 0);
>
> + e = xlu_cfg_get_defbool(config, "assisted_xapic",
> + &b_info->arch_x86.assisted_xapic, 0);
> + if (e == ESRCH) /* not specified */
> + libxl_defbool_set(&b_info->arch_x86.assisted_xapic, assisted_xapic);
...because here you only want to use the global values if they have
actually been set by the user (assisted_x{2}apic != -1):
e = xlu_cfg_get_defbool(config, "assisted_xapic",
&b_info->arch_x86.assisted_xapic, 0);
if (e == ESRCH && assisted_xapic != -1) /* use global default if present */
libxl_defbool_set(&b_info->arch_x86.assisted_xapic, assisted_xapic);
else if (e)
exit(1);
> + else if (e)
> + exit(1);
> +
> + e = xlu_cfg_get_defbool(config, "assisted_x2apic",
> + &b_info->arch_x86.assisted_x2apic, 0);
> + if (e == ESRCH) /* not specified */
> + libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, assisted_x2apic);
> + else if (e)
> + exit(1);
> +
> switch (xlu_cfg_get_list(config, "viridian",
> &viridian, &num_viridian, 1))
> {
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index ef1812dc14..d08f51e28b 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -619,6 +619,8 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
> bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
> bool hap = config->flags & XEN_DOMCTL_CDF_hap;
> bool nested_virt = config->flags & XEN_DOMCTL_CDF_nested_virt;
> + bool assisted_xapic = config->arch.misc_flags & XEN_X86_ASSISTED_XAPIC;
> + bool assisted_x2apic = config->arch.misc_flags & XEN_X86_ASSISTED_X2APIC;
> unsigned int max_vcpus;
>
> if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
> @@ -685,13 +687,30 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
> }
> }
>
> - if ( config->arch.misc_flags & ~XEN_X86_MSR_RELAXED )
> + if ( config->arch.misc_flags & ~(XEN_X86_MSR_RELAXED |
> + XEN_X86_ASSISTED_XAPIC |
> + XEN_X86_ASSISTED_X2APIC) )
> {
> dprintk(XENLOG_INFO, "Invalid arch misc flags %#x\n",
> config->arch.misc_flags);
> return -EINVAL;
> }
>
> + if ( (assisted_xapic || assisted_x2apic) && !hvm )
> + {
> + dprintk(XENLOG_INFO,
> + "Interrupt Controller Virtualization not supported for PV\n");
> + return -EINVAL;
> + }
> +
> + if ( (assisted_xapic && !assisted_xapic_available) ||
> + (assisted_x2apic && !assisted_x2apic_available) )
> + {
> + dprintk(XENLOG_INFO, "x%sAPIC requested but not available\n",
This should be a little bit more concise, as Xen does always offer
a fully software virtualized x{2}APIC.
"hardware assisted x%sAPIC requested but not available\n"
Thanks, Roger.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
2022-02-01 10:02 ` Roger Pau Monné
@ 2022-02-02 15:19 ` Jane Malalane
2022-02-02 16:21 ` Roger Pau Monné
0 siblings, 1 reply; 19+ messages in thread
From: Jane Malalane @ 2022-02-02 15:19 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 01/02/2022 10:02, Roger Pau Monné wrote:
> On Thu, Jan 27, 2022 at 04:01:33PM +0000, Jane Malalane wrote:
>> Introduce a new per-domain creation x86 specific flag to
>> select whether hardware assisted virtualization should be used for
>> x{2}APIC.
>>
>> A per-domain option is added to xl in order to select the usage of
>> x{2}APIC hardware assisted vitualization, as well as a global
>> configuration option.
>>
>> Having all APIC interaction exit to Xen for emulation is slow and can
>> induce much overhead. Hardware can speed up x{2}APIC by running APIC
>> read/write accesses without taking a VM exit.
>>
>> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Wei Liu <wl@xen.org>
>> CC: Anthony PERARD <anthony.perard@citrix.com>
>> CC: Juergen Gross <jgross@suse.com>
>> CC: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: George Dunlap <george.dunlap@citrix.com>
>> CC: Jan Beulich <jbeulich@suse.com>
>> CC: Julien Grall <julien@xen.org>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Christian Lindig <christian.lindig@citrix.com>
>> CC: David Scott <dave@recoil.org>
>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> CC: "Roger Pau Monné" <roger.pau@citrix.com>
>> ---
>> docs/man/xl.cfg.5.pod.in | 10 ++++++++
>> docs/man/xl.conf.5.pod.in | 12 ++++++++++
>> tools/golang/xenlight/helpers.gen.go | 12 ++++++++++
>> tools/libs/light/libxl_arch.h | 5 ++--
>> tools/libs/light/libxl_arm.c | 5 ++--
>> tools/libs/light/libxl_create.c | 21 ++++++++++-------
>> tools/libs/light/libxl_types.idl | 2 ++
>> tools/libs/light/libxl_x86.c | 43 +++++++++++++++++++++++++++++++++--
>> tools/ocaml/libs/xc/xenctrl.ml | 2 ++
>> tools/ocaml/libs/xc/xenctrl.mli | 2 ++
>> tools/xl/xl.c | 8 +++++++
>> tools/xl/xl.h | 2 ++
>> tools/xl/xl_parse.c | 14 ++++++++++++
>> xen/arch/x86/domain.c | 27 +++++++++++++++++++++-
>> xen/arch/x86/hvm/vmx/vmcs.c | 4 ++++
>> xen/arch/x86/hvm/vmx/vmx.c | 13 +++++++----
>> xen/arch/x86/include/asm/hvm/domain.h | 6 +++++
>> xen/arch/x86/traps.c | 6 +++--
>> xen/include/public/arch-x86/xen.h | 2 ++
>> 19 files changed, 174 insertions(+), 22 deletions(-)
>>
>> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
>> index b98d161398..974fe7d2d8 100644
>> --- a/docs/man/xl.cfg.5.pod.in
>> +++ b/docs/man/xl.cfg.5.pod.in
>> @@ -1862,6 +1862,16 @@ firmware tables when using certain older guest Operating
>> Systems. These tables have been superseded by newer constructs within
>> the ACPI tables.
>>
>> +=item B<assisted_xapic=BOOLEAN>
>> +B<(x86 only)> Enables or disables hardware assisted virtualization for xapic.
>> +This allows accessing APIC registers without a VM-exit.
>> +The default is settable via L<xl.conf(5)>.
>> +
>> +=item B<assisted_x2apic=BOOLEAN>
>> +B<(x86 only)> Enables or disables hardware assisted virtualization for x2apic.
>> +This allows accessing APIC registers without a VM-exit.
>> +The default is settable via L<xl.conf(5)>.
>
> Like you do below I would capitalize xAPIC and x2APIC in the option
> text.
>
>> +
>> =item B<nx=BOOLEAN>
>>
>> B<(x86 only)> Hides or exposes the No-eXecute capability. This allows a guest
>> diff --git a/docs/man/xl.conf.5.pod.in b/docs/man/xl.conf.5.pod.in
>> index df20c08137..2d0a59d019 100644
>> --- a/docs/man/xl.conf.5.pod.in
>> +++ b/docs/man/xl.conf.5.pod.in
>> @@ -107,6 +107,18 @@ Sets the default value for the C<max_grant_version> domain config value.
>>
>> Default: maximum grant version supported by the hypervisor.
>>
>> +=item B<assisted_xapic=BOOLEAN>
>> +
>> +If enabled, domains will use xAPIC hardware assisted emulation by default.
>> +
>> +Default: enabled.
>> +
>> +=item B<assisted_x2apic=BOOLEAN>
>> +
>> +If enabled, domains will use x2APIC hardware assisted emulation by default.
>> +
>> +Default: enabled.
>
> I think for both options this should be:
>
> Default: enabled if supported.
>
>> +
>> =item B<vif.default.script="PATH">
>>
>> Configures the default hotplug script used by virtual network devices.
>> diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
>> index dd4e6c9f14..90e7b9b205 100644
>> --- a/tools/golang/xenlight/helpers.gen.go
>> +++ b/tools/golang/xenlight/helpers.gen.go
>> @@ -636,6 +636,12 @@ x.Passthrough = Passthrough(xc.passthrough)
>> if err := x.XendSuspendEvtchnCompat.fromC(&xc.xend_suspend_evtchn_compat);err != nil {
>> return fmt.Errorf("converting field XendSuspendEvtchnCompat: %v", err)
>> }
>> +if err := x.ArchX86.AssistedXapic.fromC(&xc.arch_x86.assisted_xapic);err != nil {
>> +return fmt.Errorf("converting field ArchX86.AssistedXapic: %v", err)
>> +}
>> +if err := x.ArchX86.AssistedX2Apic.fromC(&xc.arch_x86.assisted_x2apic);err != nil {
>> +return fmt.Errorf("converting field ArchX86.AssistedX2Apic: %v", err)
>> +}
>>
>> return nil}
>>
>> @@ -679,6 +685,12 @@ xc.passthrough = C.libxl_passthrough(x.Passthrough)
>> if err := x.XendSuspendEvtchnCompat.toC(&xc.xend_suspend_evtchn_compat); err != nil {
>> return fmt.Errorf("converting field XendSuspendEvtchnCompat: %v", err)
>> }
>> +if err := x.ArchX86.AssistedXapic.toC(&xc.arch_x86.assisted_xapic); err != nil {
>> +return fmt.Errorf("converting field ArchX86.AssistedXapic: %v", err)
>> +}
>> +if err := x.ArchX86.AssistedX2Apic.toC(&xc.arch_x86.assisted_x2apic); err != nil {
>> +return fmt.Errorf("converting field ArchX86.AssistedX2Apic: %v", err)
>> +}
>>
>> return nil
>> }
>> diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h
>> index 00cc50394d..2eaff45526 100644
>> --- a/tools/libs/light/libxl_arch.h
>> +++ b/tools/libs/light/libxl_arch.h
>> @@ -71,8 +71,9 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
>> libxl_domain_create_info *c_info);
>>
>> _hidden
>> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> - libxl_domain_build_info *b_info);
>> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> + libxl_domain_build_info *b_info,
>> + const libxl_physinfo *physinfo);
>>
>> _hidden
>> int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 52f2545498..4d422bef96 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -1384,8 +1384,9 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
>> }
>> }
>>
>> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> - libxl_domain_build_info *b_info)
>> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> + libxl_domain_build_info *b_info,
>> + const libxl_physinfo *physinfo)
>> {
>> /* ACPI is disabled by default */
>> libxl_defbool_setdefault(&b_info->acpi, false);
>> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
>> index d7a40d7550..2bae6fef62 100644
>> --- a/tools/libs/light/libxl_create.c
>> +++ b/tools/libs/light/libxl_create.c
>> @@ -264,7 +264,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>> if (!b_info->event_channels)
>> b_info->event_channels = 1023;
>>
>> - libxl__arch_domain_build_info_setdefault(gc, b_info);
>> libxl_defbool_setdefault(&b_info->dm_restrict, false);
>>
>> if (b_info->iommu_memkb == LIBXL_MEMKB_DEFAULT)
>> @@ -456,15 +455,21 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>> libxl_defbool_setdefault(&b_info->nested_hvm, false);
>> }
>>
>> - if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
>> - libxl_physinfo info;
>> + libxl_physinfo info;
>>
>> - rc = libxl_get_physinfo(CTX, &info);
>> - if (rc) {
>> - LOG(ERROR, "failed to get hypervisor info");
>> - return rc;
>> - }
>> + rc = libxl_get_physinfo(CTX, &info);
>> + if (rc) {
>> + LOG(ERROR, "failed to get hypervisor info");
>> + return rc;
>> + }
>>
>> + rc = libxl__arch_domain_build_info_setdefault(gc, b_info, &info);
>> + if (rc) {
>> + LOG(ERROR, "unable to set domain arch build info defaults");
>> + return rc;
>> + }
>> +
>> + if (b_info->max_grant_version == LIBXL_MAX_GRANT_DEFAULT) {
>> if (info.cap_gnttab_v2)
>> b_info->max_grant_version = 2;
>> else if (info.cap_gnttab_v1)
>> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>> index 42ac6c357b..db5eb0a0b3 100644
>> --- a/tools/libs/light/libxl_types.idl
>> +++ b/tools/libs/light/libxl_types.idl
>> @@ -648,6 +648,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
>> ("vuart", libxl_vuart_type),
>> ])),
>> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>> + ("assisted_xapic", libxl_defbool),
>> + ("assisted_x2apic", libxl_defbool),
>> ])),
>> # Alternate p2m is not bound to any architecture or guest type, as it is
>> # supported by x86 HVM and ARM support is planned.
>> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
>> index 33da51fe89..b257fca756 100644
>> --- a/tools/libs/light/libxl_x86.c
>> +++ b/tools/libs/light/libxl_x86.c
>> @@ -23,6 +23,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>> if (libxl_defbool_val(d_config->b_info.arch_x86.msr_relaxed))
>> config->arch.misc_flags |= XEN_X86_MSR_RELAXED;
>>
>> + if(libxl_defbool_val(d_config->b_info.arch_x86.assisted_xapic))
>> + config->arch.misc_flags |= XEN_X86_ASSISTED_XAPIC;
>> +
>> + if(libxl_defbool_val(d_config->b_info.arch_x86.assisted_x2apic))
>> + config->arch.misc_flags |= XEN_X86_ASSISTED_X2APIC;
>> +
>> return 0;
>> }
>>
>> @@ -819,11 +825,44 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
>> {
>> }
>>
>> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> - libxl_domain_build_info *b_info)
>> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>> + libxl_domain_build_info *b_info,
>> + const libxl_physinfo *physinfo)
>> {
>> + int rc;
>> + bool assisted_xapic;
>> + bool assisted_x2apic;
>> +
>> libxl_defbool_setdefault(&b_info->acpi, true);
>> libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false);
>> +
>> + libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, false);
>> + libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, false);
>> +
>> + assisted_xapic = libxl_defbool_val(b_info->arch_x86.assisted_xapic);
>> + assisted_x2apic = libxl_defbool_val(b_info->arch_x86.assisted_x2apic);
>> +
>> + if ((assisted_xapic || assisted_x2apic) &&
>> + b_info->type == LIBXL_DOMAIN_TYPE_PV)
>> + {
>> + LOG(ERROR, "Interrupt Controller Virtualization not supported for PV");
>> + rc = ERROR_INVAL;
>> + goto out;
>> + }
>> +
>> + if ((assisted_xapic && !physinfo->cap_assisted_xapic) ||
>> + (assisted_x2apic && !physinfo->cap_assisted_x2apic))
>> + {
>> + LOG(ERROR, "x%sAPIC hardware supported emulation not available",
>> + assisted_xapic && !physinfo->cap_assisted_xapic ? "" : "2");
>> + rc = ERROR_INVAL;
>> + goto out;
>> + }
>
> I think the logic here is slightly wrong, as you are setting the
> default value of assisted_x{2}apic to false, and we would instead like
> to set it to the current value supported by the hardware in order to
> keep current behavior.
>
> Also the options are HVM/PVH only, so having them set for PV should
> result in an error regardless of the set value, ie:
>
> if (b_info->type == LIBXL_DOMAIN_TYPE_PV &&
> (!libxl_defbool_is_default(&b_info->arch_x86.assisted_xapic) ||
> !libxl_defbool_is_default(&b_info->arch_x86.assisted_x2apic)))
> ERROR
>
> libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic,
> physinfo->cap_assisted_xapic);
> libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic,
> physinfo->cap_assisted_x2apic);
>
> I don't think you need the local assisted_x{2}apic variables.
Makes sense. In that case, could I instead just have this?
if (b_info->type != LIBXL_DOMAIN_TYPE_PV)
{
if (physinfo->cap_assisted_xapic)
libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, true);
if (physinfo->cap_assisted_x2apic)
libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, true);
}
Or do i still need to also check that assisted_x{2}apic hasn't been set
elsewhere for PV domains, in which case, I'm happy to add the code you
proposed above with this code I have here too.
>
>> +
>> + rc = 0;
>> +out:
>> + return rc;
>
> The out label is not really needed here and makes the code longer.
> Just 'return ERROR_INVAL' in the error paths or 0 at the end of the
> function. You can then also drop the local rc variable.
>
>> +
>> }
>>
>> int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
>> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
>> index 7ce832d605..cce30d8731 100644
>> --- a/tools/ocaml/libs/xc/xenctrl.ml
>> +++ b/tools/ocaml/libs/xc/xenctrl.ml
>> @@ -50,6 +50,8 @@ type x86_arch_emulation_flags =
>>
>> type x86_arch_misc_flags =
>> | X86_MSR_RELAXED
>> + | X86_ASSISTED_XAPIC
>> + | X86_ASSISTED_X2APIC
>>
>> type xen_x86_arch_domainconfig =
>> {
>> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
>> index a2b15130ee..67a22ec15c 100644
>> --- a/tools/ocaml/libs/xc/xenctrl.mli
>> +++ b/tools/ocaml/libs/xc/xenctrl.mli
>> @@ -44,6 +44,8 @@ type x86_arch_emulation_flags =
>>
>> type x86_arch_misc_flags =
>> | X86_MSR_RELAXED
>> + | X86_ASSISTED_XAPIC
>> + | X86_ASSISTED_X2APIC
>>
>> type xen_x86_arch_domainconfig = {
>> emulation_flags: x86_arch_emulation_flags list;
>> diff --git a/tools/xl/xl.c b/tools/xl/xl.c
>> index 2d1ec18ea3..b97e491c9c 100644
>> --- a/tools/xl/xl.c
>> +++ b/tools/xl/xl.c
>> @@ -57,6 +57,8 @@ int max_grant_frames = -1;
>> int max_maptrack_frames = -1;
>> int max_grant_version = LIBXL_MAX_GRANT_DEFAULT;
>> libxl_domid domid_policy = INVALID_DOMID;
>> +int assisted_xapic = 0;
>> +int assisted_x2apic = 0;
>
> This should be initialized to -1, in order to denote the values are
> unset...
>
>>
>> xentoollog_level minmsglevel = minmsglevel_default;
>>
>> @@ -201,6 +203,12 @@ static void parse_global_config(const char *configfile,
>> if (!xlu_cfg_get_long (config, "claim_mode", &l, 0))
>> claim_mode = l;
>>
>> + if (!xlu_cfg_get_long (config, "assisted_xapic", &l, 0))
>> + assisted_xapic = l;
>> +
>> + if (!xlu_cfg_get_long (config, "assisted_x2apic", &l, 0))
>> + assisted_x2apic = l;
>> +
>> xlu_cfg_replace_string (config, "remus.default.netbufscript",
>> &default_remus_netbufscript, 0);
>> xlu_cfg_replace_string (config, "colo.default.proxyscript",
>> diff --git a/tools/xl/xl.h b/tools/xl/xl.h
>> index c5c4bedbdd..528deb3feb 100644
>> --- a/tools/xl/xl.h
>> +++ b/tools/xl/xl.h
>> @@ -286,6 +286,8 @@ extern libxl_bitmap global_vm_affinity_mask;
>> extern libxl_bitmap global_hvm_affinity_mask;
>> extern libxl_bitmap global_pv_affinity_mask;
>> extern libxl_domid domid_policy;
>> +extern int assisted_xapic;
>> +extern int assisted_x2apic;
>>
>> enum output_format {
>> OUTPUT_FORMAT_JSON,
>> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>> index 117fcdcb2b..16ff9e76bc 100644
>> --- a/tools/xl/xl_parse.c
>> +++ b/tools/xl/xl_parse.c
>> @@ -1681,6 +1681,20 @@ void parse_config_data(const char *config_source,
>> xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align, 0);
>> xlu_cfg_get_defbool(config, "apic", &b_info->apic, 0);
>>
>> + e = xlu_cfg_get_defbool(config, "assisted_xapic",
>> + &b_info->arch_x86.assisted_xapic, 0);
>> + if (e == ESRCH) /* not specified */
>> + libxl_defbool_set(&b_info->arch_x86.assisted_xapic, assisted_xapic);
>
> ...because here you only want to use the global values if they have
> actually been set by the user (assisted_x{2}apic != -1):
>
> e = xlu_cfg_get_defbool(config, "assisted_xapic",
> &b_info->arch_x86.assisted_xapic, 0);
> if (e == ESRCH && assisted_xapic != -1) /* use global default if present */
> libxl_defbool_set(&b_info->arch_x86.assisted_xapic, assisted_xapic);
> else if (e)
> exit(1);
Oh right I see.
>
>> + else if (e)
>> + exit(1);
>> +
>> + e = xlu_cfg_get_defbool(config, "assisted_x2apic",
>> + &b_info->arch_x86.assisted_x2apic, 0);
>> + if (e == ESRCH) /* not specified */
>> + libxl_defbool_set(&b_info->arch_x86.assisted_x2apic, assisted_x2apic);
>> + else if (e)
>> + exit(1);
>> +
>> switch (xlu_cfg_get_list(config, "viridian",
>> &viridian, &num_viridian, 1))
>> {
>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index ef1812dc14..d08f51e28b 100644
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -619,6 +619,8 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>> bool hvm = config->flags & XEN_DOMCTL_CDF_hvm;
>> bool hap = config->flags & XEN_DOMCTL_CDF_hap;
>> bool nested_virt = config->flags & XEN_DOMCTL_CDF_nested_virt;
>> + bool assisted_xapic = config->arch.misc_flags & XEN_X86_ASSISTED_XAPIC;
>> + bool assisted_x2apic = config->arch.misc_flags & XEN_X86_ASSISTED_X2APIC;
>> unsigned int max_vcpus;
>>
>> if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
>> @@ -685,13 +687,30 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>> }
>> }
>>
>> - if ( config->arch.misc_flags & ~XEN_X86_MSR_RELAXED )
>> + if ( config->arch.misc_flags & ~(XEN_X86_MSR_RELAXED |
>> + XEN_X86_ASSISTED_XAPIC |
>> + XEN_X86_ASSISTED_X2APIC) )
>> {
>> dprintk(XENLOG_INFO, "Invalid arch misc flags %#x\n",
>> config->arch.misc_flags);
>> return -EINVAL;
>> }
>>
>> + if ( (assisted_xapic || assisted_x2apic) && !hvm )
>> + {
>> + dprintk(XENLOG_INFO,
>> + "Interrupt Controller Virtualization not supported for PV\n");
>> + return -EINVAL;
>> + }
>> +
>> + if ( (assisted_xapic && !assisted_xapic_available) ||
>> + (assisted_x2apic && !assisted_x2apic_available) )
>> + {
>> + dprintk(XENLOG_INFO, "x%sAPIC requested but not available\n",
>
> This should be a little bit more concise, as Xen does always offer
> a fully software virtualized x{2}APIC.
>
> "hardware assisted x%sAPIC requested but not available\n"
>
> Thanks, Roger.
Thank you for your comments.
Jane.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
2022-02-02 15:19 ` Jane Malalane
@ 2022-02-02 16:21 ` Roger Pau Monné
0 siblings, 0 replies; 19+ messages in thread
From: Roger Pau Monné @ 2022-02-02 16:21 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 02, 2022 at 03:19:13PM +0000, Jane Malalane wrote:
> On 01/02/2022 10:02, Roger Pau Monné wrote:
> > On Thu, Jan 27, 2022 at 04:01:33PM +0000, Jane Malalane wrote:
> >> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> >> index 42ac6c357b..db5eb0a0b3 100644
> >> --- a/tools/libs/light/libxl_types.idl
> >> +++ b/tools/libs/light/libxl_types.idl
> >> @@ -819,11 +825,44 @@ void libxl__arch_domain_create_info_setdefault(libxl__gc *gc,
> >> {
> >> }
> >>
> >> -void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> >> - libxl_domain_build_info *b_info)
> >> +int libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
> >> + libxl_domain_build_info *b_info,
> >> + const libxl_physinfo *physinfo)
> >> {
> >> + int rc;
> >> + bool assisted_xapic;
> >> + bool assisted_x2apic;
> >> +
> >> libxl_defbool_setdefault(&b_info->acpi, true);
> >> libxl_defbool_setdefault(&b_info->arch_x86.msr_relaxed, false);
> >> +
> >> + libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, false);
> >> + libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, false);
> >> +
> >> + assisted_xapic = libxl_defbool_val(b_info->arch_x86.assisted_xapic);
> >> + assisted_x2apic = libxl_defbool_val(b_info->arch_x86.assisted_x2apic);
> >> +
> >> + if ((assisted_xapic || assisted_x2apic) &&
> >> + b_info->type == LIBXL_DOMAIN_TYPE_PV)
> >> + {
> >> + LOG(ERROR, "Interrupt Controller Virtualization not supported for PV");
> >> + rc = ERROR_INVAL;
> >> + goto out;
> >> + }
> >> +
> >> + if ((assisted_xapic && !physinfo->cap_assisted_xapic) ||
> >> + (assisted_x2apic && !physinfo->cap_assisted_x2apic))
> >> + {
> >> + LOG(ERROR, "x%sAPIC hardware supported emulation not available",
> >> + assisted_xapic && !physinfo->cap_assisted_xapic ? "" : "2");
> >> + rc = ERROR_INVAL;
> >> + goto out;
> >> + }
> >
> > I think the logic here is slightly wrong, as you are setting the
> > default value of assisted_x{2}apic to false, and we would instead like
> > to set it to the current value supported by the hardware in order to
> > keep current behavior.
> >
> > Also the options are HVM/PVH only, so having them set for PV should
> > result in an error regardless of the set value, ie:
> >
> > if (b_info->type == LIBXL_DOMAIN_TYPE_PV &&
> > (!libxl_defbool_is_default(&b_info->arch_x86.assisted_xapic) ||
> > !libxl_defbool_is_default(&b_info->arch_x86.assisted_x2apic)))
> > ERROR
> >
> > libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic,
> > physinfo->cap_assisted_xapic);
> > libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic,
> > physinfo->cap_assisted_x2apic);
> >
> > I don't think you need the local assisted_x{2}apic variables.
>
> Makes sense. In that case, could I instead just have this?
>
> if (b_info->type != LIBXL_DOMAIN_TYPE_PV)
> {
> if (physinfo->cap_assisted_xapic)
> libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic, true);
> if (physinfo->cap_assisted_x2apic)
> libxl_defbool_setdefault(&b_info->arch_x86.assisted_x2apic, true);
I think you actively need to set assisted_x{2}apic if they are using
default values, or else a later call to libxl_defbool_val will cause
an assert to trigger.
libxl_defbool_setdefault(&b_info->arch_x86.assisted_xapic,
physinfo->cap_assisted_xapic);
assisted_x{2}apic need to either resolve to true or false past this
point, but must not be left using it's default (uninitialized) value.
> }
>
> Or do i still need to also check that assisted_x{2}apic hasn't been set
> elsewhere for PV domains, in which case, I'm happy to add the code you
> proposed above with this code I have here too.
I would prefer if we actively rejected options that don't make
sense.
It's wrong to try to set assisted_x{2}apic for PV because there's no
APIC at all in that case. I will defer to the maintainer, but I would
prefer if an error was reported in that case. I know we are not
consistent in that regard, so I'm not going to block what you propose.
Thanks, Roger.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
2022-01-31 12:05 ` Jan Beulich
2022-01-31 16:02 ` Jane Malalane
@ 2022-02-08 13:27 ` Jane Malalane
2022-02-08 13:52 ` Jan Beulich
1 sibling, 1 reply; 19+ messages in thread
From: Jane Malalane @ 2022-02-08 13:27 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, Anthony Perard, Juergen Gross, Andrew Cooper,
George Dunlap, Julien Grall, Stefano Stabellini,
Christian Lindig, David Scott, Volodymyr Babchuk,
Roger Pau Monne, Xen-devel
On 31/01/2022 12:05, Jan Beulich wrote:
> On 27.01.2022 17:01, Jane Malalane wrote:
>> Introduce a new per-domain creation x86 specific flag to
>> select whether hardware assisted virtualization should be used for
>> x{2}APIC.
>>
>> A per-domain option is added to xl in order to select the usage of
>> x{2}APIC hardware assisted vitualization, as well as a global
>> configuration option.
>>
>> Having all APIC interaction exit to Xen for emulation is slow and can
>> induce much overhead. Hardware can speed up x{2}APIC by running APIC
>> read/write accesses without taking a VM exit.
>
> This is odd to read for a patch which makes it possible to _turn off_
> acceleration. Instead it would be interesting to know what problems
> you have encountered making it desirable to have a way to turn this off.
Hi Jan,
After speaking to Andrew he told me of a performance regression that was
reported some time ago when enabling apicv related to the pass-through
LAPIC timer of a HVM guest causing Xen to intercept the LAPIC timer MSR,
making anything that uses the LAPIC timer end up slower than it was
before. So, adressing your comment here, other than mentioning how being
able to disable acceleration for apicv can be useful when testing and
debugging, do you think it's worth mentioning (in more detail) that this
perf problem exists, in the commit message.
Thanks,
Jane.
>
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -3342,16 +3342,19 @@ static void vmx_install_vlapic_mapping(struct vcpu *v)
>>
>> void vmx_vlapic_msr_changed(struct vcpu *v)
>> {
>> - int virtualize_x2apic_mode;
>> + int virtualize_xapic_mode, virtualize_x2apic_mode;
>
> Please switch to bool as you touch and extend this.
>
>> struct vlapic *vlapic = vcpu_vlapic(v);
>> unsigned int msr;
>>
>> + virtualize_xapic_mode = ( cpu_has_vmx_virtualize_apic_accesses &&
>> + v->domain->arch.hvm.assisted_xapic );
>
> Please don't clone the bad use of blanks immediately inside parentheses
> here; instead, ...
>
>> virtualize_x2apic_mode = ( (cpu_has_vmx_apic_reg_virt ||
>> cpu_has_vmx_virtual_intr_delivery) &&
>> - cpu_has_vmx_virtualize_x2apic_mode );
>> + cpu_has_vmx_virtualize_x2apic_mode &&
>> + v->domain->arch.hvm.assisted_x2apic );
>
> ... since you're touching this anyway, please consider correcting
> the style violation here.
>
> However - do you need these expressions anymore? The per-domain fields
> can only be set if the respective CPU capabilities exit.
>
>> --- a/xen/arch/x86/include/asm/hvm/domain.h
>> +++ b/xen/arch/x86/include/asm/hvm/domain.h
>> @@ -117,6 +117,12 @@ struct hvm_domain {
>>
>> bool is_s3_suspended;
>>
>> + /* xAPIC hardware assisted emulation. */
>> + bool assisted_xapic;
>> +
>> + /* x2APIC hardware assisted emulation. */
>> + bool assisted_x2apic;
>> +
>> /* hypervisor intercepted msix table */
>> struct list_head msixtbl_list;
>
> Please follow how adjacent code pads types / names here.
>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -1115,7 +1115,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
>> if ( !is_hvm_domain(d) || subleaf != 0 )
>> break;
>>
>> - if ( cpu_has_vmx_apic_reg_virt )
>> + if ( cpu_has_vmx_apic_reg_virt &&
>> + v->domain->arch.hvm.assisted_xapic )
>> res->a |= XEN_HVM_CPUID_APIC_ACCESS_VIRT;
>>
>> /*
>> @@ -1126,7 +1127,8 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, uint32_t leaf,
>> */
>> if ( cpu_has_vmx_virtualize_x2apic_mode &&
>> cpu_has_vmx_apic_reg_virt &&
>> - cpu_has_vmx_virtual_intr_delivery )
>> + cpu_has_vmx_virtual_intr_delivery &&
>> + v->domain->arch.hvm.assisted_x2apic )
>> res->a |= XEN_HVM_CPUID_X2APIC_VIRT;
>
> Same remark as above - can't you now use _just_ the per-domain field?
> In this latter of the two cases this would then also mean bringing
> the CPU feature checks in line with what vmx_vlapic_msr_changed()
> does (as also pointed out for patch 1). Albeit it might be best to
> have a prereq patch fixing the issue, which could then be backported.
>
> Jan
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
2022-02-08 13:27 ` Jane Malalane
@ 2022-02-08 13:52 ` Jan Beulich
2022-02-08 16:00 ` Jane Malalane
0 siblings, 1 reply; 19+ messages in thread
From: Jan Beulich @ 2022-02-08 13:52 UTC (permalink / raw)
To: Jane Malalane
Cc: Wei Liu, Anthony Perard, Juergen Gross, Andrew Cooper,
George Dunlap, Julien Grall, Stefano Stabellini,
Christian Lindig, David Scott, Volodymyr Babchuk,
Roger Pau Monne, Xen-devel
On 08.02.2022 14:27, Jane Malalane wrote:
> On 31/01/2022 12:05, Jan Beulich wrote:
>> On 27.01.2022 17:01, Jane Malalane wrote:
>>> Introduce a new per-domain creation x86 specific flag to
>>> select whether hardware assisted virtualization should be used for
>>> x{2}APIC.
>>>
>>> A per-domain option is added to xl in order to select the usage of
>>> x{2}APIC hardware assisted vitualization, as well as a global
>>> configuration option.
>>>
>>> Having all APIC interaction exit to Xen for emulation is slow and can
>>> induce much overhead. Hardware can speed up x{2}APIC by running APIC
>>> read/write accesses without taking a VM exit.
>>
>> This is odd to read for a patch which makes it possible to _turn off_
>> acceleration. Instead it would be interesting to know what problems
>> you have encountered making it desirable to have a way to turn this off.
>
> After speaking to Andrew he told me of a performance regression that was
> reported some time ago when enabling apicv related to the pass-through
> LAPIC timer of a HVM guest causing Xen to intercept the LAPIC timer MSR,
> making anything that uses the LAPIC timer end up slower than it was
> before. So, adressing your comment here, other than mentioning how being
> able to disable acceleration for apicv can be useful when testing and
> debugging, do you think it's worth mentioning (in more detail) that this
> perf problem exists, in the commit message.
Yes, I think it would be worth mentioning, as then the purpose of this
change is also to be a workaround there, not just testing/debugging. In
fact this workaround may then be viewed as the primary purpose.
Jan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC
2022-02-08 13:52 ` Jan Beulich
@ 2022-02-08 16:00 ` Jane Malalane
0 siblings, 0 replies; 19+ messages in thread
From: Jane Malalane @ 2022-02-08 16:00 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, Anthony Perard, Juergen Gross, Andrew Cooper,
George Dunlap, Julien Grall, Stefano Stabellini,
Christian Lindig, David Scott, Volodymyr Babchuk,
Roger Pau Monne, Xen-devel
On 08/02/2022 13:52, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>
> On 08.02.2022 14:27, Jane Malalane wrote:
>> On 31/01/2022 12:05, Jan Beulich wrote:
>>> On 27.01.2022 17:01, Jane Malalane wrote:
>>>> Introduce a new per-domain creation x86 specific flag to
>>>> select whether hardware assisted virtualization should be used for
>>>> x{2}APIC.
>>>>
>>>> A per-domain option is added to xl in order to select the usage of
>>>> x{2}APIC hardware assisted vitualization, as well as a global
>>>> configuration option.
>>>>
>>>> Having all APIC interaction exit to Xen for emulation is slow and can
>>>> induce much overhead. Hardware can speed up x{2}APIC by running APIC
>>>> read/write accesses without taking a VM exit.
>>>
>>> This is odd to read for a patch which makes it possible to _turn off_
>>> acceleration. Instead it would be interesting to know what problems
>>> you have encountered making it desirable to have a way to turn this off.
>>
>> After speaking to Andrew he told me of a performance regression that was
>> reported some time ago when enabling apicv related to the pass-through
>> LAPIC timer of a HVM guest causing Xen to intercept the LAPIC timer MSR,
>> making anything that uses the LAPIC timer end up slower than it was
>> before. So, adressing your comment here, other than mentioning how being
>> able to disable acceleration for apicv can be useful when testing and
>> debugging, do you think it's worth mentioning (in more detail) that this
>> perf problem exists, in the commit message.
>
> Yes, I think it would be worth mentioning, as then the purpose of this
> change is also to be a workaround there, not just testing/debugging. In
> fact this workaround may then be viewed as the primary purpose.
>
Okay I will add this in a v3 (alongside other changes I'll have to make).
Thank you,
Jane.
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-02-08 16:01 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-27 16:01 [PATCH 0/2] xen: Report and use hardware APIC virtualization capabilities Jane Malalane
2022-01-27 16:01 ` [PATCH 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86 Jane Malalane
2022-01-28 17:04 ` Anthony PERARD
2022-01-31 15:54 ` Jane Malalane
2022-01-31 11:18 ` Jan Beulich
2022-01-31 11:53 ` Jan Beulich
2022-01-31 17:57 ` Roger Pau Monné
2022-01-27 16:01 ` [PATCH 2/2] x86/xen: Allow per-domain usage of hardware virtualized APIC Jane Malalane
2022-01-27 16:21 ` Christian Lindig
2022-01-31 12:05 ` Jan Beulich
2022-01-31 16:02 ` Jane Malalane
2022-02-08 13:27 ` Jane Malalane
2022-02-08 13:52 ` Jan Beulich
2022-02-08 16:00 ` Jane Malalane
2022-01-31 16:05 ` Anthony PERARD
2022-01-31 16:13 ` Jan Beulich
2022-02-01 10:02 ` Roger Pau Monné
2022-02-02 15:19 ` Jane Malalane
2022-02-02 16:21 ` Roger Pau Monné
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.