From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NebgP-0003Ab-33 for qemu-devel@nongnu.org; Mon, 08 Feb 2010 17:02:17 -0500 Received: from [199.232.76.173] (port=47297 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NebgO-0003AN-Ly for qemu-devel@nongnu.org; Mon, 08 Feb 2010 17:02:16 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NebgM-0002b1-QU for qemu-devel@nongnu.org; Mon, 08 Feb 2010 17:02:16 -0500 Received: from mx1.redhat.com ([209.132.183.28]:13136) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NebgL-0002aJ-Jl for qemu-devel@nongnu.org; Mon, 08 Feb 2010 17:02:13 -0500 Date: Mon, 8 Feb 2010 23:58:56 +0200 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register. Message-ID: <20100208215856.GA28454@redhat.com> References: <20100208173741.GB10716@redhat.com> <20100208182624.GD10716@redhat.com> <4B706C4E.2010508@redhat.com> <20100208201919.GA17088@redhat.com> <4B7074DA.10408@codemonkey.ws> <20100208203459.GC17088@redhat.com> <4B707A00.2000706@codemonkey.ws> <20100208210147.GA17248@redhat.com> <4B70888D.7000702@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4B70888D.7000702@codemonkey.ws> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Blue Swirl , Isaku Yamahata , Gerd Hoffmann , qemu-devel@nongnu.org On Mon, Feb 08, 2010 at 03:56:29PM -0600, Anthony Liguori wrote: > On 02/08/2010 03:01 PM, Michael S. Tsirkin wrote: >>> Sorry, but: >>> >>> versatile_pci.c: d->config[0x04] = 0x00; >>> versatile_pci.c: d->config[0x05] = 0x00; >>> versatile_pci.c: d->config[0x06] = 0x20; >>> versatile_pci.c: d->config[0x07] = 0x02; >>> >>> To: >>> >>> pci_config_set_command(d, 0); >>> pci_config_set_status(d, PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ); >>> >>> Is a huge improvement. >>> >> >> Yes but >> >> pci_set_word(d->config + PCI_COMMAND, 0); >> pci_set_word(d->config + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ); >> >> is not much worse, and that API is already there. >> And advatage is it uses macros from linux which have >> higher chance to be correct than what we come up with. >> > > Oh, pci_set_word() is certainly an improvement. Personally, I prefer > passing the PCIDevice as a context and adding individual accessors but > anything is better than open coded config. > >>> I'm staring at a PCI config space diagram right >>> now and I'm *still* not even sure I'm interpreting the versatile_pci >>> code correctly :-) >>> >> I spent time cleaning up devices, just did not get to bridges. >> What I did is write patches and verify that compiled code >> did not change at all. This guarantees no breakage. >> Care to volunteer to complete that work? >> Separately people that are familiar with device can clean it up. >> > > It's on my radar Just converted versatile_pci, with some nudging I might do others :) > but I've got another PCI series in flight. I've got a > branch pci-cleanup on staging that's a work in progress for adding a > proper region API along with PCI memory accessors. > > Once I find a little more time to finish converting VGA devices, I'll post. > > Regards, > > Anthony Liguori Great! That's required for proper spec compliance. VGA devices are definitely the main pain point here. -- MST