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

[Blue Swirl]

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 would simplify the device code a lot.

>>>  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
>