On Tue, Mar 08, 2022 at 05:31:17PM +0000, Jane Malalane wrote:
> Add XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC and
> XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC to report accelerated xapic
> and x2apic, on x86 hardware.
> No such features are currently implemented on AMD hardware.
>
> HW assisted xAPIC virtualization will be reported if HW, at the
> minimum, supports virtualize_apic_accesses as this feature alone means
> that an access to the APIC page will cause an APIC-access VM exit. An
> APIC-access VM exit provides a VMM with information about the access
> causing the VM exit, unlike a regular EPT fault, thus simplifying some
> internal handling.
>
> HW assisted x2APIC virtualization will be reported if HW supports
> virtualize_x2apic_mode and, at least, either apic_reg_virt or
> virtual_intr_delivery. This also means that
> sysctl follows the conditionals in vmx_vlapic_msr_changed().
>
> For that purpose, also add an arch-specific "capabilities" parameter
> to struct xen_sysctl_physinfo.
>
> Note that this interface is intended to be compatible with AMD so that
> AVIC support can be introduced in a future patch. Unlike Intel that
> has multiple controls for APIC Virtualization, AMD has one global
> 'AVIC Enable' control bit, so fine-graining of APIC virtualization
> control cannot be done on a common interface.
>
> Suggested-by: Andrew Cooper <andrew.coop...@citrix.com>
> Signed-off-by: Jane Malalane <jane.malal...@citrix.com>
Overall LGTM, just one question and one nit.
> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c
> b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index 5b4fe72c8d..7e9c32ad1b 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -712,7 +712,7 @@ CAMLprim value stub_xc_send_debug_keys(value xch, value
> keys)
> CAMLprim value stub_xc_physinfo(value xch)
> {
> CAMLparam1(xch);
> - CAMLlocal2(physinfo, cap_list);
> + CAMLlocal3(physinfo, cap_list, arch_cap_list);
> xc_physinfo_t c_physinfo;
> int r;
>
> @@ -731,7 +731,7 @@ CAMLprim value stub_xc_physinfo(value xch)
> /* ! XEN_SYSCTL_PHYSCAP_ XEN_SYSCTL_PHYSCAP_MAX max */
> (c_physinfo.capabilities);
>
> - physinfo = caml_alloc_tuple(10);
> + physinfo = caml_alloc_tuple(11);
> Store_field(physinfo, 0, Val_int(c_physinfo.threads_per_core));
> Store_field(physinfo, 1, Val_int(c_physinfo.cores_per_socket));
> Store_field(physinfo, 2, Val_int(c_physinfo.nr_cpus));
> @@ -743,6 +743,17 @@ CAMLprim value stub_xc_physinfo(value xch)
> Store_field(physinfo, 8, cap_list);
> Store_field(physinfo, 9, Val_int(c_physinfo.max_cpu_id + 1));
>
> +#if defined(__i386__) || defined(__x86_64__)
> + /*
> + * arch_capabilities: physinfo_arch_cap_flag list;
> + */
> + arch_cap_list = c_bitmap_to_ocaml_list
> + /* ! physinfo_arch_cap_flag CAP_ none */
> + /* ! XEN_SYSCTL_PHYSCAP_ XEN_SYSCTL_PHYSCAP_X86_MAX max */
> + (c_physinfo.arch_capabilities);
> + Store_field(physinfo, 10, arch_cap_list);
> +#endif
Have you tried to build this on Arm? I wonder whether the compiler
will complain about arch_cap_list being unused there?
> +
> CAMLreturn(physinfo);
> }
>
> 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 e1e1fa14e6..77ce0b2121 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
> MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
> }
>
> + /* Check whether hardware supports accelerated xapic and x2apic. */
> + if ( bsp )
> + {
> + assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
> + assisted_x2apic_available = cpu_has_vmx_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..72431df26d 100644
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -756,6 +756,9 @@ static inline void pv_inject_sw_interrupt(unsigned int
> vector)
> : is_pv_32bit_domain(d) ? PV32_VM_ASSIST_MASK \
> : PV64_VM_ASSIST_MASK)
>
> +extern bool assisted_xapic_available;
> +extern bool assisted_x2apic_available;
> +
> #endif /* __ASM_DOMAIN_H__ */
>
> /*
> diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
> index f82abc2488..ad95c86aef 100644
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -69,6 +69,9 @@ struct l3_cache_info {
> unsigned long size;
> };
>
> +bool __ro_after_init assisted_xapic_available;
> +bool __ro_after_init assisted_x2apic_available;
> +
> static void cf_check l3_cache_get(void *arg)
> {
> struct cpuid4_info info;
> @@ -135,6 +138,10 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap;
> if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
> pi->capabilities |= XEN_SYSCTL_PHYSCAP_shadow;
> + if ( assisted_xapic_available )
> + pi->arch_capabilities |= XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC;
> + if ( assisted_x2apic_available )
> + pi->arch_capabilities |= XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC;
> }
>
> long arch_do_sysctl(
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 55252e97f2..7fe05be0c9 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -35,7 +35,7 @@
> #include "domctl.h"
> #include "physdev.h"
>
> -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000014
> +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000015
>
> /*
> * Read console content from Xen buffer ring.
> @@ -111,6 +111,13 @@ struct xen_sysctl_tbuf_op {
> /* Max XEN_SYSCTL_PHYSCAP_* constant. Used for ABI checking. */
> #define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_gnttab_v2
>
> +/* The platform supports x{2}apic hardware assisted emulation. */
> +#define XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC (1u << 0)
> +#define XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC (1u << 1)
> +
> +/* Max XEN_SYSCTL_PHYSCAP_X86__* constant. Used for ABI checking. */
> +#define XEN_SYSCTL_PHYSCAP_X86_MAX XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC
> +
> struct xen_sysctl_physinfo {
> uint32_t threads_per_core;
> uint32_t cores_per_socket;
> @@ -120,6 +127,8 @@ struct xen_sysctl_physinfo {
> uint32_t max_node_id; /* Largest possible node ID on this host */
> uint32_t cpu_khz;
> uint32_t capabilities;/* XEN_SYSCTL_PHYSCAP_??? */
> + uint32_t arch_capabilities;/* XEN_SYSCTL_PHYSCAP_X86{ARM}_??? */
Nit: comment should likely be:
XEN_SYSCTL_PHYSCAP_{X86,ARM,...}_???
Thanks, Roger.
