| Message ID | 20240912065207.508808-1-adityag@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] ppc/spapr: Change printf format to %HWADDR_PRId for MIN_RMA_SLOF | expand |
Hi Aditya, On 12/9/24 08:52, Aditya Gupta wrote: > Currently starting a pSeries machine, with lesser than 128MiB shows > below error: > > qemu-system-ppc64: pSeries SLOF firmware requires >= 80ldMiB guest RMA (Real Mode Area memory) > > Above '80ldMib' is in hex, and it means 0x80 MiB = 128 MiB. > > Change format specifier for this value to use 'HWADDR_PRId', instead of > 'HWADDR_PRIx' thus showing decimal value instead of hex. > > Thus, change the message to below error: > > qemu-system-ppc64: pSeries SLOF firmware requires >= 128MiB guest RMA (Real Mode Area memory) > > Signed-off-by: Aditya Gupta <adityag@linux.ibm.com> > > --- > This is an RFC, as it confused me why does QEMU print that error even with '-m' >80 MB. > > This patch can also be considered a personal preference to see it as a decimal value instead of hex. > > Or maybe we can have '0x80 MiB' instead ? Simply use size_to_str(), see hw/ppc/pnv.c: /* allocate RAM */ if (machine->ram_size < mc->default_ram_size) { char *sz = size_to_str(mc->default_ram_size); error_report("Invalid RAM size, should be bigger than %s", sz); g_free(sz); exit(EXIT_FAILURE); } > Does the 'ldMiB' actually mean that the value is in hexadecimal ? I did not find a reason in git history. > --- > --- > hw/ppc/spapr.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 8aa3ce7449be..b2ddacc6dd01 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2819,8 +2819,8 @@ static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp) > > if (rma_size < MIN_RMA_SLOF) { > error_setg(errp, > - "pSeries SLOF firmware requires >= %" HWADDR_PRIx > - "ldMiB guest RMA (Real Mode Area memory)", > + "pSeries SLOF firmware requires >= %" HWADDR_PRId > + "MiB guest RMA (Real Mode Area memory)", > MIN_RMA_SLOF / MiB); > return 0; > } Amusingly MIN_RMA_SLOF is a constant: #define MIN_RMA_SLOF (128 * MiB) Anyhow it could be changed, so better not hard-code the value in the error message. Your patch becomes: if (rma_size < MIN_RMA_SLOF) { - error_setg(errp, - "pSeries SLOF firmware requires >= %" HWADDR_PRIx - "ldMiB guest RMA (Real Mode Area memory)", - MIN_RMA_SLOF / MiB); + g_autofree char *min_rma_size_str = size_to_str(MIN_RMA_SLOF); + + error_setg(errp, "pSeries SLOF firmware requires >= %s guest" + "RMA (Real Mode Area memory)", min_rma_size_str); return 0; } Regards, Phil.
Hi Philippe, Sorry for the late reply. On 12/09/24 12:34, Philippe Mathieu-Daudé wrote: > Hi Aditya, > > On 12/9/24 08:52, Aditya Gupta wrote: >> Currently starting a pSeries machine, with lesser than 128MiB shows >> below error: >> >> qemu-system-ppc64: pSeries SLOF firmware requires >= 80ldMiB >> guest RMA (Real Mode Area memory) >> >> Above '80ldMib' is in hex, and it means 0x80 MiB = 128 MiB. >> >> Change format specifier for this value to use 'HWADDR_PRId', instead of >> 'HWADDR_PRIx' thus showing decimal value instead of hex. >> >> Thus, change the message to below error: >> >> qemu-system-ppc64: pSeries SLOF firmware requires >= 128MiB >> guest RMA (Real Mode Area memory) >> >> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com> >> >> --- >> This is an RFC, as it confused me why does QEMU print that error even >> with '-m' >80 MB. >> >> This patch can also be considered a personal preference to see it as >> a decimal value instead of hex. >> >> Or maybe we can have '0x80 MiB' instead ? > > Simply use size_to_str(), see hw/ppc/pnv.c: > > /* allocate RAM */ > if (machine->ram_size < mc->default_ram_size) { > char *sz = size_to_str(mc->default_ram_size); > error_report("Invalid RAM size, should be bigger than %s", sz); > g_free(sz); > exit(EXIT_FAILURE); > } > I would prefer the existing printf approach, as it seems simple to me, but can change to use 'size_to_str' if that's the generally accepted approach. What do you say ? Thanks, Aditya Gupta >> Does the 'ldMiB' actually mean that the value is in hexadecimal ? I >> did not find a reason in git history. >> --- >> --- >> hw/ppc/spapr.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index 8aa3ce7449be..b2ddacc6dd01 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -2819,8 +2819,8 @@ static hwaddr spapr_rma_size(SpaprMachineState >> *spapr, Error **errp) >> if (rma_size < MIN_RMA_SLOF) { >> error_setg(errp, >> - "pSeries SLOF firmware requires >= %" HWADDR_PRIx >> - "ldMiB guest RMA (Real Mode Area memory)", >> + "pSeries SLOF firmware requires >= %" HWADDR_PRId >> + "MiB guest RMA (Real Mode Area memory)", >> MIN_RMA_SLOF / MiB); >> return 0; >> } > > Amusingly MIN_RMA_SLOF is a constant: > > #define MIN_RMA_SLOF (128 * MiB) > > Anyhow it could be changed, so better not hard-code the value in the > error message. Your patch becomes: > > if (rma_size < MIN_RMA_SLOF) { > - error_setg(errp, > - "pSeries SLOF firmware requires >= %" HWADDR_PRIx > - "ldMiB guest RMA (Real Mode Area memory)", > - MIN_RMA_SLOF / MiB); > + g_autofree char *min_rma_size_str = size_to_str(MIN_RMA_SLOF); > + > + error_setg(errp, "pSeries SLOF firmware requires >= %s guest" > + "RMA (Real Mode Area memory)", > min_rma_size_str); > return 0; > } > > Regards, > > Phil.
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 8aa3ce7449be..b2ddacc6dd01 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2819,8 +2819,8 @@ static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp) if (rma_size < MIN_RMA_SLOF) { error_setg(errp, - "pSeries SLOF firmware requires >= %" HWADDR_PRIx - "ldMiB guest RMA (Real Mode Area memory)", + "pSeries SLOF firmware requires >= %" HWADDR_PRId + "MiB guest RMA (Real Mode Area memory)", MIN_RMA_SLOF / MiB); return 0; }
Currently starting a pSeries machine, with lesser than 128MiB shows below error: qemu-system-ppc64: pSeries SLOF firmware requires >= 80ldMiB guest RMA (Real Mode Area memory) Above '80ldMib' is in hex, and it means 0x80 MiB = 128 MiB. Change format specifier for this value to use 'HWADDR_PRId', instead of 'HWADDR_PRIx' thus showing decimal value instead of hex. Thus, change the message to below error: qemu-system-ppc64: pSeries SLOF firmware requires >= 128MiB guest RMA (Real Mode Area memory) Signed-off-by: Aditya Gupta <adityag@linux.ibm.com> --- This is an RFC, as it confused me why does QEMU print that error even with '-m' >80 MB. This patch can also be considered a personal preference to see it as a decimal value instead of hex. Or maybe we can have '0x80 MiB' instead ? Does the 'ldMiB' actually mean that the value is in hexadecimal ? I did not find a reason in git history. --- --- hw/ppc/spapr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)