* Memory leak in via_isa_realize() @ 2022-03-21 10:31 Thomas Huth 2022-03-21 11:12 ` Peter Maydell 2022-03-21 13:04 ` Philippe Mathieu-Daudé 0 siblings, 2 replies; 16+ messages in thread From: Thomas Huth @ 2022-03-21 10:31 UTC (permalink / raw) To: QEMU Developers, Huacai Chen, Philippe Mathieu-Daudé; +Cc: Peter Maydell 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Memory leak in via_isa_realize() 2022-03-21 10:31 Memory leak in via_isa_realize() Thomas Huth @ 2022-03-21 11:12 ` Peter Maydell 2022-03-21 12:11 ` BALATON Zoltan 2022-03-21 18:34 ` BALATON Zoltan 2022-03-21 13:04 ` Philippe Mathieu-Daudé 1 sibling, 2 replies; 16+ messages in thread From: Peter Maydell @ 2022-03-21 11:12 UTC (permalink / raw) To: Thomas Huth; +Cc: Huacai Chen, QEMU Developers, Philippe Mathieu-Daudé 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Memory leak in via_isa_realize() 2022-03-21 11:12 ` Peter Maydell @ 2022-03-21 12:11 ` BALATON Zoltan 2022-03-21 12:59 ` Peter Maydell 2022-03-21 18:57 ` Cédric Le Goater 2022-03-21 18:34 ` BALATON Zoltan 1 sibling, 2 replies; 16+ messages in thread From: BALATON Zoltan @ 2022-03-21 12:11 UTC (permalink / raw) To: Peter Maydell Cc: Thomas Huth, Huacai Chen, QEMU Developers, Philippe Mathieu-Daudé 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Memory leak in via_isa_realize() 2022-03-21 12:11 ` BALATON Zoltan @ 2022-03-21 12:59 ` Peter Maydell 2022-03-22 7:55 ` Thomas Huth 2022-03-21 18:57 ` Cédric Le Goater 1 sibling, 1 reply; 16+ messages in thread From: Peter Maydell @ 2022-03-21 12:59 UTC (permalink / raw) To: BALATON Zoltan Cc: Thomas Huth, Huacai Chen, QEMU Developers, Philippe Mathieu-Daudé 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Memory leak in via_isa_realize() 2022-03-21 12:59 ` Peter Maydell @ 2022-03-22 7:55 ` Thomas Huth 0 siblings, 0 replies; 16+ messages in thread From: Thomas Huth @ 2022-03-22 7:55 UTC (permalink / raw) To: Peter Maydell, BALATON Zoltan Cc: Markus Armbruster, Huacai Chen, QEMU Developers, Philippe Mathieu-Daudé 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Memory leak in via_isa_realize() 2022-03-21 12:11 ` BALATON Zoltan 2022-03-21 12:59 ` Peter Maydell @ 2022-03-21 18:57 ` Cédric Le Goater 2022-03-21 21:16 ` BALATON Zoltan 1 sibling, 1 reply; 16+ messages in thread From: Cédric Le Goater @ 2022-03-21 18:57 UTC (permalink / raw) To: BALATON Zoltan, Peter Maydell Cc: Thomas Huth, Huacai Chen, Fabiano Rosas, Daniel Henrique Barboza, QEMU Developers, Philippe Mathieu-Daudé, list@suse.de:PowerPC 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Memory leak in via_isa_realize() 2022-03-21 18:57 ` Cédric Le Goater @ 2022-03-21 21:16 ` BALATON Zoltan 0 siblings, 0 replies; 16+ messages in thread From: BALATON Zoltan @ 2022-03-21 21:16 UTC (permalink / raw) To: Cédric Le Goater Cc: Peter Maydell, Thomas Huth, Huacai Chen, Fabiano Rosas, Daniel Henrique Barboza, QEMU Developers, Philippe Mathieu-Daudé, list@suse.de:PowerPC [-- 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Memory leak in via_isa_realize() 2022-03-21 11:12 ` Peter Maydell 2022-03-21 12:11 ` BALATON Zoltan @ 2022-03-21 18:34 ` BALATON Zoltan 1 sibling, 0 replies; 16+ messages in thread From: BALATON Zoltan @ 2022-03-21 18:34 UTC (permalink / raw) To: Peter Maydell Cc: Thomas Huth, Huacai Chen, QEMU Developers, Philippe Mathieu-Daudé 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Memory leak in via_isa_realize() 2022-03-21 10:31 Memory leak in via_isa_realize() Thomas Huth 2022-03-21 11:12 ` Peter Maydell @ 2022-03-21 13:04 ` Philippe Mathieu-Daudé 2022-03-21 18:55 ` Cédric Le Goater 1 sibling, 1 reply; 16+ messages in thread From: Philippe Mathieu-Daudé @ 2022-03-21 13:04 UTC (permalink / raw) To: Thomas Huth, QEMU Developers, Huacai Chen, Philippe Mathieu-Daudé Cc: Peter Maydell, Bernhard Beschow 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Memory leak in via_isa_realize() 2022-03-21 13:04 ` Philippe Mathieu-Daudé @ 2022-03-21 18:55 ` Cédric Le Goater 2022-03-21 19:03 ` Philippe Mathieu-Daudé 2022-03-21 20:35 ` Peter Maydell 0 siblings, 2 replies; 16+ messages in thread From: Cédric Le Goater @ 2022-03-21 18:55 UTC (permalink / raw) To: Philippe Mathieu-Daudé, Thomas Huth, QEMU Developers, Huacai Chen, Philippe Mathieu-Daudé Cc: list@suse.de:PowerPC, Peter Maydell, Bernhard Beschow 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Memory leak in via_isa_realize() 2022-03-21 18:55 ` Cédric Le Goater @ 2022-03-21 19:03 ` Philippe Mathieu-Daudé 2022-03-21 20:35 ` Peter Maydell 1 sibling, 0 replies; 16+ messages in thread From: Philippe Mathieu-Daudé @ 2022-03-21 19:03 UTC (permalink / raw) To: Cédric Le Goater, Thomas Huth, QEMU Developers, Huacai Chen, Philippe Mathieu-Daudé Cc: list@suse.de:PowerPC, Peter Maydell, Bernhard Beschow 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"). ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Memory leak in via_isa_realize() 2022-03-21 18:55 ` Cédric Le Goater 2022-03-21 19:03 ` Philippe Mathieu-Daudé @ 2022-03-21 20:35 ` Peter Maydell 2022-03-22 8:23 ` Mark Cave-Ayland 1 sibling, 1 reply; 16+ messages in thread From: Peter Maydell @ 2022-03-21 20:35 UTC (permalink / raw) To: Cédric Le Goater Cc: Thomas Huth, Huacai Chen, QEMU Developers, Philippe Mathieu-Daudé, Philippe Mathieu-Daudé, list@suse.de:PowerPC, Bernhard Beschow 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Memory leak in via_isa_realize() 2022-03-21 20:35 ` Peter Maydell @ 2022-03-22 8:23 ` Mark Cave-Ayland 2022-03-22 8:37 ` Thomas Huth 2022-03-23 22:53 ` Bernhard Beschow 0 siblings, 2 replies; 16+ messages in thread From: Mark Cave-Ayland @ 2022-03-22 8:23 UTC (permalink / raw) To: Peter Maydell, Cédric Le Goater Cc: Thomas Huth, Huacai Chen, QEMU Developers, Philippe Mathieu-Daudé, list@suse.de:PowerPC, Philippe Mathieu-Daudé, Bernhard Beschow 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Memory leak in via_isa_realize() 2022-03-22 8:23 ` Mark Cave-Ayland @ 2022-03-22 8:37 ` Thomas Huth 2022-03-22 8:41 ` Cédric Le Goater 2022-03-23 22:53 ` Bernhard Beschow 1 sibling, 1 reply; 16+ messages in thread From: Thomas Huth @ 2022-03-22 8:37 UTC (permalink / raw) To: Mark Cave-Ayland, Peter Maydell, Cédric Le Goater Cc: Huacai Chen, QEMU Developers, Philippe Mathieu-Daudé, list@suse.de:PowerPC, Philippe Mathieu-Daudé, Bernhard Beschow 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Memory leak in via_isa_realize() 2022-03-22 8:37 ` Thomas Huth @ 2022-03-22 8:41 ` Cédric Le Goater 0 siblings, 0 replies; 16+ messages in thread From: Cédric Le Goater @ 2022-03-22 8:41 UTC (permalink / raw) To: Thomas Huth, Mark Cave-Ayland, Peter Maydell Cc: Huacai Chen, QEMU Developers, Philippe Mathieu-Daudé, list@suse.de:PowerPC, Philippe Mathieu-Daudé, Bernhard Beschow 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Memory leak in via_isa_realize() 2022-03-22 8:23 ` Mark Cave-Ayland 2022-03-22 8:37 ` Thomas Huth @ 2022-03-23 22:53 ` Bernhard Beschow 1 sibling, 0 replies; 16+ messages in thread From: Bernhard Beschow @ 2022-03-23 22:53 UTC (permalink / raw) To: Mark Cave-Ayland, Peter Maydell, Cédric Le Goater Cc: Thomas Huth, Huacai Chen, QEMU Developers, Philippe Mathieu-Daudé, list@suse.de:PowerPC, Philippe Mathieu-Daudé 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-03-23 22:54 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-21 10:31 Memory leak in via_isa_realize() Thomas Huth 2022-03-21 11:12 ` Peter Maydell 2022-03-21 12:11 ` BALATON Zoltan 2022-03-21 12:59 ` Peter Maydell 2022-03-22 7:55 ` Thomas Huth 2022-03-21 18:57 ` Cédric Le Goater 2022-03-21 21:16 ` BALATON Zoltan 2022-03-21 18:34 ` BALATON Zoltan 2022-03-21 13:04 ` Philippe Mathieu-Daudé 2022-03-21 18:55 ` Cédric Le Goater 2022-03-21 19:03 ` Philippe Mathieu-Daudé 2022-03-21 20:35 ` Peter Maydell 2022-03-22 8:23 ` Mark Cave-Ayland 2022-03-22 8:37 ` Thomas Huth 2022-03-22 8:41 ` Cédric Le Goater 2022-03-23 22:53 ` Bernhard Beschow
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.