[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 09/12] tools: add physinfo arch_capabilities handling for Arm
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |