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 <m...@redhat.com> 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 wrote: > >>>>>>>> initialize header type register in pci generic code. > >>>>>>>> > >>>>>>>> Cc: Blue Swirl<blauwir...@gmail.com> > >>>>>>>> Cc: "Michael S. Tsirkin"<m...@redhat.com> > >>>>>>>> Signed-off-by: Isaku Yamahata<yamah...@valinux.co.jp> > >>>>>>> > >>>>>>> No objections here, I am assuming this will be followed > >>>>>>> by patches removing header type init from bridges? > >>>>>>> From qdev perspective, it is probably cleaner to make > >>>>>>> multifunction bit a separate qdev property though, right? > >>>>>> > >>>>>> From 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, > >>>>>> Gerd > >>>>> > >>>>> Actually - won't this make it possible to create broken configurations > >>>>> by tweaking properties from command-line? > >>>> > >>>> Not as property, as struct element in PCIDeviceInfo. i.e. > >>>> > >>>> static PCIDeviceInfo e1000_info = { > >>>> [ stuff which is here right now ] > >>>> .vendor_id = PCI_VENDOR_ID_INTEL, > >>>> .device_id = E1000_DEVID, > >>>> .class = PCI_CLASS_NETWORK_ETHERNET, > >>>> [ probably more stuff which makes sense ] > >>>> } > >>>> > >>>> Then setup this in generic pci code instead of having each driver doing > >>>> a bunch of pci_config_set_*() calls. > >>>> > >>>> cheers, > >>>> Gerd > >>> > >>> We still end up with class, vendor etc duplicated in 2 places. > >> > >> No. The info should be *only* in PCIDeviceInfo then. > > > > That would put a lot of code in pci config cycle path. A 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 .class = PCI_CLASS_NETWORK_ETHERNET and some magic that copies that to pci config. > >>> Why do > >>> we want stuff like vendor id in PCIDeviceInfo at all? Why can't > >>> everyone just use pci_config_set/get calls? > >> > >> You can do nice stuff like printing vendor/device IDs in the '-device ?' > >> list then. > > > > That should use pci functions as well. > > > >> cheers, > >> Gerd > >