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

[Gerd Hoffmann <kraxel@redhat.com>]

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<blauwirbel@gmail.com>
>>>>>>>> Cc: "Michael S. Tsirkin"<mst@redhat.com>
>>>>>>>> Signed-off-by: Isaku Yamahata<yamahata@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.
>
>>>   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