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

[Michael S. Tsirkin]

On Mon, Feb 08, 2010 at 03:56:29PM -0600, Anthony Liguori wrote:
> On 02/08/2010 03:01 PM, Michael S. Tsirkin wrote:
>>> 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.
>>    
>
> Oh, pci_set_word() is certainly an improvement.  Personally, I prefer  
> passing the PCIDevice as a context and adding individual accessors but  
> anything is better than open coded config.
>
>>>   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.
>>    
>
> It's on my radar

Just converted versatile_pci, with some nudging I might do others :)

> but I've got another PCI series in flight.  I've got a  
> branch pci-cleanup on staging that's a work in progress for adding a  
> proper region API along with PCI memory accessors.
>
> Once I find a little more time to finish converting VGA devices, I'll post.
>
> Regards,
>
> Anthony Liguori

Great! That's required for proper spec compliance.
VGA devices are definitely the main pain point here.

-- 
MST