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

[Anthony Liguori <anthony@codemonkey.ws>]

On 02/08/2010 02:34 PM, Michael S. Tsirkin wrote:
> On Mon, Feb 08, 2010 at 02:32:26PM -0600, Anthony Liguori wrote:
>    
>> On 02/08/2010 02:19 PM, Michael S. Tsirkin wrote:
>>      
>>> On Mon, Feb 08, 2010 at 08:55:58PM +0100, Gerd Hoffmann wrote:
>>>
>>>        
>>>>     Hi,
>>>>
>>>>
>>>>          
>>>>> 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.
>>>>>
>>>>>            
>>>> No.  There is nothing to keep in sync.  And there is no extra copy of data.
>>>>
>>>> Today you have pci_set_*() calls somewhere in PCIDeviceInfo->init().
>>>> I'd like to see them replaced with PCIDeviceInfo->$field + setup in
>>>> common code.  The information that device $foo has vendor id 42 and
>>>> device id 4711 (and other properties) just moves from code to data.
>>>>
>>>>          
>>> We still need it in config array which is read by guest.
>>> So that is two places.
>>>
>>>        
>> There's no reason that we couldn't make the config space read like all
>> of the other spaces we support.  IOW, instead of using an array to store
>> the data, store each element in a structure, and have a big switch().
>>
>> I'm not sure one's better than the other though TBH.
>>      
> Yea. So the solution that needs less code is better.
>
>    
>> I think just universally moving to a set of accessors that took a
>> PCIDevice as an argument in the form of pci_device_set_vendor() would be
>> a big improvement.
>>
>> Regards,
>>
>> Anthony Liguori
>>      
> Not sure it's such a *big* improvement, but I won't object to that.
>    

Sorry, but:

versatile_pci.c:    d->config[0x04] = 0x00;
versatile_pci.c:    d->config[0x05] = 0x00;
versatile_pci.c:    d->config[0x06] = 0x20;
versatile_pci.c:    d->config[0x07] = 0x02;

To:

pci_config_set_command(d, 0);
pci_config_set_status(d, PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ);

Is a huge improvement.  I'm staring at a PCI config space diagram right 
now and I'm *still* not even sure I'm interpreting the versatile_pci 
code correctly :-)

Having a nice structure with:

{  .command = 0,
     .status = PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_66MHZ,
}

would be nice but I can live without it.

Regards,

Anthony Liguori