Re: [PATCH v6 2/4] tools: Move MB/GB() to common-macros.h

[Anthony PERARD <anthony.perard@xxxxxxxxx>]

On Wed, Mar 27, 2024 at 05:51:00PM -0400, Jason Andryuk wrote:
> Consolidate to a single set of common macros for tools.
> 
> MB() will gain another use in libelf, so this movement makes it
> available.
> 
> Requested-by: Jan Beulich <jbeulich@xxxxxxxx>
> Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---

So, this patch fixes potential use issues with the macros in hvmloader
and init-xenstore-domain. While it's not perfect, it still better.

I'll try "MB(memory + 0)" with the different macros:


> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index 87be213dec..14078bde1e 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -38,9 +38,6 @@ void __bug(const char *file, int line) 
> __attribute__((noreturn));
>  #define BUG() __bug(__FILE__, __LINE__)
>  #define BUG_ON(p) do { if (p) BUG(); } while (0)
>  
> -#define MB(mb) (mb##ULL << 20)
> -#define GB(gb) (gb##ULL << 30)

With this change we have this change for MB(memory + 0) when applied:
-  (memory + 0ULL << 20)
+  ((memory + 0ULL) << 20)

>  static inline int test_bit(unsigned int b, const void *p)
>  {
>      return !!(((const uint8_t *)p)[b>>3] & (1u<<(b&7)));
> diff --git a/tools/helpers/init-xenstore-domain.c 
> b/tools/helpers/init-xenstore-domain.c
> index 5405842dfe..f38ba8d6b5 100644
> --- a/tools/helpers/init-xenstore-domain.c
> +++ b/tools/helpers/init-xenstore-domain.c
> @@ -20,7 +20,6 @@
>  #include "init-dom-json.h"
>  
>  #define LAPIC_BASE_ADDRESS  0xfee00000UL
> -#define GB(x)               ((uint64_t)x << 30)

With this change we have this change for GB(memory + 0) when applied:
-  ((uint64_t)memory + 0 << 30)
+  ((memory + 0ULL) << 30)


So overall, patch makes things better and less duplication:
Acked-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

Thanks,

-- 
Anthony PERARD