| Message ID | 20240327215102.136001-2-jason.andryuk@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/pvh: Support relocating dom0 kernel | expand |
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) ;-). > 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") Anyway, this patch on it's own looks fine: Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
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@citrix.com> Thank you. Regards, Jason
On 27.03.2024 22:50, Jason Andryuk wrote: > @@ -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; > +} While committing I noticed tab indentation here, which looked to be in conflict with ... > 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, ... everything else in the file. Hence I took the liberty to adjust this. Jan
On 2024-04-04 06:09, Jan Beulich wrote: > On 27.03.2024 22:50, Jason Andryuk wrote: >> @@ -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; >> +} > > While committing I noticed tab indentation here, which looked to be in > conflict with ... > >> 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, > > ... everything else in the file. Hence I took the liberty to adjust this. Thank you for catching that and fixing it up. -Jason
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,
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@amd.com> --- v4: New --- tools/helpers/init-xenstore-domain.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)