From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55633) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUCYK-0006Kg-4Y for qemu-devel@nongnu.org; Mon, 01 Aug 2016 08:42:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bUCYE-0007tI-3a for qemu-devel@nongnu.org; Mon, 01 Aug 2016 08:42:39 -0400 Received: from mail-wm0-x241.google.com ([2a00:1450:400c:c09::241]:34765) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bUCYD-0007tC-PF for qemu-devel@nongnu.org; Mon, 01 Aug 2016 08:42:34 -0400 Received: by mail-wm0-x241.google.com with SMTP id q128so26027446wma.1 for ; Mon, 01 Aug 2016 05:42:33 -0700 (PDT) Sender: Paolo Bonzini References: <20160728143808.13707-1-marcandre.lureau@redhat.com> <20160728143808.13707-14-marcandre.lureau@redhat.com> From: Paolo Bonzini Message-ID: <6bbb2445-1bd1-a23b-5a4e-1d266c6a1142@redhat.com> Date: Mon, 1 Aug 2016 14:42:31 +0200 MIME-Version: 1.0 In-Reply-To: <20160728143808.13707-14-marcandre.lureau@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 13/37] portio: keep references on portio List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: marcandre.lureau@redhat.com, qemu-devel@nongnu.org On 28/07/2016 16:37, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau > > The isa_register_portio_list() function allocates ioports > data/state. Let's keep the reference to this data on some owner. This > isn't enough to fix leaks, but at least, ASAN stops complaining of > direct leaks. Further cleanup would require calling > portio_list_del/destroy(). This is mostly not an issue because ISA devices are not hot-unpluggable, but the commit message is correct. Reviewed-by: Paolo Bonzini > Signed-off-by: Marc-André Lureau > --- > hw/audio/gus.c | 9 ++++++--- > hw/audio/sb16.c | 4 +++- > hw/block/fdc.c | 4 +++- > hw/char/parallel.c | 3 ++- > hw/display/vga-isa.c | 8 ++++++-- > hw/dma/i8257.c | 6 ++++-- > hw/ide/core.c | 6 ++++-- > hw/isa/isa-bus.c | 14 +++++--------- > include/hw/ide/internal.h | 2 ++ > include/hw/isa/i8257.h | 2 ++ > include/hw/isa/isa.h | 5 ++++- > 11 files changed, 41 insertions(+), 22 deletions(-) > > diff --git a/hw/audio/gus.c b/hw/audio/gus.c > index 6c02646..3d08a65 100644 > --- a/hw/audio/gus.c > +++ b/hw/audio/gus.c > @@ -60,6 +60,8 @@ typedef struct GUSState { > int64_t last_ticks; > qemu_irq pic; > IsaDma *isa_dma; > + PortioList portio_list1; > + PortioList portio_list2; > } GUSState; > > static uint32_t gus_readb(void *opaque, uint32_t nport) > @@ -265,9 +267,10 @@ static void gus_realizefn (DeviceState *dev, Error **errp) > s->samples = AUD_get_buffer_size_out (s->voice) >> s->shift; > s->mixbuf = g_malloc0 (s->samples << s->shift); > > - isa_register_portio_list (d, s->port, gus_portio_list1, s, "gus"); > - isa_register_portio_list (d, (s->port + 0x100) & 0xf00, > - gus_portio_list2, s, "gus"); > + isa_register_portio_list(d, &s->portio_list1, s->port, > + gus_portio_list1, s, "gus"); > + isa_register_portio_list(d, &s->portio_list2, (s->port + 0x100) & 0xf00, > + gus_portio_list2, s, "gus"); > > s->isa_dma = isa_get_dma(isa_bus_from_device(d), s->emu.gusdma); > k = ISADMA_GET_CLASS(s->isa_dma); > diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c > index 3a4a57a..6b4427f 100644 > --- a/hw/audio/sb16.c > +++ b/hw/audio/sb16.c > @@ -106,6 +106,7 @@ typedef struct SB16State { > /* mixer state */ > int mixer_nreg; > uint8_t mixer_regs[256]; > + PortioList portio_list; > } SB16State; > > static void SB_audio_callback (void *opaque, int free); > @@ -1378,7 +1379,8 @@ static void sb16_realizefn (DeviceState *dev, Error **errp) > dolog ("warning: Could not create auxiliary timer\n"); > } > > - isa_register_portio_list (isadev, s->port, sb16_ioport_list, s, "sb16"); > + isa_register_portio_list(isadev, &s->portio_list, s->port, > + sb16_ioport_list, s, "sb16"); > > s->isa_hdma = isa_get_dma(isa_bus_from_device(isadev), s->hdma); > k = ISADMA_GET_CLASS(s->isa_hdma); > diff --git a/hw/block/fdc.c b/hw/block/fdc.c > index f73af7d..b79873a 100644 > --- a/hw/block/fdc.c > +++ b/hw/block/fdc.c > @@ -692,6 +692,7 @@ struct FDCtrl { > /* Timers state */ > uint8_t timer0; > uint8_t timer1; > + PortioList portio_list; > }; > > static FloppyDriveType get_fallback_drive_type(FDrive *drv) > @@ -2495,7 +2496,8 @@ static void isabus_fdc_realize(DeviceState *dev, Error **errp) > FDCtrl *fdctrl = &isa->state; > Error *err = NULL; > > - isa_register_portio_list(isadev, isa->iobase, fdc_portio_list, fdctrl, > + isa_register_portio_list(isadev, &fdctrl->portio_list, > + isa->iobase, fdc_portio_list, fdctrl, > "fdc"); > > isa_init_irq(isadev, &fdctrl->irq, isa->irq); > diff --git a/hw/char/parallel.c b/hw/char/parallel.c > index 11c78fe..fa08566 100644 > --- a/hw/char/parallel.c > +++ b/hw/char/parallel.c > @@ -80,6 +80,7 @@ typedef struct ParallelState { > uint32_t last_read_offset; /* For debugging */ > /* Memory-mapped interface */ > int it_shift; > + PortioList portio_list; > } ParallelState; > > #define TYPE_ISA_PARALLEL "isa-parallel" > @@ -532,7 +533,7 @@ static void parallel_isa_realizefn(DeviceState *dev, Error **errp) > s->status = dummy; > } > > - isa_register_portio_list(isadev, base, > + isa_register_portio_list(isadev, &s->portio_list, base, > (s->hw_driver > ? &isa_parallel_portio_hw_list[0] > : &isa_parallel_portio_sw_list[0]), > diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c > index f5aff1c..1af9556 100644 > --- a/hw/display/vga-isa.c > +++ b/hw/display/vga-isa.c > @@ -39,6 +39,8 @@ typedef struct ISAVGAState { > ISADevice parent_obj; > > struct VGACommonState state; > + PortioList portio_vga; > + PortioList portio_vbe; > } ISAVGAState; > > static void vga_isa_reset(DeviceState *dev) > @@ -60,9 +62,11 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp) > vga_common_init(s, OBJECT(dev), true); > s->legacy_address_space = isa_address_space(isadev); > vga_io_memory = vga_init_io(s, OBJECT(dev), &vga_ports, &vbe_ports); > - isa_register_portio_list(isadev, 0x3b0, vga_ports, s, "vga"); > + isa_register_portio_list(isadev, &d->portio_vga, > + 0x3b0, vga_ports, s, "vga"); > if (vbe_ports) { > - isa_register_portio_list(isadev, 0x1ce, vbe_ports, s, "vbe"); > + isa_register_portio_list(isadev, &d->portio_vbe, > + 0x1ce, vbe_ports, s, "vbe"); > } > memory_region_add_subregion_overlap(isa_address_space(isadev), > 0x000a0000, > diff --git a/hw/dma/i8257.c b/hw/dma/i8257.c > index f345c54..bffbdea 100644 > --- a/hw/dma/i8257.c > +++ b/hw/dma/i8257.c > @@ -553,10 +553,12 @@ static void i8257_realize(DeviceState *dev, Error **errp) > memory_region_add_subregion(isa_address_space_io(isa), > d->base, &d->channel_io); > > - isa_register_portio_list(isa, d->page_base, page_portio_list, d, > + isa_register_portio_list(isa, &d->portio_page, > + d->page_base, page_portio_list, d, > "dma-page"); > if (d->pageh_base >= 0) { > - isa_register_portio_list(isa, d->pageh_base, pageh_portio_list, d, > + isa_register_portio_list(isa, &d->portio_pageh, > + d->pageh_base, pageh_portio_list, d, > "dma-pageh"); > } > > diff --git a/hw/ide/core.c b/hw/ide/core.c > index 081c9eb..95790cc 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -2617,10 +2617,12 @@ void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2) > { > /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA > bridge has been setup properly to always register with ISA. */ > - isa_register_portio_list(dev, iobase, ide_portio_list, bus, "ide"); > + isa_register_portio_list(dev, &bus->portio_list, > + iobase, ide_portio_list, bus, "ide"); > > if (iobase2) { > - isa_register_portio_list(dev, iobase2, ide_portio2_list, bus, "ide"); > + isa_register_portio_list(dev, &bus->portio2_list, > + iobase2, ide_portio2_list, bus, "ide"); > } > } > > diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c > index ce74db2..9d07b11 100644 > --- a/hw/isa/isa-bus.c > +++ b/hw/isa/isa-bus.c > @@ -131,24 +131,20 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start) > isa_init_ioport(dev, start); > } > > -void isa_register_portio_list(ISADevice *dev, uint16_t start, > +void isa_register_portio_list(ISADevice *dev, > + PortioList *piolist, uint16_t start, > const MemoryRegionPortio *pio_start, > void *opaque, const char *name) > { > - PortioList piolist; > + assert(piolist && !piolist->owner); > > /* START is how we should treat DEV, regardless of the actual > contents of the portio array. This is how the old code > actually handled e.g. the FDC device. */ > isa_init_ioport(dev, start); > > - /* FIXME: the device should store created PortioList in its state. Note > - that DEV can be NULL here and that single device can register several > - portio lists. Current implementation is leaking memory allocated > - in portio_list_init. The leak is not critical because it happens only > - at initialization time. */ > - portio_list_init(&piolist, OBJECT(dev), pio_start, opaque, name); > - portio_list_add(&piolist, isabus->address_space_io, start); > + portio_list_init(piolist, OBJECT(dev), pio_start, opaque, name); > + portio_list_add(piolist, isabus->address_space_io, start); > } > > static void isa_device_init(Object *obj) > diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h > index 7824bc3..a6dd2c3 100644 > --- a/include/hw/ide/internal.h > +++ b/include/hw/ide/internal.h > @@ -480,6 +480,8 @@ struct IDEBus { > uint8_t retry_unit; > int64_t retry_sector_num; > uint32_t retry_nsector; > + PortioList portio_list; > + PortioList portio2_list; > }; > > #define TYPE_IDE_DEVICE "ide-device" > diff --git a/include/hw/isa/i8257.h b/include/hw/isa/i8257.h > index aa211c0..88a2766 100644 > --- a/include/hw/isa/i8257.h > +++ b/include/hw/isa/i8257.h > @@ -36,6 +36,8 @@ typedef struct I8257State { > QEMUBH *dma_bh; > bool dma_bh_scheduled; > int running; > + PortioList portio_page; > + PortioList portio_pageh; > } I8257State; > > #endif > diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h > index 7693ac5..c2fdd70 100644 > --- a/include/hw/isa/isa.h > +++ b/include/hw/isa/isa.h > @@ -134,12 +134,15 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start); > * device and use the legacy portio routines. > * > * @dev: the ISADevice against which these are registered; may be NULL. > + * @piolist: the PortioList associated with the io ports > * @start: the base I/O port against which the portio->offset is applied. > * @portio: the ports, sorted by offset. > * @opaque: passed into the portio callbacks. > * @name: passed into memory_region_init_io. > */ > -void isa_register_portio_list(ISADevice *dev, uint16_t start, > +void isa_register_portio_list(ISADevice *dev, > + PortioList *piolist, > + uint16_t start, > const MemoryRegionPortio *portio, > void *opaque, const char *name); > >