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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 25 May 2023, at 10:21, Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
> 
>>> 
>>> (...)
>>> 
>>>> 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?
>> 
> 
> EDIT: I saw this doesn’t even compile, I will ask later when I will have 
> something working,
> I saw PyDict_SetItemString is used somewhere else so I will use that approach 
> before
> Proposing you a solution
> 
> 

Ok, so this is my proposed solution:

diff --git a/tools/python/xen/lowlevel/xc/xc.c 
b/tools/python/xen/lowlevel/xc/xc.c
index b3699fdac58e..e52aa88f3c5f 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -872,6 +872,7 @@ 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;
 
     if ( xc_physinfo(self->xc_handle, &pinfo) != 0 )
         return pyxc_error_to_exception(self->xc_handle);
@@ -898,20 +899,36 @@ 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 ) {
+            unsigned int sve_vl_bits;
+            PyObject *py_arm_sve_vl;
+
+            sve_vl_bits = arch_capabilities_arm_sve(pinfo.arch_capabilities);
+            py_arm_sve_vl = PyLong_FromUnsignedLong(sve_vl_bits);
+
+            if ( !py_arm_sve_vl )
+                return NULL;
+
+            if( PyDict_SetItemString(objret, "arm_sve_vl", py_arm_sve_vl) )
+                return NULL;
+        }
+    #endif
+
+    return objret;
 }
 
 static PyObject *pyxc_getcpuinfo(XcObject *self, PyObject *args, PyObject 
*kwds)

Would it work for you?



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