Re: [PATCH v4 2/5] tools/init-xenstore-domain: Replace variable MB() usage

[Anthony PERARD <anthony.perard@xxxxxxxxx>]

On Mon, Mar 25, 2024 at 04:45:12PM -0400, Jason Andryuk wrote:
> The local MB() & GB() macros will be replaced with a common
> implementation, but those only work with constant values.  Introduce a
> static inline mb_to_bytes() in place of the MB() macro to convert the
> variable values.
> 
> Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>
> ---
> v4:
> New
> ---
>  tools/helpers/init-xenstore-domain.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/helpers/init-xenstore-domain.c 
> b/tools/helpers/init-xenstore-domain.c
> index 1683438c5c..5405842dfe 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 MB(x)               ((uint64_t)x << 20)
>  #define GB(x)               ((uint64_t)x << 30)
>  
>  static uint32_t domid = ~0;
> @@ -36,6 +35,11 @@ static xc_evtchn_port_or_error_t console_evtchn;
>  static xentoollog_level minmsglevel = XTL_PROGRESS;
>  static void *logger;
>  
> +static inline uint64_t mb_to_bytes(int mem)
> +{
> +     return (uint64_t)mem << 20;
> +}
> +
>  static struct option options[] = {
>      { "kernel", 1, NULL, 'k' },
>      { "memory", 1, NULL, 'm' },
> @@ -76,8 +80,8 @@ static int build(xc_interface *xch)
>      int rv, xs_fd;
>      struct xc_dom_image *dom = NULL;
>      int limit_kb = (maxmem ? : memory) * 1024 + X86_HVM_NR_SPECIAL_PAGES * 4;
> -    uint64_t mem_size = MB(memory);
> -    uint64_t max_size = MB(maxmem ? : memory);
> +    uint64_t mem_size = mb_to_bytes(memory);
> +    uint64_t max_size = mb_to_bytes(maxmem ? : memory);

Looks like this change actually fix a bug. When `maxmem` is set, we
would have "max_size = maxmem", otherwise, it would be 
"max_size = memory << 20"

The macro should have been written as
 #define MB(x)               ((uint64_t)(x) << 20)

This patch could be backported to 4.17 and later, with:
    Fixes: 134d53f57707 ("tools/init-xenstore-domain: fix memory map for PVH 
stubdom")

Anyway, this patch on it's own looks fine:
Reviewed-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

Thanks,

-- 
Anthony PERARD