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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Anthony,

Thank you for your review

> On 30 Mar 2023, at 17:49, Anthony PERARD <anthony.perard@xxxxxxxxxx> wrote:
> 
> 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.

Yes I’ll do

> 
>> 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?

I’ve added that because sysctl.h is doing this:

#if !defined(__XEN__) && !defined(__XEN_TOOLS__)
#error "sysctl operations are intended for use by node control tools only"
#endif

But I’ve not checked if the macro is already passed through the build system, 
I’ll
try and I’ll remove it if it’s the case

> 
>> +#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".

I’ll add

> 
>>     ], 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.

Ok I’ll do the modifications

> 
>> /* 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

Yes you are right, I’ll change it

> 
>> +                        );
>> }
>> 
>> 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".

Ok I’ll move it

> 
>> #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