[PATCH v2 0/2] ppc: Fixes for fadump feature

[PATCH v2 0/2] ppc: Fixes for fadump feature

[Shivang Upadhyay]

Hi,

This patch series fixes the coverity issues on the Fadump
feature patch series by Aditya [1].

[1] https://lore.kernel.org/qemu-devel/20251021134823.1861675-1-adityag@linux.ibm.com/

Changelog
=========

v2:
  + addressed review comments from Philippe

Shivang Upadhyay (2):
  hw/ppc: Fix missing return on allocation failure
  hw/ppc: Fix memory leak in get_cpu_state_data()

 hw/ppc/spapr_fadump.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.51.0

[PATCH v2 1/2] hw/ppc: Fix missing return on allocation failure

[Shivang Upadhyay]

Fixes coverity (CID 1642026)

Cc: Aditya Gupta <adityag@linux.ibm.com>
Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
Link: https://lore.kernel.org/qemu-devel/CAFEAcA-SPmsnU1wzsWxBcFC=ZM_DDhPEg1N4iX9Q4bL1xOnwBg@mail.gmail.com/
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
---
 hw/ppc/spapr_fadump.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
index fa3aeac94c..883a60cdcf 100644
--- a/hw/ppc/spapr_fadump.c
+++ b/hw/ppc/spapr_fadump.c
@@ -234,6 +234,7 @@ static bool do_preserve_region(FadumpSection *region)
         qemu_log_mask(LOG_GUEST_ERROR,
             "FADump: Failed allocating memory (size: %zu) for copying"
             " reserved memory regions\n", FADUMP_CHUNK_SIZE);
+        return false;
     }
 
     num_chunks = ceil((src_len * 1.0f) / FADUMP_CHUNK_SIZE);
-- 
2.51.0

[PATCH v2 2/2] hw/ppc: Fix memory leak in get_cpu_state_data()

[Shivang Upadhyay]

Fixes coverity (CID 1642024)

Cc: Aditya Gupta <adityag@linux.ibm.com>
Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
Link: https://lore.kernel.org/qemu-devel/CAFEAcA_Bm52bkPi9MH_uugXRR5fj48RtpbOnPNFQtbX=7Mz_yw@mail.gmail.com/
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
---
 hw/ppc/spapr_fadump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
index 883a60cdcf..13cab0cfe1 100644
--- a/hw/ppc/spapr_fadump.c
+++ b/hw/ppc/spapr_fadump.c
@@ -453,7 +453,7 @@ static FadumpRegEntry *populate_cpu_reg_entries(CPUState *cpu,
 static void *get_cpu_state_data(uint64_t *cpu_state_len)
 {
     FadumpRegSaveAreaHeader reg_save_hdr;
-    FadumpRegEntry *reg_entries;
+    g_autofree FadumpRegEntry *reg_entries = NULL;
     FadumpRegEntry *curr_reg_entry;
     CPUState *cpu;
 
-- 
2.51.0

Re: [PATCH v2 2/2] hw/ppc: Fix memory leak in get_cpu_state_data()

[Philippe Mathieu-Daudé]

On 28/10/25 09:05, Shivang Upadhyay wrote:
> Fixes coverity (CID 1642024)
> 
> Cc: Aditya Gupta <adityag@linux.ibm.com>
> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Link: https://lore.kernel.org/qemu-devel/CAFEAcA_Bm52bkPi9MH_uugXRR5fj48RtpbOnPNFQtbX=7Mz_yw@mail.gmail.com/
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
> ---
>   hw/ppc/spapr_fadump.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 1/2] hw/ppc: Fix missing return on allocation failure

[Philippe Mathieu-Daudé]

On 28/10/25 09:05, Shivang Upadhyay wrote:
> Fixes coverity (CID 1642026)
> 
> Cc: Aditya Gupta <adityag@linux.ibm.com>
> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Link: https://lore.kernel.org/qemu-devel/CAFEAcA-SPmsnU1wzsWxBcFC=ZM_DDhPEg1N4iX9Q4bL1xOnwBg@mail.gmail.com/
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
> ---
>   hw/ppc/spapr_fadump.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
> index fa3aeac94c..883a60cdcf 100644
> --- a/hw/ppc/spapr_fadump.c
> +++ b/hw/ppc/spapr_fadump.c
> @@ -234,6 +234,7 @@ static bool do_preserve_region(FadumpSection *region)
>           qemu_log_mask(LOG_GUEST_ERROR,

FWIW host heap exhaustion is not really a *guest* error, because the
guest can not control it.

>               "FADump: Failed allocating memory (size: %zu) for copying"
>               " reserved memory regions\n", FADUMP_CHUNK_SIZE);
> +        return false;
>       }
>   
>       num_chunks = ceil((src_len * 1.0f) / FADUMP_CHUNK_SIZE);

Re: [PATCH v2 1/2] hw/ppc: Fix missing return on allocation failure

[Shivang Upadhyay]

On Tue, Oct 28, 2025 at 09:35:40AM +0100, Philippe Mathieu-Daudé wrote:
> On 28/10/25 09:05, Shivang Upadhyay wrote:
> > Fixes coverity (CID 1642026)
> > 
> > Cc: Aditya Gupta <adityag@linux.ibm.com>
> > Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > Link: https://lore.kernel.org/qemu-devel/CAFEAcA-SPmsnU1wzsWxBcFC=ZM_DDhPEg1N4iX9Q4bL1xOnwBg@mail.gmail.com/
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
> > ---
> >   hw/ppc/spapr_fadump.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
> > index fa3aeac94c..883a60cdcf 100644
> > --- a/hw/ppc/spapr_fadump.c
> > +++ b/hw/ppc/spapr_fadump.c
> > @@ -234,6 +234,7 @@ static bool do_preserve_region(FadumpSection *region)
> >           qemu_log_mask(LOG_GUEST_ERROR,
> 
> FWIW host heap exhaustion is not really a *guest* error, because the
> guest can not control it.
Hi, Philippe


Thanks for the review. There are following log level defined in log.h

	....

	#define CPU_LOG_TB_OUT_ASM (1u << 0)
	#define CPU_LOG_TB_IN_ASM  (1u << 1)
	#define CPU_LOG_TB_OP      (1u << 2)
	#define CPU_LOG_TB_OP_OPT  (1u << 3)
	#define CPU_LOG_INT        (1u << 4)
	#define CPU_LOG_EXEC       (1u << 5)
	#define CPU_LOG_PCALL      (1u << 6)
	#define CPU_LOG_TB_CPU     (1u << 8)
	#define CPU_LOG_RESET      (1u << 9)
	#define LOG_UNIMP          (1u << 10)
	#define LOG_GUEST_ERROR    (1u << 11)
	#define CPU_LOG_MMU        (1u << 12)
	#define CPU_LOG_TB_NOCHAIN (1u << 13)
	#define CPU_LOG_PAGE       (1u << 14)
	/* LOG_TRACE (1 << 15) is defined in log-for-trace.h */
	#define CPU_LOG_TB_OP_IND  (1u << 16)
	#define CPU_LOG_TB_FPU     (1u << 17)
	#define CPU_LOG_PLUGIN     (1u << 18)
	/* LOG_STRACE is used for user-mode strace logging. */
	#define LOG_STRACE         (1u << 19)
	#define LOG_PER_THREAD     (1u << 20)
	#define CPU_LOG_TB_VPU     (1u << 21)
	#define LOG_TB_OP_PLUGIN   (1u << 22)
	#define LOG_INVALID_MEM    (1u << 23)

	....

Which one do you recommend we use? or May we introduce a `LOG_HOST_ERROR`,
if that's more appropriate.

Thanks
~Shivang.
> 
> >               "FADump: Failed allocating memory (size: %zu) for copying"
> >               " reserved memory regions\n", FADUMP_CHUNK_SIZE);
> > +        return false;
> >       }
> >       num_chunks = ceil((src_len * 1.0f) / FADUMP_CHUNK_SIZE);
> 

Re: [PATCH v2 2/2] hw/ppc: Fix memory leak in get_cpu_state_data()

[Shivang Upadhyay]

On Tue, Oct 28, 2025 at 09:33:49AM +0100, Philippe Mathieu-Daudé wrote:
> On 28/10/25 09:05, Shivang Upadhyay wrote:
> > Fixes coverity (CID 1642024)
> > 
> > Cc: Aditya Gupta <adityag@linux.ibm.com>
> > Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > Link: https://lore.kernel.org/qemu-devel/CAFEAcA_Bm52bkPi9MH_uugXRR5fj48RtpbOnPNFQtbX=7Mz_yw@mail.gmail.com/
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
> > ---
> >   hw/ppc/spapr_fadump.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Thanks

~Shivang

Re: [PATCH v2 1/2] hw/ppc: Fix missing return on allocation failure

[Harsh Prateek Bora]

On 10/28/25 15:54, Shivang Upadhyay wrote:
> On Tue, Oct 28, 2025 at 09:35:40AM +0100, Philippe Mathieu-Daudé wrote:
>> On 28/10/25 09:05, Shivang Upadhyay wrote:
>>> Fixes coverity (CID 1642026)
>>>
>>> Cc: Aditya Gupta <adityag@linux.ibm.com>
>>> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
>>> Link: https://lore.kernel.org/qemu-devel/CAFEAcA-SPmsnU1wzsWxBcFC=ZM_DDhPEg1N4iX9Q4bL1xOnwBg@mail.gmail.com/
>>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>>> Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
>>> ---
>>>    hw/ppc/spapr_fadump.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
>>> index fa3aeac94c..883a60cdcf 100644
>>> --- a/hw/ppc/spapr_fadump.c
>>> +++ b/hw/ppc/spapr_fadump.c
>>> @@ -234,6 +234,7 @@ static bool do_preserve_region(FadumpSection *region)
>>>            qemu_log_mask(LOG_GUEST_ERROR,
>>
>> FWIW host heap exhaustion is not really a *guest* error, because the
>> guest can not control it.
> Hi, Philippe
> 
> 
> Thanks for the review. There are following log level defined in log.h
> 
> 	....
> 
> 	#define CPU_LOG_TB_OUT_ASM (1u << 0)
> 	#define CPU_LOG_TB_IN_ASM  (1u << 1)
> 	#define CPU_LOG_TB_OP      (1u << 2)
> 	#define CPU_LOG_TB_OP_OPT  (1u << 3)
> 	#define CPU_LOG_INT        (1u << 4)
> 	#define CPU_LOG_EXEC       (1u << 5)
> 	#define CPU_LOG_PCALL      (1u << 6)
> 	#define CPU_LOG_TB_CPU     (1u << 8)
> 	#define CPU_LOG_RESET      (1u << 9)
> 	#define LOG_UNIMP          (1u << 10)
> 	#define LOG_GUEST_ERROR    (1u << 11)
> 	#define CPU_LOG_MMU        (1u << 12)
> 	#define CPU_LOG_TB_NOCHAIN (1u << 13)
> 	#define CPU_LOG_PAGE       (1u << 14)
> 	/* LOG_TRACE (1 << 15) is defined in log-for-trace.h */
> 	#define CPU_LOG_TB_OP_IND  (1u << 16)
> 	#define CPU_LOG_TB_FPU     (1u << 17)
> 	#define CPU_LOG_PLUGIN     (1u << 18)
> 	/* LOG_STRACE is used for user-mode strace logging. */
> 	#define LOG_STRACE         (1u << 19)
> 	#define LOG_PER_THREAD     (1u << 20)
> 	#define CPU_LOG_TB_VPU     (1u << 21)
> 	#define LOG_TB_OP_PLUGIN   (1u << 22)
> 	#define LOG_INVALID_MEM    (1u << 23)
> 
> 	....
> 
> Which one do you recommend we use? or May we introduce a `LOG_HOST_ERROR`,
> if that's more appropriate.

I think it would be better to have LOG_INSUFF_MEM for this case, but 
let's hear from Philippe and others for suggestions.

Since it's unlreated to the coverity fix and can be taken separately, so:

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> 
> Thanks
> ~Shivang.
>>
>>>                "FADump: Failed allocating memory (size: %zu) for copying"
>>>                " reserved memory regions\n", FADUMP_CHUNK_SIZE);
>>> +        return false;
>>>        }
>>>        num_chunks = ceil((src_len * 1.0f) / FADUMP_CHUNK_SIZE);
>>

Re: [PATCH v2 1/2] hw/ppc: Fix missing return on allocation failure

[Shivang Upadhyay]

On Thu, Oct 30, 2025 at 01:35:11PM +0530, Harsh Prateek Bora wrote:
> 
> 
> On 10/28/25 15:54, Shivang Upadhyay wrote:
> > On Tue, Oct 28, 2025 at 09:35:40AM +0100, Philippe Mathieu-Daudé wrote:
> > > On 28/10/25 09:05, Shivang Upadhyay wrote:
> > > > Fixes coverity (CID 1642026)
> > > > 
> > > > Cc: Aditya Gupta <adityag@linux.ibm.com>
> > > > Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > > > Link: https://lore.kernel.org/qemu-devel/CAFEAcA-SPmsnU1wzsWxBcFC=ZM_DDhPEg1N4iX9Q4bL1xOnwBg@mail.gmail.com/
> > > > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > > > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > > > Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
> > > > ---
> > > >    hw/ppc/spapr_fadump.c | 1 +
> > > >    1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
> > > > index fa3aeac94c..883a60cdcf 100644
> > > > --- a/hw/ppc/spapr_fadump.c
> > > > +++ b/hw/ppc/spapr_fadump.c
> > > > @@ -234,6 +234,7 @@ static bool do_preserve_region(FadumpSection *region)
> > > >            qemu_log_mask(LOG_GUEST_ERROR,
> > > 
> > > FWIW host heap exhaustion is not really a *guest* error, because the
> > > guest can not control it.
> > Hi, Philippe
> > 
> > 
> > Thanks for the review. There are following log level defined in log.h
> > 
> > 	....
> > 
> > 	#define CPU_LOG_TB_OUT_ASM (1u << 0)
> > 	#define CPU_LOG_TB_IN_ASM  (1u << 1)
> > 	#define CPU_LOG_TB_OP      (1u << 2)
> > 	#define CPU_LOG_TB_OP_OPT  (1u << 3)
> > 	#define CPU_LOG_INT        (1u << 4)
> > 	#define CPU_LOG_EXEC       (1u << 5)
> > 	#define CPU_LOG_PCALL      (1u << 6)
> > 	#define CPU_LOG_TB_CPU     (1u << 8)
> > 	#define CPU_LOG_RESET      (1u << 9)
> > 	#define LOG_UNIMP          (1u << 10)
> > 	#define LOG_GUEST_ERROR    (1u << 11)
> > 	#define CPU_LOG_MMU        (1u << 12)
> > 	#define CPU_LOG_TB_NOCHAIN (1u << 13)
> > 	#define CPU_LOG_PAGE       (1u << 14)
> > 	/* LOG_TRACE (1 << 15) is defined in log-for-trace.h */
> > 	#define CPU_LOG_TB_OP_IND  (1u << 16)
> > 	#define CPU_LOG_TB_FPU     (1u << 17)
> > 	#define CPU_LOG_PLUGIN     (1u << 18)
> > 	/* LOG_STRACE is used for user-mode strace logging. */
> > 	#define LOG_STRACE         (1u << 19)
> > 	#define LOG_PER_THREAD     (1u << 20)
> > 	#define CPU_LOG_TB_VPU     (1u << 21)
> > 	#define LOG_TB_OP_PLUGIN   (1u << 22)
> > 	#define LOG_INVALID_MEM    (1u << 23)
> > 
> > 	....
> > 
> > Which one do you recommend we use? or May we introduce a `LOG_HOST_ERROR`,
> > if that's more appropriate.
> 
> I think it would be better to have LOG_INSUFF_MEM for this case, but let's
> hear from Philippe and others for suggestions.
> 
> Since it's unlreated to the coverity fix and can be taken separately, so:
> 
> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
Thanks Harsh.

~Shivang.

Re: [PATCH v2 1/2] hw/ppc: Fix missing return on allocation failure

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3123 bytes --]

On Thu, 30 Oct 2025, Harsh Prateek Bora wrote:
> On 10/28/25 15:54, Shivang Upadhyay wrote:
>> On Tue, Oct 28, 2025 at 09:35:40AM +0100, Philippe Mathieu-Daudé wrote:
>>> On 28/10/25 09:05, Shivang Upadhyay wrote:
>>>> Fixes coverity (CID 1642026)
>>>> 
>>>> Cc: Aditya Gupta <adityag@linux.ibm.com>
>>>> Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
>>>> Link: 
>>>> https://lore.kernel.org/qemu-devel/CAFEAcA-SPmsnU1wzsWxBcFC=ZM_DDhPEg1N4iX9Q4bL1xOnwBg@mail.gmail.com/
>>>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>>>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>>>> Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
>>>> ---
>>>>    hw/ppc/spapr_fadump.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>> 
>>>> diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
>>>> index fa3aeac94c..883a60cdcf 100644
>>>> --- a/hw/ppc/spapr_fadump.c
>>>> +++ b/hw/ppc/spapr_fadump.c
>>>> @@ -234,6 +234,7 @@ static bool do_preserve_region(FadumpSection *region)
>>>>            qemu_log_mask(LOG_GUEST_ERROR,
>>> 
>>> FWIW host heap exhaustion is not really a *guest* error, because the
>>> guest can not control it.
>> Hi, Philippe
>> 
>> 
>> Thanks for the review. There are following log level defined in log.h
>>
>> 	....
>>
>> 	#define CPU_LOG_TB_OUT_ASM (1u << 0)
>> 	#define CPU_LOG_TB_IN_ASM  (1u << 1)
>> 	#define CPU_LOG_TB_OP      (1u << 2)
>> 	#define CPU_LOG_TB_OP_OPT  (1u << 3)
>> 	#define CPU_LOG_INT        (1u << 4)
>> 	#define CPU_LOG_EXEC       (1u << 5)
>> 	#define CPU_LOG_PCALL      (1u << 6)
>> 	#define CPU_LOG_TB_CPU     (1u << 8)
>> 	#define CPU_LOG_RESET      (1u << 9)
>> 	#define LOG_UNIMP          (1u << 10)
>> 	#define LOG_GUEST_ERROR    (1u << 11)
>> 	#define CPU_LOG_MMU        (1u << 12)
>> 	#define CPU_LOG_TB_NOCHAIN (1u << 13)
>> 	#define CPU_LOG_PAGE       (1u << 14)
>> 	/* LOG_TRACE (1 << 15) is defined in log-for-trace.h */
>> 	#define CPU_LOG_TB_OP_IND  (1u << 16)
>> 	#define CPU_LOG_TB_FPU     (1u << 17)
>> 	#define CPU_LOG_PLUGIN     (1u << 18)
>> 	/* LOG_STRACE is used for user-mode strace logging. */
>> 	#define LOG_STRACE         (1u << 19)
>> 	#define LOG_PER_THREAD     (1u << 20)
>> 	#define CPU_LOG_TB_VPU     (1u << 21)
>> 	#define LOG_TB_OP_PLUGIN   (1u << 22)
>> 	#define LOG_INVALID_MEM    (1u << 23)
>>
>> 	....
>> 
>> Which one do you recommend we use? or May we introduce a `LOG_HOST_ERROR`,
>> if that's more appropriate.
>
> I think it would be better to have LOG_INSUFF_MEM for this case, but let's 
> hear from Philippe and others for suggestions.

If it's not a guest error but an error in QEMU then maybe error_report (or 
warn_report if it's recoverable)?

Regards,
BALATON Zoltan

> Since it's unlreated to the coverity fix and can be taken separately, so:
>
> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>
>> 
>> Thanks
>> ~Shivang.
>>>
>>>>                "FADump: Failed allocating memory (size: %zu) for copying"
>>>>                " reserved memory regions\n", FADUMP_CHUNK_SIZE);
>>>> +        return false;
>>>>        }
>>>>        num_chunks = ceil((src_len * 1.0f) / FADUMP_CHUNK_SIZE);
>>> 
>
>

Re: [PATCH v2 1/2] hw/ppc: Fix missing return on allocation failure

[Shivang Upadhyay]

On Thu, Oct 30, 2025 at 12:09:44PM +0100, BALATON Zoltan wrote:
> On Thu, 30 Oct 2025, Harsh Prateek Bora wrote:
> > On 10/28/25 15:54, Shivang Upadhyay wrote:
> > > On Tue, Oct 28, 2025 at 09:35:40AM +0100, Philippe Mathieu-Daudé wrote:
> > > > On 28/10/25 09:05, Shivang Upadhyay wrote:
> > > > > Fixes coverity (CID 1642026)
> > > > > 
> > > > > Cc: Aditya Gupta <adityag@linux.ibm.com>
> > > > > Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > > > > Link: https://lore.kernel.org/qemu-devel/CAFEAcA-SPmsnU1wzsWxBcFC=ZM_DDhPEg1N4iX9Q4bL1xOnwBg@mail.gmail.com/
> > > > > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > > > > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > > > > Signed-off-by: Shivang Upadhyay <shivangu@linux.ibm.com>
> > > > > ---
> > > > >    hw/ppc/spapr_fadump.c | 1 +
> > > > >    1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/hw/ppc/spapr_fadump.c b/hw/ppc/spapr_fadump.c
> > > > > index fa3aeac94c..883a60cdcf 100644
> > > > > --- a/hw/ppc/spapr_fadump.c
> > > > > +++ b/hw/ppc/spapr_fadump.c
> > > > > @@ -234,6 +234,7 @@ static bool do_preserve_region(FadumpSection *region)
> > > > >            qemu_log_mask(LOG_GUEST_ERROR,
> > > > 
> > > > FWIW host heap exhaustion is not really a *guest* error, because the
> > > > guest can not control it.
> > > Hi, Philippe
> > > 
> > > 
> > > Thanks for the review. There are following log level defined in log.h
> > > 
> > > 	....
> > > 
> > > 	#define CPU_LOG_TB_OUT_ASM (1u << 0)
> > > 	#define CPU_LOG_TB_IN_ASM  (1u << 1)
> > > 	#define CPU_LOG_TB_OP      (1u << 2)
> > > 	#define CPU_LOG_TB_OP_OPT  (1u << 3)
> > > 	#define CPU_LOG_INT        (1u << 4)
> > > 	#define CPU_LOG_EXEC       (1u << 5)
> > > 	#define CPU_LOG_PCALL      (1u << 6)
> > > 	#define CPU_LOG_TB_CPU     (1u << 8)
> > > 	#define CPU_LOG_RESET      (1u << 9)
> > > 	#define LOG_UNIMP          (1u << 10)
> > > 	#define LOG_GUEST_ERROR    (1u << 11)
> > > 	#define CPU_LOG_MMU        (1u << 12)
> > > 	#define CPU_LOG_TB_NOCHAIN (1u << 13)
> > > 	#define CPU_LOG_PAGE       (1u << 14)
> > > 	/* LOG_TRACE (1 << 15) is defined in log-for-trace.h */
> > > 	#define CPU_LOG_TB_OP_IND  (1u << 16)
> > > 	#define CPU_LOG_TB_FPU     (1u << 17)
> > > 	#define CPU_LOG_PLUGIN     (1u << 18)
> > > 	/* LOG_STRACE is used for user-mode strace logging. */
> > > 	#define LOG_STRACE         (1u << 19)
> > > 	#define LOG_PER_THREAD     (1u << 20)
> > > 	#define CPU_LOG_TB_VPU     (1u << 21)
> > > 	#define LOG_TB_OP_PLUGIN   (1u << 22)
> > > 	#define LOG_INVALID_MEM    (1u << 23)
> > > 
> > > 	....
> > > 
> > > Which one do you recommend we use? or May we introduce a `LOG_HOST_ERROR`,
> > > if that's more appropriate.
> > 
> > I think it would be better to have LOG_INSUFF_MEM for this case, but
> > let's hear from Philippe and others for suggestions.
> 
> If it's not a guest error but an error in QEMU then maybe error_report (or
> warn_report if it's recoverable)?
> 
> Regards,
> BALATON Zoltan
Hi

This allocation failure does not seem recoverable. Maybe Aditya can be more
right about this.

Also I noticed a pattern to use `g_malloc` for critical things instead
of `g_try_malloc`. But it will kill the full application if failure happens.
So maybe just `error_report` is fine here(?).

~Shivang.

Re: [PATCH v2 1/2] hw/ppc: Fix missing return on allocation failure

[Peter Maydell]

On Thu, 30 Oct 2025 at 14:23, Shivang Upadhyay <shivangu@linux.ibm.com> wrote:
> Also I noticed a pattern to use `g_malloc` for critical things instead
> of `g_try_malloc`. But it will kill the full application if failure happens.
> So maybe just `error_report` is fine here(?).

docs/devel/style.rst has some notes on malloc choices, including this:

# Care should be taken to avoid introducing places where the guest could
# trigger an exit by causing a large allocation. For small allocations,
# of the order of 4k, a failure to allocate is likely indicative of an
# overloaded host and allowing ``g_malloc`` to ``exit`` is a reasonable
# approach. However for larger allocations where we could realistically
# fall-back to a smaller one if need be we should use functions like
# ``g_try_new`` and check the result. For example this is valid approach
# for a time/space trade-off like ``tlb_mmu_resize_locked`` in the
# SoftMMU TLB code.

Since we're trying to allocate 32MB at once and this is during
the guest run rather than at startup, this is probably a reasonable
place to use g_try_malloc().

There are other places in this code that use LOG_GUEST_ERROR
for things that aren't exactly guest errors, so my suggestion
is that we take this patch as-is to fix the logic error.
We can consider whether we want to try to improve the error
reporting of this group of functions as a separate patch.

thanks
-- PMM

Re: [PATCH v2 1/2] hw/ppc: Fix missing return on allocation failure

[Shivang Upadhyay]

On Thu, Oct 30, 2025 at 02:45:07PM +0000, Peter Maydell wrote:
> docs/devel/style.rst has some notes on malloc choices, including this:
> 
> # Care should be taken to avoid introducing places where the guest could
> # trigger an exit by causing a large allocation. For small allocations,
> # of the order of 4k, a failure to allocate is likely indicative of an
> # overloaded host and allowing ``g_malloc`` to ``exit`` is a reasonable
> # approach. However for larger allocations where we could realistically
> # fall-back to a smaller one if need be we should use functions like
> # ``g_try_new`` and check the result. For example this is valid approach
> # for a time/space trade-off like ``tlb_mmu_resize_locked`` in the
> # SoftMMU TLB code.
Hi Peter,

Thanks for clearing it up.
> 
> Since we're trying to allocate 32MB at once and this is during
> the guest run rather than at startup, this is probably a reasonable
> place to use g_try_malloc().
> 
> There are other places in this code that use LOG_GUEST_ERROR
> for things that aren't exactly guest errors, so my suggestion
> is that we take this patch as-is to fix the logic error.
> We can consider whether we want to try to improve the error
> reporting of this group of functions as a separate patch.
Sure.

~Shivang.