Re: [Qemu-devel] [PATCH 11/39] msix: split msix_free from msix_uninit

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jun 05, 2013 at 09:48:19AM +0200, Paolo Bonzini wrote:
> Il 05/06/2013 06:53, Michael S. Tsirkin ha scritto:
> > On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote:
> >> Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto:
> >>>>> +    if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) {
> >>>>> +        msix_free(dev);
> >>>>> +    }
> >>>>> +
> >>>>>      dev->msix_table = g_malloc0(table_size);
> >>>>>      dev->msix_pba = g_malloc0(pba_size);
> >>>>>      dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
> >>> Wow msix_init calls msix_free, and not on error path?
> >>> What's going on here?
> >>
> >> I wasn't too sure that you could get here only with NULL
> >> msix_table/pba/entry_used and wanted to protect against leaks.  I'll
> >> change it to an assertion.
> > 
> > I don't think we should require users allocate all memory with g_malloc0.
> > So no assertion either.
> 
> Assertion that is is NULL, followed by g_malloc0?

No because who sets it to NULL the first time?
msix_init just started.

> > If there's a leak there was always a leak
> 
> No, there wasn't because msix_uninit would have freed the memory.  That is,
> 
>     msix_init
>     msix_uninit
>     msix_init
>     msix_uninit
> 
> had no leak.  Instead, now msix_free is going to be called just once,
> right before freeing the object itself:
> 
>     msix_init
>     msix_uninit
>     msix_init     ***
>     msix_uninit
>     msix_free
> 
> and will have a leak at ***.

Yes. And this looks completely sane from outside,
so this is a bad API.
The way to fix it is not with asserts in code, we need a good API:
alloc/free init/uninit ...

The problem apparently starts in generic code, let's fix it there?

>  I don't think this can happen, unrealize
> should never be followed by another realize right now,

This is not an msix specific problem, I don't think msix should
debug generic core - this will just lead to proliferation of asserts.

This really should be documented prominently in generic code.

Also how about some asserts in generic code making sure ordering
is sane?


> but perhaps in
> the future it will be if we implement something like "device_poweroff"
> and "device_poweron".
> 
> Paolo

> , let's focus on the
> > API change in this series, OK?
> > 
> >>>>> @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
> >>>>>      msix_free_irq_entries(dev);
> >>>>>      dev->msix_entries_nr = 0;
> >>>>>      memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
> >>>>> -    memory_region_destroy(&dev->msix_pba_mmio);
> >>>>> -    g_free(dev->msix_pba);
> >>>>> -    dev->msix_pba = NULL;
> >>>>>      memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
> >>>>> -    memory_region_destroy(&dev->msix_table_mmio);
> >>>>> -    g_free(dev->msix_table);
> >>>>> +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> >>>>> +}
> >>>>> +
> >>>>> +void msix_free(PCIDevice *dev)
> >>>>> +{
> >>>>> +    if (dev->msix_pba) {
> >>>>> +        memory_region_destroy(&dev->msix_pba_mmio);
> >>>>> +        g_free(dev->msix_pba);
> >>>>> +    }
> >>>>> +    dev->msix_pba = NULL;
> >>>>> +
> >>>>> +    if (dev->msix_table) {
> >>>>> +        memory_region_destroy(&dev->msix_table_mmio);
> >>>>> +        g_free(dev->msix_table);
> >>>>> +    }
> >>>>>      dev->msix_table = NULL;
> >>>>> +
> >>>>>      g_free(dev->msix_entry_used);
> >>>>>      dev->msix_entry_used = NULL;
> >>>>> -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> >>>>>  }
> >>>>>  
> >>>>>  void msix_uninit_exclusive_bar(PCIDevice *dev)
> >>> As long as we had init and uninit, it was mostly
> >>> self-documenting.
> >>> Now, there are two cleanup functions, so please add documentation.
> >>
> >> Yes, will do.
> >>
> >> Paolo
> > 
> >