From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Neaco-0002LB-Kf for qemu-devel@nongnu.org; Mon, 08 Feb 2010 15:54:30 -0500 Received: from [199.232.76.173] (port=50914 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Neaco-0002L3-7t for qemu-devel@nongnu.org; Mon, 08 Feb 2010 15:54:30 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1Neacm-0006U3-Ht for qemu-devel@nongnu.org; Mon, 08 Feb 2010 15:54:30 -0500 Received: from mail-yx0-f183.google.com ([209.85.210.183]:44748) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Neacm-0006Tr-8s for qemu-devel@nongnu.org; Mon, 08 Feb 2010 15:54:28 -0500 Received: by yxe13 with SMTP id 13so4665809yxe.18 for ; Mon, 08 Feb 2010 12:54:27 -0800 (PST) Message-ID: <4B707A00.2000706@codemonkey.ws> Date: Mon, 08 Feb 2010 14:54:24 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register. References: <20100208162753.GA28230@redhat.com> <4B7048E9.6000706@redhat.com> <20100208173204.GA10716@redhat.com> <4B704BE5.9010205@redhat.com> <20100208173741.GB10716@redhat.com> <20100208182624.GD10716@redhat.com> <4B706C4E.2010508@redhat.com> <20100208201919.GA17088@redhat.com> <4B7074DA.10408@codemonkey.ws> <20100208203459.GC17088@redhat.com> In-Reply-To: <20100208203459.GC17088@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Blue Swirl , Isaku Yamahata , Gerd Hoffmann , qemu-devel@nongnu.org On 02/08/2010 02:34 PM, Michael S. Tsirkin wrote: > On Mon, Feb 08, 2010 at 02:32:26PM -0600, Anthony Liguori wrote: > >> On 02/08/2010 02:19 PM, Michael S. Tsirkin wrote: >> >>> On Mon, Feb 08, 2010 at 08:55:58PM +0100, Gerd Hoffmann wrote: >>> >>> >>>> Hi, >>>> >>>> >>>> >>>>> This still means we have two copies of same data >>>>> and need to maintain code that keeps them in sync, >>>>> even if that is called just at init time. >>>>> >>>>> >>>> No. There is nothing to keep in sync. And there is no extra copy of data. >>>> >>>> Today you have pci_set_*() calls somewhere in PCIDeviceInfo->init(). >>>> I'd like to see them replaced with PCIDeviceInfo->$field + setup in >>>> common code. The information that device $foo has vendor id 42 and >>>> device id 4711 (and other properties) just moves from code to data. >>>> >>>> >>> We still need it in config array which is read by guest. >>> So that is two places. >>> >>> >> There's no reason that we couldn't make the config space read like all >> of the other spaces we support. IOW, instead of using an array to store >> the data, store each element in a structure, and have a big switch(). >> >> I'm not sure one's better than the other though TBH. >> > Yea. So the solution that needs less code is better. > > >> I think just universally moving to a set of accessors that took a >> PCIDevice as an argument in the form of pci_device_set_vendor() would be >> a big improvement. >> >> Regards, >> >> Anthony Liguori >> > Not sure it's such a *big* improvement, but I won't object to that. > 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. 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 :-) Having a nice structure with: { .command = 0, .status = PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ, } would be nice but I can live without it. Regards, Anthony Liguori