Re: [PATCH v6 1/4] tools/init-xenstore-domain: Replace variable MB() usage

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2024-04-03 08:41, Anthony PERARD wrote:
> On Wed, Mar 27, 2024 at 05:50:59PM -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
> By the way, this is not true, the macro introduce in the following patch
> ("tools: Move MB/GB() to common-macros.h") works (compiler doesn't
> complain) if you do something like MB(maxmem+0) ;-).
Well, I think I wrote the wrong word for the description.  The old Macro 
might have been okay, but the new one as (_AC(_mb, ULL) << 20) fails for 
MB(memory):
init-xenstore-domain.c: In function ‘build’:
init-xenstore-domain.c:82:28: error: ‘memoryULL’ undeclared (first use 
in this function); did you mean ‘memory’?
   82 |     uint64_t mem_size = MB(memory);
      |                            ^~~~~~

I should have written "only work for numeric values."

> > static inline mb_to_bytes() in place of the MB() macro to convert the
> > variable values.
> >
> > 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);
> >       struct e820entry e820[3];
> >       struct xen_domctl_createdomain config = {
> >           .ssidref = SECINITSID_DOMU,
>   
> Sorry, just notice they were more versions of the series, so repeating
> here:
>
> 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")
Nice catch!

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

Regards,
Jason