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

[Aditya Gupta <adityag@linux.ibm.com>]

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.