Re: [Qemu-devel] [PATCH v2 1/4] pci: Split pcie_host_mmcfg_map()

[Michael S. Tsirkin]

On Tue, Jan 27, 2015 at 01:55:32PM +0000, Peter Maydell wrote:
> On 21 January 2015 at 16:18, Alexander Graf <address@hidden> wrote:
> > The mmcfg space is a memory region that allows access to PCI config space
> > in the PCIe world. To maintain abstraction layers, I would like to expose
> > the mmcfg space as a sysbus mmio region rather than have it mapped straight
> > into the system's memory address space though.
> >
> > So this patch splits the initialization of the mmcfg space from the actual
> > mapping, allowing us to only have an mmfg memory region without the map.
> >
> > Signed-off-by: Alexander Graf <address@hidden>
> > Reviewed-by: Claudio Fontana <address@hidden>
> > Tested-by: Claudio Fontana <address@hidden>
> 
> Reviewed-by: Peter Maydell <address@hidden>
> ...as far as it goes, but:
> 
> Really the pcie_host_mmcfg_map/unmap/update() function is just totally
> misguided. This functionality should be pushed upwards into
> hw/pci-host/q35.c which can handle its own mapping of the MMIO region
> into the system address space at the appropriate location/size.
> 
> In particular, at the moment q35.c will leak a bunch of stuff
> every time the guest unmaps and remaps the mmcfg space, because
> we call memory_region_init_io() over and over again on the same
> MMIO object (which isn't valid).

I used to be fine before the QOM conversion I think?

Take a look at this one (and previous patch):

commit 469b046ead0671932ff3af8d6f95045b19b186ef
Author: Paolo Bonzini <address@hidden>
Date:   Wed Jun 11 12:50:43 2014 +0200

    memory: remove memory_region_destroy
    
    The function is empty after the previous patch, so remove it.
    
    Reviewed-by: Peter Crosthwaite <address@hidden>
    Signed-off-by: Paolo Bonzini <address@hidden>

> 
> Any time you see a device with its own base address in its
> device struct it's a red flag that the design's probably wrong...

I suspect this is not the only device that leaks memory now.
Paolo?

> The size of the MMCFG region should probably be a device property.
> Then the subclass realize could just rely on the baseclass realize
> to always create the mmio region, rather than having to explicitly
> call a function to get it to do the right thing.
> 
> thanks
> -- PMM

-- 
MST