Patchwork [14/23] pci: factor out the logic to get pci device from address.

login
register
mail settings
Submitter Isaku Yamahata
Date Oct. 5, 2009, 10:06 a.m.
Message ID <1254737223-16129-15-git-send-email-yamahata@valinux.co.jp>
Download mbox | patch
Permalink /patch/34990/
State Superseded
Headers show

Comments

Isaku Yamahata - Oct. 5, 2009, 10:06 a.m.
factor out conversion logic from io port address into bus+dev+func
with bit shift operation and conversion bus+dev+func into pci device.
They will be used later by pcie support.
This patch also eliminates the logic duplication.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 hw/pci.c |   83 ++++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 46 insertions(+), 37 deletions(-)
Michael S. Tsirkin - Oct. 5, 2009, 12:45 p.m.
On Mon, Oct 05, 2009 at 07:06:54PM +0900, Isaku Yamahata wrote:
> factor out conversion logic from io port address into bus+dev+func
> with bit shift operation and conversion bus+dev+func into pci device.
> They will be used later by pcie support.
> This patch also eliminates the logic duplication.

Please split removing logic duplication into separate patch.
It might be applicable already, while express stuff needs more work IMO.

> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |   83 ++++++++++++++++++++++++++++++++++---------------------------
>  1 files changed, 46 insertions(+), 37 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 6ddd256..5f808ff 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -629,44 +629,25 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
>          pci_update_mappings(d);
>  }
>  
> -void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> +static void pci_data_write_common(PCIDevice *pci_dev,
> +                                  uint32_t config_addr, uint32_t val, int len)

So as I said in the previous comment, pci express should just be able to
use pci code directly, instead of reusing this low-level stuff.
All you need to do is range-check address and ignore extended registers.

>  {
> -    PCIBus *s = opaque;
> -    PCIDevice *pci_dev;
> -    int config_addr, bus_num;
> -
> -#if 0
> -    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> -                addr, val, len);
> -#endif
> -    bus_num = (addr >> 16) & 0xff;
> -    s = pci_find_bus(s, bus_num);
> -    if (!s)
> -        return;
> -    pci_dev = s->devices[(addr >> 8) & 0xff];
> +    assert(len == 1 || len == 2 || len == 4);
>      if (!pci_dev)
>          return;
> -    config_addr = addr & 0xff;
> -    PCI_DPRINTF("pci_config_write: %s: "
> -                "addr=%02x val=%08"PRI32x" len=%d\n",
> -                pci_dev->name, config_addr, val, len);
> +
> +    PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
> +                __func__, pci_dev->name, config_addr, val, len);
>      pci_dev->config_write(pci_dev, config_addr, val, len);
>  }
>  
> -uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> +static uint32_t pci_data_read_common(PCIDevice *pci_dev,
> +                                     uint32_t config_addr, int len)
>  {
> -    PCIBus *s = opaque;
> -    PCIDevice *pci_dev;
> -    int config_addr, bus_num;
>      uint32_t val;
>  
> -    bus_num = (addr >> 16) & 0xff;
> -    s = pci_find_bus(s, bus_num);
> -    if (!s)
> -        goto fail;
> -    pci_dev = s->devices[(addr >> 8) & 0xff];
> +    assert(len == 1 || len == 2 || len == 4);
>      if (!pci_dev) {
> -    fail:
>          switch(len) {
>          case 1:
>              val = 0xff;
> @@ -679,21 +660,49 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
>              val = 0xffffffff;
>              break;
>          }
> -        goto the_end;
> +    } else {
> +        val = pci_dev->config_read(pci_dev, config_addr, len);
> +        PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> +                    __func__, pci_dev->name, config_addr, val, len);
>      }
> -    config_addr = addr & 0xff;
> -    val = pci_dev->config_read(pci_dev, config_addr, len);
> -    PCI_DPRINTF("pci_config_read: %s: "
> -                "addr=%02x val=%08"PRIx32" len=%d\n",
> -                pci_dev->name, config_addr, val, len);
> - the_end:
> +
>  #if 0
> -    PCI_DPRINTF("pci_data_read: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> -                addr, val, len);
> +    PCI_DPRINTF("%s: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> +                __func__, addr, val, len);
>  #endif

move to real pci_data_read?

>      return val;
>  }
>  
> +static inline PCIDevice *pci_addr_to_dev(PCIBus *bus, uint32_t addr)
> +{
> +    uint8_t bus_num = (addr >> 16) & 0xff;
> +    uint8_t devfn = (addr >> 8) & 0xff;
> +    return pci_find_device(bus, bus_num, PCI_SLOT(devfn), PCI_FUNC(devfn));
> +}
> +
> +static inline uint32_t pci_addr_to_config(uint32_t addr)
> +{
> +    return addr & 0xff;
> +}
> +
> +void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> +{
> +    PCIBus *s = opaque;
> +#if 0
> +    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> +                addr, val, len);
> +#endif
> +
> +    pci_data_write_common(pci_addr_to_dev(s, addr), pci_addr_to_config(addr),
> +                          val, len);

For pci express, there's extended register number in bits 11:8
so what we should do here IMO is check these bits and
ignore if they are not 0.

> +}
> +
> +uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> +{
> +    PCIBus *s = opaque;

For pci express, there's extended register number in bits 11:8
so what we should do here IMO is check these bits and
return 0 if not 0.

> +    return pci_data_read_common(pci_addr_to_dev(s, addr),
> +                                pci_addr_to_config(addr), len);
> +}
>  /***********************************************************/
>  /* generic PCI irq support */
>  
> -- 
> 1.6.0.2
Isaku Yamahata - Oct. 6, 2009, 9:50 a.m.
On Mon, Oct 05, 2009 at 02:45:33PM +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 05, 2009 at 07:06:54PM +0900, Isaku Yamahata wrote:
> > factor out conversion logic from io port address into bus+dev+func
> > with bit shift operation and conversion bus+dev+func into pci device.
> > They will be used later by pcie support.
> > This patch also eliminates the logic duplication.
> 
> Please split removing logic duplication into separate patch.
> It might be applicable already, while express stuff needs more work IMO.
> 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > ---
> >  hw/pci.c |   83 ++++++++++++++++++++++++++++++++++---------------------------
> >  1 files changed, 46 insertions(+), 37 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 6ddd256..5f808ff 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -629,44 +629,25 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> >          pci_update_mappings(d);
> >  }
> >  
> > -void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > +static void pci_data_write_common(PCIDevice *pci_dev,
> > +                                  uint32_t config_addr, uint32_t val, int len)
> 
> So as I said in the previous comment, pci express should just be able to
> use pci code directly, instead of reusing this low-level stuff.
> All you need to do is range-check address and ignore extended registers.

Maybe my patch description isn't good enough,
I'm afraid that you misunderstood the patch purpose.

PCI configuration and PCI express MMCONFIG uses different address scheme
as follows.
This patch purpose is to separate out its address parsing logic
from pci config space update logic.

PCI:
 0- 7 bit: offset in the configuration space (256bytes)
 7-15 bit: devfn
16-14 bit: bus

PCI express:
 0-11 bit: offset in the configuration space (4KBytes)
12-19 bit: devfn
20-28 bit: bus



> >  {
> > -    PCIBus *s = opaque;
> > -    PCIDevice *pci_dev;
> > -    int config_addr, bus_num;
> > -
> > -#if 0
> > -    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> > -                addr, val, len);
> > -#endif
> > -    bus_num = (addr >> 16) & 0xff;
> > -    s = pci_find_bus(s, bus_num);
> > -    if (!s)
> > -        return;
> > -    pci_dev = s->devices[(addr >> 8) & 0xff];
> > +    assert(len == 1 || len == 2 || len == 4);
> >      if (!pci_dev)
> >          return;
> > -    config_addr = addr & 0xff;
> > -    PCI_DPRINTF("pci_config_write: %s: "
> > -                "addr=%02x val=%08"PRI32x" len=%d\n",
> > -                pci_dev->name, config_addr, val, len);
> > +
> > +    PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
> > +                __func__, pci_dev->name, config_addr, val, len);
> >      pci_dev->config_write(pci_dev, config_addr, val, len);
> >  }
> >  
> > -uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> > +static uint32_t pci_data_read_common(PCIDevice *pci_dev,
> > +                                     uint32_t config_addr, int len)
> >  {
> > -    PCIBus *s = opaque;
> > -    PCIDevice *pci_dev;
> > -    int config_addr, bus_num;
> >      uint32_t val;
> >  
> > -    bus_num = (addr >> 16) & 0xff;
> > -    s = pci_find_bus(s, bus_num);
> > -    if (!s)
> > -        goto fail;
> > -    pci_dev = s->devices[(addr >> 8) & 0xff];
> > +    assert(len == 1 || len == 2 || len == 4);
> >      if (!pci_dev) {
> > -    fail:
> >          switch(len) {
> >          case 1:
> >              val = 0xff;
> > @@ -679,21 +660,49 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> >              val = 0xffffffff;
> >              break;
> >          }
> > -        goto the_end;
> > +    } else {
> > +        val = pci_dev->config_read(pci_dev, config_addr, len);
> > +        PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> > +                    __func__, pci_dev->name, config_addr, val, len);
> >      }
> > -    config_addr = addr & 0xff;
> > -    val = pci_dev->config_read(pci_dev, config_addr, len);
> > -    PCI_DPRINTF("pci_config_read: %s: "
> > -                "addr=%02x val=%08"PRIx32" len=%d\n",
> > -                pci_dev->name, config_addr, val, len);
> > - the_end:
> > +
> >  #if 0
> > -    PCI_DPRINTF("pci_data_read: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> > -                addr, val, len);
> > +    PCI_DPRINTF("%s: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> > +                __func__, addr, val, len);
> >  #endif
> 
> move to real pci_data_read?
> 
> >      return val;
> >  }
> >  
> > +static inline PCIDevice *pci_addr_to_dev(PCIBus *bus, uint32_t addr)
> > +{
> > +    uint8_t bus_num = (addr >> 16) & 0xff;
> > +    uint8_t devfn = (addr >> 8) & 0xff;
> > +    return pci_find_device(bus, bus_num, PCI_SLOT(devfn), PCI_FUNC(devfn));
> > +}
> > +
> > +static inline uint32_t pci_addr_to_config(uint32_t addr)
> > +{
> > +    return addr & 0xff;
> > +}
> > +
> > +void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > +{
> > +    PCIBus *s = opaque;
> > +#if 0
> > +    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> > +                addr, val, len);
> > +#endif
> > +
> > +    pci_data_write_common(pci_addr_to_dev(s, addr), pci_addr_to_config(addr),
> > +                          val, len);
> 
> For pci express, there's extended register number in bits 11:8
> so what we should do here IMO is check these bits and
> ignore if they are not 0.

I don't understand what you suggest.
pci_data_{write, read}() are glue for ioio or mmio to
access pci configuration space. And 'addr' parameter is used
to specify configuration space access.
It includes not only the offset in configuration space, but also
devfn and bus.


> > +}
> > +
> > +uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> > +{
> > +    PCIBus *s = opaque;
> 
> For pci express, there's extended register number in bits 11:8
> so what we should do here IMO is check these bits and
> return 0 if not 0.
> 
> > +    return pci_data_read_common(pci_addr_to_dev(s, addr),
> > +                                pci_addr_to_config(addr), len);
> > +}
> >  /***********************************************************/
> >  /* generic PCI irq support */
> >  
>
Michael S. Tsirkin - Oct. 6, 2009, 10:23 a.m.
On Tue, Oct 06, 2009 at 06:50:05PM +0900, Isaku Yamahata wrote:
> On Mon, Oct 05, 2009 at 02:45:33PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Oct 05, 2009 at 07:06:54PM +0900, Isaku Yamahata wrote:
> > > factor out conversion logic from io port address into bus+dev+func
> > > with bit shift operation and conversion bus+dev+func into pci device.
> > > They will be used later by pcie support.
> > > This patch also eliminates the logic duplication.
> > 
> > Please split removing logic duplication into separate patch.
> > It might be applicable already, while express stuff needs more work IMO.
> > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > > ---
> > >  hw/pci.c |   83 ++++++++++++++++++++++++++++++++++---------------------------
> > >  1 files changed, 46 insertions(+), 37 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index 6ddd256..5f808ff 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -629,44 +629,25 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
> > >          pci_update_mappings(d);
> > >  }
> > >  
> > > -void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > > +static void pci_data_write_common(PCIDevice *pci_dev,
> > > +                                  uint32_t config_addr, uint32_t val, int len)
> > 
> > So as I said in the previous comment, pci express should just be able to
> > use pci code directly, instead of reusing this low-level stuff.
> > All you need to do is range-check address and ignore extended registers.
> 
> Maybe my patch description isn't good enough,
> I'm afraid that you misunderstood the patch purpose.
> 
> PCI configuration and PCI express MMCONFIG uses different address scheme
> as follows.
> This patch purpose is to separate out its address parsing logic
> from pci config space update logic.
> 
> PCI:
>  0- 7 bit: offset in the configuration space (256bytes)
>  7-15 bit: devfn
> 16-14 bit: bus
> 
> PCI express:
>  0-11 bit: offset in the configuration space (4KBytes)
> 12-19 bit: devfn
> 20-28 bit: bus

The comment is basically that there's no common code:
you don't need len checks that you added for mmconfig (they are there
just for documentation purposes), and so all pci_data_write does is
address parsing.  So just call pci_dev->config_write for both mmconfig
and config.

> > >  {
> > > -    PCIBus *s = opaque;
> > > -    PCIDevice *pci_dev;
> > > -    int config_addr, bus_num;
> > > -
> > > -#if 0
> > > -    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> > > -                addr, val, len);
> > > -#endif
> > > -    bus_num = (addr >> 16) & 0xff;
> > > -    s = pci_find_bus(s, bus_num);
> > > -    if (!s)
> > > -        return;
> > > -    pci_dev = s->devices[(addr >> 8) & 0xff];
> > > +    assert(len == 1 || len == 2 || len == 4);
> > >      if (!pci_dev)
> > >          return;
> > > -    config_addr = addr & 0xff;
> > > -    PCI_DPRINTF("pci_config_write: %s: "
> > > -                "addr=%02x val=%08"PRI32x" len=%d\n",
> > > -                pci_dev->name, config_addr, val, len);
> > > +
> > > +    PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
> > > +                __func__, pci_dev->name, config_addr, val, len);
> > >      pci_dev->config_write(pci_dev, config_addr, val, len);
> > >  }
> > >  
> > > -uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> > > +static uint32_t pci_data_read_common(PCIDevice *pci_dev,
> > > +                                     uint32_t config_addr, int len)
> > >  {
> > > -    PCIBus *s = opaque;
> > > -    PCIDevice *pci_dev;
> > > -    int config_addr, bus_num;
> > >      uint32_t val;
> > >  
> > > -    bus_num = (addr >> 16) & 0xff;
> > > -    s = pci_find_bus(s, bus_num);
> > > -    if (!s)
> > > -        goto fail;
> > > -    pci_dev = s->devices[(addr >> 8) & 0xff];
> > > +    assert(len == 1 || len == 2 || len == 4);
> > >      if (!pci_dev) {
> > > -    fail:
> > >          switch(len) {
> > >          case 1:
> > >              val = 0xff;
> > > @@ -679,21 +660,49 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> > >              val = 0xffffffff;
> > >              break;
> > >          }
> > > -        goto the_end;
> > > +    } else {
> > > +        val = pci_dev->config_read(pci_dev, config_addr, len);
> > > +        PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> > > +                    __func__, pci_dev->name, config_addr, val, len);
> > >      }
> > > -    config_addr = addr & 0xff;
> > > -    val = pci_dev->config_read(pci_dev, config_addr, len);
> > > -    PCI_DPRINTF("pci_config_read: %s: "
> > > -                "addr=%02x val=%08"PRIx32" len=%d\n",
> > > -                pci_dev->name, config_addr, val, len);
> > > - the_end:
> > > +
> > >  #if 0
> > > -    PCI_DPRINTF("pci_data_read: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> > > -                addr, val, len);
> > > +    PCI_DPRINTF("%s: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> > > +                __func__, addr, val, len);
> > >  #endif
> > 
> > move to real pci_data_read?
> > 
> > >      return val;
> > >  }
> > >  
> > > +static inline PCIDevice *pci_addr_to_dev(PCIBus *bus, uint32_t addr)
> > > +{
> > > +    uint8_t bus_num = (addr >> 16) & 0xff;
> > > +    uint8_t devfn = (addr >> 8) & 0xff;
> > > +    return pci_find_device(bus, bus_num, PCI_SLOT(devfn), PCI_FUNC(devfn));
> > > +}
> > > +
> > > +static inline uint32_t pci_addr_to_config(uint32_t addr)
> > > +{
> > > +    return addr & 0xff;
> > > +}
> > > +
> > > +void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > > +{
> > > +    PCIBus *s = opaque;
> > > +#if 0
> > > +    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> > > +                addr, val, len);
> > > +#endif
> > > +
> > > +    pci_data_write_common(pci_addr_to_dev(s, addr), pci_addr_to_config(addr),
> > > +                          val, len);
> > 
> > For pci express, there's extended register number in bits 11:8
> > so what we should do here IMO is check these bits and
> > ignore if they are not 0.
> 
> I don't understand what you suggest.
> pci_data_{write, read}() are glue for ioio or mmio to
> access pci configuration space. And 'addr' parameter is used
> to specify configuration space access.
> It includes not only the offset in configuration space, but also
> devfn and bus.
> 

Okay. I was misled by naming.  What I am saying, here and elsewhere, is
that a large part of pain is because of 256 byte/4K byte difference.
Since in express you can hardware addresses > 256 to 0, I propose that
we simply make mmconfig code do this:

if (offset > 256)
	return 0;

And then basically nothing needs to be changed in pci.

> 
> > > +}
> > > +
> > > +uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> > > +{
> > > +    PCIBus *s = opaque;
> > 
> > For pci express, there's extended register number in bits 11:8
> > so what we should do here IMO is check these bits and
> > return 0 if not 0.
> > 
> > > +    return pci_data_read_common(pci_addr_to_dev(s, addr),
> > > +                                pci_addr_to_config(addr), len);
> > > +}
> > >  /***********************************************************/
> > >  /* generic PCI irq support */
> > >  
> > 
> 
> -- 
> yamahata

Patch

diff --git a/hw/pci.c b/hw/pci.c
index 6ddd256..5f808ff 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -629,44 +629,25 @@  void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
         pci_update_mappings(d);
 }
 
-void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
+static void pci_data_write_common(PCIDevice *pci_dev,
+                                  uint32_t config_addr, uint32_t val, int len)
 {
-    PCIBus *s = opaque;
-    PCIDevice *pci_dev;
-    int config_addr, bus_num;
-
-#if 0
-    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
-                addr, val, len);
-#endif
-    bus_num = (addr >> 16) & 0xff;
-    s = pci_find_bus(s, bus_num);
-    if (!s)
-        return;
-    pci_dev = s->devices[(addr >> 8) & 0xff];
+    assert(len == 1 || len == 2 || len == 4);
     if (!pci_dev)
         return;
-    config_addr = addr & 0xff;
-    PCI_DPRINTF("pci_config_write: %s: "
-                "addr=%02x val=%08"PRI32x" len=%d\n",
-                pci_dev->name, config_addr, val, len);
+
+    PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
+                __func__, pci_dev->name, config_addr, val, len);
     pci_dev->config_write(pci_dev, config_addr, val, len);
 }
 
-uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
+static uint32_t pci_data_read_common(PCIDevice *pci_dev,
+                                     uint32_t config_addr, int len)
 {
-    PCIBus *s = opaque;
-    PCIDevice *pci_dev;
-    int config_addr, bus_num;
     uint32_t val;
 
-    bus_num = (addr >> 16) & 0xff;
-    s = pci_find_bus(s, bus_num);
-    if (!s)
-        goto fail;
-    pci_dev = s->devices[(addr >> 8) & 0xff];
+    assert(len == 1 || len == 2 || len == 4);
     if (!pci_dev) {
-    fail:
         switch(len) {
         case 1:
             val = 0xff;
@@ -679,21 +660,49 @@  uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
             val = 0xffffffff;
             break;
         }
-        goto the_end;
+    } else {
+        val = pci_dev->config_read(pci_dev, config_addr, len);
+        PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
+                    __func__, pci_dev->name, config_addr, val, len);
     }
-    config_addr = addr & 0xff;
-    val = pci_dev->config_read(pci_dev, config_addr, len);
-    PCI_DPRINTF("pci_config_read: %s: "
-                "addr=%02x val=%08"PRIx32" len=%d\n",
-                pci_dev->name, config_addr, val, len);
- the_end:
+
 #if 0
-    PCI_DPRINTF("pci_data_read: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
-                addr, val, len);
+    PCI_DPRINTF("%s: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
+                __func__, addr, val, len);
 #endif
     return val;
 }
 
+static inline PCIDevice *pci_addr_to_dev(PCIBus *bus, uint32_t addr)
+{
+    uint8_t bus_num = (addr >> 16) & 0xff;
+    uint8_t devfn = (addr >> 8) & 0xff;
+    return pci_find_device(bus, bus_num, PCI_SLOT(devfn), PCI_FUNC(devfn));
+}
+
+static inline uint32_t pci_addr_to_config(uint32_t addr)
+{
+    return addr & 0xff;
+}
+
+void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
+{
+    PCIBus *s = opaque;
+#if 0
+    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
+                addr, val, len);
+#endif
+
+    pci_data_write_common(pci_addr_to_dev(s, addr), pci_addr_to_config(addr),
+                          val, len);
+}
+
+uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
+{
+    PCIBus *s = opaque;
+    return pci_data_read_common(pci_addr_to_dev(s, addr),
+                                pci_addr_to_config(addr), len);
+}
 /***********************************************************/
 /* generic PCI irq support */