Re: [Qemu-devel] [PATCH 24/37] pc: keep gsi reference

[Eduardo Habkost]

On Thu, Jul 21, 2016 at 02:28:33PM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Thu, Jul 21, 2016 at 01:27:35PM -0400, Marc-André Lureau wrote:
> > > Hi
> > > 
> > > ----- Original Message -----
> > > > On Tue, Jul 19, 2016 at 12:54:19PM +0400, address@hidden
> > > > wrote:
> > > > > From: Marc-André Lureau <address@hidden>
> > > > > 
> > > > > Further cleanup would need to call qemu_free_irq() at the appropriate
> > > > > time, but for now this silences ASAN about direct leaks.
> > > > > 
> > > > > Signed-off-by: Marc-André Lureau <address@hidden>
> > > > 
> > > > Is there a way to make ASAN happy without having to add a field
> > > > to MachineState that we're not going to use for anything?
> > > 
> > > Well, the plan is rather to release it when no longer needed.
> > > Would it be fine to call qemu_free_irqs() in
> > > machine_finalize()?
> > 
> > It would be fine, I guess, but it looks pointless if we have lots
> > of other resources allocated during PC machine initialization
> > that are never released.
> 
> The main point, right now, is to have no direct leaks when
> running ASAN or valgrind, as they hide new introduced leaks
> that may be much worse. (it would also be good if we had no
> indirect leaks either, as this may also grow over time)

I see. And you don't need to release it on finalize to reach that
goal, right? In this case I don't think we need the extra work.

(In case my previous message was unclear, I believe the field
will be useful, even if we don't release anything on finalize.)

> 
> > But, see additional comment below:
> > 
> > > 
> > > > 
> > > > > ---
> > > > >  hw/i386/pc_piix.c   | 1 +
> > > > >  hw/i386/pc_q35.c    | 1 +
> > > > >  include/hw/boards.h | 1 +
> > > > >  3 files changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > > index a07dc81..b2db274 100644
> > > > > --- a/hw/i386/pc_piix.c
> > > > > +++ b/hw/i386/pc_piix.c
> > > > > @@ -190,6 +190,7 @@ static void pc_init1(MachineState *machine,
> > > > >      } else {
> > > > >          gsi = qemu_allocate_irqs(gsi_handler, gsi_state,
> > > > >          GSI_NUM_PINS);
> > > > >      }
> > > > > +    machine->gsi = gsi;
> > > > >  
> > > > >      if (pcmc->pci_enabled) {
> > > > >          pci_bus = i440fx_init(host_type,
> > > > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > > > > index c5e8367..5dfb14f 100644
> > > > > --- a/hw/i386/pc_q35.c
> > > > > +++ b/hw/i386/pc_q35.c
> > > > > @@ -158,6 +158,7 @@ static void pc_q35_init(MachineState *machine)
> > > > >      } else {
> > > > >          gsi = qemu_allocate_irqs(gsi_handler, gsi_state,
> > > > >          GSI_NUM_PINS);
> > > > >      }
> > > > > +    machine->gsi = gsi;
> > > > >  
> > > > >      /* create pci host bus */
> > > > >      q35_host = Q35_HOST_DEVICE(qdev_create(NULL,
> > > > >      TYPE_Q35_HOST_DEVICE));
> > > > > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > > > > index e46a744..289ba52 100644
> > > > > --- a/include/hw/boards.h
> > > > > +++ b/include/hw/boards.h
> > > > > @@ -139,6 +139,7 @@ struct MachineState {
> > > > >      /*< private >*/
> > > > >      Object parent_obj;
> > > > >      Notifier sysbus_notifier;
> > > > > +    qemu_irq *gsi;
> > 
> > If this is used only by PC, doesn't it belong to PCMachineState?
> 
> right, i'll try to put it there
> 
> > Anyway, the new field would be very useful to help reduce the
> > number of parameters of PC initialization functions (by making
> > them just get a PCMachineState* argument). I would go even
> 
> Which functions do you have in mind?

i440fx_init(), pc_basic_device_init(), maybe others.

> 
> > further and remove the local 'gsi' variable and replace it with
> > 'pcms->gsi' everywhere.
> 
> ok, why not.

-- 
Eduardo