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

[Michael S. Tsirkin]

On Mon, Feb 08, 2010 at 06:43:51PM +0100, Gerd Hoffmann wrote:
> On 02/08/10 18:37, 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 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.
>>
>>>>   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.
>
> Hmm, do you mix up PCIDevice and PCIDeviceInfo structs?
>
> cheers,
>   Gerd

OK. OTOH, I don't think we need to out print header type in -device ?
so what good is it there?