ppc/spapr: Change printf format to %HWADDR_PRId for MIN_RMA_SLOF

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

Posted by Aditya Gupta 2 months, 1 week ago

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

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;
     }
-- 
2.46.0

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

Posted by Philippe Mathieu-Daudé 2 months, 1 week ago

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.

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

Posted by Aditya Gupta 2 months ago

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.