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

[Blue Swirl]

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.