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

[Anthony Liguori <anthony@codemonkey.ws>]

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