[PATCH] tools: Move MASK_INSR to common-macros.h

[PATCH] tools: Move MASK_INSR to common-macros.h

[Andrew Cooper]

MASK_EXTR() and MASK_INSR() are a matching pair.  Keep them together.

Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@citrix.com>
CC: Juergen Gross <jgross@suse.com>
CC: Luca Fancellu <luca.fancellu@arm.com>
---
 tools/include/xen-tools/common-macros.h | 1 +
 tools/libs/light/libxl_internal.h       | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h
index d53b88182560..168691be0e93 100644
--- a/tools/include/xen-tools/common-macros.h
+++ b/tools/include/xen-tools/common-macros.h
@@ -73,6 +73,7 @@
 #endif
 
 #define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
+#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
 
 #ifndef __must_check
 #define __must_check __attribute__((__warn_unused_result__))
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 8aba3e138909..61f4fe1dec55 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -132,8 +132,6 @@
 
 #define DIV_ROUNDUP(n, d) (((n) + (d) - 1) / (d))
 
-#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
-
 #define LIBXL__LOGGING_ENABLED
 
 #ifdef LIBXL__LOGGING_ENABLED
-- 
2.30.2

Re: [PATCH] tools: Move MASK_INSR to common-macros.h

[Luca Fancellu]

> On 8 Jun 2023, at 18:40, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> MASK_EXTR() and MASK_INSR() are a matching pair.  Keep them together.
> 
> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")

I don’t think this patch is fixing a bug:

### Fixes:

If your patch fixes a bug in a specific commit, e.g. you found an issue using
``git bisect``, please use the `Fixes:` tag with the first 12 characters of
the commit id, and the one line summary.

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

But it makes sense, so 

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

> ---
> CC: Anthony PERARD <anthony.perard@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: Luca Fancellu <luca.fancellu@arm.com>
> ---
> tools/include/xen-tools/common-macros.h | 1 +
> tools/libs/light/libxl_internal.h       | 2 --
> 2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tools/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h
> index d53b88182560..168691be0e93 100644
> --- a/tools/include/xen-tools/common-macros.h
> +++ b/tools/include/xen-tools/common-macros.h
> @@ -73,6 +73,7 @@
> #endif
> 
> #define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
> +#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
> 
> #ifndef __must_check
> #define __must_check __attribute__((__warn_unused_result__))
> diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
> index 8aba3e138909..61f4fe1dec55 100644
> --- a/tools/libs/light/libxl_internal.h
> +++ b/tools/libs/light/libxl_internal.h
> @@ -132,8 +132,6 @@
> 
> #define DIV_ROUNDUP(n, d) (((n) + (d) - 1) / (d))
> 
> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
> -
> #define LIBXL__LOGGING_ENABLED
> 
> #ifdef LIBXL__LOGGING_ENABLED
> -- 
> 2.30.2
> 

Re: [PATCH] tools: Move MASK_INSR to common-macros.h

[Jan Beulich]

On 08.06.2023 19:40, Andrew Cooper wrote:
> MASK_EXTR() and MASK_INSR() are a matching pair.  Keep them together.

Right, that's one thing which should have been done right away.
The other is that both macros should have been purged from
tools/tests/x86_emulator/x86-emulate.h (which includes
xen-tools/common-macros.h). Luca?

Jan

> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Anthony PERARD <anthony.perard@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: Luca Fancellu <luca.fancellu@arm.com>
> ---
>  tools/include/xen-tools/common-macros.h | 1 +
>  tools/libs/light/libxl_internal.h       | 2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tools/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h
> index d53b88182560..168691be0e93 100644
> --- a/tools/include/xen-tools/common-macros.h
> +++ b/tools/include/xen-tools/common-macros.h
> @@ -73,6 +73,7 @@
>  #endif
>  
>  #define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
> +#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
>  
>  #ifndef __must_check
>  #define __must_check __attribute__((__warn_unused_result__))
> diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
> index 8aba3e138909..61f4fe1dec55 100644
> --- a/tools/libs/light/libxl_internal.h
> +++ b/tools/libs/light/libxl_internal.h
> @@ -132,8 +132,6 @@
>  
>  #define DIV_ROUNDUP(n, d) (((n) + (d) - 1) / (d))
>  
> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
> -
>  #define LIBXL__LOGGING_ENABLED
>  
>  #ifdef LIBXL__LOGGING_ENABLED

Re: [PATCH] tools: Move MASK_INSR to common-macros.h

[Andrew Cooper]

On 09/06/2023 10:31 am, Jan Beulich wrote:
> On 08.06.2023 19:40, Andrew Cooper wrote:
>> MASK_EXTR() and MASK_INSR() are a matching pair.  Keep them together.
> Right, that's one thing which should have been done right away.
> The other is that both macros should have been purged from
> tools/tests/x86_emulator/x86-emulate.h (which includes
> xen-tools/common-macros.h). Luca?

Hmm - I explicitly checked that, and concluded they didn't...  Happy to
purge them if I'm wrong.

~Andrew

Re: [PATCH] tools: Move MASK_INSR to common-macros.h

[Luca Fancellu]

> On 9 Jun 2023, at 10:31, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 08.06.2023 19:40, Andrew Cooper wrote:
>> MASK_EXTR() and MASK_INSR() are a matching pair.  Keep them together.
> 
> Right, that's one thing which should have been done right away.
> The other is that both macros should have been purged from
> tools/tests/x86_emulator/x86-emulate.h (which includes
> xen-tools/common-macros.h). Luca?

mmm right I’ve missed that, if Andrew can handle it in this patch I’m ok, if you
want me to send a patch I can do it

> 
> Jan
> 
>> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Anthony PERARD <anthony.perard@citrix.com>
>> CC: Juergen Gross <jgross@suse.com>
>> CC: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> tools/include/xen-tools/common-macros.h | 1 +
>> tools/libs/light/libxl_internal.h       | 2 --
>> 2 files changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/tools/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h
>> index d53b88182560..168691be0e93 100644
>> --- a/tools/include/xen-tools/common-macros.h
>> +++ b/tools/include/xen-tools/common-macros.h
>> @@ -73,6 +73,7 @@
>> #endif
>> 
>> #define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
>> +#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
>> 
>> #ifndef __must_check
>> #define __must_check __attribute__((__warn_unused_result__))
>> diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
>> index 8aba3e138909..61f4fe1dec55 100644
>> --- a/tools/libs/light/libxl_internal.h
>> +++ b/tools/libs/light/libxl_internal.h
>> @@ -132,8 +132,6 @@
>> 
>> #define DIV_ROUNDUP(n, d) (((n) + (d) - 1) / (d))
>> 
>> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
>> -
>> #define LIBXL__LOGGING_ENABLED
>> 
>> #ifdef LIBXL__LOGGING_ENABLED
> 

Re: [PATCH] tools: Move MASK_INSR to common-macros.h

[Andrew Cooper]

On 08/06/2023 8:37 pm, Luca Fancellu wrote:
>> On 8 Jun 2023, at 18:40, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>
>> MASK_EXTR() and MASK_INSR() are a matching pair.  Keep them together.
>>
>> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
> I don’t think this patch is fixing a bug:
>
> ### Fixes:
>
> If your patch fixes a bug in a specific commit, e.g. you found an issue using
> ``git bisect``, please use the `Fixes:` tag with the first 12 characters of
> the commit id, and the one line summary.

That a poor explanation...

Fixes: is about corrections to the patch, not bugs.

56a7aaa16bfe is unlikely to be backported, but if a downstream were to
backport your SVE patches, Fixes: identify all other patches they need
to take.

Fixes: was specifically invented to let tooling (partially) automate the
task if finding new patches to backport, based on what had already been
backported.

Concrete bugs are the majority reason for a Fixes tag, sure, but not the
only reason.  In this case, a downstream absolutely doesn't want to get
into a position where these macros aren't together in a pair, because it
there will be a case in the future where it causes a build error.

>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> But it makes sense, so 
>
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Thanks.  As you've already indicated that you're ok with fixing up
x86-emulate.h in v2, I'll retain this.

~Andrew

Re: [PATCH] tools: Move MASK_INSR to common-macros.h

[Luca Fancellu]

> On 9 Jun 2023, at 11:06, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 08/06/2023 8:37 pm, Luca Fancellu wrote:
>>> On 8 Jun 2023, at 18:40, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> 
>>> MASK_EXTR() and MASK_INSR() are a matching pair.  Keep them together.
>>> 
>>> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
>> I don’t think this patch is fixing a bug:
>> 
>> ### Fixes:
>> 
>> If your patch fixes a bug in a specific commit, e.g. you found an issue using
>> ``git bisect``, please use the `Fixes:` tag with the first 12 characters of
>> the commit id, and the one line summary.
> 
> That a poor explanation...
> 
> Fixes: is about corrections to the patch, not bugs.
> 
> 56a7aaa16bfe is unlikely to be backported, but if a downstream were to
> backport your SVE patches, Fixes: identify all other patches they need
> to take.
> 
> Fixes: was specifically invented to let tooling (partially) automate the
> task if finding new patches to backport, based on what had already been
> backported.

Ok this makes sense, so that a tool can easily understand where to put the focus.

> 
> Concrete bugs are the majority reason for a Fixes tag, sure, but not the
> only reason.  In this case, a downstream absolutely doesn't want to get
> into a position where these macros aren't together in a pair, because it
> there will be a case in the future where it causes a build error.
> 
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> But it makes sense, so 
>> 
>> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> 
> Thanks.  As you've already indicated that you're ok with fixing up
> x86-emulate.h in v2, I'll retain this.

sure, thanks for fixing it

> 
> ~Andrew

Re: [PATCH] tools: Move MASK_INSR to common-macros.h

[Andrew Cooper]

On 09/06/2023 10:33 am, Andrew Cooper wrote:
> On 09/06/2023 10:31 am, Jan Beulich wrote:
>> On 08.06.2023 19:40, Andrew Cooper wrote:
>>> MASK_EXTR() and MASK_INSR() are a matching pair.  Keep them together.
>> Right, that's one thing which should have been done right away.
>> The other is that both macros should have been purged from
>> tools/tests/x86_emulator/x86-emulate.h (which includes
>> xen-tools/common-macros.h). Luca?
> Hmm - I explicitly checked that, and concluded they didn't...  Happy to
> purge them if I'm wrong.

I think I was looking in an old branch.  Fixed for v2.

~Andrew

Re: [PATCH] tools: Move MASK_INSR to common-macros.h

[Jason Andryuk]

On Fri, Jun 9, 2023 at 6:11 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> MASK_EXTR() and MASK_INSR() are a matching pair.  Keep them together.
>
> Drop the pair from x86-emulate.h which includes common-macros.h.
>
> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

I have (and will now be dropping) an equivalent patch for my
forthcoming HWP v4 series.

Thanks,
Jason

Re: [PATCH] tools: Move MASK_INSR to common-macros.h

[Roger Pau Monné]

On Fri, Jun 09, 2023 at 11:11:05AM +0100, Andrew Cooper wrote:
> MASK_EXTR() and MASK_INSR() are a matching pair.  Keep them together.
> 
> Drop the pair from x86-emulate.h which includes common-macros.h.
> 
> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH] tools: Move MASK_INSR to common-macros.h

[Luca Fancellu]

> On 9 Jun 2023, at 11:11, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> MASK_EXTR() and MASK_INSR() are a matching pair.  Keep them together.
> 
> Drop the pair from x86-emulate.h which includes common-macros.h.
> 
> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I think you forgot to retain my R-by:

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>



> ---
> CC: Anthony PERARD <anthony.perard@citrix.com>
> CC: Juergen Gross <jgross@suse.com>
> CC: Luca Fancellu <luca.fancellu@arm.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> 
> v2:
> * Drop from x86-emulate.h too.
> ---
> tools/include/xen-tools/common-macros.h | 1 +
> tools/libs/light/libxl_internal.h       | 2 --
> tools/tests/x86_emulator/x86-emulate.h  | 3 ---
> 3 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/tools/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h
> index d53b88182560..168691be0e93 100644
> --- a/tools/include/xen-tools/common-macros.h
> +++ b/tools/include/xen-tools/common-macros.h
> @@ -73,6 +73,7 @@
> #endif
> 
> #define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
> +#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
> 
> #ifndef __must_check
> #define __must_check __attribute__((__warn_unused_result__))
> diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
> index 8aba3e138909..61f4fe1dec55 100644
> --- a/tools/libs/light/libxl_internal.h
> +++ b/tools/libs/light/libxl_internal.h
> @@ -132,8 +132,6 @@
> 
> #define DIV_ROUNDUP(n, d) (((n) + (d) - 1) / (d))
> 
> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
> -
> #define LIBXL__LOGGING_ENABLED
> 
> #ifdef LIBXL__LOGGING_ENABLED
> diff --git a/tools/tests/x86_emulator/x86-emulate.h b/tools/tests/x86_emulator/x86-emulate.h
> index de1f82668010..aa1ed75ec890 100644
> --- a/tools/tests/x86_emulator/x86-emulate.h
> +++ b/tools/tests/x86_emulator/x86-emulate.h
> @@ -51,9 +51,6 @@
> #define DEFINE_PER_CPU(type, var) type per_cpu_##var
> #define this_cpu(var) per_cpu_##var
> 
> -#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
> -#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
> -
> #define __init
> #define __maybe_unused __attribute__((__unused__))
> 
> -- 
> 2.30.2
> 

[PATCH] tools: Move MASK_INSR to common-macros.h

[Andrew Cooper]

MASK_EXTR() and MASK_INSR() are a matching pair.  Keep them together.

Drop the pair from x86-emulate.h which includes common-macros.h.

Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Anthony PERARD <anthony.perard@citrix.com>
CC: Juergen Gross <jgross@suse.com>
CC: Luca Fancellu <luca.fancellu@arm.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

v2:
 * Drop from x86-emulate.h too.
---
 tools/include/xen-tools/common-macros.h | 1 +
 tools/libs/light/libxl_internal.h       | 2 --
 tools/tests/x86_emulator/x86-emulate.h  | 3 ---
 3 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/tools/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h
index d53b88182560..168691be0e93 100644
--- a/tools/include/xen-tools/common-macros.h
+++ b/tools/include/xen-tools/common-macros.h
@@ -73,6 +73,7 @@
 #endif
 
 #define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
+#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
 
 #ifndef __must_check
 #define __must_check __attribute__((__warn_unused_result__))
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 8aba3e138909..61f4fe1dec55 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -132,8 +132,6 @@
 
 #define DIV_ROUNDUP(n, d) (((n) + (d) - 1) / (d))
 
-#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
-
 #define LIBXL__LOGGING_ENABLED
 
 #ifdef LIBXL__LOGGING_ENABLED
diff --git a/tools/tests/x86_emulator/x86-emulate.h b/tools/tests/x86_emulator/x86-emulate.h
index de1f82668010..aa1ed75ec890 100644
--- a/tools/tests/x86_emulator/x86-emulate.h
+++ b/tools/tests/x86_emulator/x86-emulate.h
@@ -51,9 +51,6 @@
 #define DEFINE_PER_CPU(type, var) type per_cpu_##var
 #define this_cpu(var) per_cpu_##var
 
-#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
-#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
-
 #define __init
 #define __maybe_unused __attribute__((__unused__))
 
-- 
2.30.2