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

[Peter Maydell]

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

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