From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39111) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YG76o-00072v-L5 for qemu-devel@nongnu.org; Tue, 27 Jan 2015 09:27:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YG76l-0001K9-TO for qemu-devel@nongnu.org; Tue, 27 Jan 2015 09:27:14 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55210) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YG76l-0001Jf-LV for qemu-devel@nongnu.org; Tue, 27 Jan 2015 09:27:11 -0500 Date: Tue, 27 Jan 2015 16:24:57 +0200 From: "Michael S. Tsirkin" Message-ID: <20150127142457.GA9335@redhat.com> References: <1421857131-18539-1-git-send-email-agraf@suse.de> <1421857131-18539-2-git-send-email-agraf@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v2 1/4] pci: Split pcie_host_mmcfg_map() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Rob Herring , Ard Biesheuvel , QEMU Developers , Claudio Fontana , Alvise Rigo , Stuart Yoder , Alexander Graf , pbonzini@redhat.com On Tue, Jan 27, 2015 at 01:55:32PM +0000, Peter Maydell wrote: > On 21 January 2015 at 16:18, Alexander Graf 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 > > Reviewed-by: Claudio Fontana > > Tested-by: Claudio Fontana > > Reviewed-by: Peter Maydell > ...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 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 Signed-off-by: Paolo Bonzini > > 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