Re: [PATCH v4 09/12] tools: add physinfo arch_capabilities handling for Arm

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Mon, Mar 27, 2023 at 11:59:41AM +0100, Luca Fancellu wrote:
> ---
>  tools/golang/xenlight/helpers.gen.go    |  2 ++
>  tools/golang/xenlight/types.gen.go      |  1 +
>  tools/include/arm-arch-capabilities.h   | 33 +++++++++++++++++++++++++

Could you move that new file into "tools/include/xen-tools/", where
"common-macros.h" is. The top-dir "tools/include" already has a mixture
of installed and internal headers, but most of them are installed. So
the fairly recent "xen-tools" dir which have been introduced to share
macros at build time seems more appropriate for another built-time
macro.

>  tools/include/xen-tools/common-macros.h |  2 ++
> 
> diff --git a/tools/include/arm-arch-capabilities.h 
> b/tools/include/arm-arch-capabilities.h
> new file mode 100644
> index 000000000000..46e876651052
> --- /dev/null
> +++ b/tools/include/arm-arch-capabilities.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2023 ARM Ltd.
> + */
> +
> +#ifndef ARM_ARCH_CAPABILITIES_H
> +#define ARM_ARCH_CAPABILITIES_H
> +
> +/* Tell the Xen public headers we are a user-space tools build. */
> +#ifndef __XEN_TOOLS__
> +#define __XEN_TOOLS__ 1

Somehow, this doesn't seems appropriate in this header. This macro
should instead be set on the command line. Any reason you've added this
in the header?

> +#endif
> +
> +#include <stdint.h>
> +#include <xen/sysctl.h>
> +
> +#include <xen-tools/common-macros.h>
> +
> +static inline
> +unsigned int arch_capabilities_arm_sve(unsigned int arch_capabilities)
> +{
> +#if defined(__aarch64__)
> +    unsigned int sve_vl = MASK_EXTR(arch_capabilities,
> +                                    XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
> +
> +    /* Vector length is divided by 128 before storing it in 
> arch_capabilities */
> +    return sve_vl * 128U;
> +#else
> +    return 0;
> +#endif
> +}
> +
> +#endif /* ARM_ARCH_CAPABILITIES_H */
> diff --git a/tools/libs/light/libxl_types.idl 
> b/tools/libs/light/libxl_types.idl
> index c10292e0d7e3..fd31dacf7d5a 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -1133,6 +1133,7 @@ libxl_physinfo = Struct("physinfo", [
>      ("cap_vpmu", bool),
>      ("cap_gnttab_v1", bool),
>      ("cap_gnttab_v2", bool),
> +    ("arch_capabilities", uint32),

This additional field needs a new LIBXL_HAVE_ macro in "libxl.h".

>      ], dir=DIR_OUT)
>  
>  libxl_connectorinfo = Struct("connectorinfo", [
> diff --git a/tools/python/xen/lowlevel/xc/xc.c 
> b/tools/python/xen/lowlevel/xc/xc.c
> index 35901c2d63b6..254d3b5dccd2 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -7,6 +7,7 @@
>  #define PY_SSIZE_T_CLEAN
>  #include <Python.h>
>  #define XC_WANT_COMPAT_MAP_FOREIGN_API
> +#include <arm-arch-capabilities.h>

Could you add this header ...

>  #include <xenctrl.h>
>  #include <xenguest.h>
>  #include <fcntl.h>
> @@ -22,8 +23,6 @@
>  #include <xen/hvm/hvm_info_table.h>
>  #include <xen/hvm/params.h>
>  
> -#include <xen-tools/common-macros.h>
> -

... here, instead?

Also, I think #include common-macros, can stay.

>  /* Needed for Python versions earlier than 2.3. */
>  #ifndef PyMODINIT_FUNC
>  #define PyMODINIT_FUNC DL_EXPORT(void)
> @@ -897,7 +896,7 @@ static PyObject *pyxc_physinfo(XcObject *self)
>      if ( p != virt_caps )
>        *(p-1) = '\0';
>  
> -    return Py_BuildValue("{s:i,s:i,s:i,s:i,s:l,s:l,s:l,s:i,s:s,s:s}",
> +    return Py_BuildValue("{s:i,s:i,s:i,s:i,s:l,s:l,s:l,s:i,s:s,s:s,s:i}",
>                              "nr_nodes",         pinfo.nr_nodes,
>                              "threads_per_core", pinfo.threads_per_core,
>                              "cores_per_socket", pinfo.cores_per_socket,
> @@ -907,7 +906,10 @@ static PyObject *pyxc_physinfo(XcObject *self)
>                              "scrub_memory",     
> pages_to_kib(pinfo.scrub_pages),
>                              "cpu_khz",          pinfo.cpu_khz,
>                              "hw_caps",          cpu_cap,
> -                            "virt_caps",        virt_caps);
> +                            "virt_caps",        virt_caps,
> +                            "arm_sve_vl",
> +                              
> arch_capabilities_arm_sve(pinfo.arch_capabilities)

arch_capabilities_arm_sve() returns an "unsigned int", but the format is
"i", which seems to be an "int". Shouldn't the format be "I" instead?

https://docs.python.org/3/c-api/arg.html#c.Py_BuildValue

> +                        );
>  }
>  
>  static PyObject *pyxc_getcpuinfo(XcObject *self, PyObject *args, PyObject 
> *kwds)
> diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
> index 712b7638b013..bf18ba2449ef 100644
> --- a/tools/xl/xl_info.c
> +++ b/tools/xl/xl_info.c
> @@ -14,6 +14,7 @@
>  
>  #define _GNU_SOURCE
>  
> +#include <arm-arch-capabilities.h>

Any reason reason to have this header first?
I feel like private headers should come after public ones, so here, this
include would be added between <libxlutil.h> and "xl.h".

>  #include <fcntl.h>
>  #include <inttypes.h>
>  #include <stdlib.h>
> @@ -224,6 +225,13 @@ static void output_physinfo(void)
>           info.cap_gnttab_v2 ? " gnttab-v2" : ""
>          );
>  
> +    /* Print arm SVE vector length only on ARM platforms */
> +#if defined(__aarch64__)
> +    maybe_printf("arm_sve_vector_length  : %u\n",
> +         arch_capabilities_arm_sve(info.arch_capabilities)
> +        );
> +#endif
> +
>      vinfo = libxl_get_version_info(ctx);
>      if (vinfo) {
>          i = (1 << 20) / vinfo->pagesize;

Thanks,

-- 
Anthony PERARD