[PATCH 1/2] tools: Fix ifdef for aarch64 that should include also arm

[PATCH 1/2] tools: Fix ifdef for aarch64 that should include also arm

[Luca Fancellu]

Commit 56a7aaa16bfe introduced some SVE related code that is protected by
'#if defined(__aarch64__)', the issue is that this doesn't take into
consideration when the toolstack is compiled for an arm32 Dom0 running on
an arm64 platform, it should be able to create SVE enabled guests but with
the current code it's not.

So fix the issue by compiling the code when the toolstack is compiled for
both arm32 and arm64.

Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/include/xen-tools/arm-arch-capabilities.h | 2 +-
 tools/python/xen/lowlevel/xc/xc.c               | 2 +-
 tools/xl/xl_info.c                              | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/include/xen-tools/arm-arch-capabilities.h b/tools/include/xen-tools/arm-arch-capabilities.h
index 3849e897925d..4aa4c6c34a99 100644
--- a/tools/include/xen-tools/arm-arch-capabilities.h
+++ b/tools/include/xen-tools/arm-arch-capabilities.h
@@ -14,7 +14,7 @@
 static inline
 unsigned int arch_capabilities_arm_sve(unsigned int arch_capabilities)
 {
-#if defined(__aarch64__)
+#if defined(__arm__) || defined(__aarch64__)
     unsigned int sve_vl = MASK_EXTR(arch_capabilities,
                                     XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
 
diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index 491e88977fd3..e14e223ec903 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -911,7 +911,7 @@ static PyObject *pyxc_physinfo(XcObject *self)
                            "hw_caps",          cpu_cap,
                            "virt_caps",        virt_caps);
 
-#if defined(__aarch64__)
+#if defined(__arm__) || defined(__aarch64__)
     if ( objret ) {
         unsigned int sve_vl_bits;
         PyObject *py_arm_sve_vl;
diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
index ddc42f96b979..72e87eac46d1 100644
--- a/tools/xl/xl_info.c
+++ b/tools/xl/xl_info.c
@@ -226,7 +226,7 @@ static void output_physinfo(void)
         );
 
     /* Print arm SVE vector length only on ARM platforms */
-#if defined(__aarch64__)
+#if defined(__arm__) || defined(__aarch64__)
     maybe_printf("arm_sve_vector_length  : %u\n",
          arch_capabilities_arm_sve(info.arch_capabilities)
         );
-- 
2.34.1

[PATCH 2/2] tools/python: Fix memory leak on error path

[Luca Fancellu]

Commit 56a7aaa16bfe introduced a memory leak on the error path for a
Py_BuildValue built object that on some newly introduced error path
has not the correct reference count handling, fix that by decrementing
the refcount in these path.

Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/python/xen/lowlevel/xc/xc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index e14e223ec903..d3ea350e07b9 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -919,11 +919,16 @@ static PyObject *pyxc_physinfo(XcObject *self)
         sve_vl_bits = arch_capabilities_arm_sve(pinfo.arch_capabilities);
         py_arm_sve_vl = PyLong_FromUnsignedLong(sve_vl_bits);
 
-        if ( !py_arm_sve_vl )
+        if ( !py_arm_sve_vl ) {
+            Py_DECREF(objret);
             return NULL;
+        }
 
-        if( PyDict_SetItemString(objret, "arm_sve_vl", py_arm_sve_vl) )
+        if( PyDict_SetItemString(objret, "arm_sve_vl", py_arm_sve_vl) ) {
+            Py_DECREF(py_arm_sve_vl);
+            Py_DECREF(objret);
             return NULL;
+        }
     }
 #endif
 
-- 
2.34.1

Re: [PATCH 2/2] tools/python: Fix memory leak on error path

[Luca Fancellu]

> On 8 Jun 2023, at 14:59, Luca Fancellu <Luca.Fancellu@arm.com> wrote:
> 
> Commit 56a7aaa16bfe introduced a memory leak on the error path for a
> Py_BuildValue built object that on some newly introduced error path
> has not the correct reference count handling, fix that by decrementing
> the refcount in these path.
> 
> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---

Hi all,

Is there any chance to have this one reviewed by the end of the month?
I’m asking because I have a Jira task attached to this patch and my PM is chasing me :)

If it’s not possible it’s fine either and I’ll have just to report that.

Cheers,
Luca

> tools/python/xen/lowlevel/xc/xc.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
> index e14e223ec903..d3ea350e07b9 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -919,11 +919,16 @@ static PyObject *pyxc_physinfo(XcObject *self)
>         sve_vl_bits = arch_capabilities_arm_sve(pinfo.arch_capabilities);
>         py_arm_sve_vl = PyLong_FromUnsignedLong(sve_vl_bits);
> 
> -        if ( !py_arm_sve_vl )
> +        if ( !py_arm_sve_vl ) {
> +            Py_DECREF(objret);
>             return NULL;
> +        }
> 
> -        if( PyDict_SetItemString(objret, "arm_sve_vl", py_arm_sve_vl) )
> +        if( PyDict_SetItemString(objret, "arm_sve_vl", py_arm_sve_vl) ) {
> +            Py_DECREF(py_arm_sve_vl);
> +            Py_DECREF(objret);
>             return NULL;
> +        }
>     }
> #endif
> 
> -- 
> 2.34.1
> 
> 

Re: [PATCH 2/2] tools/python: Fix memory leak on error path

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1687 bytes --]

On Thu, Jun 08, 2023 at 02:59:13PM +0100, Luca Fancellu wrote:
> Commit 56a7aaa16bfe introduced a memory leak on the error path for a
> Py_BuildValue built object that on some newly introduced error path
> has not the correct reference count handling, fix that by decrementing
> the refcount in these path.
> 
> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> ---
>  tools/python/xen/lowlevel/xc/xc.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
> index e14e223ec903..d3ea350e07b9 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -919,11 +919,16 @@ static PyObject *pyxc_physinfo(XcObject *self)
>          sve_vl_bits = arch_capabilities_arm_sve(pinfo.arch_capabilities);
>          py_arm_sve_vl = PyLong_FromUnsignedLong(sve_vl_bits);
>  
> -        if ( !py_arm_sve_vl )
> +        if ( !py_arm_sve_vl ) {
> +            Py_DECREF(objret);
>              return NULL;
> +        }
>  
> -        if( PyDict_SetItemString(objret, "arm_sve_vl", py_arm_sve_vl) )
> +        if( PyDict_SetItemString(objret, "arm_sve_vl", py_arm_sve_vl) ) {
> +            Py_DECREF(py_arm_sve_vl);
> +            Py_DECREF(objret);
>              return NULL;
> +        }
>      }
>  #endif
>  
> -- 
> 2.34.1
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] tools/python: Fix memory leak on error path

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]

On Mon, Jun 26, 2023 at 08:07:46PM +0200, Marek Marczykowski-Górecki wrote:
> On Thu, Jun 08, 2023 at 02:59:13PM +0100, Luca Fancellu wrote:
> > Commit 56a7aaa16bfe introduced a memory leak on the error path for a
> > Py_BuildValue built object that on some newly introduced error path
> > has not the correct reference count handling, fix that by decrementing
> > the refcount in these path.
> > 
> > Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
> > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Oh, and BTW, in relation to the discussion on the summit about
committing process, the buggy version was committed without my ack,
after waiting for my review for about two weeks.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] tools/python: Fix memory leak on error path

[Julien Grall]

Hi Marek,

On 27/06/2023 13:29, Marek Marczykowski-Górecki wrote:
> On Mon, Jun 26, 2023 at 08:07:46PM +0200, Marek Marczykowski-Górecki wrote:
>> On Thu, Jun 08, 2023 at 02:59:13PM +0100, Luca Fancellu wrote:
>>> Commit 56a7aaa16bfe introduced a memory leak on the error path for a
>>> Py_BuildValue built object that on some newly introduced error path
>>> has not the correct reference count handling, fix that by decrementing
>>> the refcount in these path.
>>>
>>> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
>>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> 
> Oh, and BTW, in relation to the discussion on the summit about
> committing process, the buggy version was committed without my ack,
> after waiting for my review for about two weeks.

I was the committer for the series. In this case, this was not a case of 
committing because I thought the patch was uncontroversial. Instead, 
this was a lack of my side to properly check the acks on the patch. 
Sorry for that.

In general, as I mentioned during the design session, I don't commit 
with missing acks without explicitly saying so on the mailing list and 
this is often after consulting with others on IRC.

On a similar topic. So far, checking the ack is a manual process for me. 
So this is not entirely an error-free process (in particular for patch 
requiring multiple maintainers acks). I would love to have a script that 
could tell me if a patch is fully approved and/or what acks are missing.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/2] tools: Fix ifdef for aarch64 that should include also arm

[Luca Fancellu]

> On 8 Jun 2023, at 14:59, Luca Fancellu <Luca.Fancellu@arm.com> wrote:
> 
> Commit 56a7aaa16bfe introduced some SVE related code that is protected by
> '#if defined(__aarch64__)', the issue is that this doesn't take into
> consideration when the toolstack is compiled for an arm32 Dom0 running on
> an arm64 platform, it should be able to create SVE enabled guests but with
> the current code it's not.
> 
> So fix the issue by compiling the code when the toolstack is compiled for
> both arm32 and arm64.
> 
> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---

Hi all,

Is there any chance to have this one reviewed by the end of the month?
I’m asking because I have a Jira task attached to this patch and my PM is chasing me :)

If it’s not possible it’s fine either and I’ll have just to report that.

Cheers,
Luca


> tools/include/xen-tools/arm-arch-capabilities.h | 2 +-
> tools/python/xen/lowlevel/xc/xc.c               | 2 +-
> tools/xl/xl_info.c                              | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/include/xen-tools/arm-arch-capabilities.h b/tools/include/xen-tools/arm-arch-capabilities.h
> index 3849e897925d..4aa4c6c34a99 100644
> --- a/tools/include/xen-tools/arm-arch-capabilities.h
> +++ b/tools/include/xen-tools/arm-arch-capabilities.h
> @@ -14,7 +14,7 @@
> static inline
> unsigned int arch_capabilities_arm_sve(unsigned int arch_capabilities)
> {
> -#if defined(__aarch64__)
> +#if defined(__arm__) || defined(__aarch64__)
>     unsigned int sve_vl = MASK_EXTR(arch_capabilities,
>                                     XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
> 
> diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
> index 491e88977fd3..e14e223ec903 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -911,7 +911,7 @@ static PyObject *pyxc_physinfo(XcObject *self)
>                            "hw_caps",          cpu_cap,
>                            "virt_caps",        virt_caps);
> 
> -#if defined(__aarch64__)
> +#if defined(__arm__) || defined(__aarch64__)
>     if ( objret ) {
>         unsigned int sve_vl_bits;
>         PyObject *py_arm_sve_vl;
> diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
> index ddc42f96b979..72e87eac46d1 100644
> --- a/tools/xl/xl_info.c
> +++ b/tools/xl/xl_info.c
> @@ -226,7 +226,7 @@ static void output_physinfo(void)
>         );
> 
>     /* Print arm SVE vector length only on ARM platforms */
> -#if defined(__aarch64__)
> +#if defined(__arm__) || defined(__aarch64__)
>     maybe_printf("arm_sve_vector_length  : %u\n",
>          arch_capabilities_arm_sve(info.arch_capabilities)
>         );
> -- 
> 2.34.1
> 
> 

Re: [PATCH 1/2] tools: Fix ifdef for aarch64 that should include also arm

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2819 bytes --]

On Thu, Jun 08, 2023 at 02:59:12PM +0100, Luca Fancellu wrote:
> Commit 56a7aaa16bfe introduced some SVE related code that is protected by
> '#if defined(__aarch64__)', the issue is that this doesn't take into
> consideration when the toolstack is compiled for an arm32 Dom0 running on
> an arm64 platform, it should be able to create SVE enabled guests but with
> the current code it's not.
> 
> So fix the issue by compiling the code when the toolstack is compiled for
> both arm32 and arm64.
> 
> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

> ---
>  tools/include/xen-tools/arm-arch-capabilities.h | 2 +-
>  tools/python/xen/lowlevel/xc/xc.c               | 2 +-
>  tools/xl/xl_info.c                              | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/include/xen-tools/arm-arch-capabilities.h b/tools/include/xen-tools/arm-arch-capabilities.h
> index 3849e897925d..4aa4c6c34a99 100644
> --- a/tools/include/xen-tools/arm-arch-capabilities.h
> +++ b/tools/include/xen-tools/arm-arch-capabilities.h
> @@ -14,7 +14,7 @@
>  static inline
>  unsigned int arch_capabilities_arm_sve(unsigned int arch_capabilities)
>  {
> -#if defined(__aarch64__)
> +#if defined(__arm__) || defined(__aarch64__)
>      unsigned int sve_vl = MASK_EXTR(arch_capabilities,
>                                      XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
>  
> diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
> index 491e88977fd3..e14e223ec903 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -911,7 +911,7 @@ static PyObject *pyxc_physinfo(XcObject *self)
>                             "hw_caps",          cpu_cap,
>                             "virt_caps",        virt_caps);
>  
> -#if defined(__aarch64__)
> +#if defined(__arm__) || defined(__aarch64__)
>      if ( objret ) {
>          unsigned int sve_vl_bits;
>          PyObject *py_arm_sve_vl;
> diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
> index ddc42f96b979..72e87eac46d1 100644
> --- a/tools/xl/xl_info.c
> +++ b/tools/xl/xl_info.c
> @@ -226,7 +226,7 @@ static void output_physinfo(void)
>          );
>  
>      /* Print arm SVE vector length only on ARM platforms */
> -#if defined(__aarch64__)
> +#if defined(__arm__) || defined(__aarch64__)
>      maybe_printf("arm_sve_vector_length  : %u\n",
>           arch_capabilities_arm_sve(info.arch_capabilities)
>          );
> -- 
> 2.34.1
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] tools: Fix ifdef for aarch64 that should include also arm

[Anthony PERARD]

On Thu, Jun 08, 2023 at 02:59:12PM +0100, Luca Fancellu wrote:
> Commit 56a7aaa16bfe introduced some SVE related code that is protected by
> '#if defined(__aarch64__)', the issue is that this doesn't take into
> consideration when the toolstack is compiled for an arm32 Dom0 running on
> an arm64 platform, it should be able to create SVE enabled guests but with
> the current code it's not.
> 
> So fix the issue by compiling the code when the toolstack is compiled for
> both arm32 and arm64.
> 
> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH 1/2] tools: Fix ifdef for aarch64 that should include also arm

[Julien Grall]

Hi,

Replying here because there is no cover letter (in the future please add 
one if you have more than two patches bundled together).

On 04/07/2023 10:42, Anthony PERARD wrote:
> On Thu, Jun 08, 2023 at 02:59:12PM +0100, Luca Fancellu wrote:
>> Commit 56a7aaa16bfe introduced some SVE related code that is protected by
>> '#if defined(__aarch64__)', the issue is that this doesn't take into
>> consideration when the toolstack is compiled for an arm32 Dom0 running on
>> an arm64 platform, it should be able to create SVE enabled guests but with
>> the current code it's not.
>>
>> So fix the issue by compiling the code when the toolstack is compiled for
>> both arm32 and arm64.
>>
>> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>

I have now committed the series.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/2] tools: Fix ifdef for aarch64 that should include also arm

[Luca Fancellu]

> On 4 Jul 2023, at 20:11, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> Replying here because there is no cover letter (in the future please add one if you have more than two patches bundled together).

Sure I will

> 
> On 04/07/2023 10:42, Anthony PERARD wrote:
>> On Thu, Jun 08, 2023 at 02:59:12PM +0100, Luca Fancellu wrote:
>>> Commit 56a7aaa16bfe introduced some SVE related code that is protected by
>>> '#if defined(__aarch64__)', the issue is that this doesn't take into
>>> consideration when the toolstack is compiled for an arm32 Dom0 running on
>>> an arm64 platform, it should be able to create SVE enabled guests but with
>>> the current code it's not.
>>> 
>>> So fix the issue by compiling the code when the toolstack is compiled for
>>> both arm32 and arm64.
>>> 
>>> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
>>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Acked-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> I have now committed the series.

Thank you

> 
> Cheers,
> 
> -- 
> Julien Grall