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

[Jane Malalane <Jane.Malalane@xxxxxxxxxx>]

On 08/02/2022 15:26, Roger Pau Monné wrote:
> On Mon, Feb 07, 2022 at 06:21:00PM +0000, Jane Malalane wrote:
>> Add XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_xapic and
>> XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_x2apic to report accelerated xapic
>> and x2apic, on x86 hardware.
>> No such features are currently implemented on AMD hardware.
>>
>> For that purpose, also add an arch-specific "capabilities" parameter
>> to struct xen_sysctl_physinfo.
>>
>> Signed-off-by: Jane Malalane <jane.malalane@xxxxxxxxxx>
>> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> 
> Tag order should be inverted, first Suggested-by, then SoB.
> 
>> ---
>> CC: Wei Liu <wl@xxxxxxx>
>> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>> CC: Juergen Gross <jgross@xxxxxxxx>
>> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> CC: George Dunlap <george.dunlap@xxxxxxxxxx>
>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>> CC: Julien Grall <julien@xxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
>> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
>> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
>> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
>> CC: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
>>
>> 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.