From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NJ2k9-00072V-3e for qemu-devel@nongnu.org; Fri, 11 Dec 2009 05:29:01 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NJ2k4-0006zp-KL for qemu-devel@nongnu.org; Fri, 11 Dec 2009 05:29:00 -0500 Received: from [199.232.76.173] (port=38239 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NJ2k4-0006zg-7e for qemu-devel@nongnu.org; Fri, 11 Dec 2009 05:28:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:23519) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NJ2k4-0006ta-3m for qemu-devel@nongnu.org; Fri, 11 Dec 2009 05:28:56 -0500 Date: Fri, 11 Dec 2009 12:26:01 +0200 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] [PATCH 00/17] pci: switch a ton of drivers to symbolic names Message-ID: <20091211102601.GA29972@redhat.com> References: <20091210180939.GA25707@redhat.com> <200912101828.39821.paul@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200912101828.39821.paul@codesourcery.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paul Brook Cc: qemu-devel@nongnu.org On Thu, Dec 10, 2009 at 06:28:39PM +0000, Paul Brook wrote: > On Thursday 10 December 2009, Michael S. Tsirkin wrote: > > The recent e1000 bug made the important of using > > symbolic macros for pci config access clear for me. > > So I started going over drivers and converting > > to symbolic constants instead of hard-coded ones. > > I did a large part until I run out of steam. > > Maybe some brave soul will take up converting > > the rest of them, or maybe I will: note that > > when converting bridges one should be careful > > to use bridge macros where appropriate. > > Seeing as you're introducing a huge amount of churn, > wouldn't it be > better to come up with a sane abstraction for initializing the the PCI > config data Yes, I am looking at a larger cleanup, but just decoding the magic numbers is a necesary intermediate step to make that one safe. Patch removing - /* TODO: no need to do this, BAR registration does it already */ - d->config[PCI_BASE_ADDRESS_0] = PCI_BASE_ADDRESS_SPACE_IO; will be much clearer than if it just killed this line outright: - d->config[0x10] = 0x0001; And the recent bug introduced in e1000 made it clear to me that it is better to submit this one meanwhile, so that one can use grep to find code affecting a register. > (c.f. pci_config_set_vendor_id)? > > Paul I'm not sure pci_config_set_vendor_id is an example of a good abstraction. I think good abstractions would be higher level, c.f. pci_add_capability() I also have no idea when will a larger cleanup be done. -- MST