From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=42627 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q7Qe3-0000Ab-4M for qemu-devel@nongnu.org; Wed, 06 Apr 2011 07:11:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q7Qe2-00035Z-1Y for qemu-devel@nongnu.org; Wed, 06 Apr 2011 07:11:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30755) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q7Qe1-00035F-JP for qemu-devel@nongnu.org; Wed, 06 Apr 2011 07:11:29 -0400 Date: Wed, 6 Apr 2011 14:11:07 +0300 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] Re: [PATCH 00/10] pci: pci_register_bar_simple Message-ID: <20110406111107.GB30884@redhat.com> References: <1301930887-20705-1-git-send-email-avi@redhat.com> <20110404155917.GA13423@redhat.com> <4D99F061.1090208@codemonkey.ws> <4D99F34D.3060804@redhat.com> <20110404194455.GA13721@redhat.com> <20110404212649.GA16327@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: Avi Kivity , qemu-devel@nongnu.org On Tue, Apr 05, 2011 at 09:02:45PM +0300, Blue Swirl wrote: > On Tue, Apr 5, 2011 at 12:26 AM, Michael S. Tsirkin wr= ote: > > On Tue, Apr 05, 2011 at 12:05:08AM +0300, Blue Swirl wrote: > >> On Mon, Apr 4, 2011 at 10:44 PM, Michael S. Tsirkin = wrote: > >> > On Mon, Apr 04, 2011 at 08:02:23PM +0300, Blue Swirl wrote: > >> >> On Mon, Apr 4, 2011 at 7:35 PM, Avi Kivity wrote= : > >> >> > On 04/04/2011 07:22 PM, Anthony Liguori wrote: > >> >> >> > >> >> >> On 04/04/2011 10:59 AM, Michael S. Tsirkin wrote: > >> >> >>> > >> >> >>> On Mon, Apr 04, 2011 at 06:27:57PM +0300, Avi Kivity wrote: > >> >> >>>> > >> >> >>>> Many PCI BARs that use the memory address space map a single= MMIO region > >> >> >>>> into > >> >> >>>> the entire BAR range. =A0Introduce an API pci_register_bar_s= imple() for > >> >> >>>> that use > >> >> >>>> case, and convert all users where this can be done trivially. > >> >> >>>> > >> >> >>>> This will reduce the work required to introduce a PCI memory= API; it's > >> >> >>>> also > >> >> >>>> a nice code reduction in its own right. > >> >> >>> > >> >> >>> This will save some code, so > >> >> >>> Acked-by: Michael S. Tsirkin > >> >> >>> > >> >> >>> I really hope the rest of devices will follow. > >> >> >> > >> >> >> How complete is this? > >> >> > > >> >> > I converted all devices which were easy to convert. =A0There ma= y be one or two > >> >> > more that can be converted with additional work (and perhaps wi= th an > >> >> > additional pic_bar_get_current_address() API, and a pci_bar_set= _coalescing() > >> >> > API). =A0The rest likely need to stick with the callback-based = API. > >> >> > >> >> In my version which I sent earlier but didn't commit, also other = BARs > >> >> besides the first one and also tricky devices like VGA were handl= ed. > >> > > >> > Yes, I liked that patchset too. What happened to it? > >> > >> Nothing, but I thought that there could be a "perfect" solution. > >> > >> I like in Avi's version that unnecessary API changes are avoided. > > > > Yes, it's nice that it's incremental. > > > >> >> But I didn't commit it because I felt it was not going to right > >> >> direction. I think the BARs should be specified in PCIDeviceInfo > >> >> instead of adding more function calls. The same applies to this p= atch > >> >> set. > >> > > >> > Is that really that fundamental? What I do care about is > >> > making pci.c track and register all device memory > >> > so that we can finally implement pci bridge features > >> > such as master abort handling and unmapped memory. > >> > >> The structure version can be done later. Right, pci.c should manage > >> the device mappings. > > > > OK, so applying Avi's patchset and building on that is > > your preferred approach too? >=20 > Avi's version is a bit too simple, at least multiple regions in a BAR > should be handled (for example macio.c needs that). But also that can > be added later (pci_register_bar_not_so_simple()?), so Avi's version > could be used as the starting point. OK I've put it on my branch so people have the chance to comment, and they can be built upon. --=20 MST