Re: [PATCH v6 1/2] xen+tools: Report Interrupt Controller Virtualization capabilities on x86

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Mar 08, 2022 at 05:31:17PM +0000, Jane Malalane wrote:
> Add XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC and
> XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC to report accelerated xapic
> and x2apic, on x86 hardware.
> No such features are currently implemented on AMD hardware.
> 
> HW assisted xAPIC virtualization will be reported if HW, at the
> minimum, supports virtualize_apic_accesses as this feature alone means
> that an access to the APIC page will cause an APIC-access VM exit. An
> APIC-access VM exit provides a VMM with information about the access
> causing the VM exit, unlike a regular EPT fault, thus simplifying some
> internal handling.
> 
> HW assisted x2APIC virtualization will be reported if HW supports
> virtualize_x2apic_mode and, at least, either apic_reg_virt or
> virtual_intr_delivery. This also means that
> sysctl follows the conditionals in vmx_vlapic_msr_changed().
> 
> For that purpose, also add an arch-specific "capabilities" parameter
> to struct xen_sysctl_physinfo.
> 
> Note that this interface is intended to be compatible with AMD so that
> AVIC support can be introduced in a future patch. Unlike Intel that
> has multiple controls for APIC Virtualization, AMD has one global
> 'AVIC Enable' control bit, so fine-graining of APIC virtualization
> control cannot be done on a common interface.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jane Malalane <jane.malalane@xxxxxxxxxx>

Overall LGTM, just one question and one nit.

> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
> b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index 5b4fe72c8d..7e9c32ad1b 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -712,7 +712,7 @@ CAMLprim value stub_xc_send_debug_keys(value xch, value 
> keys)
>  CAMLprim value stub_xc_physinfo(value xch)
>  {
>       CAMLparam1(xch);
> -     CAMLlocal2(physinfo, cap_list);
> +     CAMLlocal3(physinfo, cap_list, arch_cap_list);
>       xc_physinfo_t c_physinfo;
>       int r;
>  
> @@ -731,7 +731,7 @@ CAMLprim value stub_xc_physinfo(value xch)
>               /* ! XEN_SYSCTL_PHYSCAP_ XEN_SYSCTL_PHYSCAP_MAX max */
>               (c_physinfo.capabilities);
>  
> -     physinfo = caml_alloc_tuple(10);
> +     physinfo = caml_alloc_tuple(11);
>       Store_field(physinfo, 0, Val_int(c_physinfo.threads_per_core));
>       Store_field(physinfo, 1, Val_int(c_physinfo.cores_per_socket));
>       Store_field(physinfo, 2, Val_int(c_physinfo.nr_cpus));
> @@ -743,6 +743,17 @@ CAMLprim value stub_xc_physinfo(value xch)
>       Store_field(physinfo, 8, cap_list);
>       Store_field(physinfo, 9, Val_int(c_physinfo.max_cpu_id + 1));
>  
> +#if defined(__i386__) || defined(__x86_64__)
> +     /*
> +      * arch_capabilities: physinfo_arch_cap_flag list;
> +      */
> +     arch_cap_list = c_bitmap_to_ocaml_list
> +             /* ! physinfo_arch_cap_flag CAP_ none */
> +             /* ! XEN_SYSCTL_PHYSCAP_ XEN_SYSCTL_PHYSCAP_X86_MAX max */
> +             (c_physinfo.arch_capabilities);
> +     Store_field(physinfo, 10, arch_cap_list);
> +#endif

Have you tried to build this on Arm? I wonder whether the compiler
will complain about arch_cap_list being unused there?

> +
>       CAMLreturn(physinfo);
>  }
>  
> diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
> index 712b7638b0..3205270754 100644
> --- a/tools/xl/xl_info.c
> +++ b/tools/xl/xl_info.c
> @@ -210,7 +210,7 @@ static void output_physinfo(void)
>           info.hw_cap[4], info.hw_cap[5], info.hw_cap[6], info.hw_cap[7]
>          );
>  
> -    maybe_printf("virt_caps              :%s%s%s%s%s%s%s%s%s%s%s\n",
> +    maybe_printf("virt_caps              :%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
>           info.cap_pv ? " pv" : "",
>           info.cap_hvm ? " hvm" : "",
>           info.cap_hvm && info.cap_hvm_directio ? " hvm_directio" : "",
> @@ -221,7 +221,9 @@ static void output_physinfo(void)
>           info.cap_vmtrace ? " vmtrace" : "",
>           info.cap_vpmu ? " vpmu" : "",
>           info.cap_gnttab_v1 ? " gnttab-v1" : "",
> -         info.cap_gnttab_v2 ? " gnttab-v2" : ""
> +         info.cap_gnttab_v2 ? " gnttab-v2" : "",
> +         info.cap_assisted_xapic ? " assisted_xapic" : "",
> +         info.cap_assisted_x2apic ? " assisted_x2apic" : ""
>          );
>  
>      vinfo = libxl_get_version_info(ctx);
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index e1e1fa14e6..77ce0b2121 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -343,6 +343,15 @@ static int vmx_init_vmcs_config(bool bsp)
>              MSR_IA32_VMX_PROCBASED_CTLS2, &mismatch);
>      }
>  
> +    /* Check whether hardware supports accelerated xapic and x2apic. */
> +    if ( bsp )
> +    {
> +        assisted_xapic_available = cpu_has_vmx_virtualize_apic_accesses;
> +        assisted_x2apic_available = cpu_has_vmx_virtualize_x2apic_mode &&
> +                                    (cpu_has_vmx_apic_reg_virt ||
> +                                     cpu_has_vmx_virtual_intr_delivery);
> +    }
> +
>      /* The IA32_VMX_EPT_VPID_CAP MSR exists only when EPT or VPID available 
> */
>      if ( _vmx_secondary_exec_control & (SECONDARY_EXEC_ENABLE_EPT |
>                                          SECONDARY_EXEC_ENABLE_VPID) )
> diff --git a/xen/arch/x86/include/asm/domain.h 
> b/xen/arch/x86/include/asm/domain.h
> index e62e109598..72431df26d 100644
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -756,6 +756,9 @@ static inline void pv_inject_sw_interrupt(unsigned int 
> vector)
>                        : is_pv_32bit_domain(d) ? PV32_VM_ASSIST_MASK \
>                                                : PV64_VM_ASSIST_MASK)
>  
> +extern bool assisted_xapic_available;
> +extern bool assisted_x2apic_available;
> +
>  #endif /* __ASM_DOMAIN_H__ */
>  
>  /*
> diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
> index f82abc2488..ad95c86aef 100644
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -69,6 +69,9 @@ struct l3_cache_info {
>      unsigned long size;
>  };
>  
> +bool __ro_after_init assisted_xapic_available;
> +bool __ro_after_init assisted_x2apic_available;
> +
>  static void cf_check l3_cache_get(void *arg)
>  {
>      struct cpuid4_info info;
> @@ -135,6 +138,10 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>          pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap;
>      if ( IS_ENABLED(CONFIG_SHADOW_PAGING) )
>          pi->capabilities |= XEN_SYSCTL_PHYSCAP_shadow;
> +    if ( assisted_xapic_available )
> +        pi->arch_capabilities |= XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC;
> +    if ( assisted_x2apic_available )
> +        pi->arch_capabilities |= XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC;
>  }
>  
>  long arch_do_sysctl(
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 55252e97f2..7fe05be0c9 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -35,7 +35,7 @@
>  #include "domctl.h"
>  #include "physdev.h"
>  
> -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000014
> +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000015
>  
>  /*
>   * Read console content from Xen buffer ring.
> @@ -111,6 +111,13 @@ struct xen_sysctl_tbuf_op {
>  /* Max XEN_SYSCTL_PHYSCAP_* constant.  Used for ABI checking. */
>  #define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_gnttab_v2
>  
> +/* The platform supports x{2}apic hardware assisted emulation. */
> +#define XEN_SYSCTL_PHYSCAP_X86_ASSISTED_XAPIC  (1u << 0)
> +#define XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC (1u << 1)
> +
> +/* Max XEN_SYSCTL_PHYSCAP_X86__* constant. Used for ABI checking. */
> +#define XEN_SYSCTL_PHYSCAP_X86_MAX XEN_SYSCTL_PHYSCAP_X86_ASSISTED_X2APIC
> +
>  struct xen_sysctl_physinfo {
>      uint32_t threads_per_core;
>      uint32_t cores_per_socket;
> @@ -120,6 +127,8 @@ struct xen_sysctl_physinfo {
>      uint32_t max_node_id; /* Largest possible node ID on this host */
>      uint32_t cpu_khz;
>      uint32_t capabilities;/* XEN_SYSCTL_PHYSCAP_??? */
> +    uint32_t arch_capabilities;/* XEN_SYSCTL_PHYSCAP_X86{ARM}_??? */

Nit: comment should likely be:

XEN_SYSCTL_PHYSCAP_{X86,ARM,...}_???

Thanks, Roger.