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

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

On Thu, Jan 27, 2022 at 04:01:32PM +0000, Jane Malalane wrote:
> Add XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_xapic and
> XEN_SYSCTL_PHYSCAP_ARCH_ASSISTED_x2apic to report accelerated xapic
> and x2apic, on x86 hardware.
> No such features are currently implemented on AMD hardware.
> 
> For that purpose, also add an arch-specific "capabilities" parameter
> to struct xen_sysctl_physinfo.
> 
> Signed-off-by: Jane Malalane <jane.malalane@xxxxxxxxxx>
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> 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>
> ---
>  tools/golang/xenlight/helpers.gen.go |  4 ++++
>  tools/golang/xenlight/types.gen.go   |  6 ++++++
>  tools/include/libxl.h                | 14 ++++++++++++++
>  tools/libs/light/libxl.c             |  3 +++
>  tools/libs/light/libxl_arch.h        |  4 ++++
>  tools/libs/light/libxl_arm.c         |  4 ++++
>  tools/libs/light/libxl_types.idl     |  2 ++
>  tools/libs/light/libxl_x86.c         | 11 +++++++++++
>  tools/ocaml/libs/xc/xenctrl.ml       |  5 +++++
>  tools/ocaml/libs/xc/xenctrl.mli      |  5 +++++
>  tools/xl/xl_info.c                   |  6 ++++--
>  xen/arch/x86/hvm/vmx/vmcs.c          |  6 ++++++
>  xen/arch/x86/include/asm/domain.h    |  4 ++++
>  xen/arch/x86/sysctl.c                |  7 +++++++
>  xen/include/public/arch-x86/xen.h    |  4 ++++
>  xen/include/public/sysctl.h          |  1 +
>  16 files changed, 84 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/golang/xenlight/helpers.gen.go 
> b/tools/golang/xenlight/helpers.gen.go
> index b746ff1081..dd4e6c9f14 100644
> --- a/tools/golang/xenlight/helpers.gen.go
> +++ b/tools/golang/xenlight/helpers.gen.go
> @@ -3373,6 +3373,8 @@ x.CapVmtrace = bool(xc.cap_vmtrace)
>  x.CapVpmu = bool(xc.cap_vpmu)
>  x.CapGnttabV1 = bool(xc.cap_gnttab_v1)
>  x.CapGnttabV2 = bool(xc.cap_gnttab_v2)
> +x.CapAssistedXapic = bool(xc.cap_assisted_xapic)
> +x.CapAssistedX2Apic = bool(xc.cap_assisted_x2apic)
>  
>   return nil}
>  
> @@ -3407,6 +3409,8 @@ xc.cap_vmtrace = C.bool(x.CapVmtrace)
>  xc.cap_vpmu = C.bool(x.CapVpmu)
>  xc.cap_gnttab_v1 = C.bool(x.CapGnttabV1)
>  xc.cap_gnttab_v2 = C.bool(x.CapGnttabV2)
> +xc.cap_assisted_xapic = C.bool(x.CapAssistedXapic)
> +xc.cap_assisted_x2apic = C.bool(x.CapAssistedX2Apic)
>  
>   return nil
>   }
> diff --git a/tools/golang/xenlight/types.gen.go 
> b/tools/golang/xenlight/types.gen.go
> index b1e84d5258..5f384b767c 100644
> --- a/tools/golang/xenlight/types.gen.go
> +++ b/tools/golang/xenlight/types.gen.go
> @@ -389,6 +389,10 @@ RunHotplugScripts Defbool
>  DriverDomain Defbool
>  Passthrough Passthrough
>  XendSuspendEvtchnCompat Defbool
> +ArchX86 struct {
> +AssistedXapic Defbool
> +AssistedX2Apic Defbool
> +}
>  }
>  
>  type DomainRestoreParams struct {
> @@ -1014,6 +1018,8 @@ CapVmtrace bool
>  CapVpmu bool
>  CapGnttabV1 bool
>  CapGnttabV2 bool
> +CapAssistedXApic bool
> +CapAssistedX2apic bool
>  }
>  
>  type Connectorinfo struct {
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index 2bbbd21f0b..6d51e56704 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -528,6 +528,20 @@
>  #define LIBXL_HAVE_MAX_GRANT_VERSION 1
>  
>  /*
> + * LIBXL_HAVE_PHYSINFO_ASSISTED_XAPIC indicates that libxl_physinfo has a
> + * cap_assisted_xapic field, which indicates the availability of xAPIC
> + * hardware assisted emulation.
> + */
> +#define LIBXL_HAVE_PHYSINFO_ASSISTED_XAPIC 1
> +
> +/*
> + * LIBXL_HAVE_PHYSINFO_ASSISTED_X2APIC indicates that libxl_physinfo has a
> + * cap_assisted_x2apic field, which indicates the availability of x2APIC
> + * hardware assisted emulation.
> + */
> +#define LIBXL_HAVE_PHYSINFO_ASSISTED_X2APIC 1

I think you could likely introduce a single define that covers both
features, as they are introduced in the same commit, ie:
LIBXL_HAVE_PHYSINFO_ASSISTED_APIC.

> +
> +/*
>   * libxl ABI compatibility
>   *
>   * The only guarantee which libxl makes regarding ABI compatibility
> diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c
> index 667ae6409b..1588701d19 100644
> --- a/tools/libs/light/libxl.c
> +++ b/tools/libs/light/libxl.c
> @@ -15,6 +15,7 @@
>  #include "libxl_osdeps.h"
>  
>  #include "libxl_internal.h"
> +#include "libxl_arch.h"
>  
>  int libxl_ctx_alloc(libxl_ctx **pctx, int version,
>                      unsigned flags, xentoollog_logger * lg)
> @@ -410,6 +411,8 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo 
> *physinfo)
>      physinfo->cap_gnttab_v2 =
>          !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_gnttab_v2);
>  
> +    libxl__arch_get_physinfo(physinfo, xcphysinfo);
> +
>      GC_FREE;
>      return 0;
>  }
> diff --git a/tools/libs/light/libxl_arch.h b/tools/libs/light/libxl_arch.h
> index 1522ecb97f..00cc50394d 100644
> --- a/tools/libs/light/libxl_arch.h
> +++ b/tools/libs/light/libxl_arch.h
> @@ -86,6 +86,10 @@ int libxl__arch_extra_memory(libxl__gc *gc,
>                               uint64_t *out);
>  
>  _hidden
> +void libxl__arch_get_physinfo(libxl_physinfo *physinfo,
> +                              xc_physinfo_t xcphysinfo);
> +
> +_hidden
>  void libxl__arch_update_domain_config(libxl__gc *gc,
>                                        libxl_domain_config *dst,
>                                        const libxl_domain_config *src);
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index eef1de0939..52f2545498 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -1431,6 +1431,10 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc 
> *gc,
>      return rc;
>  }
>  
> +void libxl__arch_get_physinfo(libxl_physinfo *physinfo, xc_physinfo_t 
> xcphysinfo)
> +{
> +}
> +
>  void libxl__arch_update_domain_config(libxl__gc *gc,
>                                        libxl_domain_config *dst,
>                                        const libxl_domain_config *src)
> diff --git a/tools/libs/light/libxl_types.idl 
> b/tools/libs/light/libxl_types.idl
> index 2a42da2f7d..42ac6c357b 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -1068,6 +1068,8 @@ libxl_physinfo = Struct("physinfo", [
>      ("cap_vpmu", bool),
>      ("cap_gnttab_v1", bool),
>      ("cap_gnttab_v2", bool),
> +    ("cap_assisted_xapic", bool),
> +    ("cap_assisted_x2apic", bool),
>      ], dir=DIR_OUT)
>  
>  libxl_connectorinfo = Struct("connectorinfo", [
> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> index 1feadebb18..33da51fe89 100644
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -866,6 +866,17 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc 
> *gc,
>      return rc;
>  }
>  
> +void libxl__arch_get_physinfo(libxl_physinfo *physinfo,
> +                              xc_physinfo_t xcphysinfo)

If you make xcphysinfo passed by reference as Anthony suggested also
make it const.

Thanks, Roger.