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

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

Mar 27, 2024, 2:50 PM

Post #1 of 5 (20 views)

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(-)

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,
--
2.44.0

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

anthony.perard at cloud

Apr 3, 2024, 5:41 AM

Post #2 of 5 (16 views)

Permalink

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>

--
Anthony PERARD

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

jason.andryuk at amd

Apr 3, 2024, 5:51 AM

Post #3 of 5 (16 views)

Permalink

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

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

jbeulich at suse

Apr 4, 2024, 3:09 AM

Post #4 of 5 (14 views)

Permalink

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

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

jason.andryuk at amd

Apr 4, 2024, 6:55 AM

Post #5 of 5 (13 views)

Permalink

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