| Message ID | 20230327105944.1360856-10-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SVE feature for arm guests | expand |
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,
Hi Anthony, Thank you for your review > On 30 Mar 2023, at 17:49, Anthony PERARD <anthony.perard@citrix.com> 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
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go index 0a203d22321f..35397be2f9e2 100644 --- a/tools/golang/xenlight/helpers.gen.go +++ b/tools/golang/xenlight/helpers.gen.go @@ -3506,6 +3506,7 @@ 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.ArchCapabilities = uint32(xc.arch_capabilities) return nil} @@ -3540,6 +3541,7 @@ 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.arch_capabilities = C.uint32_t(x.ArchCapabilities) return nil } diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go index a7c17699f80e..3d968a496744 100644 --- a/tools/golang/xenlight/types.gen.go +++ b/tools/golang/xenlight/types.gen.go @@ -1079,6 +1079,7 @@ CapVmtrace bool CapVpmu bool CapGnttabV1 bool CapGnttabV2 bool +ArchCapabilities uint32 } type Connectorinfo struct { 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 +#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/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h index 76b55bf62085..d53b88182560 100644 --- a/tools/include/xen-tools/common-macros.h +++ b/tools/include/xen-tools/common-macros.h @@ -72,6 +72,8 @@ #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1)) #endif +#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) + #ifndef __must_check #define __must_check __attribute__((__warn_unused_result__)) #endif diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c index a0bf7d186f69..175d6dde0b80 100644 --- a/tools/libs/light/libxl.c +++ b/tools/libs/light/libxl.c @@ -409,6 +409,7 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo) !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_gnttab_v1); physinfo->cap_gnttab_v2 = !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_gnttab_v2); + physinfo->arch_capabilities = xcphysinfo.arch_capabilities; GC_FREE; return 0; diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h index 5244fde6239a..8aba3e138909 100644 --- a/tools/libs/light/libxl_internal.h +++ b/tools/libs/light/libxl_internal.h @@ -132,7 +132,6 @@ #define DIV_ROUNDUP(n, d) (((n) + (d) - 1) / (d)) -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m))) #define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m)) #define LIBXL__LOGGING_ENABLED 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), ], dir=DIR_OUT) libxl_connectorinfo = Struct("connectorinfo", [ diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml index e4096bf92c1d..bf23ca50bb15 100644 --- a/tools/ocaml/libs/xc/xenctrl.ml +++ b/tools/ocaml/libs/xc/xenctrl.ml @@ -128,12 +128,10 @@ type physinfo_cap_flag = | CAP_Gnttab_v1 | CAP_Gnttab_v2 -type arm_physinfo_cap_flag - type x86_physinfo_cap_flag type arch_physinfo_cap_flags = - | ARM of arm_physinfo_cap_flag list + | ARM of int | X86 of x86_physinfo_cap_flag list type physinfo = diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli index ef2254537430..ed1e28ea30a0 100644 --- a/tools/ocaml/libs/xc/xenctrl.mli +++ b/tools/ocaml/libs/xc/xenctrl.mli @@ -113,12 +113,10 @@ type physinfo_cap_flag = | CAP_Gnttab_v1 | CAP_Gnttab_v2 -type arm_physinfo_cap_flag - type x86_physinfo_cap_flag type arch_physinfo_cap_flags = - | ARM of arm_physinfo_cap_flag list + | ARM of int | X86 of x86_physinfo_cap_flag list type physinfo = { diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c index 6ec9ed6d1e6f..526a3610fa42 100644 --- a/tools/ocaml/libs/xc/xenctrl_stubs.c +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c @@ -853,13 +853,15 @@ CAMLprim value stub_xc_physinfo(value xch_val) arch_cap_list = Tag_cons; arch_cap_flags_tag = 1; /* tag x86 */ -#else - caml_failwith("Unhandled architecture"); -#endif arch_cap_flags = caml_alloc_small(1, arch_cap_flags_tag); Store_field(arch_cap_flags, 0, arch_cap_list); Store_field(physinfo, 10, arch_cap_flags); +#elif defined(__aarch64__) + Store_field(physinfo, 10, Val_int(c_physinfo.arch_capabilities)); +#else + caml_failwith("Unhandled architecture"); +#endif CAMLreturn(physinfo); } 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> #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> - /* 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) + ); } 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> #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;