Re: [Qemu-devel] Re: [PATCH] pci: initialize header type register.

[Michael S. Tsirkin]

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 <address@hidden> wrote:
> > 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 <address@hidden> 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<address@hidden>
> >> >>>>>>>> Cc: "Michael S. Tsirkin"<address@hidden>
> >> >>>>>>>> Signed-off-by: Isaku Yamahata<address@hidden>
> >> >>>>>>>
> >> >>>>>>> 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.
> 
> 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?

-- 
MST