From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NeZak-0001jL-2I for qemu-devel@nongnu.org; Mon, 08 Feb 2010 14:48:18 -0500 Received: from [199.232.76.173] (port=40809 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NeZaj-0001jC-KK for qemu-devel@nongnu.org; Mon, 08 Feb 2010 14:48:17 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NeZag-0005YR-JO for qemu-devel@nongnu.org; Mon, 08 Feb 2010 14:48:17 -0500 Received: from mx1.redhat.com ([209.132.183.28]:3870) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NeZaf-0005XC-PS for qemu-devel@nongnu.org; Mon, 08 Feb 2010 14:48:14 -0500 Date: Mon, 8 Feb 2010 21:44:57 +0200 From: "Michael S. Tsirkin" Subject: Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register. Message-ID: <20100208194457.GA14650@redhat.com> References: <20100208101753.GA14662@redhat.com> <4B6FF203.6010304@redhat.com> <20100208162753.GA28230@redhat.com> <4B7048E9.6000706@redhat.com> <20100208173204.GA10716@redhat.com> <4B704BE5.9010205@redhat.com> <20100208173741.GB10716@redhat.com> <20100208182624.GD10716@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: Isaku Yamahata , Gerd Hoffmann , qemu-devel@nongnu.org On Mon, Feb 08, 2010 at 09:32:54PM +0200, Blue Swirl wrote: > On Mon, Feb 8, 2010 at 8:26 PM, Michael S. Tsirkin wro= te: > > On Mon, Feb 08, 2010 at 07:56:27PM +0200, Blue Swirl wrote: > >> On Mon, Feb 8, 2010 at 7:37 PM, Michael S. Tsirkin = wrote: > >> > On Mon, Feb 08, 2010 at 06:37:41PM +0100, Gerd Hoffmann wrote: > >> >> On 02/08/10 18:32, Michael S. Tsirkin wrote: > >> >>> On Mon, Feb 08, 2010 at 06:24:57PM +0100, Gerd Hoffmann wrote: > >> >>>> On 02/08/10 17:27, Michael S. Tsirkin wrote: > >> >>>>> On Mon, Feb 08, 2010 at 12:14:11PM +0100, Gerd Hoffmann wrote: > >> >>>>>> On 02/08/10 11:17, Michael S. Tsirkin wrote: > >> >>>>>>> On Mon, Feb 08, 2010 at 03:41:47PM +0900, Isaku Yamahata wro= te: > >> >>>>>>>> initialize header type register in pci generic code. > >> >>>>>>>> > >> >>>>>>>> Cc: Blue Swirl > >> >>>>>>>> Cc: "Michael S. Tsirkin" > >> >>>>>>>> Signed-off-by: Isaku Yamahata > >> >>>>>>> > >> >>>>>>> No objections here, I am assuming this will be followed > >> >>>>>>> by patches removing header type init from bridges? > >> >>>>>>> =A0 =A0From qdev perspective, it is probably cleaner to make > >> >>>>>>> multifunction bit a separate qdev property though, right? > >> >>>>>> > >> >>>>>> =A0 =A0From a qdev perspective it would make *alot* of sense = to move a bunch of > >> >>>>>> pci config stuff (including, but not limited to header type) = into > >> >>>>>> PCIDeviceInfo. > >> >>>>>> > >> >>>>>> cheers, > >> >>>>>> =A0 =A0 Gerd > >> >>>>> > >> >>>>> Actually - won't this make it possible to create broken config= urations > >> >>>>> by tweaking properties from command-line? > >> >>>> > >> >>>> Not as property, as struct element in PCIDeviceInfo. =A0i.e. > >> >>>> > >> >>>> static PCIDeviceInfo e1000_info =3D { > >> >>>> =A0 =A0 =A0[ stuff which is here right now ] > >> >>>> =A0 =A0 =A0.vendor_id =3D PCI_VENDOR_ID_INTEL, > >> >>>> =A0 =A0 =A0.device_id =3D E1000_DEVID, > >> >>>> =A0 =A0 =A0.class =A0 =A0 =3D PCI_CLASS_NETWORK_ETHERNET, > >> >>>> =A0 =A0 =A0[ probably more stuff which makes sense ] > >> >>>> } > >> >>>> > >> >>>> Then setup this in generic pci code instead of having each driv= er doing > >> >>>> a bunch of pci_config_set_*() calls. > >> >>>> > >> >>>> cheers, > >> >>>> =A0 =A0Gerd > >> >>> > >> >>> We still end up with class, vendor etc duplicated in 2 places. > >> >> > >> >> No. =A0The info should be *only* in PCIDeviceInfo then. > >> > > >> > That would put a lot of code in pci config cycle path. =A0A single= array > >> > mirroring the whole config space is much cleaner. > >> > >> I'd suppose the arrays would remain as they are now, they just would > >> be initialized (using the pci functions) based on PCIDeviceInfo > >> structure. > > > > 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. > > > >> This would simplify the device code a lot. > > > > Well, I think > > > > pci_set_class(pci_dev, PCI_CLASS_NETWORK_ETHERNET) > > > > is simpler than > > > > =A0 =A0 =A0 =A0.class =3D PCI_CLASS_NETWORK_ETHERNET > > > > and some magic that copies that to pci config. >=20 > The advantage is that if the code happens to change, only the magic > (which is in one place) needs to be changed. Similar process has > happened with savevm. Yes, one place is good. But magic is bad. What's wrong with pci_set_header type or something like that? Why do we need header type in qdev? --=20 MST