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

[Aditya Gupta]

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.