Memory leak in via_isa_realize()

Memory leak in via_isa_realize()

[Thomas Huth]

  Hi!

FYI, I'm seeing a memory leak in via_isa_realize() when building
QEMU with sanitizers enabled or when running QEMU through valgrind:

$ valgrind --leak-check=full --show-leak-kinds=definite ./qemu-system-mips64el --nographic -M fuloong2e
==210405== Memcheck, a memory error detector
==210405== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==210405== Using Valgrind-3.17.0 and LibVEX; rerun with -h for copyright info
==210405== Command: ./qemu-system-mips64el --nographic -M fuloong2e
==210405==
==210405== Warning: set address range perms: large range [0x15c9f000, 0x55c9f000) (defined)
==210405== Warning: set address range perms: large range [0x59ea4000, 0x99ea4000) (defined)
==210405== Warning: set address range perms: large range [0x99ea4000, 0xaa0a4000) (noaccess)
QEMU 6.2.90 monitor - type 'help' for more information
(qemu) q
==210405==
==210405== HEAP SUMMARY:
==210405==     in use at exit: 8,409,442 bytes in 23,516 blocks
==210405==   total heap usage: 37,073 allocs, 13,557 frees, 32,674,469 bytes allocated
==210405==
==210405== 8 bytes in 1 blocks are definitely lost in loss record 715 of 6,085
==210405==    at 0x4C360A5: malloc (vg_replace_malloc.c:380)
==210405==    by 0x7059475: g_malloc (in /usr/lib64/libglib-2.0.so.0.5600.4)
==210405==    by 0x96C52C: qemu_extend_irqs (irq.c:57)
==210405==    by 0x96C5B8: qemu_allocate_irqs (irq.c:66)
==210405==    by 0x5FFA47: via_isa_realize (vt82c686.c:591)
==210405==    by 0x5FFCDA: vt82c686b_realize (vt82c686.c:646)
==210405==    by 0x681502: pci_qdev_realize (pci.c:2192)
==210405==    by 0x969A5D: device_set_realized (qdev.c:531)
==210405==    by 0x97354A: property_set_bool (object.c:2273)
==210405==    by 0x9715A0: object_property_set (object.c:1408)
==210405==    by 0x975938: object_property_set_qobject (qom-qobject.c:28)
==210405==    by 0x971907: object_property_set_bool (object.c:1477)
==210405==
==210405== LEAK SUMMARY:
==210405==    definitely lost: 8 bytes in 1 blocks
==210405==    indirectly lost: 0 bytes in 0 blocks
==210405==      possibly lost: 3,794 bytes in 45 blocks
==210405==    still reachable: 8,405,640 bytes in 23,470 blocks
==210405==                       of which reachable via heuristic:
==210405==                         newarray           : 1,536 bytes in 16 blocks
==210405==         suppressed: 0 bytes in 0 blocks
==210405== Reachable blocks (those to which a pointer was found) are not shown.
==210405== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==210405==
==210405== For lists of detected and suppressed errors, rerun with: -s
==210405== ERROR SUMMARY: 46 errors from 46 contexts (suppressed: 0 from 0)

Same problem happens with qemu-system-ppc64 and the pegasos2 machine.

No clue how to properly fix this... is it safe to free the pointer
at the end of the function?

  Thomas

Re: Memory leak in via_isa_realize()

[Peter Maydell]

On Mon, 21 Mar 2022 at 10:31, Thomas Huth <thuth@redhat.com> wrote:
> FYI, I'm seeing a memory leak in via_isa_realize() when building
> QEMU with sanitizers enabled or when running QEMU through valgrind:
> Same problem happens with qemu-system-ppc64 and the pegasos2 machine.
>
> No clue how to properly fix this... is it safe to free the pointer
> at the end of the function?

This is because the code is still using the old function
qemu_allocate_irqs(), which is almost always going to involve
it leaking memory. The fix is usually to rewrite the code to not use
that function at all, i.e. to manage its irq/gpio lines differently.
Probably the i8259 code should have a named GPIO output line
rather than wanting to be passed a qemu_irq in an init function,
and the via code should have an input GPIO line which it connects
up to the i8259. It looks from a quick glance like the i8259 and
its callers have perhaps not been completely QOMified.

In this specific case, though, it seems like the only thing that
the via_isa_request_i8259_irq() function does is pass the interrupt
signal through to its own s->cpu_intr, so I think a relatively
self-contained way to deal with the leak is to pass s->cpu_intr
into i8259_init() and drop the isa_irq allocated irq and its
associated helper function entirely. (There might be some subtlety
I'm missing that means that wouldn't work, of course.)

-- PMM

Re: Memory leak in via_isa_realize()

[BALATON Zoltan]

On Mon, 21 Mar 2022, Peter Maydell wrote:
> On Mon, 21 Mar 2022 at 10:31, Thomas Huth <thuth@redhat.com> wrote:
>> FYI, I'm seeing a memory leak in via_isa_realize() when building
>> QEMU with sanitizers enabled or when running QEMU through valgrind:
>> Same problem happens with qemu-system-ppc64 and the pegasos2 machine.
>>
>> No clue how to properly fix this... is it safe to free the pointer
>> at the end of the function?
>
> This is because the code is still using the old function
> qemu_allocate_irqs(), which is almost always going to involve
> it leaking memory. The fix is usually to rewrite the code to not use
> that function at all, i.e. to manage its irq/gpio lines differently.
> Probably the i8259 code should have a named GPIO output line
> rather than wanting to be passed a qemu_irq in an init function,
> and the via code should have an input GPIO line which it connects
> up to the i8259. It looks from a quick glance like the i8259 and
> its callers have perhaps not been completely QOMified.

Everything involving ISA emulation in QEMU is not completely QOMified and 
this has caused some problems before but I did not want to try to fix it 
both becuase it's too much unrelated work and because it's used by too 
many things that could break that I can't even test. So I'd rather 
somebody more comfortable with this would look at ISA QOMification.

> In this specific case, though, it seems like the only thing that
> the via_isa_request_i8259_irq() function does is pass the interrupt
> signal through to its own s->cpu_intr, so I think a relatively
> self-contained way to deal with the leak is to pass s->cpu_intr
> into i8259_init() and drop the isa_irq allocated irq and its
> associated helper function entirely. (There might be some subtlety
> I'm missing that means that wouldn't work, of course.)

I think I've tried to do that first and it did not work for some reason 
then I got this way from some other device model which works but I forgot 
the details. You can test it by booting MorphOS or Debian Linux 8.11 PPC 
on pegasos2 which support this machine or maybe I can have a look later 
this week if it's not urgent and try something but I don't mind if 
somebody comes up with a fix before that.

Regards,
BALATON Zoltan

Re: Memory leak in via_isa_realize()

[Peter Maydell]

On Mon, 21 Mar 2022 at 12:11, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> On Mon, 21 Mar 2022, Peter Maydell wrote:
> > On Mon, 21 Mar 2022 at 10:31, Thomas Huth <thuth@redhat.com> wrote:
> >> FYI, I'm seeing a memory leak in via_isa_realize() when building
> >> QEMU with sanitizers enabled or when running QEMU through valgrind:
> >> Same problem happens with qemu-system-ppc64 and the pegasos2 machine.
> >>
> >> No clue how to properly fix this... is it safe to free the pointer
> >> at the end of the function?
> >
> > This is because the code is still using the old function
> > qemu_allocate_irqs(), which is almost always going to involve
> > it leaking memory. The fix is usually to rewrite the code to not use
> > that function at all, i.e. to manage its irq/gpio lines differently.
> > Probably the i8259 code should have a named GPIO output line
> > rather than wanting to be passed a qemu_irq in an init function,
> > and the via code should have an input GPIO line which it connects
> > up to the i8259. It looks from a quick glance like the i8259 and
> > its callers have perhaps not been completely QOMified.
>
> Everything involving ISA emulation in QEMU is not completely QOMified and
> this has caused some problems before but I did not want to try to fix it
> both becuase it's too much unrelated work and because it's used by too
> many things that could break that I can't even test. So I'd rather
> somebody more comfortable with this would look at ISA QOMification.

Yeah, there's usually a reason that these things haven't been more
thoroughly QOMified, and that reason is often because it's a pile of
work for not very clear benefit.

In this particular case, although there is a "leak", it happens exactly
once at QEMU startup and in practice we need that memory to hang around
until QEMU exits anyway. The only real reason to fix this kind of leak in
my opinion is because it clutters up the output of valgrind or clang/gcc
address sanitizer runs and prevents us from having our CI do a
leak-sanitizer test run that would guard against new leaks being added
to the codebase. We still have a fair number of this sort of one-off
startup leak in various arm boards/devices, for instance -- I occasionally
have a look through and fix some of the more tractable ones.

thanks
-- PMM

Re: Memory leak in via_isa_realize()

[Philippe Mathieu-Daudé]

Cc'ing Bernhard who did a similar cleanup recently.

On 21/3/22 11:31, Thomas Huth wrote:
> 
>   Hi!
> 
> FYI, I'm seeing a memory leak in via_isa_realize() when building
> QEMU with sanitizers enabled or when running QEMU through valgrind:
> 
> $ valgrind --leak-check=full --show-leak-kinds=definite 
> ./qemu-system-mips64el --nographic -M fuloong2e
> ==210405== Memcheck, a memory error detector
> ==210405== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
> ==210405== Using Valgrind-3.17.0 and LibVEX; rerun with -h for copyright 
> info
> ==210405== Command: ./qemu-system-mips64el --nographic -M fuloong2e
> ==210405==
> ==210405== Warning: set address range perms: large range [0x15c9f000, 
> 0x55c9f000) (defined)
> ==210405== Warning: set address range perms: large range [0x59ea4000, 
> 0x99ea4000) (defined)
> ==210405== Warning: set address range perms: large range [0x99ea4000, 
> 0xaa0a4000) (noaccess)
> QEMU 6.2.90 monitor - type 'help' for more information
> (qemu) q
> ==210405==
> ==210405== HEAP SUMMARY:
> ==210405==     in use at exit: 8,409,442 bytes in 23,516 blocks
> ==210405==   total heap usage: 37,073 allocs, 13,557 frees, 32,674,469 
> bytes allocated
> ==210405==
> ==210405== 8 bytes in 1 blocks are definitely lost in loss record 715 of 
> 6,085
> ==210405==    at 0x4C360A5: malloc (vg_replace_malloc.c:380)
> ==210405==    by 0x7059475: g_malloc (in 
> /usr/lib64/libglib-2.0.so.0.5600.4)
> ==210405==    by 0x96C52C: qemu_extend_irqs (irq.c:57)
> ==210405==    by 0x96C5B8: qemu_allocate_irqs (irq.c:66)
> ==210405==    by 0x5FFA47: via_isa_realize (vt82c686.c:591)
> ==210405==    by 0x5FFCDA: vt82c686b_realize (vt82c686.c:646)
> ==210405==    by 0x681502: pci_qdev_realize (pci.c:2192)
> ==210405==    by 0x969A5D: device_set_realized (qdev.c:531)
> ==210405==    by 0x97354A: property_set_bool (object.c:2273)
> ==210405==    by 0x9715A0: object_property_set (object.c:1408)
> ==210405==    by 0x975938: object_property_set_qobject (qom-qobject.c:28)
> ==210405==    by 0x971907: object_property_set_bool (object.c:1477)
> ==210405==
> ==210405== LEAK SUMMARY:
> ==210405==    definitely lost: 8 bytes in 1 blocks
> ==210405==    indirectly lost: 0 bytes in 0 blocks
> ==210405==      possibly lost: 3,794 bytes in 45 blocks
> ==210405==    still reachable: 8,405,640 bytes in 23,470 blocks
> ==210405==                       of which reachable via heuristic:
> ==210405==                         newarray           : 1,536 bytes in 
> 16 blocks
> ==210405==         suppressed: 0 bytes in 0 blocks
> ==210405== Reachable blocks (those to which a pointer was found) are not 
> shown.
> ==210405== To see them, rerun with: --leak-check=full --show-leak-kinds=all
> ==210405==
> ==210405== For lists of detected and suppressed errors, rerun with: -s
> ==210405== ERROR SUMMARY: 46 errors from 46 contexts (suppressed: 0 from 0)
> 
> Same problem happens with qemu-system-ppc64 and the pegasos2 machine.
> 
> No clue how to properly fix this... is it safe to free the pointer
> at the end of the function?
> 
>   Thomas
> 

Re: Memory leak in via_isa_realize()

[BALATON Zoltan]

On Mon, 21 Mar 2022, Peter Maydell wrote:
> On Mon, 21 Mar 2022 at 10:31, Thomas Huth <thuth@redhat.com> wrote:
>> FYI, I'm seeing a memory leak in via_isa_realize() when building
>> QEMU with sanitizers enabled or when running QEMU through valgrind:
>> Same problem happens with qemu-system-ppc64 and the pegasos2 machine.
>>
>> No clue how to properly fix this... is it safe to free the pointer
>> at the end of the function?
>
> This is because the code is still using the old function
> qemu_allocate_irqs(), which is almost always going to involve
> it leaking memory. The fix is usually to rewrite the code to not use
> that function at all, i.e. to manage its irq/gpio lines differently.
> Probably the i8259 code should have a named GPIO output line
> rather than wanting to be passed a qemu_irq in an init function,
> and the via code should have an input GPIO line which it connects
> up to the i8259. It looks from a quick glance like the i8259 and
> its callers have perhaps not been completely QOMified.
>
> In this specific case, though, it seems like the only thing that
> the via_isa_request_i8259_irq() function does is pass the interrupt
> signal through to its own s->cpu_intr, so I think a relatively
> self-contained way to deal with the leak is to pass s->cpu_intr
> into i8259_init() and drop the isa_irq allocated irq and its
> associated helper function entirely. (There might be some subtlety
> I'm missing that means that wouldn't work, of course.)

So the recent piix4 cleanups did not change this and it seems to have the 
same qemu_allocate_irqs and forwarding as well as i82378. Does similar 
leak exist for those too? And probably all other callers of i8259_init(), 
so this would probably be better fixed in that device model instead but I 
don't know anything about that so I don't want to touch it. Within 
vt82c686 we could probably store the allocated irq in the device state and 
free it in a finalize function or simliar if that's possible but that 
would only fix one case of simliar problems and it's unnecessary 
complication if this could be properly fixed elsewhere. So I give up on 
this for now and wait if somebody could do something about i8259_init 
instead. That seems like a legacy init function that should be resolved,

Regards,
BALATON Zoltan

Re: Memory leak in via_isa_realize()

[Cédric Le Goater]

On 3/21/22 14:04, Philippe Mathieu-Daudé wrote:
> Cc'ing Bernhard who did a similar cleanup recently.
> 
> On 21/3/22 11:31, Thomas Huth wrote:
>>
>>   Hi!
>>
>> FYI, I'm seeing a memory leak in via_isa_realize() when building
>> QEMU with sanitizers enabled or when running QEMU through valgrind:
>>
>> $ valgrind --leak-check=full --show-leak-kinds=definite ./qemu-system-mips64el --nographic -M fuloong2e
>> ==210405== Memcheck, a memory error detector
>> ==210405== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
>> ==210405== Using Valgrind-3.17.0 and LibVEX; rerun with -h for copyright info
>> ==210405== Command: ./qemu-system-mips64el --nographic -M fuloong2e
>> ==210405==
>> ==210405== Warning: set address range perms: large range [0x15c9f000, 0x55c9f000) (defined)
>> ==210405== Warning: set address range perms: large range [0x59ea4000, 0x99ea4000) (defined)
>> ==210405== Warning: set address range perms: large range [0x99ea4000, 0xaa0a4000) (noaccess)
>> QEMU 6.2.90 monitor - type 'help' for more information
>> (qemu) q
>> ==210405==
>> ==210405== HEAP SUMMARY:
>> ==210405==     in use at exit: 8,409,442 bytes in 23,516 blocks
>> ==210405==   total heap usage: 37,073 allocs, 13,557 frees, 32,674,469 bytes allocated
>> ==210405==
>> ==210405== 8 bytes in 1 blocks are definitely lost in loss record 715 of 6,085
>> ==210405==    at 0x4C360A5: malloc (vg_replace_malloc.c:380)
>> ==210405==    by 0x7059475: g_malloc (in /usr/lib64/libglib-2.0.so.0.5600.4)
>> ==210405==    by 0x96C52C: qemu_extend_irqs (irq.c:57)
>> ==210405==    by 0x96C5B8: qemu_allocate_irqs (irq.c:66)
>> ==210405==    by 0x5FFA47: via_isa_realize (vt82c686.c:591)
>> ==210405==    by 0x5FFCDA: vt82c686b_realize (vt82c686.c:646)
>> ==210405==    by 0x681502: pci_qdev_realize (pci.c:2192)
>> ==210405==    by 0x969A5D: device_set_realized (qdev.c:531)
>> ==210405==    by 0x97354A: property_set_bool (object.c:2273)
>> ==210405==    by 0x9715A0: object_property_set (object.c:1408)
>> ==210405==    by 0x975938: object_property_set_qobject (qom-qobject.c:28)
>> ==210405==    by 0x971907: object_property_set_bool (object.c:1477)
>> ==210405==
>> ==210405== LEAK SUMMARY:
>> ==210405==    definitely lost: 8 bytes in 1 blocks
>> ==210405==    indirectly lost: 0 bytes in 0 blocks
>> ==210405==      possibly lost: 3,794 bytes in 45 blocks
>> ==210405==    still reachable: 8,405,640 bytes in 23,470 blocks
>> ==210405==                       of which reachable via heuristic:
>> ==210405==                         newarray           : 1,536 bytes in 16 blocks
>> ==210405==         suppressed: 0 bytes in 0 blocks
>> ==210405== Reachable blocks (those to which a pointer was found) are not shown.
>> ==210405== To see them, rerun with: --leak-check=full --show-leak-kinds=all
>> ==210405==
>> ==210405== For lists of detected and suppressed errors, rerun with: -s
>> ==210405== ERROR SUMMARY: 46 errors from 46 contexts (suppressed: 0 from 0)
>>
>> Same problem happens with qemu-system-ppc64 and the pegasos2 machine.
>>
>> No clue how to properly fix this... is it safe to free the pointer
>> at the end of the function?

I introduced quite a few of these calls,

   hw/ppc/pnv_lpc.c:    irqs = qemu_allocate_irqs(handler, lpc, ISA_NUM_IRQS);
   hw/ppc/pnv_psi.c:    psi->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
   hw/ppc/pnv_psi.c:    psi->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc, xsrc->nr_irqs);
   hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppc6xx_set_irq, cpu,
   hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppc970_set_irq, cpu,
   hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&power7_set_irq, cpu,
   hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&power9_set_irq, cpu,
   hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppc40x_set_irq,
   hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppce500_set_irq,
   hw/ppc/spapr_irq.c:    spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr,

and may be I can remove some. What's the best practice ?

Thanks,

C.

Re: Memory leak in via_isa_realize()

[Cédric Le Goater]

On 3/21/22 13:11, BALATON Zoltan wrote:
> On Mon, 21 Mar 2022, Peter Maydell wrote:
>> On Mon, 21 Mar 2022 at 10:31, Thomas Huth <thuth@redhat.com> wrote:
>>> FYI, I'm seeing a memory leak in via_isa_realize() when building
>>> QEMU with sanitizers enabled or when running QEMU through valgrind:
>>> Same problem happens with qemu-system-ppc64 and the pegasos2 machine.
>>>
>>> No clue how to properly fix this... is it safe to free the pointer
>>> at the end of the function?
>>
>> This is because the code is still using the old function
>> qemu_allocate_irqs(), which is almost always going to involve
>> it leaking memory. The fix is usually to rewrite the code to not use
>> that function at all, i.e. to manage its irq/gpio lines differently.
>> Probably the i8259 code should have a named GPIO output line
>> rather than wanting to be passed a qemu_irq in an init function,
>> and the via code should have an input GPIO line which it connects
>> up to the i8259. It looks from a quick glance like the i8259 and
>> its callers have perhaps not been completely QOMified.
> 
> Everything involving ISA emulation in QEMU is not completely QOMified and this has caused some problems before but I did not want to try to fix it both becuase it's too much unrelated work and because it's used by too many things that could break that I can't even test. So I'd rather somebody more comfortable with this would look at ISA QOMification.

Regarding the ppc44x and ppc40x machines, we have a lot of tests in place
and the QOM translation shouldn't be too complex. This is about changing
the board and device instantiation and not the CPU internal models,
like exceptions and SoftTLBs, which can be much more complex to test.

>> In this specific case, though, it seems like the only thing that
>> the via_isa_request_i8259_irq() function does is pass the interrupt
>> signal through to its own s->cpu_intr, so I think a relatively
>> self-contained way to deal with the leak is to pass s->cpu_intr
>> into i8259_init() and drop the isa_irq allocated irq and its
>> associated helper function entirely. (There might be some subtlety
>> I'm missing that means that wouldn't work, of course.)
> 
> I think I've tried to do that first and it did not work for some reason then I got this way from some other device model which works but I forgot the details. You can test it by booting MorphOS or Debian Linux 8.11 PPC on pegasos2 which support this machine or maybe I can have a look later this week if it's not urgent and try something but I don't mind if somebody comes up with a fix before that.

I will support this work and I can even send some patches I have started
for the 405 to cleanup the bit-rotting models.

Thanks,

C.

Re: Memory leak in via_isa_realize()

[Philippe Mathieu-Daudé]

On 21/3/22 19:55, Cédric Le Goater wrote:
> On 3/21/22 14:04, Philippe Mathieu-Daudé wrote:
>> Cc'ing Bernhard who did a similar cleanup recently.

>>> Same problem happens with qemu-system-ppc64 and the pegasos2 machine.
>>>
>>> No clue how to properly fix this... is it safe to free the pointer
>>> at the end of the function?
> 
> I introduced quite a few of these calls,
> 
>    hw/ppc/pnv_lpc.c:    irqs = qemu_allocate_irqs(handler, lpc, 
> ISA_NUM_IRQS);
>    hw/ppc/pnv_psi.c:    psi->qirqs = qemu_allocate_irqs(ics_set_irq, 
> ics, ics->nr_irqs);
>    hw/ppc/pnv_psi.c:    psi->qirqs = 
> qemu_allocate_irqs(xive_source_set_irq, xsrc, xsrc->nr_irqs);
>    hw/ppc/ppc.c:    env->irq_inputs = (void 
> **)qemu_allocate_irqs(&ppc6xx_set_irq, cpu,
>    hw/ppc/ppc.c:    env->irq_inputs = (void 
> **)qemu_allocate_irqs(&ppc970_set_irq, cpu,
>    hw/ppc/ppc.c:    env->irq_inputs = (void 
> **)qemu_allocate_irqs(&power7_set_irq, cpu,
>    hw/ppc/ppc.c:    env->irq_inputs = (void 
> **)qemu_allocate_irqs(&power9_set_irq, cpu,
>    hw/ppc/ppc.c:    env->irq_inputs = (void 
> **)qemu_allocate_irqs(&ppc40x_set_irq,
>    hw/ppc/ppc.c:    env->irq_inputs = (void 
> **)qemu_allocate_irqs(&ppce500_set_irq,
>    hw/ppc/spapr_irq.c:    spapr->qirqs = 
> qemu_allocate_irqs(spapr_set_irq, spapr,
> 
> and may be I can remove some. What's the best practice ?

I recommend looking at how Peter did it recently in commit 3391953660
("hw/sparc: Make grlib-irqmp device handle its own inbound IRQ lines")
and d9cd403972 ("hw/m68k/next-cube: Make next_irq GPIO inputs to NEXT_PC
device").

Re: Memory leak in via_isa_realize()

[Peter Maydell]

On Mon, 21 Mar 2022 at 18:55, Cédric Le Goater <clg@kaod.org> wrote:
> I introduced quite a few of these calls,
>
>    hw/ppc/pnv_lpc.c:    irqs = qemu_allocate_irqs(handler, lpc, ISA_NUM_IRQS);
>    hw/ppc/pnv_psi.c:    psi->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
>    hw/ppc/pnv_psi.c:    psi->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc, xsrc->nr_irqs);
>    hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppc6xx_set_irq, cpu,
>    hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppc970_set_irq, cpu,
>    hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&power7_set_irq, cpu,
>    hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&power9_set_irq, cpu,
>    hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppc40x_set_irq,
>    hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppce500_set_irq,
>    hw/ppc/spapr_irq.c:    spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr,
>
> and may be I can remove some. What's the best practice ?

The 'best practice' is that if you have an irq line it should be
because it is the input (gpio or sysbus irq) or output (gpio) of
some device, ie something that is a subtype of TYPE_DEVICE.

For the ones in hw/ppc/ppc.c: we used to need to write code like that
because CPU objects weren't TYPE_DEVICE; now they are, and so you
can give them inbound gpio lines using qdev_init_gpio_in(), typically
in the cpu initfn. (See target/riscv for an example, or grep for
that function name in target/ for others.) Then the board code
needs to wire up to those IRQs in the usual way for GPIO lines,
ie using qdev_get_gpio_in(cpudev, ...), instead of directly
reaching into the CPU struct env->irq_inputs. (There's probably
a way to structure this change to avoid having to change the CPU
and all the board code at once, but I haven't thought it through.)

For the spapr one: this is in machine model code, and currently machines
aren't subtypes of TYPE_DEVICE. I'd leave this one alone for now;
we can come back and think about it later.

For pnv_psi.c: these appear to be because the PnvPsi device is
allocating irq lines which really should belong to the ICSState
object (and as a result the ICSState code is having to expose
ics->nr_irqs and the ics_set_irq function when they could be
internal to the ICSState code). The ICSState's init function
should be creating these as qdev gpio lines.

pnv_lpc.c seems to be ISA related. hw/isa/lpc_ich9.c is an
example of setting up IRQs for isa_bus_irqs() without using
qemu_allocate_irqs(), but there may be some more generalised
ISA cleanup possible here.

thanks
-- PMM

Re: Memory leak in via_isa_realize()

[BALATON Zoltan]

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

On Mon, 21 Mar 2022, Cédric Le Goater wrote:
> On 3/21/22 13:11, BALATON Zoltan wrote:
>> On Mon, 21 Mar 2022, Peter Maydell wrote:
>>> On Mon, 21 Mar 2022 at 10:31, Thomas Huth <thuth@redhat.com> wrote:
>>>> FYI, I'm seeing a memory leak in via_isa_realize() when building
>>>> QEMU with sanitizers enabled or when running QEMU through valgrind:
>>>> Same problem happens with qemu-system-ppc64 and the pegasos2 machine.
>>>> 
>>>> No clue how to properly fix this... is it safe to free the pointer
>>>> at the end of the function?
>>> 
>>> This is because the code is still using the old function
>>> qemu_allocate_irqs(), which is almost always going to involve
>>> it leaking memory. The fix is usually to rewrite the code to not use
>>> that function at all, i.e. to manage its irq/gpio lines differently.
>>> Probably the i8259 code should have a named GPIO output line
>>> rather than wanting to be passed a qemu_irq in an init function,
>>> and the via code should have an input GPIO line which it connects
>>> up to the i8259. It looks from a quick glance like the i8259 and
>>> its callers have perhaps not been completely QOMified.
>> 
>> Everything involving ISA emulation in QEMU is not completely QOMified and 
>> this has caused some problems before but I did not want to try to fix it 
>> both becuase it's too much unrelated work and because it's used by too many 
>> things that could break that I can't even test. So I'd rather somebody more 
>> comfortable with this would look at ISA QOMification.
>
> Regarding the ppc44x and ppc40x machines, we have a lot of tests in place
> and the QOM translation shouldn't be too complex. This is about changing
> the board and device instantiation and not the CPU internal models,
> like exceptions and SoftTLBs, which can be much more complex to test.
>
>>> In this specific case, though, it seems like the only thing that
>>> the via_isa_request_i8259_irq() function does is pass the interrupt
>>> signal through to its own s->cpu_intr, so I think a relatively
>>> self-contained way to deal with the leak is to pass s->cpu_intr
>>> into i8259_init() and drop the isa_irq allocated irq and its
>>> associated helper function entirely. (There might be some subtlety
>>> I'm missing that means that wouldn't work, of course.)
>> 
>> I think I've tried to do that first and it did not work for some reason 
>> then I got this way from some other device model which works but I forgot 
>> the details. You can test it by booting MorphOS or Debian Linux 8.11 PPC on 
>> pegasos2 which support this machine or maybe I can have a look later this 
>> week if it's not urgent and try something but I don't mind if somebody 
>> comes up with a fix before that.
>
> I will support this work and I can even send some patches I have started
> for the 405 to cleanup the bit-rotting models.

I'm not sure what do you mean as this was about a memory leak in via-isa 
used only by PPC pegasos2 and MIPS fuloong2e machines. None of the 4xx 
machines are involved. Fixing this properly would ideally need changing 
the i8259 model which is also used by a lot of other non-PPC machines that 
I can't test and don't want to break so I just did what all other similar 
code calling i8259_init does. This was just found out with the via model 
but I think it's not specific to PPC.

That said there may be other unrelated clean up opportunities in the 4xx 
machines but this thread wasn't about that.

Regards,
BALATON Zoltan

Re: Memory leak in via_isa_realize()

[Thomas Huth]

On 21/03/2022 13.59, Peter Maydell wrote:
> On Mon, 21 Mar 2022 at 12:11, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>
>> On Mon, 21 Mar 2022, Peter Maydell wrote:
>>> On Mon, 21 Mar 2022 at 10:31, Thomas Huth <thuth@redhat.com> wrote:
>>>> FYI, I'm seeing a memory leak in via_isa_realize() when building
>>>> QEMU with sanitizers enabled or when running QEMU through valgrind:
>>>> Same problem happens with qemu-system-ppc64 and the pegasos2 machine.
>>>>
>>>> No clue how to properly fix this... is it safe to free the pointer
>>>> at the end of the function?
>>>
>>> This is because the code is still using the old function
>>> qemu_allocate_irqs(), which is almost always going to involve
>>> it leaking memory. The fix is usually to rewrite the code to not use
>>> that function at all, i.e. to manage its irq/gpio lines differently.
>>> Probably the i8259 code should have a named GPIO output line
>>> rather than wanting to be passed a qemu_irq in an init function,
>>> and the via code should have an input GPIO line which it connects
>>> up to the i8259. It looks from a quick glance like the i8259 and
>>> its callers have perhaps not been completely QOMified.
>>
>> Everything involving ISA emulation in QEMU is not completely QOMified and
>> this has caused some problems before but I did not want to try to fix it
>> both becuase it's too much unrelated work and because it's used by too
>> many things that could break that I can't even test. So I'd rather
>> somebody more comfortable with this would look at ISA QOMification.
> 
> Yeah, there's usually a reason that these things haven't been more
> thoroughly QOMified, and that reason is often because it's a pile of
> work for not very clear benefit.
> 
> In this particular case, although there is a "leak", it happens exactly
> once at QEMU startup and in practice we need that memory to hang around
> until QEMU exits anyway.

Yes. I guess as a workaround (to silence Valgrind et al. here), it would
be sufficient to store the pointer in the ViaISAState stucture.

> The only real reason to fix this kind of leak in
> my opinion is because it clutters up the output of valgrind or clang/gcc
> address sanitizer runs and prevents us from having our CI do a
> leak-sanitizer test run that would guard against new leaks being added
> to the codebase.

Yes, that's my concern, too. It's hard to spot real problems if the output
is cluttered with a lot of these not-so-serious leaks.

> We still have a fair number of this sort of one-off
> startup leak in various arm boards/devices, for instance -- I occasionally
> have a look through and fix some of the more tractable ones.

Hmm, yes, running the device-introspect-test shows a lot of leaks for the
arm devices...

$ QTEST_QEMU_BINARY="valgrind --leak-check=full \
   --show-leak-kinds=definite ./qemu-system-aarch64" \
   tests/qtest/device-introspect-test \
  -p /aarch64/device/introspect/concrete/defaults/none

...
==377771== 112,629 (66,304 direct, 46,325 indirect) bytes in 13 blocks are definitely lost in loss record 7,800 of 7,810
==377771==    at 0x4C360A5: malloc (vg_replace_malloc.c:380)
==377771==    by 0x6364475: g_malloc (in /usr/lib64/libglib-2.0.so.0.5600.4)
==377771==    by 0xB207C3: object_new_with_type (object.c:722)
==377771==    by 0xB20864: object_new (object.c:744)
==377771==    by 0xB1C731: qmp_device_list_properties (qom-qmp-cmds.c:153)
==377771==    by 0xC900BC: qmp_marshal_device_list_properties (qapi-commands-qdev.c:59)
==377771==    by 0xCA4DBE: do_qmp_dispatch_bh (qmp-dispatch.c:110)
==377771==    by 0xCDF19D: aio_bh_call (async.c:136)
==377771==    by 0xCDF2A7: aio_bh_poll (async.c:164)
==377771==    by 0xCA9D08: aio_dispatch (aio-posix.c:381)
==377771==    by 0xCDF6DA: aio_ctx_dispatch (async.c:306)
==377771==    by 0x635E95C: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.5600.4)
==377771==
==377771== 530,646 (88 direct, 530,558 indirect) bytes in 1 blocks are definitely lost in loss record 7,806 of 7,810
==377771==    at 0x4C360A5: malloc (vg_replace_malloc.c:380)
==377771==    by 0x6364475: g_malloc (in /usr/lib64/libglib-2.0.so.0.5600.4)
==377771==    by 0x637C086: g_slice_alloc (in /usr/lib64/libglib-2.0.so.0.5600.4)
==377771==    by 0x634CBE1: g_hash_table_new_full (in /usr/lib64/libglib-2.0.so.0.5600.4)
==377771==    by 0xB20122: object_initialize_with_type (object.c:513)
==377771==    by 0xB2080D: object_new_with_type (object.c:729)
==377771==    by 0xB20864: object_new (object.c:744)
==377771==    by 0xB1C731: qmp_device_list_properties (qom-qmp-cmds.c:153)
==377771==    by 0x78F83C: qdev_device_help (qdev-monitor.c:283)
==377771==    by 0x791001: qmp_device_add (qdev-monitor.c:801)
==377771==    by 0x79145D: hmp_device_add (qdev-monitor.c:916)
==377771==    by 0x60EC48: handle_hmp_command (hmp.c:1100)
==377771==
==377771== 536,518 (704 direct, 535,814 indirect) bytes in 8 blocks are definitely lost in loss record 7,807 of 7,810
==377771==    at 0x4C360A5: malloc (vg_replace_malloc.c:380)
==377771==    by 0x6364475: g_malloc (in /usr/lib64/libglib-2.0.so.0.5600.4)
==377771==    by 0x637C086: g_slice_alloc (in /usr/lib64/libglib-2.0.so.0.5600.4)
==377771==    by 0x634CBE1: g_hash_table_new_full (in /usr/lib64/libglib-2.0.so.0.5600.4)
==377771==    by 0xB20122: object_initialize_with_type (object.c:513)
==377771==    by 0xB201B5: object_initialize (object.c:534)
==377771==    by 0xB202F3: object_initialize_child_with_propsv (object.c:564)
==377771==    by 0xB2028E: object_initialize_child_with_props (object.c:547)
==377771==    by 0xB203DC: object_initialize_child_internal (object.c:601)
==377771==    by 0x8349DC: bcm2835_peripherals_init (bcm2835_peripherals.c:122)
==377771==    by 0xB1FC1B: object_init_with_type (object.c:375)
==377771==    by 0xB20140: object_initialize_with_type (object.c:515)
...

Looks like we're leaking memory in the object initializiation
some times?

  Thomas

Re: Memory leak in via_isa_realize()

[Mark Cave-Ayland]

On 21/03/2022 20:35, Peter Maydell wrote:

> On Mon, 21 Mar 2022 at 18:55, Cédric Le Goater <clg@kaod.org> wrote:
>> I introduced quite a few of these calls,
>>
>>     hw/ppc/pnv_lpc.c:    irqs = qemu_allocate_irqs(handler, lpc, ISA_NUM_IRQS);
>>     hw/ppc/pnv_psi.c:    psi->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
>>     hw/ppc/pnv_psi.c:    psi->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc, xsrc->nr_irqs);
>>     hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppc6xx_set_irq, cpu,
>>     hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppc970_set_irq, cpu,
>>     hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&power7_set_irq, cpu,
>>     hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&power9_set_irq, cpu,
>>     hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppc40x_set_irq,
>>     hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppce500_set_irq,
>>     hw/ppc/spapr_irq.c:    spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr,
>>
>> and may be I can remove some. What's the best practice ?
> 
> The 'best practice' is that if you have an irq line it should be
> because it is the input (gpio or sysbus irq) or output (gpio) of
> some device, ie something that is a subtype of TYPE_DEVICE.
> 
> For the ones in hw/ppc/ppc.c: we used to need to write code like that
> because CPU objects weren't TYPE_DEVICE; now they are, and so you
> can give them inbound gpio lines using qdev_init_gpio_in(), typically
> in the cpu initfn. (See target/riscv for an example, or grep for
> that function name in target/ for others.) Then the board code
> needs to wire up to those IRQs in the usual way for GPIO lines,
> ie using qdev_get_gpio_in(cpudev, ...), instead of directly
> reaching into the CPU struct env->irq_inputs. (There's probably
> a way to structure this change to avoid having to change the CPU
> and all the board code at once, but I haven't thought it through.)
> 
> For the spapr one: this is in machine model code, and currently machines
> aren't subtypes of TYPE_DEVICE. I'd leave this one alone for now;
> we can come back and think about it later.
> 
> For pnv_psi.c: these appear to be because the PnvPsi device is
> allocating irq lines which really should belong to the ICSState
> object (and as a result the ICSState code is having to expose
> ics->nr_irqs and the ics_set_irq function when they could be
> internal to the ICSState code). The ICSState's init function
> should be creating these as qdev gpio lines.
> 
> pnv_lpc.c seems to be ISA related. hw/isa/lpc_ich9.c is an
> example of setting up IRQs for isa_bus_irqs() without using
> qemu_allocate_irqs(), but there may be some more generalised
> ISA cleanup possible here.

The issue with PPC IRQs also affects the OpenPIC implementation: when I last looked a 
while back I didn't see any obvious issues against using gpio IRQs, but the main 
blocker for me was not being able to test all the different PPC machine configurations.

Out of curiosity does anyone know how to test the KVM in-kernel OpenPIC 
implementation in hw/intc/openpic_kvm.c? It seems to be used for e500 only.

I think there is some good work to be done converting ISA devices over to using GPIOs 
and improving the interaction with PCI, but it's something that still remains on my 
TODO list. Again the changes would be mostly mechanical with the main concern being 
over testing to ensure that there are no regressions.


ATB,

Mark.

Re: Memory leak in via_isa_realize()

[Thomas Huth]

On 22/03/2022 09.23, Mark Cave-Ayland wrote:
[...]
> but the main blocker for me was not being able to test all the different PPC 
> machine configurations.

I think the best you can do is to run the avocado tests:

  make check-venv
  ./tests/venv/bin/avocado run -t arch:ppc64 tests/avocado/
  ./tests/venv/bin/avocado run -t arch:ppc tests/avocado/

That tests at least that the machines mpc8544ds, prep, virtex-ml507, bamboo, 
ref405ep, ppce500, powernv, g3beige and mac99 are basically still working, 
which is IMHO a good set already.

> Out of curiosity does anyone know how to test the KVM in-kernel OpenPIC 
> implementation in hw/intc/openpic_kvm.c? It seems to be used for e500 only.

I guess you need some old real e500 silicon to test this...?

  Thomas

Re: Memory leak in via_isa_realize()

[Cédric Le Goater]

On 3/22/22 09:37, Thomas Huth wrote:
> On 22/03/2022 09.23, Mark Cave-Ayland wrote:
> [...]
>> but the main blocker for me was not being able to test all the different PPC machine configurations.
> 
> I think the best you can do is to run the avocado tests:
> 
>   make check-venv
>   ./tests/venv/bin/avocado run -t arch:ppc64 tests/avocado/
>   ./tests/venv/bin/avocado run -t arch:ppc tests/avocado/
> 
> That tests at least that the machines mpc8544ds, prep, virtex-ml507, bamboo, ref405ep, ppce500, powernv, g3beige and mac99 are basically still working, which is IMHO a good set already.

yes. There are little more combinations with :

	https://github.com/legoater/qemu-ppc-boot

>> Out of curiosity does anyone know how to test the KVM in-kernel OpenPIC implementation in hw/intc/openpic_kvm.c? It seems to be used for e500 only.
> 
> I guess you need some old real e500 silicon to test this...?

That's the problem :/

Thanks,

C.

Re: Memory leak in via_isa_realize()

[Bernhard Beschow]

Am 22. März 2022 08:23:09 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 21/03/2022 20:35, Peter Maydell wrote:
>
>> On Mon, 21 Mar 2022 at 18:55, Cédric Le Goater <clg@kaod.org> wrote:
>>> I introduced quite a few of these calls,
>>>
>>>     hw/ppc/pnv_lpc.c:    irqs = qemu_allocate_irqs(handler, lpc, ISA_NUM_IRQS);
>>>     hw/ppc/pnv_psi.c:    psi->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
>>>     hw/ppc/pnv_psi.c:    psi->qirqs = qemu_allocate_irqs(xive_source_set_irq, xsrc, xsrc->nr_irqs);
>>>     hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppc6xx_set_irq, cpu,
>>>     hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppc970_set_irq, cpu,
>>>     hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&power7_set_irq, cpu,
>>>     hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&power9_set_irq, cpu,
>>>     hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppc40x_set_irq,
>>>     hw/ppc/ppc.c:    env->irq_inputs = (void **)qemu_allocate_irqs(&ppce500_set_irq,
>>>     hw/ppc/spapr_irq.c:    spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr,
>>>
>>> and may be I can remove some. What's the best practice ?
>> 
>> The 'best practice' is that if you have an irq line it should be
>> because it is the input (gpio or sysbus irq) or output (gpio) of
>> some device, ie something that is a subtype of TYPE_DEVICE.
>> 
>> For the ones in hw/ppc/ppc.c: we used to need to write code like that
>> because CPU objects weren't TYPE_DEVICE; now they are, and so you
>> can give them inbound gpio lines using qdev_init_gpio_in(), typically
>> in the cpu initfn. (See target/riscv for an example, or grep for
>> that function name in target/ for others.) Then the board code
>> needs to wire up to those IRQs in the usual way for GPIO lines,
>> ie using qdev_get_gpio_in(cpudev, ...), instead of directly
>> reaching into the CPU struct env->irq_inputs. (There's probably
>> a way to structure this change to avoid having to change the CPU
>> and all the board code at once, but I haven't thought it through.)
>> 
>> For the spapr one: this is in machine model code, and currently machines
>> aren't subtypes of TYPE_DEVICE. I'd leave this one alone for now;
>> we can come back and think about it later.
>> 
>> For pnv_psi.c: these appear to be because the PnvPsi device is
>> allocating irq lines which really should belong to the ICSState
>> object (and as a result the ICSState code is having to expose
>> ics->nr_irqs and the ics_set_irq function when they could be
>> internal to the ICSState code). The ICSState's init function
>> should be creating these as qdev gpio lines.
>> 
>> pnv_lpc.c seems to be ISA related. hw/isa/lpc_ich9.c is an
>> example of setting up IRQs for isa_bus_irqs() without using
>> qemu_allocate_irqs(), but there may be some more generalised
>> ISA cleanup possible here.
>
>The issue with PPC IRQs also affects the OpenPIC implementation: when I last looked a 
>while back I didn't see any obvious issues against using gpio IRQs, but the main 
>blocker for me was not being able to test all the different PPC machine configurations.
>
>Out of curiosity does anyone know how to test the KVM in-kernel OpenPIC 
>implementation in hw/intc/openpic_kvm.c? It seems to be used for e500 only.
>
>I think there is some good work to be done converting ISA devices over to using GPIOs 
>and improving the interaction with PCI, but it's something that still remains on my 
>TODO list. Again the changes would be mostly mechanical with the main concern being 
>over testing to ensure that there are no regressions.

If the changes would be mostly mechanical: wouldn't they make for some good, bite-sized junior jobs? That way, progress could also be stretched over time, allowing potential regressions to be ascribed more easily.

Moreover, I would be interested in converting hw/ide/piix.c. AFAICS it contains the only invocation of isa_get_irq() where NULL is passed for *dev. If this invocation could be moved and a meaningful non-NULL value be passed, I think it'd be possible to remove the isabus global. This means that - in theory - we could create as many ISABuses as we'd like! Testing would be easy, too, because the Malta board seems to use this code path (at least it crashes when isa_get_irq() asserts dev != NULL).

Best regards,
Bernhard
>
>
>ATB,
>
>Mark.