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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

>> 
>> (...)
>> 
>>> 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



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