[v2,1/4] pci: Split pcie_host_mmcfg_map()

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 <agraf@suse.de>
Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
Tested-by: Claudio Fontana <claudio.fontana@huawei.com>
---
 hw/pci/pcie_host.c         | 9 +++++++--
 include/hw/pci/pcie_host.h | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

On 21 January 2015 at 16:18, Alexander Graf <agraf@suse.de> 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 <agraf@suse.de>
> Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> Tested-by: Claudio Fontana <claudio.fontana@huawei.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
...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).

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...

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

On Tue, Jan 27, 2015 at 01:55:32PM +0000, Peter Maydell wrote:
> On 21 January 2015 at 16:18, Alexander Graf <agraf@suse.de> 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 <agraf@suse.de>
> > Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> > Tested-by: Claudio Fontana <claudio.fontana@huawei.com>
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ...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 <pbonzini@redhat.com>
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 <peter.crosthwaite@xilinx.com>
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> 
> 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

On 27/01/2015 15:24, Michael S. Tsirkin wrote:
> I suspect this is not the only device that leaks memory now.
> Paolo?

It should be, this is a bug.

Commit d8d95814609e89e5438a3318a647ec322fc4ff16 missed this place
due to a rebase error.  (I can tell it's a rebase error because I
have the fixed patch in a private branch).

Paolo


Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Wed Jun 11 12:42:01 2014 +0200

    memory: convert memory_region_destroy to object_unparent

    Explicitly call object_unparent in the few places where we
    will re-create the memory region.  If the memory region is
    simply being destroyed as part of device teardown, let QOM
    handle it.

    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>