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

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

[Michael S. Tsirkin]

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.

Instead of testing a huge number of configurations,
I compared binaries before and after conversion.
Almost all of them generate exact same stripped binary
before and after the change.
The only object changed was eepro100, objdump showed
that the change was because gcc for some reason
decides to use a bit more stack for init function
after comments are added there.

This methodology was the reason that I added TODOs where I saw
deviations from spec or other code ugliness, will have to be fixed
separately.

I also verified separately that eepro100 still works
and survives light usage.

This patcheset applies on top of e1000 bugfix
I have posted previously.

Michael S. Tsirkin (17):
  e1000: switch to symbolic names for pci registers
  ne2000: switch to symbolic names for pci registers
  rtl: switch to symbolic names for pci registers
  pcnet: switch to symbolic names for pci registers
  pci: add more status bits
  eepro100: symbolic names for pci registers
  piix: symbolic constants
  cmd646: symbolic names for pci registers
  vmware_vga: symbolic names for pci registers
  lsi: symbolic names for pci registers
  pci: add another devsel macro
  es1370: symbolic names for pci registers
  wdt_i6300esb: symbolic names for pci registers
  ac97: symbolic names for pci registers
  usb-uhci: symbolic names for pci registers
  usb-uhci: symbolic names for pci registers
  pci: remove unused macro

 hw/ac97.c         |   57 ++++++++++++++++++++++++++++------------------------
 hw/e1000.c        |   11 ++++++---
 hw/eepro100.c     |   49 +++++++++++++++++++++++++++++---------------
 hw/es1370.c       |   29 ++++++++++++++-------------
 hw/ide/cmd646.c   |    7 +++--
 hw/ide/piix.c     |   13 +++++++----
 hw/lsi53c895a.c   |   10 +++++---
 hw/ne2000.c       |    3 +-
 hw/pci.h          |    5 +++-
 hw/pcnet.c        |   26 +++++++++++++++--------
 hw/rtl8139.c      |   16 +++++++++-----
 hw/usb-ohci.c     |    6 +++-
 hw/usb-uhci.c     |    7 +++--
 hw/vmware_vga.c   |   20 ++++++++++--------
 hw/wdt_i6300esb.c |    2 +-
 15 files changed, 156 insertions(+), 105 deletions(-)

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

[Glauber Costa]

On Thu, Dec 10, 2009 at 4:09 PM, Michael S. Tsirkin <mst@redhat.com> 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.
>
> Instead of testing a huge number of configurations,
> I compared binaries before and after conversion.
> Almost all of them generate exact same stripped binary
> before and after the change.
> The only object changed was eepro100, objdump showed
> that the change was because gcc for some reason
> decides to use a bit more stack for init function
> after comments are added there.
>
> This methodology was the reason that I added TODOs where I saw
> deviations from spec or other code ugliness, will have to be fixed
> separately.
>

IMHO, this is a huge enhancement.

I myself was found expending huge amounts of time trying do figure out the
meaning of some specific constants in the past.

+1

-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

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

[Paul Brook]

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 (c.f. 
pci_config_set_vendor_id)?

Paul

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

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

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> 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.

Nice patchset.  And makes things way clear.

Acked-by: Juan Quintela