Re: [RFC PATCH] ppc/spapr: Change printf format to %HWADDR_PRId for MIN_RMA_SLOF

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.