Re: [Qemu-devel] [PATCH 00/17] pci: switch a ton of drivers to symbolic names

[Michael S. Tsirkin]

On Thu, Dec 10, 2009 at 06:28:39PM +0000, Paul Brook wrote:
> On Thursday 10 December 2009, Michael S. Tsirkin wrote:
> > The recent e1000 bug made the important of using
> > symbolic macros for pci config access clear for me.
> > So I started going over drivers and converting
> > to symbolic constants instead of hard-coded ones.
> > I did a large part until I run out of steam.
> > Maybe some brave soul will take up converting
> > the rest of them, or maybe I will: note that
> > when converting bridges one should be careful
> > to use bridge macros where appropriate.
> 
> Seeing as you're introducing a huge amount of churn,
> wouldn't it be
> better to come up with a sane abstraction for initializing the the PCI
> config data

Yes, I am looking at a larger cleanup, but just decoding the magic
numbers is a necesary intermediate step to make that one safe.

Patch removing
- /* TODO: no need to do this, BAR registration does it already */
- d->config[PCI_BASE_ADDRESS_0] = PCI_BASE_ADDRESS_SPACE_IO;
will be much clearer than if it just killed this line outright:
- d->config[0x10] = 0x0001;

And the recent bug introduced in e1000 made it clear to me that it is
better to submit this one meanwhile, so that one can use grep to find
code affecting a register.

> (c.f.  pci_config_set_vendor_id)?
> 
> Paul

I'm not sure pci_config_set_vendor_id is an example of
a good abstraction.  I think good abstractions would
be higher level, c.f. pci_add_capability()

I also have no idea when will a larger cleanup be done.

-- 
MST