Re: [Patch v5 6/6] hw/ppc: Pass errp to load_image_targphys() and report errors

[Vishal Chourasia <vishalc@linux.ibm.com>]

On 21/10/25 18:09, BALATON Zoltan wrote:
> On Tue, 21 Oct 2025, Vishal Chourasia wrote:
>> Pass errp to load_image_targphys() calls in ppc machine initialization
>> to capture detailed error information when loading firmware, kernel,
>> and initrd images.
>>
>> Use error_reportf_err() instead of error_report() to print the
>> underlying error details along with context about which image failed
>> to load.
>>
>> Signed-off-by: Vishal Chourasia <vishalc@linux.ibm.com>
>> ---
>> hw/ppc/amigaone.c      | 15 ++++++++-------
>> hw/ppc/e500.c          | 17 +++++++++--------
>> hw/ppc/mac_newworld.c  | 25 +++++++++++++++----------
>> hw/ppc/mac_oldworld.c  | 25 +++++++++++++++----------
>> hw/ppc/pegasos2.c      | 17 +++++++++++------
>> hw/ppc/pnv.c           | 31 ++++++++++++++-----------------
>> hw/ppc/ppc440_bamboo.c |  9 +++++----
>> hw/ppc/prep.c          | 25 +++++++++++++++----------
>> hw/ppc/sam460ex.c      |  9 +++++----
>> hw/ppc/spapr.c         | 15 ++++++++-------
>> hw/ppc/virtex_ml507.c  | 17 +++++++++++------
>> 11 files changed, 116 insertions(+), 89 deletions(-)
>>
>> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
>> index 5c5acc9872..bd14bed243 100644
>> --- a/hw/ppc/amigaone.c
>> +++ b/hw/ppc/amigaone.c
>> @@ -276,6 +276,7 @@ static void amigaone_init(MachineState *machine)
>>     DriveInfo *di;
>>     hwaddr loadaddr;
>>     struct boot_info *bi = NULL;
>> +    Error *errp = NULL;
>>
>>     /* init CPU */
>>     cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
>> @@ -324,9 +325,9 @@ static void amigaone_init(MachineState *machine)
>>             error_report("Could not find firmware '%s'", 
>> machine->firmware);
>>             exit(1);
>>         }
>> -        sz = load_image_targphys(filename, PROM_ADDR, PROM_SIZE, NULL);
>> -        if (sz <= 0 || sz > PROM_SIZE) {
>> -            error_report("Could not load firmware '%s'", filename);
>> +        sz = load_image_targphys(filename, PROM_ADDR, PROM_SIZE, 
>> &errp);
>> +        if (errp) {
>> +            error_reportf_err(errp, "Could not load firmware '%s': 
>> ", filename);
>>             exit(1);
>
> If all that's done with an errp is checking for error, printing it and 
> then exit then isn't that what passing &error_fatal is for? So these 
> should all just do that instead of open coding it, don't they? This 
> would lose the general local error messages and those would be 
> replaced by the more specific ones from the load functions that should 
> be enough. (I'm always unsure how to correctly use error and errp so 
> some more expert opinion could be useful but otherwise this series 
> does not seem to simplify things much.)

I am planning to add filename information inside the 
load_image_targphys() so that we won't need additional error reporting 
regarding the file type in the caller.
We can use error_fatal where ever we are just exiting after printing the 
error. However, in some cases, it may proceed for different reasons in 
which case errp would be more appropriate.

> If you don't want to convert all these other machines I'm also OK with 
> leaving these passing NULL to load_image_targphys for now and keep 
> current error handling which can then be converted later so 
> alternatively you can drop this patch if you can't finish it before 
> the freeze. 

I will try to improve at least for the PPC boards and see if that would 
work better.

Thanks,
Vishal

>
> Regards,
> BALATON Zoltan