qemu-devel
[Top][All Lists]
Advanced

[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);
>  
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]