[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 13/37] portio: keep references on portio
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH v2 13/37] portio: keep references on portio |
Date: |
Mon, 1 Aug 2016 14:42:31 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
On 28/07/2016 16:37, address@hidden wrote:
> From: Marc-André Lureau <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
> 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);
>
>
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v2 13/37] portio: keep references on portio,
Paolo Bonzini <=