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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 25 May 2023, at 09:39, Marek Marczykowski-Górecki 
> <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> 
> On Tue, May 23, 2023 at 08:43:23AM +0100, Luca Fancellu wrote:
>> On Arm, the SVE vector length is encoded in arch_capabilities field
>> of struct xen_sysctl_physinfo, make use of this field in the tools
>> when building for arm.
>> 
>> Create header arm-arch-capabilities.h to handle the arch_capabilities
>> field of physinfo for Arm.
>> 
>> Removed include for xen-tools/common-macros.h in
>> python/xen/lowlevel/xc/xc.c because it is already included by the
>> arm-arch-capabilities.h header.
>> 
>> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>> Acked-by: George Dunlap <george.dunlap@xxxxxxxxxx>
>> Acked-by: Christian Lindig <christian.lindig@xxxxxxxxx>
>> Reviewed-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>> ---
>> Changes from v6:
>> - Fix licence header in arm-atch-capabilities.h, add R-by (Anthony)
>> Changes from v5:
>> - no changes
>> Changes from v4:
>> - Move arm-arch-capabilities.h into xen-tools/, add LIBXL_HAVE_,
>>   fixed python return type to I instead of i. (Anthony)
>> Changes from v3:
>> - add Ack-by for the Golang bits (George)
>> - add Ack-by for the OCaml tools (Christian)
>> - now xen-tools/libs.h is named xen-tools/common-macros.h
>> - changed commit message to explain why the header modification
>>   in python/xen/lowlevel/xc/xc.c
>> Changes from v2:
>> - rename arm_arch_capabilities.h in arm-arch-capabilities.h, use
>>   MASK_EXTR.
>> - Now arm-arch-capabilities.h needs MASK_EXTR macro, but it is
>>   defined in libxl_internal.h, it doesn't feel right to include
>>   that header so move MASK_EXTR into xen-tools/libs.h that is also
>>   included in libxl_internal.h
>> Changes from v1:
>> - now SVE VL is encoded in arch_capabilities on Arm
>> Changes from RFC:
>> - new patch
>> ---
>> tools/golang/xenlight/helpers.gen.go          |  2 ++
>> tools/golang/xenlight/types.gen.go            |  1 +
>> tools/include/libxl.h                         |  6 ++++
>> .../include/xen-tools/arm-arch-capabilities.h | 28 +++++++++++++++++++
>> tools/include/xen-tools/common-macros.h       |  2 ++
>> tools/libs/light/libxl.c                      |  1 +
>> tools/libs/light/libxl_internal.h             |  1 -
>> tools/libs/light/libxl_types.idl              |  1 +
>> tools/ocaml/libs/xc/xenctrl.ml                |  4 +--
>> tools/ocaml/libs/xc/xenctrl.mli               |  4 +--
>> tools/ocaml/libs/xc/xenctrl_stubs.c           |  8 ++++--
>> tools/python/xen/lowlevel/xc/xc.c             |  8 ++++--
>> tools/xl/xl_info.c                            |  8 ++++++
>> 13 files changed, 62 insertions(+), 12 deletions(-)
>> create mode 100644 tools/include/xen-tools/arm-arch-capabilities.h
>> 
> 
> (...)
> 
>> diff --git a/tools/python/xen/lowlevel/xc/xc.c 
>> b/tools/python/xen/lowlevel/xc/xc.c
>> index 9728b34185ac..b3699fdac58e 100644
>> --- a/tools/python/xen/lowlevel/xc/xc.c
>> +++ b/tools/python/xen/lowlevel/xc/xc.c
>> @@ -22,6 +22,7 @@
>> #include <xen/hvm/hvm_info_table.h>
>> #include <xen/hvm/params.h>
>> 
>> +#include <xen-tools/arm-arch-capabilities.h>
>> #include <xen-tools/common-macros.h>
>> 
>> /* Needed for Python versions earlier than 2.3. */
>> @@ -897,7 +898,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 +908,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)
>> +                        );
> 
> This should be added only when building for ARM, similar as for other
> bindings.

Hi Marek,

Thank you for taking the time to review this, are you ok if I make these 
changes to the code?

diff --git a/tools/python/xen/lowlevel/xc/xc.c 
b/tools/python/xen/lowlevel/xc/xc.c
index b3699fdac58e..c7f690189770 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -872,6 +872,8 @@ static PyObject *pyxc_physinfo(XcObject *self)
     const char *virtcap_names[] = { "hvm", "pv" };
     const unsigned virtcaps_bits[] = { XEN_SYSCTL_PHYSCAP_hvm,
                                        XEN_SYSCTL_PHYSCAP_pv };
+    PyObject *objret;
+    int retcode;
 
     if ( xc_physinfo(self->xc_handle, &pinfo) != 0 )
         return pyxc_error_to_exception(self->xc_handle);
@@ -898,20 +900,31 @@ 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,s:I}",
-                            "nr_nodes",         pinfo.nr_nodes,
-                            "threads_per_core", pinfo.threads_per_core,
-                            "cores_per_socket", pinfo.cores_per_socket,
-                            "nr_cpus",          pinfo.nr_cpus,
-                            "total_memory",     
pages_to_kib(pinfo.total_pages),
-                            "free_memory",      pages_to_kib(pinfo.free_pages),
-                            "scrub_memory",     
pages_to_kib(pinfo.scrub_pages),
-                            "cpu_khz",          pinfo.cpu_khz,
-                            "hw_caps",          cpu_cap,
-                            "virt_caps",        virt_caps,
-                            "arm_sve_vl",
-                              
arch_capabilities_arm_sve(pinfo.arch_capabilities)
+    objret = Py_BuildValue("{s:i,s:i,s:i,s:i,s:l,s:l,s:l,s:i,s:s,s:s}",
+                           "nr_nodes",         pinfo.nr_nodes,
+                           "threads_per_core", pinfo.threads_per_core,
+                           "cores_per_socket", pinfo.cores_per_socket,
+                           "nr_cpus",          pinfo.nr_cpus,
+                           "total_memory",     pages_to_kib(pinfo.total_pages),
+                           "free_memory",      pages_to_kib(pinfo.free_pages),
+                           "scrub_memory",     pages_to_kib(pinfo.scrub_pages),
+                           "cpu_khz",          pinfo.cpu_khz,
+                           "hw_caps",          cpu_cap,
+                           "virt_caps",        virt_caps
                         );
+
+    #if defined(__aarch64__)
+        if (objret) {
+            retcode = PyDict_SetItemString(
+                            objret, "arm_sve_vl",
+                            arch_capabilities_arm_sve(pinfo.arch_capabilities)
+                        );
+            if ( retcode < 0 )
+                return NULL;
+        }
+    #endif
+
+    return objret;
 }
 
 static PyObject *pyxc_getcpuinfo(XcObject *self, PyObject *args, PyObject 
*kwds)


Please notice that now we can have a path that could return NULL, are you ok for
It or should I just ignore the return code for PyDict_SetItemString?

Also, do you want me to protect the include to 
<xen-tools/arm-arch-capabilities.h>
with ifdef?

> 
>> }
>> 
>> static PyObject *pyxc_getcpuinfo(XcObject *self, PyObject *args, PyObject 
>> *kwds)
> 
> -- 
> Best Regards,
> Marek Marczykowski-Górecki
> Invisible Things Lab