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

[Michael S. Tsirkin]

On Mon, Feb 08, 2010 at 02:54:24PM -0600, Anthony Liguori wrote:
> 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.


Yes but 

pci_set_word(d->config + PCI_COMMAND, 0);
pci_set_word(d->config + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM | 
PCI_STATUS_66MHZ);

is not much worse, and that API is already there.
And advatage is it uses macros from linux which have
higher chance to be correct than what we come up with.


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

I spent time cleaning up devices, just did not get to bridges.
What I did is write patches and verify that compiled code
did not change at all. This guarantees no breakage.
Care to volunteer to complete that work?
Separately people that are familiar with device can clean it up.

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