Re: [PATCH 08/10] tools: add SVE capability and SVE vector length for physinfo

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Feb 02, 2023 at 11:08:14AM +0000, Luca Fancellu wrote:
> Recent changes added in struct xen_sysctl_physinfo a new flag for
> arch_capabilities to understand when the platform is SVE capable,
> another field having the maximum SVE vector length in bits was
> added as well, so update the tools to handle these new fields.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> Changes from RFC:
>  - new patch
> ---
> This patch is mostly dependent on the decisions we make in the previous, 
> anyway
> I touched some part of the toolstack where my knowledge is limited (ocaml) so
> I might need a feedback for something I may have done wrong.
> ---
>  tools/golang/xenlight/helpers.gen.go |  4 ++++
>  tools/golang/xenlight/types.gen.go   |  2 ++
>  tools/include/libxl.h                |  8 ++++++++
>  tools/libs/light/libxl.c             |  3 +++
>  tools/libs/light/libxl_types.idl     |  2 ++
>  tools/ocaml/libs/xc/xenctrl.ml       |  4 +++-
>  tools/ocaml/libs/xc/xenctrl.mli      |  4 +++-
>  tools/ocaml/libs/xc/xenctrl_stubs.c  | 17 +++++++++++++----
>  tools/python/xen/lowlevel/xc/xc.c    | 17 +++++++++++++++--
>  tools/xl/xl_info.c                   | 10 ++++++++++
>  10 files changed, 63 insertions(+), 8 deletions(-)

For the python part:

> diff --git a/tools/python/xen/lowlevel/xc/xc.c 
> b/tools/python/xen/lowlevel/xc/xc.c
> index fd008610329b..516fa57161a6 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -870,6 +870,11 @@ static PyObject *pyxc_physinfo(XcObject *self)
>      const char *virtcap_names[] = { "hvm", "pv" };
>      const unsigned virtcaps_bits[] = { XEN_SYSCTL_PHYSCAP_hvm,
>                                         XEN_SYSCTL_PHYSCAP_pv };
> +#if defined(__aarch64__)
> +    const char py_buildval[] = 
> "{s:i,s:i,s:i,s:i,s:l,s:l,s:l,s:i,s:s,s:s,s:i}";
> +#else
> +    const char py_buildval[] = "{s:i,s:i,s:i,s:i,s:l,s:l,s:l,s:i,s:s,s:s}";
> +#endif

I don't like this #if for a different return format depending on the
architecture, especially when the underlying structure is in fact the
same (just not all fields are used for every arch). It would make adding
further fields unnecessarily error-prone.

Instead, you can do common stuff with Py_BuildValue and then add
architecture-specific stuff with PyDict_SetItemString:
https://docs.python.org/3/c-api/dict.html

>      if ( xc_physinfo(self->xc_handle, &pinfo) != 0 )
>          return pyxc_error_to_exception(self->xc_handle);
> @@ -893,10 +898,14 @@ static PyObject *pyxc_physinfo(XcObject *self)
>          for ( i = 0; i < ARRAY_SIZE(virtcaps_bits); i++ )
>              if ( pinfo.capabilities & virtcaps_bits[i] )
>                p += sprintf(p, "%s_directio ", virtcap_names[i]);
> +#if defined(__aarch64__)
> +    if ( pinfo.arch_capabilities & XEN_SYSCTL_PHYSCAP_ARM_sve )
> +        p += sprintf(p, "%s ", "arm_sve");
> +#endif
>      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(py_buildval,
>                              "nr_nodes",         pinfo.nr_nodes,
>                              "threads_per_core", pinfo.threads_per_core,
>                              "cores_per_socket", pinfo.cores_per_socket,
> @@ -906,7 +915,11 @@ 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
> +#if defined(__aarch64__)
> +                            , "arm_sve_vl_bits",  pinfo.arm_sve_vl_bits
> +#endif
> +                            );
>  }
>  

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature