[v2,1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86

Commit Message

Jane Malalane Feb. 7, 2022, 6:21 p.m. UTC

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 <[e-mail]>
Suggested-by: Andrew Cooper <[e-mail]>
---
CC: Wei Liu <[e-mail]>
CC: Anthony PERARD <[e-mail]>
CC: Juergen Gross <[e-mail]>
CC: Andrew Cooper <[e-mail]>
CC: George Dunlap <[e-mail]>
CC: Jan Beulich <[e-mail]>
CC: Julien Grall <[e-mail]>
CC: Stefano Stabellini <[e-mail]>
CC: Volodymyr Babchuk <[e-mail]>
CC: Bertrand Marquis <[e-mail]>
CC: Jun Nakajima <[e-mail]>
CC: Kevin Tian <[e-mail]>
CC: "Roger Pau Monné" <[e-mail]>

v2:
 * Use one macro LIBXL_HAVE_PHYSINFO_ASSISTED_APIC instead of two
 * Pass xcpyshinfo as a pointer in libxl__arch_get_physinfo
 * Set assisted_x{2}apic_available to be conditional upon "bsp" and
   annotate it with __ro_after_init
 * Change XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_X{2}APIC to
   .._X86_ASSISTED_X{2}APIC
 * Keep XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X{2}APIC contained within
   sysctl.h
 * Fix padding introduced in struct xen_sysctl_physinfo and bump
   XEN_SYSCTL_INTERFACE_VERSION
---
 tools/golang/xenlight/helpers.gen.go |  4 ++++
 tools/golang/xenlight/types.gen.go   |  6 ++++++
 tools/include/libxl.h                |  7 +++++++
 tools/libs/light/libxl.c             |  3 +++
 tools/libs/light/libxl_arch.h        |  4 ++++
 tools/libs/light/libxl_arm.c         |  5 +++++
 tools/libs/light/libxl_types.idl     |  2 ++
 tools/libs/light/libxl_x86.c         | 11 +++++++++++
 tools/ocaml/libs/xc/xenctrl.ml       |  5 +++++
 tools/ocaml/libs/xc/xenctrl.mli      |  5 +++++
 tools/xl/xl_info.c                   |  6 ++++--
 xen/arch/x86/hvm/vmx/vmcs.c          |  9 +++++++++
 xen/arch/x86/include/asm/domain.h    |  3 +++
 xen/arch/x86/sysctl.c                |  7 +++++++
 xen/include/public/sysctl.h          |  8 +++++++-
 15 files changed, 82 insertions(+), 3 deletions(-)

Comments

Roger Pau Monné Feb. 8, 2022, 3:26 p.m. UTC | #1

On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
> Add XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_xapic and
> XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_x2apic to report accelerated xapic
> and x2apic, on x86 hardware.
> No such features are currently implemented on AMD hardware.
> 
> For that purpose, also add an arch-specific "capabilities" parameter
> to struct xen_sysctl_physinfo.
> 
> Signed-off-by: Jane Malalane <[e-mail]>
> Suggested-by: Andrew Cooper <[e-mail]>

Tag order should be inverted, first Suggested-by, then SoB.

> ---
> CC: Wei Liu <[e-mail]>
> CC: Anthony PERARD <[e-mail]>
> CC: Juergen Gross <[e-mail]>
> CC: Andrew Cooper <[e-mail]>
> CC: George Dunlap <[e-mail]>
> CC: Jan Beulich <[e-mail]>
> CC: Julien Grall <[e-mail]>
> CC: Stefano Stabellini <[e-mail]>
> CC: Volodymyr Babchuk <[e-mail]>
> CC: Bertrand Marquis <[e-mail]>
> CC: Jun Nakajima <[e-mail]>
> CC: Kevin Tian <[e-mail]>
> CC: "Roger Pau Monné" <[e-mail]>
> 
> v2:
>  * Use one macro LIBXL_HAVE_PHYSINFO_ASSISTED_APIC instead of two
>  * Pass xcpyshinfo as a pointer in libxl__arch_get_physinfo
>  * Set assisted_x{2}apic_available to be conditional upon "bsp" and
>    annotate it with __ro_after_init
>  * Change XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_X{2}APIC to
>    .._X86_ASSISTED_X{2}APIC
>  * Keep XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X{2}APIC contained within
>    sysctl.h
>  * Fix padding introduced in struct xen_sysctl_physinfo and bump
>    XEN_SYSCTL_INTERFACE_VERSION
> ---
>  tools/golang/xenlight/helpers.gen.go |  4 ++++
>  tools/golang/xenlight/types.gen.go   |  6 ++++++
>  tools/include/libxl.h                |  7 +++++++
>  tools/libs/light/libxl.c             |  3 +++
>  tools/libs/light/libxl_arch.h        |  4 ++++
>  tools/libs/light/libxl_arm.c         |  5 +++++
>  tools/libs/light/libxl_types.idl     |  2 ++
>  tools/libs/light/libxl_x86.c         | 11 +++++++++++
>  tools/ocaml/libs/xc/xenctrl.ml       |  5 +++++
>  tools/ocaml/libs/xc/xenctrl.mli      |  5 +++++
>  tools/xl/xl_info.c                   |  6 ++++--
>  xen/arch/x86/hvm/vmx/vmcs.c          |  9 +++++++++
>  xen/arch/x86/include/asm/domain.h    |  3 +++
>  xen/arch/x86/sysctl.c                |  7 +++++++
>  xen/include/public/sysctl.h          |  8 +++++++-
>  15 files changed, 82 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
> index b746ff1081..dd4e6c9f14 100644
> --- a/tools/golang/xenlight/helpers.gen.go
> +++ b/tools/golang/xenlight/helpers.gen.go
> @@ -3373,6 +3373,8 @@ x.CapVmtrace = bool(xc.cap_vmtrace)
>  x.CapVpmu = bool(xc.cap_vpmu)
>  x.CapGnttabV1 = bool(xc.cap_gnttab_v1)
>  x.CapGnttabV2 = bool(xc.cap_gnttab_v2)
> +x.CapAssistedXapic = bool(xc.cap_assisted_xapic)
> +x.CapAssistedX2Apic = bool(xc.cap_assisted_x2apic)
>  
>   return nil}
>  
> @@ -3407,6 +3409,8 @@ xc.cap_vmtrace = C.bool(x.CapVmtrace)
>  xc.cap_vpmu = C.bool(x.CapVpmu)
>  xc.cap_gnttab_v1 = C.bool(x.CapGnttabV1)
>  xc.cap_gnttab_v2 = C.bool(x.CapGnttabV2)
> +xc.cap_assisted_xapic = C.bool(x.CapAssistedXapic)
> +xc.cap_assisted_x2apic = C.bool(x.CapAssistedX2Apic)
>  
>   return nil
>   }
> diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
> index b1e84d5258..5f384b767c 100644
> --- a/tools/golang/xenlight/types.gen.go
> +++ b/tools/golang/xenlight/types.gen.go
> @@ -389,6 +389,10 @@ RunHotplugScripts Defbool
>  DriverDomain Defbool
>  Passthrough Passthrough
>  XendSuspendEvtchnCompat Defbool
> +ArchX86 struct {
> +AssistedXapic Defbool
> +AssistedX2Apic Defbool

Don't you need some indentation here?

Also name would better be Assistedx{2}APIC IMO if possible. Having a
capital 'X' and lowercase 'apic' looks really strange.

> +}
>  }
>  
>  type DomainRestoreParams struct {
> @@ -1014,6 +1018,8 @@ CapVmtrace bool
>  CapVpmu bool
>  CapGnttabV1 bool
>  CapGnttabV2 bool
> +CapAssistedXApic bool
> +CapAssistedX2apic bool
>  }
>  
>  type Connectorinfo struct {
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index 2bbbd21f0b..924e142628 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -528,6 +528,13 @@
>  #define LIBXL_HAVE_MAX_GRANT_VERSION 1
>  
>  /*
> + * LIBXL_HAVE_PHYSINFO_ASSISTED_APIC indicates that libxl_physinfo has
> + * cap_assisted_x{2}apic fields, which indicates the availability of x{2}APIC
> + * hardware assisted virtualization.
> + */
> +#define LIBXL_HAVE_PHYSINFO_ASSISTED_APIC 1
> +
> +/*
>   * libxl ABI compatibility
>   *
>   * The only guarantee which libxl makes regarding ABI compatibility
> diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c
> index 667ae6409b..fabb474221 100644
> --- a/tools/libs/light/libxl.c
> +++ b/tools/libs/light/libxl.c
> @@ -15,6 +15,7 @@
>  #include "libxl_osdeps.h"
>  
>  #include "libxl_internal.h"
> +#include "libxl_arch.h"
>  
>  int libxl_ctx_alloc(libxl_ctx **pctx, int version,
>                      unsigned flags, xentoollog_logger * lg)
> @@ -410,6 +411,8 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo)
>      physinfo->cap_gnttab_v2 =
>          !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_gnttab_v2);
>  
> +    libxl__arch_get_physinfo(physinfo, &xcphysinfo);
> +
>      GC_FREE;
>      return 0;
>  }
> diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h
> index 1522ecb97f..207ceac6a1 100644
> --- a/tools/libs/light/libxl_arch.h
> +++ b/tools/libs/light/libxl_arch.h
> @@ -86,6 +86,10 @@ int libxl__arch_extra_memory(libxl__gc *gc,
>                               uint64_t *out);
>  
>  _hidden
> +void libxl__arch_get_physinfo(libxl_physinfo *physinfo,
> +                              const xc_physinfo_t *xcphysinfo);
> +
> +_hidden
>  void libxl__arch_update_domain_config(libxl__gc *gc,
>                                        libxl_domain_config *dst,
>                                        const libxl_domain_config *src);
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index eef1de0939..39fdca1b49 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -1431,6 +1431,11 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
>      return rc;
>  }
>  
> +void libxl__arch_get_physinfo(libxl_physinfo *physinfo,
> +                              const xc_physinfo_t *xcphysinfo)
> +{
> +}
> +
>  void libxl__arch_update_domain_config(libxl__gc *gc,
>                                        libxl_domain_config *dst,
>                                        const libxl_domain_config *src)
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index 2a42da2f7d..42ac6c357b 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -1068,6 +1068,8 @@ libxl_physinfo = Struct("physinfo", [
>      ("cap_vpmu", bool),
>      ("cap_gnttab_v1", bool),
>      ("cap_gnttab_v2", bool),
> +    ("cap_assisted_xapic", bool),
> +    ("cap_assisted_x2apic", bool),
>      ], dir=DIR_OUT)
>  
>  libxl_connectorinfo = Struct("connectorinfo", [
> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> index 1feadebb18..e0a06ecfe3 100644
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -866,6 +866,17 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
>      return rc;
>  }
>  
> +void libxl__arch_get_physinfo(libxl_physinfo *physinfo,
> +                              const xc_physinfo_t *xcphysinfo)
> +{
> +    physinfo->cap_assisted_xapic =
> +        !!(xcphysinfo->arch_capabilities &
> +           XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC);
> +    physinfo->cap_assisted_x2apic =
> +        !!(xcphysinfo->arch_capabilities &
> +           XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC);
> +}
> +
>  void libxl__arch_update_domain_config(libxl__gc *gc,
>                                        libxl_domain_config *dst,
>                                        const libxl_domain_config *src)
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> index 7503031d8f..7ce832d605 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -127,6 +127,10 @@ type physinfo_cap_flag =
>  	| CAP_Gnttab_v1
>  	| CAP_Gnttab_v2
>  
> +type physinfo_cap_arch_flag =
> +	| CAP_ARCH_ASSISTED_XAPIC
> +	| CAP_ARCH_ASSISTED_X2APIC
> +
>  type physinfo =
>  {
>  	threads_per_core : int;
> @@ -139,6 +143,7 @@ type physinfo =
>  	scrub_pages      : nativeint;
>  	(* XXX hw_cap *)
>  	capabilities     : physinfo_cap_flag list;
> +	arch_capabilities : physinfo_cap_arch_flag list;

I know very little about Ocaml, but I think you are not setting this
field anywhere? I would expect a call to ocaml_list_to_c_bitmap and
then you will likely need to define XEN_SYSCTL_PHYSCAP_X86_MAX so you
can check the options. See XEN_SYSCTL_PHYSCAP_MAX for example.

>  	max_nr_cpus      : int;
>  }
>  
> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
> index d1d9c9247a..a2b15130ee 100644
> --- a/tools/ocaml/libs/xc/xenctrl.mli
> +++ b/tools/ocaml/libs/xc/xenctrl.mli
> @@ -112,6 +112,10 @@ type physinfo_cap_flag =
>    | CAP_Gnttab_v1
>    | CAP_Gnttab_v2
>  
> +type physinfo_cap_arch_flag =
> +  | CAP_ARCH_ASSISTED_XAPIC
> +  | CAP_ARCH_ASSISTED_X2APIC
> +
>  type physinfo = {
>    threads_per_core : int;
>    cores_per_socket : int;
> @@ -122,6 +126,7 @@ type physinfo = {
>    free_pages       : nativeint;
>    scrub_pages      : nativeint;
>    capabilities     : physinfo_cap_flag list;
> +  arch_capabilities : physinfo_cap_arch_flag list;
>    max_nr_cpus      : int; (** compile-time max possible number of nr_cpus *)
>  }
>  type version = { major : int; minor : int; extra : string; }
> diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
> index 712b7638b0..3205270754 100644
> --- a/tools/xl/xl_info.c
> +++ b/tools/xl/xl_info.c
> @@ -210,7 +210,7 @@ static void output_physinfo(void)
>           info.hw_cap[4], info.hw_cap[5], info.hw_cap[6], info.hw_cap[7]
>          );
>  
> -    maybe_printf("virt_caps              :%s%s%s%s%s%s%s%s%s%s%s\n",
> +    maybe_printf("virt_caps              :%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
>           info.cap_pv ? " pv" : "",
>           info.cap_hvm ? " hvm" : "",
>           info.cap_hvm && info.cap_hvm_directio ? " hvm_directio" : "",
> @@ -221,7 +221,9 @@ static void output_physinfo(void)
>           info.cap_vmtrace ? " vmtrace" : "",
>           info.cap_vpmu ? " vpmu" : "",
>           info.cap_gnttab_v1 ? " gnttab-v1" : "",
> -         info.cap_gnttab_v2 ? " gnttab-v2" : ""
> +         info.cap_gnttab_v2 ? " gnttab-v2" : "",
> +         info.cap_assisted_xapic ? " assisted_xapic" : "",
> +         info.cap_assisted_x2apic ? " assisted_x2apic" : ""
>          );
>  
>      vinfo = libxl_get_version_info(ctx);
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 7ab15e07a0..4060aef1bd 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>              MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>      }
>  
> +    /* Check whether hardware supports accelerated xapic and x2apic. */
> +    if ( bsp )
> +    {
> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
> +                                     cpu_has_vmx_virtual_intr_delivery) &&
> +                                    cpu_has_vmx_virtualize_x2apic_mode;
> +    }
> +
>      /* The IA32_VMX_EPT_VPID_CAP MSR exists only when EPT or VPID available */
>      if ( _vmx_secondary_exec_control & (SECONDARY_EXEC_ENABLE_EPT |
>                                          SECONDARY_EXEC_ENABLE_VPID) )
> diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
> index e62e109598..72431df26d 100644
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -756,6 +756,9 @@ static inline void pv_inject_sw_interrupt(unsigned int vector)
>                        : is_pv_32bit_domain(d) ? PV32_VM_ASSIST_MASK \
>                                                : PV64_VM_ASSIST_MASK)
>  
> +extern bool assisted_xapic_available;
> +extern bool assisted_x2apic_available;
> +
>  #endif /* __ASM_DOMAIN_H__ */
>  
>  /*
> diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
> index aff52a13f3..642cc96985 100644
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -69,6 +69,9 @@ struct l3_cache_info {
>      unsigned long size;
>  };
>  
> +bool __ro_after_init assisted_xapic_available;
> +bool __ro_after_init assisted_x2apic_available;

You could likely shorten this by dropping the _available suffix.

Thanks, Roger.

Jane Malalane Feb. 9, 2022, 12:26 p.m. UTC | #2

On 08/02/2022 15:26, Roger Pau Monné wrote:
> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
>> Add XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_xapic and
>> XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_x2apic to report accelerated xapic
>> and x2apic, on x86 hardware.
>> No such features are currently implemented on AMD hardware.
>>
>> For that purpose, also add an arch-specific "capabilities" parameter
>> to struct xen_sysctl_physinfo.
>>
>> Signed-off-by: Jane Malalane <[e-mail]>
>> Suggested-by: Andrew Cooper <[e-mail]>
> 
> Tag order should be inverted, first Suggested-by, then SoB.
> 
>> ---
>> CC: Wei Liu <[e-mail]>
>> CC: Anthony PERARD <[e-mail]>
>> CC: Juergen Gross <[e-mail]>
>> CC: Andrew Cooper <[e-mail]>
>> CC: George Dunlap <[e-mail]>
>> CC: Jan Beulich <[e-mail]>
>> CC: Julien Grall <[e-mail]>
>> CC: Stefano Stabellini <[e-mail]>
>> CC: Volodymyr Babchuk <[e-mail]>
>> CC: Bertrand Marquis <[e-mail]>
>> CC: Jun Nakajima <[e-mail]>
>> CC: Kevin Tian <[e-mail]>
>> CC: "Roger Pau Monné" <[e-mail]>
>>
>> v2:
>>   * Use one macro LIBXL_HAVE_PHYSINFO_ASSISTED_APIC instead of two
>>   * Pass xcpyshinfo as a pointer in libxl__arch_get_physinfo
>>   * Set assisted_x{2}apic_available to be conditional upon "bsp" and
>>     annotate it with __ro_after_init
>>   * Change XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_X{2}APIC to
>>     .._X86_ASSISTED_X{2}APIC
>>   * Keep XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X{2}APIC contained within
>>     sysctl.h
>>   * Fix padding introduced in struct xen_sysctl_physinfo and bump
>>     XEN_SYSCTL_INTERFACE_VERSION
>> ---
>>   tools/golang/xenlight/helpers.gen.go |  4 ++++
>>   tools/golang/xenlight/types.gen.go   |  6 ++++++
>>   tools/include/libxl.h                |  7 +++++++
>>   tools/libs/light/libxl.c             |  3 +++
>>   tools/libs/light/libxl_arch.h        |  4 ++++
>>   tools/libs/light/libxl_arm.c         |  5 +++++
>>   tools/libs/light/libxl_types.idl     |  2 ++
>>   tools/libs/light/libxl_x86.c         | 11 +++++++++++
>>   tools/ocaml/libs/xc/xenctrl.ml       |  5 +++++
>>   tools/ocaml/libs/xc/xenctrl.mli      |  5 +++++
>>   tools/xl/xl_info.c                   |  6 ++++--
>>   xen/arch/x86/hvm/vmx/vmcs.c          |  9 +++++++++
>>   xen/arch/x86/include/asm/domain.h    |  3 +++
>>   xen/arch/x86/sysctl.c                |  7 +++++++
>>   xen/include/public/sysctl.h          |  8 +++++++-
>>   15 files changed, 82 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
>> index b746ff1081..dd4e6c9f14 100644
>> --- a/tools/golang/xenlight/helpers.gen.go
>> +++ b/tools/golang/xenlight/helpers.gen.go
>> @@ -3373,6 +3373,8 @@ x.CapVmtrace = bool(xc.cap_vmtrace)
>>   x.CapVpmu = bool(xc.cap_vpmu)
>>   x.CapGnttabV1 = bool(xc.cap_gnttab_v1)
>>   x.CapGnttabV2 = bool(xc.cap_gnttab_v2)
>> +x.CapAssistedXapic = bool(xc.cap_assisted_xapic)
>> +x.CapAssistedX2Apic = bool(xc.cap_assisted_x2apic)
>>   
>>    return nil}
>>   
>> @@ -3407,6 +3409,8 @@ xc.cap_vmtrace = C.bool(x.CapVmtrace)
>>   xc.cap_vpmu = C.bool(x.CapVpmu)
>>   xc.cap_gnttab_v1 = C.bool(x.CapGnttabV1)
>>   xc.cap_gnttab_v2 = C.bool(x.CapGnttabV2)
>> +xc.cap_assisted_xapic = C.bool(x.CapAssistedXapic)
>> +xc.cap_assisted_x2apic = C.bool(x.CapAssistedX2Apic)
>>   
>>    return nil
>>    }
>> diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
>> index b1e84d5258..5f384b767c 100644
>> --- a/tools/golang/xenlight/types.gen.go
>> +++ b/tools/golang/xenlight/types.gen.go
>> @@ -389,6 +389,10 @@ RunHotplugScripts Defbool
>>   DriverDomain Defbool
>>   Passthrough Passthrough
>>   XendSuspendEvtchnCompat Defbool
>> +ArchX86 struct {
>> +AssistedXapic Defbool
>> +AssistedX2Apic Defbool
> 
> Don't you need some indentation here?
I hadn't realized it appeared like this here (and the same happens for 
other parts of my code as I'm seeing now) because the git output is 
correct. I will fix it.
> 
> Also name would better be Assistedx{2}APIC IMO if possible. Having a
> capital 'X' and lowercase 'apic' looks really strange.
Okay.
> 
>> +}
>>   }
>>   
>>   type DomainRestoreParams struct {
>> @@ -1014,6 +1018,8 @@ CapVmtrace bool
>>   CapVpmu bool
>>   CapGnttabV1 bool
>>   CapGnttabV2 bool
>> +CapAssistedXApic bool
>> +CapAssistedX2apic bool
>>   }
>>   
>>   type Connectorinfo struct {
>> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
>> index 2bbbd21f0b..924e142628 100644
>> --- a/tools/include/libxl.h
>> +++ b/tools/include/libxl.h
>> @@ -528,6 +528,13 @@
>>   #define LIBXL_HAVE_MAX_GRANT_VERSION 1
>>   
>>   /*
>> + * LIBXL_HAVE_PHYSINFO_ASSISTED_APIC indicates that libxl_physinfo has
>> + * cap_assisted_x{2}apic fields, which indicates the availability of x{2}APIC
>> + * hardware assisted virtualization.
>> + */
>> +#define LIBXL_HAVE_PHYSINFO_ASSISTED_APIC 1
>> +
>> +/*
>>    * libxl ABI compatibility
>>    *
>>    * The only guarantee which libxl makes regarding ABI compatibility
>> diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c
>> index 667ae6409b..fabb474221 100644
>> --- a/tools/libs/light/libxl.c
>> +++ b/tools/libs/light/libxl.c
>> @@ -15,6 +15,7 @@
>>   #include "libxl_osdeps.h"
>>   
>>   #include "libxl_internal.h"
>> +#include "libxl_arch.h"
>>   
>>   int libxl_ctx_alloc(libxl_ctx **pctx, int version,
>>                       unsigned flags, xentoollog_logger * lg)
>> @@ -410,6 +411,8 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo)
>>       physinfo->cap_gnttab_v2 =
>>           !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_gnttab_v2);
>>   
>> +    libxl__arch_get_physinfo(physinfo, &xcphysinfo);
>> +
>>       GC_FREE;
>>       return 0;
>>   }
>> diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h
>> index 1522ecb97f..207ceac6a1 100644
>> --- a/tools/libs/light/libxl_arch.h
>> +++ b/tools/libs/light/libxl_arch.h
>> @@ -86,6 +86,10 @@ int libxl__arch_extra_memory(libxl__gc *gc,
>>                                uint64_t *out);
>>   
>>   _hidden
>> +void libxl__arch_get_physinfo(libxl_physinfo *physinfo,
>> +                              const xc_physinfo_t *xcphysinfo);
>> +
>> +_hidden
>>   void libxl__arch_update_domain_config(libxl__gc *gc,
>>                                         libxl_domain_config *dst,
>>                                         const libxl_domain_config *src);
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index eef1de0939..39fdca1b49 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -1431,6 +1431,11 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
>>       return rc;
>>   }
>>   
>> +void libxl__arch_get_physinfo(libxl_physinfo *physinfo,
>> +                              const xc_physinfo_t *xcphysinfo)
>> +{
>> +}
>> +
>>   void libxl__arch_update_domain_config(libxl__gc *gc,
>>                                         libxl_domain_config *dst,
>>                                         const libxl_domain_config *src)
>> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>> index 2a42da2f7d..42ac6c357b 100644
>> --- a/tools/libs/light/libxl_types.idl
>> +++ b/tools/libs/light/libxl_types.idl
>> @@ -1068,6 +1068,8 @@ libxl_physinfo = Struct("physinfo", [
>>       ("cap_vpmu", bool),
>>       ("cap_gnttab_v1", bool),
>>       ("cap_gnttab_v2", bool),
>> +    ("cap_assisted_xapic", bool),
>> +    ("cap_assisted_x2apic", bool),
>>       ], dir=DIR_OUT)
>>   
>>   libxl_connectorinfo = Struct("connectorinfo", [
>> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
>> index 1feadebb18..e0a06ecfe3 100644
>> --- a/tools/libs/light/libxl_x86.c
>> +++ b/tools/libs/light/libxl_x86.c
>> @@ -866,6 +866,17 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
>>       return rc;
>>   }
>>   
>> +void libxl__arch_get_physinfo(libxl_physinfo *physinfo,
>> +                              const xc_physinfo_t *xcphysinfo)
>> +{
>> +    physinfo->cap_assisted_xapic =
>> +        !!(xcphysinfo->arch_capabilities &
>> +           XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC);
>> +    physinfo->cap_assisted_x2apic =
>> +        !!(xcphysinfo->arch_capabilities &
>> +           XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC);
>> +}
>> +
>>   void libxl__arch_update_domain_config(libxl__gc *gc,
>>                                         libxl_domain_config *dst,
>>                                         const libxl_domain_config *src)
>> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
>> index 7503031d8f..7ce832d605 100644
>> --- a/tools/ocaml/libs/xc/xenctrl.ml
>> +++ b/tools/ocaml/libs/xc/xenctrl.ml
>> @@ -127,6 +127,10 @@ type physinfo_cap_flag =
>>   	| CAP_Gnttab_v1
>>   	| CAP_Gnttab_v2
>>   
>> +type physinfo_cap_arch_flag =
>> +	| CAP_ARCH_ASSISTED_XAPIC
>> +	| CAP_ARCH_ASSISTED_X2APIC
>> +
>>   type physinfo =
>>   {
>>   	threads_per_core : int;
>> @@ -139,6 +143,7 @@ type physinfo =
>>   	scrub_pages      : nativeint;
>>   	(* XXX hw_cap *)
>>   	capabilities     : physinfo_cap_flag list;
>> +	arch_capabilities : physinfo_cap_arch_flag list;
> 
> I know very little about Ocaml, but I think you are not setting this
> field anywhere? I would expect a call to ocaml_list_to_c_bitmap and
> then you will likely need to define XEN_SYSCTL_PHYSCAP_X86_MAX so you
> can check the options. See XEN_SYSCTL_PHYSCAP_MAX for example.
Yes, you're right, I will add that in the v3.
> 
>>   	max_nr_cpus      : int;
>>   }
>>   
>> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
>> index d1d9c9247a..a2b15130ee 100644
>> --- a/tools/ocaml/libs/xc/xenctrl.mli
>> +++ b/tools/ocaml/libs/xc/xenctrl.mli
>> @@ -112,6 +112,10 @@ type physinfo_cap_flag =
>>     | CAP_Gnttab_v1
>>     | CAP_Gnttab_v2
>>   
>> +type physinfo_cap_arch_flag =
>> +  | CAP_ARCH_ASSISTED_XAPIC
>> +  | CAP_ARCH_ASSISTED_X2APIC
>> +
>>   type physinfo = {
>>     threads_per_core : int;
>>     cores_per_socket : int;
>> @@ -122,6 +126,7 @@ type physinfo = {
>>     free_pages       : nativeint;
>>     scrub_pages      : nativeint;
>>     capabilities     : physinfo_cap_flag list;
>> +  arch_capabilities : physinfo_cap_arch_flag list;
>>     max_nr_cpus      : int; (** compile-time max possible number of nr_cpus *)
>>   }
>>   type version = { major : int; minor : int; extra : string; }
>> diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
>> index 712b7638b0..3205270754 100644
>> --- a/tools/xl/xl_info.c
>> +++ b/tools/xl/xl_info.c
>> @@ -210,7 +210,7 @@ static void output_physinfo(void)
>>            info.hw_cap[4], info.hw_cap[5], info.hw_cap[6], info.hw_cap[7]
>>           );
>>   
>> -    maybe_printf("virt_caps              :%s%s%s%s%s%s%s%s%s%s%s\n",
>> +    maybe_printf("virt_caps              :%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
>>            info.cap_pv ? " pv" : "",
>>            info.cap_hvm ? " hvm" : "",
>>            info.cap_hvm && info.cap_hvm_directio ? " hvm_directio" : "",
>> @@ -221,7 +221,9 @@ static void output_physinfo(void)
>>            info.cap_vmtrace ? " vmtrace" : "",
>>            info.cap_vpmu ? " vpmu" : "",
>>            info.cap_gnttab_v1 ? " gnttab-v1" : "",
>> -         info.cap_gnttab_v2 ? " gnttab-v2" : ""
>> +         info.cap_gnttab_v2 ? " gnttab-v2" : "",
>> +         info.cap_assisted_xapic ? " assisted_xapic" : "",
>> +         info.cap_assisted_x2apic ? " assisted_x2apic" : ""
>>           );
>>   
>>       vinfo = libxl_get_version_info(ctx);
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index 7ab15e07a0..4060aef1bd 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>>               MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>>       }
>>   
>> +    /* Check whether hardware supports accelerated xapic and x2apic. */
>> +    if ( bsp )
>> +    {
>> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
>> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
>> +                                     cpu_has_vmx_virtual_intr_delivery) &&
>> +                                    cpu_has_vmx_virtualize_x2apic_mode;
>> +    }
>> +
>>       /* The IA32_VMX_EPT_VPID_CAP MSR exists only when EPT or VPID available */
>>       if ( _vmx_secondary_exec_control & (SECONDARY_EXEC_ENABLE_EPT |
>>                                           SECONDARY_EXEC_ENABLE_VPID) )
>> diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
>> index e62e109598..72431df26d 100644
>> --- a/xen/arch/x86/include/asm/domain.h
>> +++ b/xen/arch/x86/include/asm/domain.h
>> @@ -756,6 +756,9 @@ static inline void pv_inject_sw_interrupt(unsigned int vector)
>>                         : is_pv_32bit_domain(d) ? PV32_VM_ASSIST_MASK \
>>                                                 : PV64_VM_ASSIST_MASK)
>>   
>> +extern bool assisted_xapic_available;
>> +extern bool assisted_x2apic_available;
>> +
>>   #endif /* __ASM_DOMAIN_H__ */
>>   
>>   /*
>> diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
>> index aff52a13f3..642cc96985 100644
>> --- a/xen/arch/x86/sysctl.c
>> +++ b/xen/arch/x86/sysctl.c
>> @@ -69,6 +69,9 @@ struct l3_cache_info {
>>       unsigned long size;
>>   };
>>   
>> +bool __ro_after_init assisted_xapic_available;
>> +bool __ro_after_init assisted_x2apic_available;
> 
> You could likely shorten this by dropping the _available suffix.
Okay.

Thanks,

Jane.

Anthony PERARD Feb. 9, 2022, 1:48 p.m. UTC | #3

On Wed, Feb 09, 2022 at 12:26:05PM +0000, Jane Malalane wrote:
> On 08/02/2022 15:26, Roger Pau Monné wrote:
> > On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
> >> diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
> >> index b1e84d5258..5f384b767c 100644
> >> --- a/tools/golang/xenlight/types.gen.go
> >> +++ b/tools/golang/xenlight/types.gen.go
> >> @@ -389,6 +389,10 @@ RunHotplugScripts Defbool
> >>   DriverDomain Defbool
> >>   Passthrough Passthrough
> >>   XendSuspendEvtchnCompat Defbool
> >> +ArchX86 struct {
> >> +AssistedXapic Defbool
> >> +AssistedX2Apic Defbool
> > 
> > Don't you need some indentation here?
> I hadn't realized it appeared like this here (and the same happens for 
> other parts of my code as I'm seeing now) because the git output is 
> correct. I will fix it.
> > 
> > Also name would better be Assistedx{2}APIC IMO if possible. Having a
> > capital 'X' and lowercase 'apic' looks really strange.
> Okay.


This is a generated file, you can't change indentation or fields names.
It would be rebuilt automatically if you had golang installed and where
rebuilding all the tools.

There's two ways to generate it, you could install golang and build all
the tools. Or without golang: just
`cd tools/golang/xenlight; make types.gen.go`. Both should regenerate
both "helpers.gen.go" "types.gen.go" files.

There's an even easier way, tell the committer to regen the files when
committing :-).

Cheers,

Jane Malalane Feb. 9, 2022, 3:28 p.m. UTC | #4

On 09/02/2022 13:48, Anthony PERARD wrote:
> On Wed, Feb 09, 2022 at 12:26:05PM +0000, Jane Malalane wrote:
>> On 08/02/2022 15:26, Roger Pau Monné wrote:
>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
>>>> diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
>>>> index b1e84d5258..5f384b767c 100644
>>>> --- a/tools/golang/xenlight/types.gen.go
>>>> +++ b/tools/golang/xenlight/types.gen.go
>>>> @@ -389,6 +389,10 @@ RunHotplugScripts Defbool
>>>>    DriverDomain Defbool
>>>>    Passthrough Passthrough
>>>>    XendSuspendEvtchnCompat Defbool
>>>> +ArchX86 struct {
>>>> +AssistedXapic Defbool
>>>> +AssistedX2Apic Defbool
>>>
>>> Don't you need some indentation here?
>> I hadn't realized it appeared like this here (and the same happens for
>> other parts of my code as I'm seeing now) because the git output is
>> correct. I will fix it.
>>>
>>> Also name would better be Assistedx{2}APIC IMO if possible. Having a
>>> capital 'X' and lowercase 'apic' looks really strange.
>> Okay.
> 
> 
> This is a generated file, you can't change indentation or fields names.
> It would be rebuilt automatically if you had golang installed and where
> rebuilding all the tools.
> 
> There's two ways to generate it, you could install golang and build all
> the tools. Or without golang: just
> `cd tools/golang/xenlight; make types.gen.go`. Both should regenerate
> both "helpers.gen.go" "types.gen.go" files.
> 
> There's an even easier way, tell the committer to regen the files when
> committing :-).

Oh, yeah I forgot they were automatically generated. Right, thank you.

Jane.

Roger Pau Monné Feb. 10, 2022, 10:03 a.m. UTC | #5

On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index 7ab15e07a0..4060aef1bd 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>              MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>      }
>  
> +    /* Check whether hardware supports accelerated xapic and x2apic. */
> +    if ( bsp )
> +    {
> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
> +                                     cpu_has_vmx_virtual_intr_delivery) &&
> +                                    cpu_has_vmx_virtualize_x2apic_mode;

I've been think about this, and it seems kind of asymmetric that for
xAPIC mode we report hw assisted support only with
virtualize_apic_accesses available, while for x2APIC we require
virtualize_x2apic_mode plus either apic_reg_virt or
virtual_intr_delivery.

I think we likely need to be more consistent here, and report hw
assisted x2APIC support as long as virtualize_x2apic_mode is
available.

This will likely have some effect on patch 2 also, as you will have to
adjust vmx_vlapic_msr_changed.

Thanks, Roger.

Jane Malalane Feb. 11, 2022, 10:06 a.m. UTC | #6

On 10/02/2022 10:03, Roger Pau Monné wrote:
> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index 7ab15e07a0..4060aef1bd 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>>               MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>>       }
>>   
>> +    /* Check whether hardware supports accelerated xapic and x2apic. */
>> +    if ( bsp )
>> +    {
>> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
>> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
>> +                                     cpu_has_vmx_virtual_intr_delivery) &&
>> +                                    cpu_has_vmx_virtualize_x2apic_mode;
> 
> I've been think about this, and it seems kind of asymmetric that for
> xAPIC mode we report hw assisted support only with
> virtualize_apic_accesses available, while for x2APIC we require
> virtualize_x2apic_mode plus either apic_reg_virt or
> virtual_intr_delivery.
> 
> I think we likely need to be more consistent here, and report hw
> assisted x2APIC support as long as virtualize_x2apic_mode is
> available.
> 
> This will likely have some effect on patch 2 also, as you will have to
> adjust vmx_vlapic_msr_changed.
> 
> Thanks, Roger.

Any other thoughts on this? As on one hand it is asymmetric but also 
there isn't much assistance with only virtualize_x2apic_mode set as, in 
this case, a VM exit will be avoided only when trying to access the TPR 
register.

Thanks,

Jane.

Roger Pau Monné Feb. 11, 2022, 11:29 a.m. UTC | #7

On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote:
> On 10/02/2022 10:03, Roger Pau Monné wrote:
> > On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
> >> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> >> index 7ab15e07a0..4060aef1bd 100644
> >> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> >> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
> >>               MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
> >>       }
> >>   
> >> +    /* Check whether hardware supports accelerated xapic and x2apic. */
> >> +    if ( bsp )
> >> +    {
> >> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
> >> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
> >> +                                     cpu_has_vmx_virtual_intr_delivery) &&
> >> +                                    cpu_has_vmx_virtualize_x2apic_mode;
> > 
> > I've been think about this, and it seems kind of asymmetric that for
> > xAPIC mode we report hw assisted support only with
> > virtualize_apic_accesses available, while for x2APIC we require
> > virtualize_x2apic_mode plus either apic_reg_virt or
> > virtual_intr_delivery.
> > 
> > I think we likely need to be more consistent here, and report hw
> > assisted x2APIC support as long as virtualize_x2apic_mode is
> > available.
> > 
> > This will likely have some effect on patch 2 also, as you will have to
> > adjust vmx_vlapic_msr_changed.
> > 
> > Thanks, Roger.
> 
> Any other thoughts on this? As on one hand it is asymmetric but also 
> there isn't much assistance with only virtualize_x2apic_mode set as, in 
> this case, a VM exit will be avoided only when trying to access the TPR 
> register.

I've been thinking about this, and reporting hardware assisted
x{2}APIC virtualization with just
SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or
SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While
those provide some assistance to the VMM in order to handle APIC
accesses, it will still require a trap into the hypervisor to handle
most of the accesses.

So maybe we should only report hardware assisted support when the
mentioned features are present together with
SECONDARY_EXEC_APIC_REGISTER_VIRT?

Thanks, Roger.

Jan Beulich Feb. 11, 2022, 11:46 a.m. UTC | #8

On 11.02.2022 12:29, Roger Pau Monné wrote:
> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote:
>> On 10/02/2022 10:03, Roger Pau Monné wrote:
>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>>>> index 7ab15e07a0..4060aef1bd 100644
>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>>>>               MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>>>>       }
>>>>   
>>>> +    /* Check whether hardware supports accelerated xapic and x2apic. */
>>>> +    if ( bsp )
>>>> +    {
>>>> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
>>>> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
>>>> +                                     cpu_has_vmx_virtual_intr_delivery) &&
>>>> +                                    cpu_has_vmx_virtualize_x2apic_mode;
>>>
>>> I've been think about this, and it seems kind of asymmetric that for
>>> xAPIC mode we report hw assisted support only with
>>> virtualize_apic_accesses available, while for x2APIC we require
>>> virtualize_x2apic_mode plus either apic_reg_virt or
>>> virtual_intr_delivery.
>>>
>>> I think we likely need to be more consistent here, and report hw
>>> assisted x2APIC support as long as virtualize_x2apic_mode is
>>> available.
>>>
>>> This will likely have some effect on patch 2 also, as you will have to
>>> adjust vmx_vlapic_msr_changed.
>>>
>>> Thanks, Roger.
>>
>> Any other thoughts on this? As on one hand it is asymmetric but also 
>> there isn't much assistance with only virtualize_x2apic_mode set as, in 
>> this case, a VM exit will be avoided only when trying to access the TPR 
>> register.
> 
> I've been thinking about this, and reporting hardware assisted
> x{2}APIC virtualization with just
> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or
> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While
> those provide some assistance to the VMM in order to handle APIC
> accesses, it will still require a trap into the hypervisor to handle
> most of the accesses.
> 
> So maybe we should only report hardware assisted support when the
> mentioned features are present together with
> SECONDARY_EXEC_APIC_REGISTER_VIRT?

Not sure - "some assistance" seems still a little better than none at all.
Which route to go depends on what exactly we intend the bit to be used for.

Jan

Jane Malalane Feb. 14, 2022, 1:11 p.m. UTC | #9

On 11/02/2022 11:46, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> 
> On 11.02.2022 12:29, Roger Pau Monné wrote:
>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote:
>>> On 10/02/2022 10:03, Roger Pau Monné wrote:
>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>> index 7ab15e07a0..4060aef1bd 100644
>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>>>>>                MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>>>>>        }
>>>>>    
>>>>> +    /* Check whether hardware supports accelerated xapic and x2apic. */
>>>>> +    if ( bsp )
>>>>> +    {
>>>>> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
>>>>> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
>>>>> +                                     cpu_has_vmx_virtual_intr_delivery) &&
>>>>> +                                    cpu_has_vmx_virtualize_x2apic_mode;
>>>>
>>>> I've been think about this, and it seems kind of asymmetric that for
>>>> xAPIC mode we report hw assisted support only with
>>>> virtualize_apic_accesses available, while for x2APIC we require
>>>> virtualize_x2apic_mode plus either apic_reg_virt or
>>>> virtual_intr_delivery.
>>>>
>>>> I think we likely need to be more consistent here, and report hw
>>>> assisted x2APIC support as long as virtualize_x2apic_mode is
>>>> available.
>>>>
>>>> This will likely have some effect on patch 2 also, as you will have to
>>>> adjust vmx_vlapic_msr_changed.
>>>>
>>>> Thanks, Roger.
>>>
>>> Any other thoughts on this? As on one hand it is asymmetric but also
>>> there isn't much assistance with only virtualize_x2apic_mode set as, in
>>> this case, a VM exit will be avoided only when trying to access the TPR
>>> register.
>>
>> I've been thinking about this, and reporting hardware assisted
>> x{2}APIC virtualization with just
>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or
>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While
>> those provide some assistance to the VMM in order to handle APIC
>> accesses, it will still require a trap into the hypervisor to handle
>> most of the accesses.
>>
>> So maybe we should only report hardware assisted support when the
>> mentioned features are present together with
>> SECONDARY_EXEC_APIC_REGISTER_VIRT?
> 
> Not sure - "some assistance" seems still a little better than none at all.
> Which route to go depends on what exactly we intend the bit to be used for.
> 
> Jan
> 
True. I intended this bit to be specifically for enabling 
assisted_x{2}apic. So, would it be inconsistent to report hardware 
assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE 
but still claim that x{2}apic is virtualized if no MSR accesses are 
intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you 
say, the guest gets at least "some assistance" instead of none but we 
still claim x{2}apic virtualization when it is actually complete? Maybe 
I could also add a comment alluding to this in the xl documentation.

Thanks,

Jane.

Jan Beulich Feb. 14, 2022, 1:18 p.m. UTC | #10

On 14.02.2022 14:11, Jane Malalane wrote:
> On 11/02/2022 11:46, Jan Beulich wrote:
>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>
>> On 11.02.2022 12:29, Roger Pau Monné wrote:
>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote:
>>>> On 10/02/2022 10:03, Roger Pau Monné wrote:
>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>> index 7ab15e07a0..4060aef1bd 100644
>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>>>>>>                MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>>>>>>        }
>>>>>>    
>>>>>> +    /* Check whether hardware supports accelerated xapic and x2apic. */
>>>>>> +    if ( bsp )
>>>>>> +    {
>>>>>> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
>>>>>> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
>>>>>> +                                     cpu_has_vmx_virtual_intr_delivery) &&
>>>>>> +                                    cpu_has_vmx_virtualize_x2apic_mode;
>>>>>
>>>>> I've been think about this, and it seems kind of asymmetric that for
>>>>> xAPIC mode we report hw assisted support only with
>>>>> virtualize_apic_accesses available, while for x2APIC we require
>>>>> virtualize_x2apic_mode plus either apic_reg_virt or
>>>>> virtual_intr_delivery.
>>>>>
>>>>> I think we likely need to be more consistent here, and report hw
>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is
>>>>> available.
>>>>>
>>>>> This will likely have some effect on patch 2 also, as you will have to
>>>>> adjust vmx_vlapic_msr_changed.
>>>>>
>>>>> Thanks, Roger.
>>>>
>>>> Any other thoughts on this? As on one hand it is asymmetric but also
>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in
>>>> this case, a VM exit will be avoided only when trying to access the TPR
>>>> register.
>>>
>>> I've been thinking about this, and reporting hardware assisted
>>> x{2}APIC virtualization with just
>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or
>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While
>>> those provide some assistance to the VMM in order to handle APIC
>>> accesses, it will still require a trap into the hypervisor to handle
>>> most of the accesses.
>>>
>>> So maybe we should only report hardware assisted support when the
>>> mentioned features are present together with
>>> SECONDARY_EXEC_APIC_REGISTER_VIRT?
>>
>> Not sure - "some assistance" seems still a little better than none at all.
>> Which route to go depends on what exactly we intend the bit to be used for.
>>
> True. I intended this bit to be specifically for enabling 
> assisted_x{2}apic. So, would it be inconsistent to report hardware 
> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE 
> but still claim that x{2}apic is virtualized if no MSR accesses are 
> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you 
> say, the guest gets at least "some assistance" instead of none but we 
> still claim x{2}apic virtualization when it is actually complete? Maybe 
> I could also add a comment alluding to this in the xl documentation.

To rephrase my earlier point: Which kind of decisions are the consumer(s)
of us reporting hardware assistance going to take? In how far is there a
risk that "some assistance" is overall going to lead to a loss of
performance? I guess I'd need to see comment and actual code all in one
place ...

Jan

Jane Malalane Feb. 14, 2022, 5:09 p.m. UTC | #11

On 14/02/2022 13:18, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> 
> On 14.02.2022 14:11, Jane Malalane wrote:
>> On 11/02/2022 11:46, Jan Beulich wrote:
>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>
>>> On 11.02.2022 12:29, Roger Pau Monné wrote:
>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote:
>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote:
>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>> index 7ab15e07a0..4060aef1bd 100644
>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>>>>>>>                 MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>>>>>>>         }
>>>>>>>     
>>>>>>> +    /* Check whether hardware supports accelerated xapic and x2apic. */
>>>>>>> +    if ( bsp )
>>>>>>> +    {
>>>>>>> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
>>>>>>> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
>>>>>>> +                                     cpu_has_vmx_virtual_intr_delivery) &&
>>>>>>> +                                    cpu_has_vmx_virtualize_x2apic_mode;
>>>>>>
>>>>>> I've been think about this, and it seems kind of asymmetric that for
>>>>>> xAPIC mode we report hw assisted support only with
>>>>>> virtualize_apic_accesses available, while for x2APIC we require
>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or
>>>>>> virtual_intr_delivery.
>>>>>>
>>>>>> I think we likely need to be more consistent here, and report hw
>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is
>>>>>> available.
>>>>>>
>>>>>> This will likely have some effect on patch 2 also, as you will have to
>>>>>> adjust vmx_vlapic_msr_changed.
>>>>>>
>>>>>> Thanks, Roger.
>>>>>
>>>>> Any other thoughts on this? As on one hand it is asymmetric but also
>>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in
>>>>> this case, a VM exit will be avoided only when trying to access the TPR
>>>>> register.
>>>>
>>>> I've been thinking about this, and reporting hardware assisted
>>>> x{2}APIC virtualization with just
>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or
>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While
>>>> those provide some assistance to the VMM in order to handle APIC
>>>> accesses, it will still require a trap into the hypervisor to handle
>>>> most of the accesses.
>>>>
>>>> So maybe we should only report hardware assisted support when the
>>>> mentioned features are present together with
>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT?
>>>
>>> Not sure - "some assistance" seems still a little better than none at all.
>>> Which route to go depends on what exactly we intend the bit to be used for.
>>>
>> True. I intended this bit to be specifically for enabling
>> assisted_x{2}apic. So, would it be inconsistent to report hardware
>> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE
>> but still claim that x{2}apic is virtualized if no MSR accesses are
>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you
>> say, the guest gets at least "some assistance" instead of none but we
>> still claim x{2}apic virtualization when it is actually complete? Maybe
>> I could also add a comment alluding to this in the xl documentation.
> 
> To rephrase my earlier point: Which kind of decisions are the consumer(s)
> of us reporting hardware assistance going to take? In how far is there a
> risk that "some assistance" is overall going to lead to a loss of
> performance? I guess I'd need to see comment and actual code all in one
> place ...
> 
So, I was thinking of adding something along the lines of:

+=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
+Enables or disables hardware assisted virtualization for xAPIC. This
+allows accessing APIC registers without a VM-exit. Notice enabling
+this does not guarantee full virtualization for xAPIC, as this can
+only be achieved if hardware supports “APIC-register virtualization”
+and “virtual-interrupt delivery”. The default is settable via
+L<xl.conf(5)>.

and going for assisted_x2apic_available = 
cpu_has_vmx_virtualize_x2apic_mode.

This would prevent the customer from expecting full acceleration when 
apic_register_virt and/or virtual_intr_delivery aren't available whilst 
still offering some if they are not available as Xen currently does. In 
a future patch, we could also expose and add config options for these 
controls if we wanted to.

Thank you for your help,

Jane.

Jan Beulich Feb. 15, 2022, 7:09 a.m. UTC | #12

On 14.02.2022 18:09, Jane Malalane wrote:
> On 14/02/2022 13:18, Jan Beulich wrote:
>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>
>> On 14.02.2022 14:11, Jane Malalane wrote:
>>> On 11/02/2022 11:46, Jan Beulich wrote:
>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>>
>>>> On 11.02.2022 12:29, Roger Pau Monné wrote:
>>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote:
>>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote:
>>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
>>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>> index 7ab15e07a0..4060aef1bd 100644
>>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>>>>>>>>                 MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>>>>>>>>         }
>>>>>>>>     
>>>>>>>> +    /* Check whether hardware supports accelerated xapic and x2apic. */
>>>>>>>> +    if ( bsp )
>>>>>>>> +    {
>>>>>>>> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
>>>>>>>> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
>>>>>>>> +                                     cpu_has_vmx_virtual_intr_delivery) &&
>>>>>>>> +                                    cpu_has_vmx_virtualize_x2apic_mode;
>>>>>>>
>>>>>>> I've been think about this, and it seems kind of asymmetric that for
>>>>>>> xAPIC mode we report hw assisted support only with
>>>>>>> virtualize_apic_accesses available, while for x2APIC we require
>>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or
>>>>>>> virtual_intr_delivery.
>>>>>>>
>>>>>>> I think we likely need to be more consistent here, and report hw
>>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is
>>>>>>> available.
>>>>>>>
>>>>>>> This will likely have some effect on patch 2 also, as you will have to
>>>>>>> adjust vmx_vlapic_msr_changed.
>>>>>>>
>>>>>>> Thanks, Roger.
>>>>>>
>>>>>> Any other thoughts on this? As on one hand it is asymmetric but also
>>>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in
>>>>>> this case, a VM exit will be avoided only when trying to access the TPR
>>>>>> register.
>>>>>
>>>>> I've been thinking about this, and reporting hardware assisted
>>>>> x{2}APIC virtualization with just
>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or
>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While
>>>>> those provide some assistance to the VMM in order to handle APIC
>>>>> accesses, it will still require a trap into the hypervisor to handle
>>>>> most of the accesses.
>>>>>
>>>>> So maybe we should only report hardware assisted support when the
>>>>> mentioned features are present together with
>>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT?
>>>>
>>>> Not sure - "some assistance" seems still a little better than none at all.
>>>> Which route to go depends on what exactly we intend the bit to be used for.
>>>>
>>> True. I intended this bit to be specifically for enabling
>>> assisted_x{2}apic. So, would it be inconsistent to report hardware
>>> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE
>>> but still claim that x{2}apic is virtualized if no MSR accesses are
>>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you
>>> say, the guest gets at least "some assistance" instead of none but we
>>> still claim x{2}apic virtualization when it is actually complete? Maybe
>>> I could also add a comment alluding to this in the xl documentation.
>>
>> To rephrase my earlier point: Which kind of decisions are the consumer(s)
>> of us reporting hardware assistance going to take? In how far is there a
>> risk that "some assistance" is overall going to lead to a loss of
>> performance? I guess I'd need to see comment and actual code all in one
>> place ...
>>
> So, I was thinking of adding something along the lines of:
> 
> +=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
> +Enables or disables hardware assisted virtualization for xAPIC. This
> +allows accessing APIC registers without a VM-exit. Notice enabling
> +this does not guarantee full virtualization for xAPIC, as this can
> +only be achieved if hardware supports “APIC-register virtualization”
> +and “virtual-interrupt delivery”. The default is settable via
> +L<xl.conf(5)>.

But isn't this contradictory? Doesn't lack of APIC-register virtualization
mean VM exits upon (most) accesses?

Jan

> and going for assisted_x2apic_available = 
> cpu_has_vmx_virtualize_x2apic_mode.
> 
> This would prevent the customer from expecting full acceleration when 
> apic_register_virt and/or virtual_intr_delivery aren't available whilst 
> still offering some if they are not available as Xen currently does. In 
> a future patch, we could also expose and add config options for these 
> controls if we wanted to.
> 
> Thank you for your help,
> 
> Jane.

Jane Malalane Feb. 15, 2022, 10:14 a.m. UTC | #13

On 15/02/2022 07:09, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> 
> On 14.02.2022 18:09, Jane Malalane wrote:
>> On 14/02/2022 13:18, Jan Beulich wrote:
>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>
>>> On 14.02.2022 14:11, Jane Malalane wrote:
>>>> On 11/02/2022 11:46, Jan Beulich wrote:
>>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>>>
>>>>> On 11.02.2022 12:29, Roger Pau Monné wrote:
>>>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote:
>>>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote:
>>>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
>>>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>>> index 7ab15e07a0..4060aef1bd 100644
>>>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>>>>>>>>>                  MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>>>>>>>>>          }
>>>>>>>>>      
>>>>>>>>> +    /* Check whether hardware supports accelerated xapic and x2apic. */
>>>>>>>>> +    if ( bsp )
>>>>>>>>> +    {
>>>>>>>>> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
>>>>>>>>> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
>>>>>>>>> +                                     cpu_has_vmx_virtual_intr_delivery) &&
>>>>>>>>> +                                    cpu_has_vmx_virtualize_x2apic_mode;
>>>>>>>>
>>>>>>>> I've been think about this, and it seems kind of asymmetric that for
>>>>>>>> xAPIC mode we report hw assisted support only with
>>>>>>>> virtualize_apic_accesses available, while for x2APIC we require
>>>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or
>>>>>>>> virtual_intr_delivery.
>>>>>>>>
>>>>>>>> I think we likely need to be more consistent here, and report hw
>>>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is
>>>>>>>> available.
>>>>>>>>
>>>>>>>> This will likely have some effect on patch 2 also, as you will have to
>>>>>>>> adjust vmx_vlapic_msr_changed.
>>>>>>>>
>>>>>>>> Thanks, Roger.
>>>>>>>
>>>>>>> Any other thoughts on this? As on one hand it is asymmetric but also
>>>>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in
>>>>>>> this case, a VM exit will be avoided only when trying to access the TPR
>>>>>>> register.
>>>>>>
>>>>>> I've been thinking about this, and reporting hardware assisted
>>>>>> x{2}APIC virtualization with just
>>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or
>>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While
>>>>>> those provide some assistance to the VMM in order to handle APIC
>>>>>> accesses, it will still require a trap into the hypervisor to handle
>>>>>> most of the accesses.
>>>>>>
>>>>>> So maybe we should only report hardware assisted support when the
>>>>>> mentioned features are present together with
>>>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT?
>>>>>
>>>>> Not sure - "some assistance" seems still a little better than none at all.
>>>>> Which route to go depends on what exactly we intend the bit to be used for.
>>>>>
>>>> True. I intended this bit to be specifically for enabling
>>>> assisted_x{2}apic. So, would it be inconsistent to report hardware
>>>> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE
>>>> but still claim that x{2}apic is virtualized if no MSR accesses are
>>>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you
>>>> say, the guest gets at least "some assistance" instead of none but we
>>>> still claim x{2}apic virtualization when it is actually complete? Maybe
>>>> I could also add a comment alluding to this in the xl documentation.
>>>
>>> To rephrase my earlier point: Which kind of decisions are the consumer(s)
>>> of us reporting hardware assistance going to take? In how far is there a
>>> risk that "some assistance" is overall going to lead to a loss of
>>> performance? I guess I'd need to see comment and actual code all in one
>>> place ...
>>>
>> So, I was thinking of adding something along the lines of:
>>
>> +=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
>> +Enables or disables hardware assisted virtualization for xAPIC. This
>> +allows accessing APIC registers without a VM-exit. Notice enabling
>> +this does not guarantee full virtualization for xAPIC, as this can
>> +only be achieved if hardware supports “APIC-register virtualization”
>> +and “virtual-interrupt delivery”. The default is settable via
>> +L<xl.conf(5)>.
> 
> But isn't this contradictory? Doesn't lack of APIC-register virtualization
> mean VM exits upon (most) accesses?

Yes, it does mean. I guess the alternative wouuld be then to require 
APIC-register virtualization for enabling xAPIC. But also, although this 
doesn't provide much acceleration, even getting a VM exit is some 
assistance if compared to instead getting an EPT fault and having to 
decode the access.

Thanks,

Jane.

Jan Beulich Feb. 15, 2022, 10:19 a.m. UTC | #14

On 15.02.2022 11:14, Jane Malalane wrote:
> On 15/02/2022 07:09, Jan Beulich wrote:
>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>
>> On 14.02.2022 18:09, Jane Malalane wrote:
>>> On 14/02/2022 13:18, Jan Beulich wrote:
>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>>
>>>> On 14.02.2022 14:11, Jane Malalane wrote:
>>>>> On 11/02/2022 11:46, Jan Beulich wrote:
>>>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>>>>
>>>>>> On 11.02.2022 12:29, Roger Pau Monné wrote:
>>>>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote:
>>>>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote:
>>>>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
>>>>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>>>> index 7ab15e07a0..4060aef1bd 100644
>>>>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>>>>>>>>>>                  MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>>>>>>>>>>          }
>>>>>>>>>>      
>>>>>>>>>> +    /* Check whether hardware supports accelerated xapic and x2apic. */
>>>>>>>>>> +    if ( bsp )
>>>>>>>>>> +    {
>>>>>>>>>> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
>>>>>>>>>> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
>>>>>>>>>> +                                     cpu_has_vmx_virtual_intr_delivery) &&
>>>>>>>>>> +                                    cpu_has_vmx_virtualize_x2apic_mode;
>>>>>>>>>
>>>>>>>>> I've been think about this, and it seems kind of asymmetric that for
>>>>>>>>> xAPIC mode we report hw assisted support only with
>>>>>>>>> virtualize_apic_accesses available, while for x2APIC we require
>>>>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or
>>>>>>>>> virtual_intr_delivery.
>>>>>>>>>
>>>>>>>>> I think we likely need to be more consistent here, and report hw
>>>>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is
>>>>>>>>> available.
>>>>>>>>>
>>>>>>>>> This will likely have some effect on patch 2 also, as you will have to
>>>>>>>>> adjust vmx_vlapic_msr_changed.
>>>>>>>>>
>>>>>>>>> Thanks, Roger.
>>>>>>>>
>>>>>>>> Any other thoughts on this? As on one hand it is asymmetric but also
>>>>>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in
>>>>>>>> this case, a VM exit will be avoided only when trying to access the TPR
>>>>>>>> register.
>>>>>>>
>>>>>>> I've been thinking about this, and reporting hardware assisted
>>>>>>> x{2}APIC virtualization with just
>>>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or
>>>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While
>>>>>>> those provide some assistance to the VMM in order to handle APIC
>>>>>>> accesses, it will still require a trap into the hypervisor to handle
>>>>>>> most of the accesses.
>>>>>>>
>>>>>>> So maybe we should only report hardware assisted support when the
>>>>>>> mentioned features are present together with
>>>>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT?
>>>>>>
>>>>>> Not sure - "some assistance" seems still a little better than none at all.
>>>>>> Which route to go depends on what exactly we intend the bit to be used for.
>>>>>>
>>>>> True. I intended this bit to be specifically for enabling
>>>>> assisted_x{2}apic. So, would it be inconsistent to report hardware
>>>>> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE
>>>>> but still claim that x{2}apic is virtualized if no MSR accesses are
>>>>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you
>>>>> say, the guest gets at least "some assistance" instead of none but we
>>>>> still claim x{2}apic virtualization when it is actually complete? Maybe
>>>>> I could also add a comment alluding to this in the xl documentation.
>>>>
>>>> To rephrase my earlier point: Which kind of decisions are the consumer(s)
>>>> of us reporting hardware assistance going to take? In how far is there a
>>>> risk that "some assistance" is overall going to lead to a loss of
>>>> performance? I guess I'd need to see comment and actual code all in one
>>>> place ...
>>>>
>>> So, I was thinking of adding something along the lines of:
>>>
>>> +=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
>>> +Enables or disables hardware assisted virtualization for xAPIC. This
>>> +allows accessing APIC registers without a VM-exit. Notice enabling
>>> +this does not guarantee full virtualization for xAPIC, as this can
>>> +only be achieved if hardware supports “APIC-register virtualization”
>>> +and “virtual-interrupt delivery”. The default is settable via
>>> +L<xl.conf(5)>.
>>
>> But isn't this contradictory? Doesn't lack of APIC-register virtualization
>> mean VM exits upon (most) accesses?
> 
> Yes, it does mean. I guess the alternative wouuld be then to require 
> APIC-register virtualization for enabling xAPIC. But also, although this 
> doesn't provide much acceleration, even getting a VM exit is some 
> assistance if compared to instead getting an EPT fault and having to 
> decode the access.

I agree here, albeit I'd like to mention that EPT faults are also VM
exits. All my earlier comment was about is that this piece of doc
wants to express reality, whichever way it is that things end up
being implemented.

Jan

Jane Malalane Feb. 15, 2022, 3:10 p.m. UTC | #15

On 15/02/2022 10:19, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> 
> On 15.02.2022 11:14, Jane Malalane wrote:
>> On 15/02/2022 07:09, Jan Beulich wrote:
>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>
>>> On 14.02.2022 18:09, Jane Malalane wrote:
>>>> On 14/02/2022 13:18, Jan Beulich wrote:
>>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>>>
>>>>> On 14.02.2022 14:11, Jane Malalane wrote:
>>>>>> On 11/02/2022 11:46, Jan Beulich wrote:
>>>>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>>>>>
>>>>>>> On 11.02.2022 12:29, Roger Pau Monné wrote:
>>>>>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote:
>>>>>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote:
>>>>>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
>>>>>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>>>>> index 7ab15e07a0..4060aef1bd 100644
>>>>>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>>>>>>>>>>>                   MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>>>>>>>>>>>           }
>>>>>>>>>>>       
>>>>>>>>>>> +    /* Check whether hardware supports accelerated xapic and x2apic. */
>>>>>>>>>>> +    if ( bsp )
>>>>>>>>>>> +    {
>>>>>>>>>>> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
>>>>>>>>>>> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
>>>>>>>>>>> +                                     cpu_has_vmx_virtual_intr_delivery) &&
>>>>>>>>>>> +                                    cpu_has_vmx_virtualize_x2apic_mode;
>>>>>>>>>>
>>>>>>>>>> I've been think about this, and it seems kind of asymmetric that for
>>>>>>>>>> xAPIC mode we report hw assisted support only with
>>>>>>>>>> virtualize_apic_accesses available, while for x2APIC we require
>>>>>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or
>>>>>>>>>> virtual_intr_delivery.
>>>>>>>>>>
>>>>>>>>>> I think we likely need to be more consistent here, and report hw
>>>>>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is
>>>>>>>>>> available.
>>>>>>>>>>
>>>>>>>>>> This will likely have some effect on patch 2 also, as you will have to
>>>>>>>>>> adjust vmx_vlapic_msr_changed.
>>>>>>>>>>
>>>>>>>>>> Thanks, Roger.
>>>>>>>>>
>>>>>>>>> Any other thoughts on this? As on one hand it is asymmetric but also
>>>>>>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in
>>>>>>>>> this case, a VM exit will be avoided only when trying to access the TPR
>>>>>>>>> register.
>>>>>>>>
>>>>>>>> I've been thinking about this, and reporting hardware assisted
>>>>>>>> x{2}APIC virtualization with just
>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or
>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While
>>>>>>>> those provide some assistance to the VMM in order to handle APIC
>>>>>>>> accesses, it will still require a trap into the hypervisor to handle
>>>>>>>> most of the accesses.
>>>>>>>>
>>>>>>>> So maybe we should only report hardware assisted support when the
>>>>>>>> mentioned features are present together with
>>>>>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT?
>>>>>>>
>>>>>>> Not sure - "some assistance" seems still a little better than none at all.
>>>>>>> Which route to go depends on what exactly we intend the bit to be used for.
>>>>>>>
>>>>>> True. I intended this bit to be specifically for enabling
>>>>>> assisted_x{2}apic. So, would it be inconsistent to report hardware
>>>>>> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE
>>>>>> but still claim that x{2}apic is virtualized if no MSR accesses are
>>>>>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you
>>>>>> say, the guest gets at least "some assistance" instead of none but we
>>>>>> still claim x{2}apic virtualization when it is actually complete? Maybe
>>>>>> I could also add a comment alluding to this in the xl documentation.
>>>>>
>>>>> To rephrase my earlier point: Which kind of decisions are the consumer(s)
>>>>> of us reporting hardware assistance going to take? In how far is there a
>>>>> risk that "some assistance" is overall going to lead to a loss of
>>>>> performance? I guess I'd need to see comment and actual code all in one
>>>>> place ...
>>>>>
>>>> So, I was thinking of adding something along the lines of:
>>>>
>>>> +=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
>>>> +Enables or disables hardware assisted virtualization for xAPIC. This
>>>> +allows accessing APIC registers without a VM-exit. Notice enabling
>>>> +this does not guarantee full virtualization for xAPIC, as this can
>>>> +only be achieved if hardware supports “APIC-register virtualization”
>>>> +and “virtual-interrupt delivery”. The default is settable via
>>>> +L<xl.conf(5)>.
>>>
>>> But isn't this contradictory? Doesn't lack of APIC-register virtualization
>>> mean VM exits upon (most) accesses?
>>
>> Yes, it does mean. I guess the alternative wouuld be then to require
>> APIC-register virtualization for enabling xAPIC. But also, although this
>> doesn't provide much acceleration, even getting a VM exit is some
>> assistance if compared to instead getting an EPT fault and having to
>> decode the access.
> 
> I agree here, albeit I'd like to mention that EPT faults are also VM
> exits. All my earlier comment was about is that this piece of doc
> wants to express reality, whichever way it is that things end up
> being implemented.

Oh yes. Right, I see how this info could be misleading.

How about this?...

+=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
+
+B<(x86 only)> Enables or disables hardware assisted virtualization for
+xAPIC. With this option enabled, a memory-mapped APIC access will be
+decoded by hardware and either issue a VM exit with an exit reason
+instead of an EPT fault or altogether avoid a VM exit. Notice
+full virtualization for xAPIC can only be achieved if hardware
+supports “APIC-register virtualization” and “virtual-interrupt
+delivery”. The default is settable via L<xl.conf(5)>.

+=item B<assisted_x2apic=BOOLEAN>
+
+B<(x86 only)> Enables or disables hardware assisted virtualization for
+x2APIC. With this option enabled, an MSR-Based APIC access will either
+issue a VM exit or altogether avoid one. Notice full virtualization
+for x2APIC can only be achieved if hardware supports “APIC-register
+virtualization” and “virtual-interrupt delivery”. The default is
+settable via L<xl.conf(5)>.


...because with only VIRTUALIZE_APIC_ACCESSES enabled, hardware decodes 
accesses to the xAPIC page and the VM exit gives an exit reason.
And if VIRTUALIZE_X2APIC_MODE is set, although no assistance is provided 
w.r.t. to decoding x2APIC accesses as the MSR that the VM tried to 
access is already part of the vmexit information, VM exits for accesses 
to the TPR MSR are avoided, regardless of whether shadow TPR is set or 
not for e.g.

I hope this makes sense but I welcome any other suggestions/corrections.

Thank you,

Jane.

Jan Beulich Feb. 15, 2022, 3:21 p.m. UTC | #16

On 15.02.2022 16:10, Jane Malalane wrote:
> On 15/02/2022 10:19, Jan Beulich wrote:
>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>
>> On 15.02.2022 11:14, Jane Malalane wrote:
>>> On 15/02/2022 07:09, Jan Beulich wrote:
>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>>
>>>> On 14.02.2022 18:09, Jane Malalane wrote:
>>>>> On 14/02/2022 13:18, Jan Beulich wrote:
>>>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>>>>
>>>>>> On 14.02.2022 14:11, Jane Malalane wrote:
>>>>>>> On 11/02/2022 11:46, Jan Beulich wrote:
>>>>>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>>>>>>
>>>>>>>> On 11.02.2022 12:29, Roger Pau Monné wrote:
>>>>>>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote:
>>>>>>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote:
>>>>>>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
>>>>>>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>>>>>> index 7ab15e07a0..4060aef1bd 100644
>>>>>>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>>>>>>>>>>>>                   MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>>>>>>>>>>>>           }
>>>>>>>>>>>>       
>>>>>>>>>>>> +    /* Check whether hardware supports accelerated xapic and x2apic. */
>>>>>>>>>>>> +    if ( bsp )
>>>>>>>>>>>> +    {
>>>>>>>>>>>> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
>>>>>>>>>>>> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
>>>>>>>>>>>> +                                     cpu_has_vmx_virtual_intr_delivery) &&
>>>>>>>>>>>> +                                    cpu_has_vmx_virtualize_x2apic_mode;
>>>>>>>>>>>
>>>>>>>>>>> I've been think about this, and it seems kind of asymmetric that for
>>>>>>>>>>> xAPIC mode we report hw assisted support only with
>>>>>>>>>>> virtualize_apic_accesses available, while for x2APIC we require
>>>>>>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or
>>>>>>>>>>> virtual_intr_delivery.
>>>>>>>>>>>
>>>>>>>>>>> I think we likely need to be more consistent here, and report hw
>>>>>>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is
>>>>>>>>>>> available.
>>>>>>>>>>>
>>>>>>>>>>> This will likely have some effect on patch 2 also, as you will have to
>>>>>>>>>>> adjust vmx_vlapic_msr_changed.
>>>>>>>>>>>
>>>>>>>>>>> Thanks, Roger.
>>>>>>>>>>
>>>>>>>>>> Any other thoughts on this? As on one hand it is asymmetric but also
>>>>>>>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in
>>>>>>>>>> this case, a VM exit will be avoided only when trying to access the TPR
>>>>>>>>>> register.
>>>>>>>>>
>>>>>>>>> I've been thinking about this, and reporting hardware assisted
>>>>>>>>> x{2}APIC virtualization with just
>>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or
>>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While
>>>>>>>>> those provide some assistance to the VMM in order to handle APIC
>>>>>>>>> accesses, it will still require a trap into the hypervisor to handle
>>>>>>>>> most of the accesses.
>>>>>>>>>
>>>>>>>>> So maybe we should only report hardware assisted support when the
>>>>>>>>> mentioned features are present together with
>>>>>>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT?
>>>>>>>>
>>>>>>>> Not sure - "some assistance" seems still a little better than none at all.
>>>>>>>> Which route to go depends on what exactly we intend the bit to be used for.
>>>>>>>>
>>>>>>> True. I intended this bit to be specifically for enabling
>>>>>>> assisted_x{2}apic. So, would it be inconsistent to report hardware
>>>>>>> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE
>>>>>>> but still claim that x{2}apic is virtualized if no MSR accesses are
>>>>>>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you
>>>>>>> say, the guest gets at least "some assistance" instead of none but we
>>>>>>> still claim x{2}apic virtualization when it is actually complete? Maybe
>>>>>>> I could also add a comment alluding to this in the xl documentation.
>>>>>>
>>>>>> To rephrase my earlier point: Which kind of decisions are the consumer(s)
>>>>>> of us reporting hardware assistance going to take? In how far is there a
>>>>>> risk that "some assistance" is overall going to lead to a loss of
>>>>>> performance? I guess I'd need to see comment and actual code all in one
>>>>>> place ...
>>>>>>
>>>>> So, I was thinking of adding something along the lines of:
>>>>>
>>>>> +=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
>>>>> +Enables or disables hardware assisted virtualization for xAPIC. This
>>>>> +allows accessing APIC registers without a VM-exit. Notice enabling
>>>>> +this does not guarantee full virtualization for xAPIC, as this can
>>>>> +only be achieved if hardware supports “APIC-register virtualization”
>>>>> +and “virtual-interrupt delivery”. The default is settable via
>>>>> +L<xl.conf(5)>.
>>>>
>>>> But isn't this contradictory? Doesn't lack of APIC-register virtualization
>>>> mean VM exits upon (most) accesses?
>>>
>>> Yes, it does mean. I guess the alternative wouuld be then to require
>>> APIC-register virtualization for enabling xAPIC. But also, although this
>>> doesn't provide much acceleration, even getting a VM exit is some
>>> assistance if compared to instead getting an EPT fault and having to
>>> decode the access.
>>
>> I agree here, albeit I'd like to mention that EPT faults are also VM
>> exits. All my earlier comment was about is that this piece of doc
>> wants to express reality, whichever way it is that things end up
>> being implemented.
> 
> Oh yes. Right, I see how this info could be misleading.
> 
> How about this?...

Getting close. The thing I can't judge is whether this level of technical
detail is suitable for this doc. Just one further remark:

> +=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
> +
> +B<(x86 only)> Enables or disables hardware assisted virtualization for
> +xAPIC. With this option enabled, a memory-mapped APIC access will be
> +decoded by hardware and either issue a VM exit with an exit reason
> +instead of an EPT fault or altogether avoid a VM exit. Notice

As said before, EPT faults also are VM exits and also provide an exit
reason. Therefore maybe "... and either issue a VM exit with a more
specific exit reason than an EPT fault would provide, or altogether
avoid a VM exit" or "... and either issue a more specific VM exit than
just an EPT fault, or altogether avoid a VM exit"?

Jan

> +full virtualization for xAPIC can only be achieved if hardware
> +supports “APIC-register virtualization” and “virtual-interrupt
> +delivery”. The default is settable via L<xl.conf(5)>.
> 
> +=item B<assisted_x2apic=BOOLEAN>
> +
> +B<(x86 only)> Enables or disables hardware assisted virtualization for
> +x2APIC. With this option enabled, an MSR-Based APIC access will either
> +issue a VM exit or altogether avoid one. Notice full virtualization
> +for x2APIC can only be achieved if hardware supports “APIC-register
> +virtualization” and “virtual-interrupt delivery”. The default is
> +settable via L<xl.conf(5)>.
> 
> 
> ...because with only VIRTUALIZE_APIC_ACCESSES enabled, hardware decodes 
> accesses to the xAPIC page and the VM exit gives an exit reason.
> And if VIRTUALIZE_X2APIC_MODE is set, although no assistance is provided 
> w.r.t. to decoding x2APIC accesses as the MSR that the VM tried to 
> access is already part of the vmexit information, VM exits for accesses 
> to the TPR MSR are avoided, regardless of whether shadow TPR is set or 
> not for e.g.
> 
> I hope this makes sense but I welcome any other suggestions/corrections.
> 
> Thank you,
> 
> Jane.

Jane Malalane Feb. 15, 2022, 4:33 p.m. UTC | #17

On 15/02/2022 15:21, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> 
> On 15.02.2022 16:10, Jane Malalane wrote:
>> On 15/02/2022 10:19, Jan Beulich wrote:
>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>
>>> On 15.02.2022 11:14, Jane Malalane wrote:
>>>> On 15/02/2022 07:09, Jan Beulich wrote:
>>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>>>
>>>>> On 14.02.2022 18:09, Jane Malalane wrote:
>>>>>> On 14/02/2022 13:18, Jan Beulich wrote:
>>>>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>>>>>
>>>>>>> On 14.02.2022 14:11, Jane Malalane wrote:
>>>>>>>> On 11/02/2022 11:46, Jan Beulich wrote:
>>>>>>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
>>>>>>>>>
>>>>>>>>> On 11.02.2022 12:29, Roger Pau Monné wrote:
>>>>>>>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote:
>>>>>>>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote:
>>>>>>>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
>>>>>>>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>>>>>>> index 7ab15e07a0..4060aef1bd 100644
>>>>>>>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>>>>>>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>>>>>>>>>>>>>                    MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>>>>>>>>>>>>>            }
>>>>>>>>>>>>>        
>>>>>>>>>>>>> +    /* Check whether hardware supports accelerated xapic and x2apic. */
>>>>>>>>>>>>> +    if ( bsp )
>>>>>>>>>>>>> +    {
>>>>>>>>>>>>> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
>>>>>>>>>>>>> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
>>>>>>>>>>>>> +                                     cpu_has_vmx_virtual_intr_delivery) &&
>>>>>>>>>>>>> +                                    cpu_has_vmx_virtualize_x2apic_mode;
>>>>>>>>>>>>
>>>>>>>>>>>> I've been think about this, and it seems kind of asymmetric that for
>>>>>>>>>>>> xAPIC mode we report hw assisted support only with
>>>>>>>>>>>> virtualize_apic_accesses available, while for x2APIC we require
>>>>>>>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or
>>>>>>>>>>>> virtual_intr_delivery.
>>>>>>>>>>>>
>>>>>>>>>>>> I think we likely need to be more consistent here, and report hw
>>>>>>>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is
>>>>>>>>>>>> available.
>>>>>>>>>>>>
>>>>>>>>>>>> This will likely have some effect on patch 2 also, as you will have to
>>>>>>>>>>>> adjust vmx_vlapic_msr_changed.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks, Roger.
>>>>>>>>>>>
>>>>>>>>>>> Any other thoughts on this? As on one hand it is asymmetric but also
>>>>>>>>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in
>>>>>>>>>>> this case, a VM exit will be avoided only when trying to access the TPR
>>>>>>>>>>> register.
>>>>>>>>>>
>>>>>>>>>> I've been thinking about this, and reporting hardware assisted
>>>>>>>>>> x{2}APIC virtualization with just
>>>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or
>>>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While
>>>>>>>>>> those provide some assistance to the VMM in order to handle APIC
>>>>>>>>>> accesses, it will still require a trap into the hypervisor to handle
>>>>>>>>>> most of the accesses.
>>>>>>>>>>
>>>>>>>>>> So maybe we should only report hardware assisted support when the
>>>>>>>>>> mentioned features are present together with
>>>>>>>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT?
>>>>>>>>>
>>>>>>>>> Not sure - "some assistance" seems still a little better than none at all.
>>>>>>>>> Which route to go depends on what exactly we intend the bit to be used for.
>>>>>>>>>
>>>>>>>> True. I intended this bit to be specifically for enabling
>>>>>>>> assisted_x{2}apic. So, would it be inconsistent to report hardware
>>>>>>>> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE
>>>>>>>> but still claim that x{2}apic is virtualized if no MSR accesses are
>>>>>>>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you
>>>>>>>> say, the guest gets at least "some assistance" instead of none but we
>>>>>>>> still claim x{2}apic virtualization when it is actually complete? Maybe
>>>>>>>> I could also add a comment alluding to this in the xl documentation.
>>>>>>>
>>>>>>> To rephrase my earlier point: Which kind of decisions are the consumer(s)
>>>>>>> of us reporting hardware assistance going to take? In how far is there a
>>>>>>> risk that "some assistance" is overall going to lead to a loss of
>>>>>>> performance? I guess I'd need to see comment and actual code all in one
>>>>>>> place ...
>>>>>>>
>>>>>> So, I was thinking of adding something along the lines of:
>>>>>>
>>>>>> +=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
>>>>>> +Enables or disables hardware assisted virtualization for xAPIC. This
>>>>>> +allows accessing APIC registers without a VM-exit. Notice enabling
>>>>>> +this does not guarantee full virtualization for xAPIC, as this can
>>>>>> +only be achieved if hardware supports “APIC-register virtualization”
>>>>>> +and “virtual-interrupt delivery”. The default is settable via
>>>>>> +L<xl.conf(5)>.
>>>>>
>>>>> But isn't this contradictory? Doesn't lack of APIC-register virtualization
>>>>> mean VM exits upon (most) accesses?
>>>>
>>>> Yes, it does mean. I guess the alternative wouuld be then to require
>>>> APIC-register virtualization for enabling xAPIC. But also, although this
>>>> doesn't provide much acceleration, even getting a VM exit is some
>>>> assistance if compared to instead getting an EPT fault and having to
>>>> decode the access.
>>>
>>> I agree here, albeit I'd like to mention that EPT faults are also VM
>>> exits. All my earlier comment was about is that this piece of doc
>>> wants to express reality, whichever way it is that things end up
>>> being implemented.
>>
>> Oh yes. Right, I see how this info could be misleading.
>>
>> How about this?...
> 
> Getting close. The thing I can't judge is whether this level of technical
> detail is suitable for this doc. Just one further remark:

Unsure too.

>> +=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
>> +
>> +B<(x86 only)> Enables or disables hardware assisted virtualization for
>> +xAPIC. With this option enabled, a memory-mapped APIC access will be
>> +decoded by hardware and either issue a VM exit with an exit reason
>> +instead of an EPT fault or altogether avoid a VM exit. Notice
> 
> As said before, EPT faults also are VM exits and also provide an exit
> reason. Therefore maybe "... and either issue a VM exit with a more
> specific exit reason than an EPT fault would provide, or altogether
> avoid a VM exit" or "... and either issue a more specific VM exit than
> just an EPT fault, or altogether avoid a VM exit"?

Yes, that's better.

Thank you,

Jane.

Roger Pau Monné March 9, 2022, 1:47 p.m. UTC | #18

On Tue, Feb 15, 2022 at 04:33:15PM +0000, Jane Malalane wrote:
> On 15/02/2022 15:21, Jan Beulich wrote:
> > [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> > 
> > On 15.02.2022 16:10, Jane Malalane wrote:
> >> On 15/02/2022 10:19, Jan Beulich wrote:
> >>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> >>>
> >>> On 15.02.2022 11:14, Jane Malalane wrote:
> >>>> On 15/02/2022 07:09, Jan Beulich wrote:
> >>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> >>>>>
> >>>>> On 14.02.2022 18:09, Jane Malalane wrote:
> >>>>>> On 14/02/2022 13:18, Jan Beulich wrote:
> >>>>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> >>>>>>>
> >>>>>>> On 14.02.2022 14:11, Jane Malalane wrote:
> >>>>>>>> On 11/02/2022 11:46, Jan Beulich wrote:
> >>>>>>>>> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> >>>>>>>>>
> >>>>>>>>> On 11.02.2022 12:29, Roger Pau Monné wrote:
> >>>>>>>>>> On Fri, Feb 11, 2022 at 10:06:48AM +0000, Jane Malalane wrote:
> >>>>>>>>>>> On 10/02/2022 10:03, Roger Pau Monné wrote:
> >>>>>>>>>>>> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
> >>>>>>>>>>>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> >>>>>>>>>>>>> index 7ab15e07a0..4060aef1bd 100644
> >>>>>>>>>>>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> >>>>>>>>>>>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> >>>>>>>>>>>>> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
> >>>>>>>>>>>>>                    MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
> >>>>>>>>>>>>>            }
> >>>>>>>>>>>>>        
> >>>>>>>>>>>>> +    /* Check whether hardware supports accelerated xapic and x2apic. */
> >>>>>>>>>>>>> +    if ( bsp )
> >>>>>>>>>>>>> +    {
> >>>>>>>>>>>>> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
> >>>>>>>>>>>>> +        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
> >>>>>>>>>>>>> +                                     cpu_has_vmx_virtual_intr_delivery) &&
> >>>>>>>>>>>>> +                                    cpu_has_vmx_virtualize_x2apic_mode;
> >>>>>>>>>>>>
> >>>>>>>>>>>> I've been think about this, and it seems kind of asymmetric that for
> >>>>>>>>>>>> xAPIC mode we report hw assisted support only with
> >>>>>>>>>>>> virtualize_apic_accesses available, while for x2APIC we require
> >>>>>>>>>>>> virtualize_x2apic_mode plus either apic_reg_virt or
> >>>>>>>>>>>> virtual_intr_delivery.
> >>>>>>>>>>>>
> >>>>>>>>>>>> I think we likely need to be more consistent here, and report hw
> >>>>>>>>>>>> assisted x2APIC support as long as virtualize_x2apic_mode is
> >>>>>>>>>>>> available.
> >>>>>>>>>>>>
> >>>>>>>>>>>> This will likely have some effect on patch 2 also, as you will have to
> >>>>>>>>>>>> adjust vmx_vlapic_msr_changed.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Thanks, Roger.
> >>>>>>>>>>>
> >>>>>>>>>>> Any other thoughts on this? As on one hand it is asymmetric but also
> >>>>>>>>>>> there isn't much assistance with only virtualize_x2apic_mode set as, in
> >>>>>>>>>>> this case, a VM exit will be avoided only when trying to access the TPR
> >>>>>>>>>>> register.
> >>>>>>>>>>
> >>>>>>>>>> I've been thinking about this, and reporting hardware assisted
> >>>>>>>>>> x{2}APIC virtualization with just
> >>>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES or
> >>>>>>>>>> SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE doesn't seem very helpful. While
> >>>>>>>>>> those provide some assistance to the VMM in order to handle APIC
> >>>>>>>>>> accesses, it will still require a trap into the hypervisor to handle
> >>>>>>>>>> most of the accesses.
> >>>>>>>>>>
> >>>>>>>>>> So maybe we should only report hardware assisted support when the
> >>>>>>>>>> mentioned features are present together with
> >>>>>>>>>> SECONDARY_EXEC_APIC_REGISTER_VIRT?
> >>>>>>>>>
> >>>>>>>>> Not sure - "some assistance" seems still a little better than none at all.
> >>>>>>>>> Which route to go depends on what exactly we intend the bit to be used for.
> >>>>>>>>>
> >>>>>>>> True. I intended this bit to be specifically for enabling
> >>>>>>>> assisted_x{2}apic. So, would it be inconsistent to report hardware
> >>>>>>>> assistance with just VIRTUALIZE_APIC_ACCESSES or VIRTUALIZE_X2APIC_MODE
> >>>>>>>> but still claim that x{2}apic is virtualized if no MSR accesses are
> >>>>>>>> intercepted with XEN_HVM_CPUID_X2APIC_VIRT (in traps.c) so that, as you
> >>>>>>>> say, the guest gets at least "some assistance" instead of none but we
> >>>>>>>> still claim x{2}apic virtualization when it is actually complete? Maybe
> >>>>>>>> I could also add a comment alluding to this in the xl documentation.
> >>>>>>>
> >>>>>>> To rephrase my earlier point: Which kind of decisions are the consumer(s)
> >>>>>>> of us reporting hardware assistance going to take? In how far is there a
> >>>>>>> risk that "some assistance" is overall going to lead to a loss of
> >>>>>>> performance? I guess I'd need to see comment and actual code all in one
> >>>>>>> place ...
> >>>>>>>
> >>>>>> So, I was thinking of adding something along the lines of:
> >>>>>>
> >>>>>> +=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
> >>>>>> +Enables or disables hardware assisted virtualization for xAPIC. This
> >>>>>> +allows accessing APIC registers without a VM-exit. Notice enabling
> >>>>>> +this does not guarantee full virtualization for xAPIC, as this can
> >>>>>> +only be achieved if hardware supports “APIC-register virtualization”
> >>>>>> +and “virtual-interrupt delivery”. The default is settable via
> >>>>>> +L<xl.conf(5)>.
> >>>>>
> >>>>> But isn't this contradictory? Doesn't lack of APIC-register virtualization
> >>>>> mean VM exits upon (most) accesses?
> >>>>
> >>>> Yes, it does mean. I guess the alternative wouuld be then to require
> >>>> APIC-register virtualization for enabling xAPIC. But also, although this
> >>>> doesn't provide much acceleration, even getting a VM exit is some
> >>>> assistance if compared to instead getting an EPT fault and having to
> >>>> decode the access.
> >>>
> >>> I agree here, albeit I'd like to mention that EPT faults are also VM
> >>> exits. All my earlier comment was about is that this piece of doc
> >>> wants to express reality, whichever way it is that things end up
> >>> being implemented.
> >>
> >> Oh yes. Right, I see how this info could be misleading.
> >>
> >> How about this?...
> > 
> > Getting close. The thing I can't judge is whether this level of technical
> > detail is suitable for this doc. Just one further remark:
> 
> Unsure too.
> 
> >> +=item B<assisted_xapic=BOOLEAN> B<(x86 only)>
> >> +
> >> +B<(x86 only)> Enables or disables hardware assisted virtualization for
> >> +xAPIC. With this option enabled, a memory-mapped APIC access will be
> >> +decoded by hardware and either issue a VM exit with an exit reason
> >> +instead of an EPT fault or altogether avoid a VM exit. Notice
> > 
> > As said before, EPT faults also are VM exits and also provide an exit
> > reason. Therefore maybe "... and either issue a VM exit with a more
> > specific exit reason than an EPT fault would provide, or altogether
> > avoid a VM exit" or "... and either issue a more specific VM exit than
> > just an EPT fault, or altogether avoid a VM exit"?
> 
> Yes, that's better.

I would avoid mentioning EPT, as that's an Intel specific technology.
Could we instead use 'p2m fault'?

Thanks, Roger.

Patch

diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index b746ff1081..dd4e6c9f14 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -3373,6 +3373,8 @@  x.CapVmtrace = bool(xc.cap_vmtrace)
 x.CapVpmu = bool(xc.cap_vpmu)
 x.CapGnttabV1 = bool(xc.cap_gnttab_v1)
 x.CapGnttabV2 = bool(xc.cap_gnttab_v2)
+x.CapAssistedXapic = bool(xc.cap_assisted_xapic)
+x.CapAssistedX2Apic = bool(xc.cap_assisted_x2apic)
 
  return nil}
 
@@ -3407,6 +3409,8 @@  xc.cap_vmtrace = C.bool(x.CapVmtrace)
 xc.cap_vpmu = C.bool(x.CapVpmu)
 xc.cap_gnttab_v1 = C.bool(x.CapGnttabV1)
 xc.cap_gnttab_v2 = C.bool(x.CapGnttabV2)
+xc.cap_assisted_xapic = C.bool(x.CapAssistedXapic)
+xc.cap_assisted_x2apic = C.bool(x.CapAssistedX2Apic)
 
  return nil
  }
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index b1e84d5258..5f384b767c 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -389,6 +389,10 @@  RunHotplugScripts Defbool
 DriverDomain Defbool
 Passthrough Passthrough
 XendSuspendEvtchnCompat Defbool
+ArchX86 struct {
+AssistedXapic Defbool
+AssistedX2Apic Defbool
+}
 }
 
 type DomainRestoreParams struct {
@@ -1014,6 +1018,8 @@  CapVmtrace bool
 CapVpmu bool
 CapGnttabV1 bool
 CapGnttabV2 bool
+CapAssistedXApic bool
+CapAssistedX2apic bool
 }
 
 type Connectorinfo struct {
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 2bbbd21f0b..924e142628 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -528,6 +528,13 @@ 
 #define LIBXL_HAVE_MAX_GRANT_VERSION 1
 
 /*
+ * LIBXL_HAVE_PHYSINFO_ASSISTED_APIC indicates that libxl_physinfo has
+ * cap_assisted_x{2}apic fields, which indicates the availability of x{2}APIC
+ * hardware assisted virtualization.
+ */
+#define LIBXL_HAVE_PHYSINFO_ASSISTED_APIC 1
+
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c
index 667ae6409b..fabb474221 100644
--- a/tools/libs/light/libxl.c
+++ b/tools/libs/light/libxl.c
@@ -15,6 +15,7 @@ 
 #include "libxl_osdeps.h"
 
 #include "libxl_internal.h"
+#include "libxl_arch.h"
 
 int libxl_ctx_alloc(libxl_ctx **pctx, int version,
                     unsigned flags, xentoollog_logger * lg)
@@ -410,6 +411,8 @@  int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo)
     physinfo->cap_gnttab_v2 =
         !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_gnttab_v2);
 
+    libxl__arch_get_physinfo(physinfo, &xcphysinfo);
+
     GC_FREE;
     return 0;
 }
diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h
index 1522ecb97f..207ceac6a1 100644
--- a/tools/libs/light/libxl_arch.h
+++ b/tools/libs/light/libxl_arch.h
@@ -86,6 +86,10 @@  int libxl__arch_extra_memory(libxl__gc *gc,
                              uint64_t *out);
 
 _hidden
+void libxl__arch_get_physinfo(libxl_physinfo *physinfo,
+                              const xc_physinfo_t *xcphysinfo);
+
+_hidden
 void libxl__arch_update_domain_config(libxl__gc *gc,
                                       libxl_domain_config *dst,
                                       const libxl_domain_config *src);
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index eef1de0939..39fdca1b49 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -1431,6 +1431,11 @@  int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
     return rc;
 }
 
+void libxl__arch_get_physinfo(libxl_physinfo *physinfo,
+                              const xc_physinfo_t *xcphysinfo)
+{
+}
+
 void libxl__arch_update_domain_config(libxl__gc *gc,
                                       libxl_domain_config *dst,
                                       const libxl_domain_config *src)
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 2a42da2f7d..42ac6c357b 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -1068,6 +1068,8 @@  libxl_physinfo = Struct("physinfo", [
     ("cap_vpmu", bool),
     ("cap_gnttab_v1", bool),
     ("cap_gnttab_v2", bool),
+    ("cap_assisted_xapic", bool),
+    ("cap_assisted_x2apic", bool),
     ], dir=DIR_OUT)
 
 libxl_connectorinfo = Struct("connectorinfo", [
diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
index 1feadebb18..e0a06ecfe3 100644
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -866,6 +866,17 @@  int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
     return rc;
 }
 
+void libxl__arch_get_physinfo(libxl_physinfo *physinfo,
+                              const xc_physinfo_t *xcphysinfo)
+{
+    physinfo->cap_assisted_xapic =
+        !!(xcphysinfo->arch_capabilities &
+           XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC);
+    physinfo->cap_assisted_x2apic =
+        !!(xcphysinfo->arch_capabilities &
+           XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC);
+}
+
 void libxl__arch_update_domain_config(libxl__gc *gc,
                                       libxl_domain_config *dst,
                                       const libxl_domain_config *src)
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 7503031d8f..7ce832d605 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -127,6 +127,10 @@  type physinfo_cap_flag =
 	| CAP_Gnttab_v1
 	| CAP_Gnttab_v2
 
+type physinfo_cap_arch_flag =
+	| CAP_ARCH_ASSISTED_XAPIC
+	| CAP_ARCH_ASSISTED_X2APIC
+
 type physinfo =
 {
 	threads_per_core : int;
@@ -139,6 +143,7 @@  type physinfo =
 	scrub_pages      : nativeint;
 	(* XXX hw_cap *)
 	capabilities     : physinfo_cap_flag list;
+	arch_capabilities : physinfo_cap_arch_flag list;
 	max_nr_cpus      : int;
 }
 
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index d1d9c9247a..a2b15130ee 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -112,6 +112,10 @@  type physinfo_cap_flag =
   | CAP_Gnttab_v1
   | CAP_Gnttab_v2
 
+type physinfo_cap_arch_flag =
+  | CAP_ARCH_ASSISTED_XAPIC
+  | CAP_ARCH_ASSISTED_X2APIC
+
 type physinfo = {
   threads_per_core : int;
   cores_per_socket : int;
@@ -122,6 +126,7 @@  type physinfo = {
   free_pages       : nativeint;
   scrub_pages      : nativeint;
   capabilities     : physinfo_cap_flag list;
+  arch_capabilities : physinfo_cap_arch_flag list;
   max_nr_cpus      : int; (** compile-time max possible number of nr_cpus *)
 }
 type version = { major : int; minor : int; extra : string; }
diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
index 712b7638b0..3205270754 100644
--- a/tools/xl/xl_info.c
+++ b/tools/xl/xl_info.c
@@ -210,7 +210,7 @@  static void output_physinfo(void)
          info.hw_cap[4], info.hw_cap[5], info.hw_cap[6], info.hw_cap[7]
         );
 
-    maybe_printf("virt_caps              :%s%s%s%s%s%s%s%s%s%s%s\n",
+    maybe_printf("virt_caps              :%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
          info.cap_pv ? " pv" : "",
          info.cap_hvm ? " hvm" : "",
          info.cap_hvm && info.cap_hvm_directio ? " hvm_directio" : "",
@@ -221,7 +221,9 @@  static void output_physinfo(void)
          info.cap_vmtrace ? " vmtrace" : "",
          info.cap_vpmu ? " vpmu" : "",
          info.cap_gnttab_v1 ? " gnttab-v1" : "",
-         info.cap_gnttab_v2 ? " gnttab-v2" : ""
+         info.cap_gnttab_v2 ? " gnttab-v2" : "",
+         info.cap_assisted_xapic ? " assisted_xapic" : "",
+         info.cap_assisted_x2apic ? " assisted_x2apic" : ""
         );
 
     vinfo = libxl_get_version_info(ctx);
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 7ab15e07a0..4060aef1bd 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -343,6 +343,15 @@  static int vmx_init_vmcs_config(bool bsp)
             MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
     }
 
+    /* Check whether hardware supports accelerated xapic and x2apic. */
+    if ( bsp )
+    {
+        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
+        assisted_x2apic_available = (cpu_has_vmx_apic_reg_virt ||
+                                     cpu_has_vmx_virtual_intr_delivery) &&
+                                    cpu_has_vmx_virtualize_x2apic_mode;
+    }
+
     /* The IA32_VMX_EPT_VPID_CAP MSR exists only when EPT or VPID available */
     if ( _vmx_secondary_exec_control & (SECONDARY_EXEC_ENABLE_EPT |
                                         SECONDARY_EXEC_ENABLE_VPID) )
diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
index e62e109598..72431df26d 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -756,6 +756,9 @@  static inline void pv_inject_sw_interrupt(unsigned int vector)
                       : is_pv_32bit_domain(d) ? PV32_VM_ASSIST_MASK \
                                               : PV64_VM_ASSIST_MASK)
 
+extern bool assisted_xapic_available;
+extern bool assisted_x2apic_available;
+
 #endif /* __ASM_DOMAIN_H__ */
 
 /*
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index aff52a13f3..642cc96985 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -69,6 +69,9 @@  struct l3_cache_info {
     unsigned long size;
 };
 
+bool __ro_after_init assisted_xapic_available;
+bool __ro_after_init assisted_x2apic_available;
+
 static void l3_cache_get(void *arg)
 {
     struct cpuid4_info info;
@@ -135,6 +138,10 @@  void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
         pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap;
     if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
         pi->capabilities |= XEN_SYSCTL_PHYSCAP_shadow;
+    if ( assisted_xapic_available )
+        pi->arch_capabilities |= XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC;
+    if ( assisted_x2apic_available )
+        pi->arch_capabilities |= XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC;
 }
 
 long arch_do_sysctl(
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 55252e97f2..11328bbf78 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -35,7 +35,7 @@ 
 #include "domctl.h"
 #include "physdev.h"
 
-#define XEN_SYSCTL_INTERFACE_VERSION 0x00000014
+#define XEN_SYSCTL_INTERFACE_VERSION 0x00000015
 
 /*
  * Read console content from Xen buffer ring.
@@ -111,6 +111,10 @@  struct xen_sysctl_tbuf_op {
 /* Max XEN_SYSCTL_PHYSCAP_* constant.  Used for ABI checking. */
 #define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_gnttab_v2
 
+/* The platform supports x{2}apic hardware assisted emulation. */
+#define XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC  (1u << 0)
+#define XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC (1u << 1)
+
 struct xen_sysctl_physinfo {
     uint32_t threads_per_core;
     uint32_t cores_per_socket;
@@ -120,6 +124,8 @@  struct xen_sysctl_physinfo {
     uint32_t max_node_id; /* Largest possible node ID on this host */
     uint32_t cpu_khz;
     uint32_t capabilities;/* XEN_SYSCTL_PHYSCAP_??? */
+    uint32_t arch_capabilities;/* XEN_SYSCTL_PHYSCAP_X86{ARM}_??? */
+    uint32_t pad; /* Must be zero. */
     uint64_aligned_t total_pages;
     uint64_aligned_t free_pages;
     uint64_aligned_t scrub_pages;