Patchwork [V6,12/32] pci_host.h: move functions in pci_host.h into .c file.

login
register
mail settings
Submitter Isaku Yamahata
Date Oct. 30, 2009, 12:21 p.m.
Message ID <1256905286-25435-13-git-send-email-yamahata@valinux.co.jp>
Download mbox | patch
Permalink /patch/37289/
State New
Headers show

Comments

Isaku Yamahata - Oct. 30, 2009, 12:21 p.m.
split static functions in pci_host.h into pci_host.c and
pci_host_template.h.
Later a structures declared in pci_host.h, PCIHostState, will be used.
However pci_host.h doesn't allow to include itself easily. This patches
addresses it.

pci_host.h includes functions which are instantiated in .c by including
pci_host.h with typedefing pci_addr_t.
pci_addr_t is per pci host bridge and is typedef'ed to uint32_t for ioio
or target_phys_addr_t for mmio in .c file.
That prevents from including pci_host.h to use PCIHostState because of
requiring type, pci_addr_t.

Its purpose to include is to instantiate io function for mmio or ioio
depending on which pci host bridge requires ioio or mmio.
To avoid including code, we always instantiate both version.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 Makefile.target                        |    2 +-
 hw/apb_pci.c                           |   19 +------
 hw/grackle_pci.c                       |   24 +-------
 hw/gt64xxx.c                           |   11 +----
 hw/pci_host.c                          |   75 ++++++++++++++++++++++++++
 hw/pci_host.h                          |   90 +++-----------------------------
 hw/{pci_host.h => pci_host_template.h} |   36 ++++---------
 hw/piix_pci.c                          |   11 +---
 hw/ppc4xx_pci.c                        |   18 +------
 hw/ppce500_pci.c                       |   17 +------
 hw/prep_pci.c                          |    9 +---
 hw/unin_pci.c                          |   40 ++------------
 12 files changed, 111 insertions(+), 241 deletions(-)
 create mode 100644 hw/pci_host.c
 copy hw/{pci_host.h => pci_host_template.h} (80%)
Michael S. Tsirkin - Nov. 3, 2009, 1:31 p.m.
On Fri, Oct 30, 2009 at 09:21:06PM +0900, Isaku Yamahata wrote:
> split static functions in pci_host.h into pci_host.c and
> pci_host_template.h.
> Later a structures declared in pci_host.h, PCIHostState, will be used.
> However pci_host.h doesn't allow to include itself easily. This patches
> addresses it.
> 
> pci_host.h includes functions which are instantiated in .c by including
> pci_host.h with typedefing pci_addr_t.
> pci_addr_t is per pci host bridge and is typedef'ed to uint32_t for ioio
> or target_phys_addr_t for mmio in .c file.
> That prevents from including pci_host.h to use PCIHostState because of
> requiring type, pci_addr_t.
> 
> Its purpose to include is to instantiate io function for mmio or ioio
> depending on which pci host bridge requires ioio or mmio.
> To avoid including code, we always instantiate both version.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

I think this is good cleanup.  Maybe names can be shorter:
pci_host_data_register_io_memory -> pci_host_register_io_memory
pci_host_data_register_ioport -> pci_host_register_ioport

What do you think?

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  Makefile.target                        |    2 +-
>  hw/apb_pci.c                           |   19 +------
>  hw/grackle_pci.c                       |   24 +-------
>  hw/gt64xxx.c                           |   11 +----
>  hw/pci_host.c                          |   75 ++++++++++++++++++++++++++
>  hw/pci_host.h                          |   90 +++-----------------------------
>  hw/{pci_host.h => pci_host_template.h} |   36 ++++---------
>  hw/piix_pci.c                          |   11 +---
>  hw/ppc4xx_pci.c                        |   18 +------
>  hw/ppce500_pci.c                       |   17 +------
>  hw/prep_pci.c                          |    9 +---
>  hw/unin_pci.c                          |   40 ++------------
>  12 files changed, 111 insertions(+), 241 deletions(-)
>  create mode 100644 hw/pci_host.c
>  copy hw/{pci_host.h => pci_host_template.h} (80%)
> 
> diff --git a/Makefile.target b/Makefile.target
> index fefd7ac..db0d0ab 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -154,7 +154,7 @@ endif #CONFIG_BSD_USER
>  # System emulator target
>  ifdef CONFIG_SOFTMMU
>  
> -obj-y = vl.o async.o monitor.o pci.o machine.o gdbstub.o
> +obj-y = vl.o async.o monitor.o pci.o pci_host.o machine.o gdbstub.o
>  # virtio has to be here due to weird dependency between PCI and virtio-net.
>  # need to fix this properly
>  obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o virtio-pci.o
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 1ea3e0d..560617a 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -28,6 +28,7 @@
>  
>  #include "sysbus.h"
>  #include "pci.h"
> +#include "pci_host.h"
>  
>  /* debug APB */
>  //#define DEBUG_APB
> @@ -48,9 +49,6 @@ do { printf("APB: " fmt , ## __VA_ARGS__); } while (0)
>   * http://www.sun.com/processors/manuals/805-1251.pdf
>   */
>  
> -typedef target_phys_addr_t pci_addr_t;
> -#include "pci_host.h"
> -
>  typedef struct APBState {
>      SysBusDevice busdev;
>      PCIHostState host_state;
> @@ -145,18 +143,6 @@ static CPUReadMemoryFunc * const apb_config_read[] = {
>      &apb_config_readl,
>  };
>  
> -static CPUWriteMemoryFunc * const pci_apb_write[] = {
> -    &pci_host_data_writeb,
> -    &pci_host_data_writew,
> -    &pci_host_data_writel,
> -};
> -
> -static CPUReadMemoryFunc * const pci_apb_read[] = {
> -    &pci_host_data_readb,
> -    &pci_host_data_readw,
> -    &pci_host_data_readl,
> -};
> -
>  static void pci_apb_iowriteb (void *opaque, target_phys_addr_t addr,
>                                    uint32_t val)
>  {
> @@ -293,8 +279,7 @@ static int pci_pbm_init_device(SysBusDevice *dev)
>                                              pci_apb_config_write, s);
>      sysbus_init_mmio(dev, 0x10ULL, pci_mem_config);
>      /* mem_data */
> -    pci_mem_data = cpu_register_io_memory(pci_apb_read,
> -                                          pci_apb_write, &s->host_state);
> +    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
>      sysbus_init_mmio(dev, 0x10000000ULL, pci_mem_data);
>      return 0;
>  }
> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> index b49cf1e..8407cd2 100644
> --- a/hw/grackle_pci.c
> +++ b/hw/grackle_pci.c
> @@ -26,6 +26,7 @@
>  #include "sysbus.h"
>  #include "ppc_mac.h"
>  #include "pci.h"
> +#include "pci_host.h"
>  
>  /* debug Grackle */
>  //#define DEBUG_GRACKLE
> @@ -37,9 +38,6 @@
>  #define GRACKLE_DPRINTF(fmt, ...)
>  #endif
>  
> -typedef target_phys_addr_t pci_addr_t;
> -#include "pci_host.h"
> -
>  typedef struct GrackleState {
>      SysBusDevice busdev;
>      PCIHostState host_state;
> @@ -84,18 +82,6 @@ static CPUReadMemoryFunc * const pci_grackle_config_read[] = {
>      &pci_grackle_config_readl,
>  };
>  
> -static CPUWriteMemoryFunc * const pci_grackle_write[] = {
> -    &pci_host_data_writeb,
> -    &pci_host_data_writew,
> -    &pci_host_data_writel,
> -};
> -
> -static CPUReadMemoryFunc * const pci_grackle_read[] = {
> -    &pci_host_data_readb,
> -    &pci_host_data_readw,
> -    &pci_host_data_readl,
> -};
> -
>  /* Don't know if this matches real hardware, but it agrees with OHW.  */
>  static int pci_grackle_map_irq(PCIDevice *pci_dev, int irq_num)
>  {
> @@ -163,9 +149,7 @@ static int pci_grackle_init_device(SysBusDevice *dev)
>  
>      pci_mem_config = cpu_register_io_memory(pci_grackle_config_read,
>                                              pci_grackle_config_write, s);
> -    pci_mem_data = cpu_register_io_memory(pci_grackle_read,
> -                                          pci_grackle_write,
> -                                          &s->host_state);
> +    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
>  
> @@ -185,9 +169,7 @@ static int pci_dec_21154_init_device(SysBusDevice *dev)
>  
>      pci_mem_config = cpu_register_io_memory(pci_grackle_config_read,
>                                              pci_grackle_config_write, s);
> -    pci_mem_data = cpu_register_io_memory(pci_grackle_read,
> -                                          pci_grackle_write,
> -                                          &s->host_state);
> +    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
>      return 0;
> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> index 8f9ae4a..fb7f5bd 100644
> --- a/hw/gt64xxx.c
> +++ b/hw/gt64xxx.c
> @@ -25,10 +25,8 @@
>  #include "hw.h"
>  #include "mips.h"
>  #include "pci.h"
> -#include "pc.h"
> -
> -typedef target_phys_addr_t pci_addr_t;
>  #include "pci_host.h"
> +#include "pc.h"
>  
>  //#define DEBUG
>  
> @@ -1119,13 +1117,6 @@ PCIBus *pci_gt64120_init(qemu_irq *pic)
>      GT64120State *s;
>      PCIDevice *d;
>  
> -    (void)&pci_host_data_writeb; /* avoid warning */
> -    (void)&pci_host_data_writew; /* avoid warning */
> -    (void)&pci_host_data_writel; /* avoid warning */
> -    (void)&pci_host_data_readb; /* avoid warning */
> -    (void)&pci_host_data_readw; /* avoid warning */
> -    (void)&pci_host_data_readl; /* avoid warning */
> -
>      s = qemu_mallocz(sizeof(GT64120State));
>      s->pci = qemu_mallocz(sizeof(GT64120PCIState));
>  
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> new file mode 100644
> index 0000000..45da1e7
> --- /dev/null
> +++ b/hw/pci_host.c
> @@ -0,0 +1,75 @@
> +/*
> + * pci_host.c
> + *
> + * Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp>
> + *                    VA Linux Systems Japan K.K.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include "pci.h"
> +#include "pci_host.h"
> +
> +/* debug PCI */
> +//#define DEBUG_PCI
> +
> +#ifdef DEBUG_PCI
> +#define PCI_DPRINTF(fmt, ...) \
> +do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define PCI_DPRINTF(fmt, ...)
> +#endif
> +
> +#define PCI_ADDR_T      target_phys_addr_t
> +#define PCI_HOST_SUFFIX _mmio
> +
> +#include "pci_host_template.h"
> +
> +static CPUWriteMemoryFunc * const pci_host_data_write_mmio[] = {
> +    pci_host_data_writeb_mmio,
> +    pci_host_data_writew_mmio,
> +    pci_host_data_writel_mmio,
> +};
> +
> +static CPUReadMemoryFunc * const pci_host_data_read_mmio[] = {
> +    pci_host_data_readb_mmio,
> +    pci_host_data_readw_mmio,
> +    pci_host_data_readl_mmio,
> +};
> +
> +int pci_host_data_register_io_memory(PCIHostState *s)
> +{
> +    return cpu_register_io_memory(pci_host_data_read_mmio,
> +                                  pci_host_data_write_mmio,
> +                                  s);
> +}
> +
> +#undef PCI_ADDR_T
> +#undef PCI_HOST_SUFFIX
> +
> +#define PCI_ADDR_T      uint32_t
> +#define PCI_HOST_SUFFIX _ioport
> +
> +#include "pci_host_template.h"
> +
> +void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s)
> +{
> +    register_ioport_write(ioport, 4, 1, pci_host_data_writeb_ioport, s);
> +    register_ioport_write(ioport, 4, 2, pci_host_data_writew_ioport, s);
> +    register_ioport_write(ioport, 4, 4, pci_host_data_writel_ioport, s);
> +    register_ioport_read(ioport, 4, 1, pci_host_data_readb_ioport, s);
> +    register_ioport_read(ioport, 4, 2, pci_host_data_readw_ioport, s);
> +    register_ioport_read(ioport, 4, 4, pci_host_data_readl_ioport, s);
> +}
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index 48862b5..92a35f9 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -25,97 +25,21 @@
>  /* Worker routines for a PCI host controller that uses an {address,data}
>     register pair to access PCI configuration space.  */
>  
> -/* debug PCI */
> -//#define DEBUG_PCI
> +#ifndef PCI_HOST_H
> +#define PCI_HOST_H
>  
>  #include "sysbus.h"
>  
> -#ifdef DEBUG_PCI
> -#define PCI_DPRINTF(fmt, ...) \
> -do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define PCI_DPRINTF(fmt, ...)
> -#endif
> -
>  typedef struct {
>      SysBusDevice busdev;
>      uint32_t config_reg;
>      PCIBus *bus;
>  } PCIHostState;
>  
> -static void pci_host_data_writeb(void* opaque, pci_addr_t addr, uint32_t val)
> -{
> -    PCIHostState *s = opaque;
> -
> -    PCI_DPRINTF("writeb addr " TARGET_FMT_plx " val %x\n",
> -                (target_phys_addr_t)addr, val);
> -    if (s->config_reg & (1u << 31))
> -        pci_data_write(s->bus, s->config_reg | (addr & 3), val, 1);
> -}
> -
> -static void pci_host_data_writew(void* opaque, pci_addr_t addr, uint32_t val)
> -{
> -    PCIHostState *s = opaque;
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap16(val);
> -#endif
> -    PCI_DPRINTF("writew addr " TARGET_FMT_plx " val %x\n",
> -                (target_phys_addr_t)addr, val);
> -    if (s->config_reg & (1u << 31))
> -        pci_data_write(s->bus, s->config_reg | (addr & 3), val, 2);
> -}
> -
> -static void pci_host_data_writel(void* opaque, pci_addr_t addr, uint32_t val)
> -{
> -    PCIHostState *s = opaque;
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -    PCI_DPRINTF("writel addr " TARGET_FMT_plx " val %x\n",
> -                (target_phys_addr_t)addr, val);
> -    if (s->config_reg & (1u << 31))
> -        pci_data_write(s->bus, s->config_reg, val, 4);
> -}
> -
> -static uint32_t pci_host_data_readb(void* opaque, pci_addr_t addr)
> -{
> -    PCIHostState *s = opaque;
> -    uint32_t val;
> -
> -    if (!(s->config_reg & (1 << 31)))
> -        return 0xff;
> -    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 1);
> -    PCI_DPRINTF("readb addr " TARGET_FMT_plx " val %x\n",
> -                (target_phys_addr_t)addr, val);
> -    return val;
> -}
> +/* for mmio */
> +int pci_host_data_register_io_memory(PCIHostState *s);
>  
> -static uint32_t pci_host_data_readw(void* opaque, pci_addr_t addr)
> -{
> -    PCIHostState *s = opaque;
> -    uint32_t val;
> -    if (!(s->config_reg & (1 << 31)))
> -        return 0xffff;
> -    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 2);
> -    PCI_DPRINTF("readw addr " TARGET_FMT_plx " val %x\n",
> -                (target_phys_addr_t)addr, val);
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap16(val);
> -#endif
> -    return val;
> -}
> +/* for ioio */
> +void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s);
>  
> -static uint32_t pci_host_data_readl(void* opaque, pci_addr_t addr)
> -{
> -    PCIHostState *s = opaque;
> -    uint32_t val;
> -    if (!(s->config_reg & (1 << 31)))
> -        return 0xffffffff;
> -    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 4);
> -    PCI_DPRINTF("readl addr " TARGET_FMT_plx " val %x\n",
> -                (target_phys_addr_t)addr, val);
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -    return val;
> -}
> +#endif /* PCI_HOST_H */
> diff --git a/hw/pci_host.h b/hw/pci_host_template.h
> similarity index 80%
> copy from hw/pci_host.h
> copy to hw/pci_host_template.h
> index 48862b5..11e6c88 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host_template.h
> @@ -25,25 +25,8 @@
>  /* Worker routines for a PCI host controller that uses an {address,data}
>     register pair to access PCI configuration space.  */
>  
> -/* debug PCI */
> -//#define DEBUG_PCI
> -
> -#include "sysbus.h"
> -
> -#ifdef DEBUG_PCI
> -#define PCI_DPRINTF(fmt, ...) \
> -do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> -#else
> -#define PCI_DPRINTF(fmt, ...)
> -#endif
> -
> -typedef struct {
> -    SysBusDevice busdev;
> -    uint32_t config_reg;
> -    PCIBus *bus;
> -} PCIHostState;
> -
> -static void pci_host_data_writeb(void* opaque, pci_addr_t addr, uint32_t val)
> +static void glue(pci_host_data_writeb, PCI_HOST_SUFFIX)(
> +    void* opaque, PCI_ADDR_T addr, uint32_t val)
>  {
>      PCIHostState *s = opaque;
>  
> @@ -53,7 +36,8 @@ static void pci_host_data_writeb(void* opaque, pci_addr_t addr, uint32_t val)
>          pci_data_write(s->bus, s->config_reg | (addr & 3), val, 1);
>  }
>  
> -static void pci_host_data_writew(void* opaque, pci_addr_t addr, uint32_t val)
> +static void glue(pci_host_data_writew, PCI_HOST_SUFFIX)(
> +    void* opaque, PCI_ADDR_T addr, uint32_t val)
>  {
>      PCIHostState *s = opaque;
>  #ifdef TARGET_WORDS_BIGENDIAN
> @@ -65,7 +49,8 @@ static void pci_host_data_writew(void* opaque, pci_addr_t addr, uint32_t val)
>          pci_data_write(s->bus, s->config_reg | (addr & 3), val, 2);
>  }
>  
> -static void pci_host_data_writel(void* opaque, pci_addr_t addr, uint32_t val)
> +static void glue(pci_host_data_writel, PCI_HOST_SUFFIX)(
> +    void* opaque, PCI_ADDR_T addr, uint32_t val)
>  {
>      PCIHostState *s = opaque;
>  #ifdef TARGET_WORDS_BIGENDIAN
> @@ -77,7 +62,8 @@ static void pci_host_data_writel(void* opaque, pci_addr_t addr, uint32_t val)
>          pci_data_write(s->bus, s->config_reg, val, 4);
>  }
>  
> -static uint32_t pci_host_data_readb(void* opaque, pci_addr_t addr)
> +static uint32_t glue(pci_host_data_readb, PCI_HOST_SUFFIX)(
> +    void* opaque, PCI_ADDR_T addr)
>  {
>      PCIHostState *s = opaque;
>      uint32_t val;
> @@ -90,7 +76,8 @@ static uint32_t pci_host_data_readb(void* opaque, pci_addr_t addr)
>      return val;
>  }
>  
> -static uint32_t pci_host_data_readw(void* opaque, pci_addr_t addr)
> +static uint32_t glue(pci_host_data_readw, PCI_HOST_SUFFIX)(
> +    void* opaque, PCI_ADDR_T addr)
>  {
>      PCIHostState *s = opaque;
>      uint32_t val;
> @@ -105,7 +92,8 @@ static uint32_t pci_host_data_readw(void* opaque, pci_addr_t addr)
>      return val;
>  }
>  
> -static uint32_t pci_host_data_readl(void* opaque, pci_addr_t addr)
> +static uint32_t glue(pci_host_data_readl, PCI_HOST_SUFFIX)(
> +    void* opaque, PCI_ADDR_T addr)
>  {
>      PCIHostState *s = opaque;
>      uint32_t val;
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index ed036fe..866348d 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -25,12 +25,10 @@
>  #include "hw.h"
>  #include "pc.h"
>  #include "pci.h"
> +#include "pci_host.h"
>  #include "isa.h"
>  #include "sysbus.h"
>  
> -typedef uint32_t pci_addr_t;
> -#include "pci_host.h"
> -
>  typedef PCIHostState I440FXState;
>  
>  typedef struct PIIX3State {
> @@ -197,12 +195,7 @@ static int i440fx_pcihost_initfn(SysBusDevice *dev)
>      register_ioport_write(0xcf8, 4, 4, i440fx_addr_writel, s);
>      register_ioport_read(0xcf8, 4, 4, i440fx_addr_readl, s);
>  
> -    register_ioport_write(0xcfc, 4, 1, pci_host_data_writeb, s);
> -    register_ioport_write(0xcfc, 4, 2, pci_host_data_writew, s);
> -    register_ioport_write(0xcfc, 4, 4, pci_host_data_writel, s);
> -    register_ioport_read(0xcfc, 4, 1, pci_host_data_readb, s);
> -    register_ioport_read(0xcfc, 4, 2, pci_host_data_readw, s);
> -    register_ioport_read(0xcfc, 4, 4, pci_host_data_readl, s);
> +    pci_host_data_register_ioport(0xcfc, s);
>      return 0;
>  }
>  
> diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
> index 655fe86..3aa7489 100644
> --- a/hw/ppc4xx_pci.c
> +++ b/hw/ppc4xx_pci.c
> @@ -22,8 +22,6 @@
>  #include "hw.h"
>  #include "ppc.h"
>  #include "ppc4xx.h"
> -
> -typedef target_phys_addr_t pci_addr_t;
>  #include "pci.h"
>  #include "pci_host.h"
>  #include "bswap.h"
> @@ -117,18 +115,6 @@ static CPUWriteMemoryFunc * const pci4xx_cfgaddr_write[] = {
>      &pci4xx_cfgaddr_writel,
>  };
>  
> -static CPUReadMemoryFunc * const pci4xx_cfgdata_read[] = {
> -    &pci_host_data_readb,
> -    &pci_host_data_readw,
> -    &pci_host_data_readl,
> -};
> -
> -static CPUWriteMemoryFunc * const pci4xx_cfgdata_write[] = {
> -    &pci_host_data_writeb,
> -    &pci_host_data_writew,
> -    &pci_host_data_writel,
> -};
> -
>  static void ppc4xx_pci_reg_write4(void *opaque, target_phys_addr_t offset,
>                                    uint32_t value)
>  {
> @@ -392,9 +378,7 @@ PCIBus *ppc4xx_pci_init(CPUState *env, qemu_irq pci_irqs[4],
>      cpu_register_physical_memory(config_space + PCIC0_CFGADDR, 4, index);
>  
>      /* CFGDATA */
> -    index = cpu_register_io_memory(pci4xx_cfgdata_read,
> -                                   pci4xx_cfgdata_write,
> -                                   &controller->pci_state);
> +    index = pci_host_data_register_io_memory(&controller->pci_state);
>      if (index < 0)
>          goto free;
>      cpu_register_physical_memory(config_space + PCIC0_CFGDATA, 4, index);
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 64fccfd..7c8cdad 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -17,7 +17,6 @@
>  #include "hw.h"
>  #include "ppc.h"
>  #include "ppce500.h"
> -typedef target_phys_addr_t pci_addr_t;
>  #include "pci.h"
>  #include "pci_host.h"
>  #include "bswap.h"
> @@ -116,18 +115,6 @@ static CPUWriteMemoryFunc * const pcie500_cfgaddr_write[] = {
>      &pcie500_cfgaddr_writel,
>  };
>  
> -static CPUReadMemoryFunc * const pcie500_cfgdata_read[] = {
> -    &pci_host_data_readb,
> -    &pci_host_data_readw,
> -    &pci_host_data_readl,
> -};
> -
> -static CPUWriteMemoryFunc * const pcie500_cfgdata_write[] = {
> -    &pci_host_data_writeb,
> -    &pci_host_data_writew,
> -    &pci_host_data_writel,
> -};
> -
>  static uint32_t pci_reg_read4(void *opaque, target_phys_addr_t addr)
>  {
>      PPCE500PCIState *pci = opaque;
> @@ -344,9 +331,7 @@ PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
>      cpu_register_physical_memory(registers + PCIE500_CFGADDR, 4, index);
>  
>      /* CFGDATA */
> -    index = cpu_register_io_memory(pcie500_cfgdata_read,
> -                                   pcie500_cfgdata_write,
> -                                   &controller->pci_state);
> +    index = pci_host_data_register_io_memory(&controller->pci_state);
>      if (index < 0)
>          goto free;
>      cpu_register_physical_memory(registers + PCIE500_CFGDATA, 4, index);
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 2d8a0fa..5a5b3da 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -24,8 +24,6 @@
>  
>  #include "hw.h"
>  #include "pci.h"
> -
> -typedef uint32_t pci_addr_t;
>  #include "pci_host.h"
>  
>  typedef PCIHostState PREPPCIState;
> @@ -144,12 +142,7 @@ PCIBus *pci_prep_init(qemu_irq *pic)
>      register_ioport_write(0xcf8, 4, 4, pci_prep_addr_writel, s);
>      register_ioport_read(0xcf8, 4, 4, pci_prep_addr_readl, s);
>  
> -    register_ioport_write(0xcfc, 4, 1, pci_host_data_writeb, s);
> -    register_ioport_write(0xcfc, 4, 2, pci_host_data_writew, s);
> -    register_ioport_write(0xcfc, 4, 4, pci_host_data_writel, s);
> -    register_ioport_read(0xcfc, 4, 1, pci_host_data_readb, s);
> -    register_ioport_read(0xcfc, 4, 2, pci_host_data_readw, s);
> -    register_ioport_read(0xcfc, 4, 4, pci_host_data_readl, s);
> +    pci_host_data_register_ioport(0xcfc, s);
>  
>      PPC_io_memory = cpu_register_io_memory(PPC_PCIIO_read,
>                                             PPC_PCIIO_write, s);
> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> index 4abb5c8..6b8f98b 100644
> --- a/hw/unin_pci.c
> +++ b/hw/unin_pci.c
> @@ -24,6 +24,7 @@
>  #include "hw.h"
>  #include "ppc_mac.h"
>  #include "pci.h"
> +#include "pci_host.h"
>  
>  /* debug UniNorth */
>  //#define DEBUG_UNIN
> @@ -35,9 +36,6 @@
>  #define UNIN_DPRINTF(fmt, ...)
>  #endif
>  
> -typedef target_phys_addr_t pci_addr_t;
> -#include "pci_host.h"
> -
>  typedef struct UNINState {
>      SysBusDevice busdev;
>      PCIHostState host_state;
> @@ -83,18 +81,6 @@ static CPUReadMemoryFunc * const pci_unin_main_config_read[] = {
>      &pci_unin_main_config_readl,
>  };
>  
> -static CPUWriteMemoryFunc * const pci_unin_main_write[] = {
> -    &pci_host_data_writeb,
> -    &pci_host_data_writew,
> -    &pci_host_data_writel,
> -};
> -
> -static CPUReadMemoryFunc * const pci_unin_main_read[] = {
> -    &pci_host_data_readb,
> -    &pci_host_data_readw,
> -    &pci_host_data_readl,
> -};
> -
>  static void pci_unin_config_writel (void *opaque, target_phys_addr_t addr,
>                                      uint32_t val)
>  {
> @@ -123,18 +109,6 @@ static CPUReadMemoryFunc * const pci_unin_config_read[] = {
>      &pci_unin_config_readl,
>  };
>  
> -static CPUWriteMemoryFunc * const pci_unin_write[] = {
> -    &pci_host_data_writeb,
> -    &pci_host_data_writew,
> -    &pci_host_data_writel,
> -};
> -
> -static CPUReadMemoryFunc * const pci_unin_read[] = {
> -    &pci_host_data_readb,
> -    &pci_host_data_readw,
> -    &pci_host_data_readl,
> -};
> -
>  /* Don't know if this matches real hardware, but it agrees with OHW.  */
>  static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num)
>  {
> @@ -180,8 +154,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
>  
>      pci_mem_config = cpu_register_io_memory(pci_unin_main_config_read,
>                                              pci_unin_main_config_write, s);
> -    pci_mem_data = cpu_register_io_memory(pci_unin_main_read,
> -                                          pci_unin_main_write, &s->host_state);
> +    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
>  
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
> @@ -203,8 +176,7 @@ static int pci_dec_21154_init_device(SysBusDevice *dev)
>      // XXX: s = &pci_bridge[2];
>      pci_mem_config = cpu_register_io_memory(pci_unin_config_read,
>                                              pci_unin_config_write, s);
> -    pci_mem_data = cpu_register_io_memory(pci_unin_main_read,
> -                                          pci_unin_main_write, &s->host_state);
> +    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
>      return 0;
> @@ -220,8 +192,7 @@ static int pci_unin_agp_init_device(SysBusDevice *dev)
>  
>      pci_mem_config = cpu_register_io_memory(pci_unin_config_read,
>                                              pci_unin_config_write, s);
> -    pci_mem_data = cpu_register_io_memory(pci_unin_main_read,
> -                                          pci_unin_main_write, &s->host_state);
> +    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
>      return 0;
> @@ -237,8 +208,7 @@ static int pci_unin_internal_init_device(SysBusDevice *dev)
>  
>      pci_mem_config = cpu_register_io_memory(pci_unin_config_read,
>                                              pci_unin_config_write, s);
> -    pci_mem_data = cpu_register_io_memory(pci_unin_read,
> -                                          pci_unin_write, s);
> +    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
>      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
>      return 0;
> -- 
> 1.6.0.2
Michael S. Tsirkin - Nov. 3, 2009, 1:35 p.m.
On Tue, Nov 03, 2009 at 03:31:51PM +0200, Michael S. Tsirkin wrote:
> On Fri, Oct 30, 2009 at 09:21:06PM +0900, Isaku Yamahata wrote:
> > split static functions in pci_host.h into pci_host.c and
> > pci_host_template.h.
> > Later a structures declared in pci_host.h, PCIHostState, will be used.
> > However pci_host.h doesn't allow to include itself easily. This patches
> > addresses it.
> > 
> > pci_host.h includes functions which are instantiated in .c by including
> > pci_host.h with typedefing pci_addr_t.
> > pci_addr_t is per pci host bridge and is typedef'ed to uint32_t for ioio
> > or target_phys_addr_t for mmio in .c file.
> > That prevents from including pci_host.h to use PCIHostState because of
> > requiring type, pci_addr_t.
> > 
> > Its purpose to include is to instantiate io function for mmio or ioio
> > depending on which pci host bridge requires ioio or mmio.
> > To avoid including code, we always instantiate both version.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> I think this is good cleanup.  Maybe names can be shorter:
> pci_host_data_register_io_memory -> pci_host_register_io_memory
> pci_host_data_register_ioport -> pci_host_register_ioport
> 
> What do you think?

Or maybe not - we have to avoid confusing this with config...


> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> > ---
> >  Makefile.target                        |    2 +-
> >  hw/apb_pci.c                           |   19 +------
> >  hw/grackle_pci.c                       |   24 +-------
> >  hw/gt64xxx.c                           |   11 +----
> >  hw/pci_host.c                          |   75 ++++++++++++++++++++++++++
> >  hw/pci_host.h                          |   90 +++-----------------------------
> >  hw/{pci_host.h => pci_host_template.h} |   36 ++++---------
> >  hw/piix_pci.c                          |   11 +---
> >  hw/ppc4xx_pci.c                        |   18 +------
> >  hw/ppce500_pci.c                       |   17 +------
> >  hw/prep_pci.c                          |    9 +---
> >  hw/unin_pci.c                          |   40 ++------------
> >  12 files changed, 111 insertions(+), 241 deletions(-)
> >  create mode 100644 hw/pci_host.c
> >  copy hw/{pci_host.h => pci_host_template.h} (80%)
> > 
> > diff --git a/Makefile.target b/Makefile.target
> > index fefd7ac..db0d0ab 100644
> > --- a/Makefile.target
> > +++ b/Makefile.target
> > @@ -154,7 +154,7 @@ endif #CONFIG_BSD_USER
> >  # System emulator target
> >  ifdef CONFIG_SOFTMMU
> >  
> > -obj-y = vl.o async.o monitor.o pci.o machine.o gdbstub.o
> > +obj-y = vl.o async.o monitor.o pci.o pci_host.o machine.o gdbstub.o
> >  # virtio has to be here due to weird dependency between PCI and virtio-net.
> >  # need to fix this properly
> >  obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o virtio-pci.o
> > diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> > index 1ea3e0d..560617a 100644
> > --- a/hw/apb_pci.c
> > +++ b/hw/apb_pci.c
> > @@ -28,6 +28,7 @@
> >  
> >  #include "sysbus.h"
> >  #include "pci.h"
> > +#include "pci_host.h"
> >  
> >  /* debug APB */
> >  //#define DEBUG_APB
> > @@ -48,9 +49,6 @@ do { printf("APB: " fmt , ## __VA_ARGS__); } while (0)
> >   * http://www.sun.com/processors/manuals/805-1251.pdf
> >   */
> >  
> > -typedef target_phys_addr_t pci_addr_t;
> > -#include "pci_host.h"
> > -
> >  typedef struct APBState {
> >      SysBusDevice busdev;
> >      PCIHostState host_state;
> > @@ -145,18 +143,6 @@ static CPUReadMemoryFunc * const apb_config_read[] = {
> >      &apb_config_readl,
> >  };
> >  
> > -static CPUWriteMemoryFunc * const pci_apb_write[] = {
> > -    &pci_host_data_writeb,
> > -    &pci_host_data_writew,
> > -    &pci_host_data_writel,
> > -};
> > -
> > -static CPUReadMemoryFunc * const pci_apb_read[] = {
> > -    &pci_host_data_readb,
> > -    &pci_host_data_readw,
> > -    &pci_host_data_readl,
> > -};
> > -
> >  static void pci_apb_iowriteb (void *opaque, target_phys_addr_t addr,
> >                                    uint32_t val)
> >  {
> > @@ -293,8 +279,7 @@ static int pci_pbm_init_device(SysBusDevice *dev)
> >                                              pci_apb_config_write, s);
> >      sysbus_init_mmio(dev, 0x10ULL, pci_mem_config);
> >      /* mem_data */
> > -    pci_mem_data = cpu_register_io_memory(pci_apb_read,
> > -                                          pci_apb_write, &s->host_state);
> > +    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> >      sysbus_init_mmio(dev, 0x10000000ULL, pci_mem_data);
> >      return 0;
> >  }
> > diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> > index b49cf1e..8407cd2 100644
> > --- a/hw/grackle_pci.c
> > +++ b/hw/grackle_pci.c
> > @@ -26,6 +26,7 @@
> >  #include "sysbus.h"
> >  #include "ppc_mac.h"
> >  #include "pci.h"
> > +#include "pci_host.h"
> >  
> >  /* debug Grackle */
> >  //#define DEBUG_GRACKLE
> > @@ -37,9 +38,6 @@
> >  #define GRACKLE_DPRINTF(fmt, ...)
> >  #endif
> >  
> > -typedef target_phys_addr_t pci_addr_t;
> > -#include "pci_host.h"
> > -
> >  typedef struct GrackleState {
> >      SysBusDevice busdev;
> >      PCIHostState host_state;
> > @@ -84,18 +82,6 @@ static CPUReadMemoryFunc * const pci_grackle_config_read[] = {
> >      &pci_grackle_config_readl,
> >  };
> >  
> > -static CPUWriteMemoryFunc * const pci_grackle_write[] = {
> > -    &pci_host_data_writeb,
> > -    &pci_host_data_writew,
> > -    &pci_host_data_writel,
> > -};
> > -
> > -static CPUReadMemoryFunc * const pci_grackle_read[] = {
> > -    &pci_host_data_readb,
> > -    &pci_host_data_readw,
> > -    &pci_host_data_readl,
> > -};
> > -
> >  /* Don't know if this matches real hardware, but it agrees with OHW.  */
> >  static int pci_grackle_map_irq(PCIDevice *pci_dev, int irq_num)
> >  {
> > @@ -163,9 +149,7 @@ static int pci_grackle_init_device(SysBusDevice *dev)
> >  
> >      pci_mem_config = cpu_register_io_memory(pci_grackle_config_read,
> >                                              pci_grackle_config_write, s);
> > -    pci_mem_data = cpu_register_io_memory(pci_grackle_read,
> > -                                          pci_grackle_write,
> > -                                          &s->host_state);
> > +    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> >      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
> >      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
> >  
> > @@ -185,9 +169,7 @@ static int pci_dec_21154_init_device(SysBusDevice *dev)
> >  
> >      pci_mem_config = cpu_register_io_memory(pci_grackle_config_read,
> >                                              pci_grackle_config_write, s);
> > -    pci_mem_data = cpu_register_io_memory(pci_grackle_read,
> > -                                          pci_grackle_write,
> > -                                          &s->host_state);
> > +    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> >      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
> >      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
> >      return 0;
> > diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> > index 8f9ae4a..fb7f5bd 100644
> > --- a/hw/gt64xxx.c
> > +++ b/hw/gt64xxx.c
> > @@ -25,10 +25,8 @@
> >  #include "hw.h"
> >  #include "mips.h"
> >  #include "pci.h"
> > -#include "pc.h"
> > -
> > -typedef target_phys_addr_t pci_addr_t;
> >  #include "pci_host.h"
> > +#include "pc.h"
> >  
> >  //#define DEBUG
> >  
> > @@ -1119,13 +1117,6 @@ PCIBus *pci_gt64120_init(qemu_irq *pic)
> >      GT64120State *s;
> >      PCIDevice *d;
> >  
> > -    (void)&pci_host_data_writeb; /* avoid warning */
> > -    (void)&pci_host_data_writew; /* avoid warning */
> > -    (void)&pci_host_data_writel; /* avoid warning */
> > -    (void)&pci_host_data_readb; /* avoid warning */
> > -    (void)&pci_host_data_readw; /* avoid warning */
> > -    (void)&pci_host_data_readl; /* avoid warning */
> > -
> >      s = qemu_mallocz(sizeof(GT64120State));
> >      s->pci = qemu_mallocz(sizeof(GT64120PCIState));
> >  
> > diff --git a/hw/pci_host.c b/hw/pci_host.c
> > new file mode 100644
> > index 0000000..45da1e7
> > --- /dev/null
> > +++ b/hw/pci_host.c
> > @@ -0,0 +1,75 @@
> > +/*
> > + * pci_host.c
> > + *
> > + * Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp>
> > + *                    VA Linux Systems Japan K.K.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > +
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > +
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program; if not, write to the Free Software Foundation, Inc.,
> > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> > + */
> > +
> > +#include "pci.h"
> > +#include "pci_host.h"
> > +
> > +/* debug PCI */
> > +//#define DEBUG_PCI
> > +
> > +#ifdef DEBUG_PCI
> > +#define PCI_DPRINTF(fmt, ...) \
> > +do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> > +#else
> > +#define PCI_DPRINTF(fmt, ...)
> > +#endif
> > +
> > +#define PCI_ADDR_T      target_phys_addr_t
> > +#define PCI_HOST_SUFFIX _mmio
> > +
> > +#include "pci_host_template.h"
> > +
> > +static CPUWriteMemoryFunc * const pci_host_data_write_mmio[] = {
> > +    pci_host_data_writeb_mmio,
> > +    pci_host_data_writew_mmio,
> > +    pci_host_data_writel_mmio,
> > +};
> > +
> > +static CPUReadMemoryFunc * const pci_host_data_read_mmio[] = {
> > +    pci_host_data_readb_mmio,
> > +    pci_host_data_readw_mmio,
> > +    pci_host_data_readl_mmio,
> > +};
> > +
> > +int pci_host_data_register_io_memory(PCIHostState *s)
> > +{
> > +    return cpu_register_io_memory(pci_host_data_read_mmio,
> > +                                  pci_host_data_write_mmio,
> > +                                  s);
> > +}
> > +
> > +#undef PCI_ADDR_T
> > +#undef PCI_HOST_SUFFIX
> > +
> > +#define PCI_ADDR_T      uint32_t
> > +#define PCI_HOST_SUFFIX _ioport
> > +
> > +#include "pci_host_template.h"
> > +
> > +void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s)
> > +{
> > +    register_ioport_write(ioport, 4, 1, pci_host_data_writeb_ioport, s);
> > +    register_ioport_write(ioport, 4, 2, pci_host_data_writew_ioport, s);
> > +    register_ioport_write(ioport, 4, 4, pci_host_data_writel_ioport, s);
> > +    register_ioport_read(ioport, 4, 1, pci_host_data_readb_ioport, s);
> > +    register_ioport_read(ioport, 4, 2, pci_host_data_readw_ioport, s);
> > +    register_ioport_read(ioport, 4, 4, pci_host_data_readl_ioport, s);
> > +}
> > diff --git a/hw/pci_host.h b/hw/pci_host.h
> > index 48862b5..92a35f9 100644
> > --- a/hw/pci_host.h
> > +++ b/hw/pci_host.h
> > @@ -25,97 +25,21 @@
> >  /* Worker routines for a PCI host controller that uses an {address,data}
> >     register pair to access PCI configuration space.  */
> >  
> > -/* debug PCI */
> > -//#define DEBUG_PCI
> > +#ifndef PCI_HOST_H
> > +#define PCI_HOST_H
> >  
> >  #include "sysbus.h"
> >  
> > -#ifdef DEBUG_PCI
> > -#define PCI_DPRINTF(fmt, ...) \
> > -do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> > -#else
> > -#define PCI_DPRINTF(fmt, ...)
> > -#endif
> > -
> >  typedef struct {
> >      SysBusDevice busdev;
> >      uint32_t config_reg;
> >      PCIBus *bus;
> >  } PCIHostState;
> >  
> > -static void pci_host_data_writeb(void* opaque, pci_addr_t addr, uint32_t val)
> > -{
> > -    PCIHostState *s = opaque;
> > -
> > -    PCI_DPRINTF("writeb addr " TARGET_FMT_plx " val %x\n",
> > -                (target_phys_addr_t)addr, val);
> > -    if (s->config_reg & (1u << 31))
> > -        pci_data_write(s->bus, s->config_reg | (addr & 3), val, 1);
> > -}
> > -
> > -static void pci_host_data_writew(void* opaque, pci_addr_t addr, uint32_t val)
> > -{
> > -    PCIHostState *s = opaque;
> > -#ifdef TARGET_WORDS_BIGENDIAN
> > -    val = bswap16(val);
> > -#endif
> > -    PCI_DPRINTF("writew addr " TARGET_FMT_plx " val %x\n",
> > -                (target_phys_addr_t)addr, val);
> > -    if (s->config_reg & (1u << 31))
> > -        pci_data_write(s->bus, s->config_reg | (addr & 3), val, 2);
> > -}
> > -
> > -static void pci_host_data_writel(void* opaque, pci_addr_t addr, uint32_t val)
> > -{
> > -    PCIHostState *s = opaque;
> > -#ifdef TARGET_WORDS_BIGENDIAN
> > -    val = bswap32(val);
> > -#endif
> > -    PCI_DPRINTF("writel addr " TARGET_FMT_plx " val %x\n",
> > -                (target_phys_addr_t)addr, val);
> > -    if (s->config_reg & (1u << 31))
> > -        pci_data_write(s->bus, s->config_reg, val, 4);
> > -}
> > -
> > -static uint32_t pci_host_data_readb(void* opaque, pci_addr_t addr)
> > -{
> > -    PCIHostState *s = opaque;
> > -    uint32_t val;
> > -
> > -    if (!(s->config_reg & (1 << 31)))
> > -        return 0xff;
> > -    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 1);
> > -    PCI_DPRINTF("readb addr " TARGET_FMT_plx " val %x\n",
> > -                (target_phys_addr_t)addr, val);
> > -    return val;
> > -}
> > +/* for mmio */
> > +int pci_host_data_register_io_memory(PCIHostState *s);
> >  
> > -static uint32_t pci_host_data_readw(void* opaque, pci_addr_t addr)
> > -{
> > -    PCIHostState *s = opaque;
> > -    uint32_t val;
> > -    if (!(s->config_reg & (1 << 31)))
> > -        return 0xffff;
> > -    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 2);
> > -    PCI_DPRINTF("readw addr " TARGET_FMT_plx " val %x\n",
> > -                (target_phys_addr_t)addr, val);
> > -#ifdef TARGET_WORDS_BIGENDIAN
> > -    val = bswap16(val);
> > -#endif
> > -    return val;
> > -}
> > +/* for ioio */
> > +void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s);
> >  
> > -static uint32_t pci_host_data_readl(void* opaque, pci_addr_t addr)
> > -{
> > -    PCIHostState *s = opaque;
> > -    uint32_t val;
> > -    if (!(s->config_reg & (1 << 31)))
> > -        return 0xffffffff;
> > -    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 4);
> > -    PCI_DPRINTF("readl addr " TARGET_FMT_plx " val %x\n",
> > -                (target_phys_addr_t)addr, val);
> > -#ifdef TARGET_WORDS_BIGENDIAN
> > -    val = bswap32(val);
> > -#endif
> > -    return val;
> > -}
> > +#endif /* PCI_HOST_H */
> > diff --git a/hw/pci_host.h b/hw/pci_host_template.h
> > similarity index 80%
> > copy from hw/pci_host.h
> > copy to hw/pci_host_template.h
> > index 48862b5..11e6c88 100644
> > --- a/hw/pci_host.h
> > +++ b/hw/pci_host_template.h
> > @@ -25,25 +25,8 @@
> >  /* Worker routines for a PCI host controller that uses an {address,data}
> >     register pair to access PCI configuration space.  */
> >  
> > -/* debug PCI */
> > -//#define DEBUG_PCI
> > -
> > -#include "sysbus.h"
> > -
> > -#ifdef DEBUG_PCI
> > -#define PCI_DPRINTF(fmt, ...) \
> > -do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
> > -#else
> > -#define PCI_DPRINTF(fmt, ...)
> > -#endif
> > -
> > -typedef struct {
> > -    SysBusDevice busdev;
> > -    uint32_t config_reg;
> > -    PCIBus *bus;
> > -} PCIHostState;
> > -
> > -static void pci_host_data_writeb(void* opaque, pci_addr_t addr, uint32_t val)
> > +static void glue(pci_host_data_writeb, PCI_HOST_SUFFIX)(
> > +    void* opaque, PCI_ADDR_T addr, uint32_t val)
> >  {
> >      PCIHostState *s = opaque;
> >  
> > @@ -53,7 +36,8 @@ static void pci_host_data_writeb(void* opaque, pci_addr_t addr, uint32_t val)
> >          pci_data_write(s->bus, s->config_reg | (addr & 3), val, 1);
> >  }
> >  
> > -static void pci_host_data_writew(void* opaque, pci_addr_t addr, uint32_t val)
> > +static void glue(pci_host_data_writew, PCI_HOST_SUFFIX)(
> > +    void* opaque, PCI_ADDR_T addr, uint32_t val)
> >  {
> >      PCIHostState *s = opaque;
> >  #ifdef TARGET_WORDS_BIGENDIAN
> > @@ -65,7 +49,8 @@ static void pci_host_data_writew(void* opaque, pci_addr_t addr, uint32_t val)
> >          pci_data_write(s->bus, s->config_reg | (addr & 3), val, 2);
> >  }
> >  
> > -static void pci_host_data_writel(void* opaque, pci_addr_t addr, uint32_t val)
> > +static void glue(pci_host_data_writel, PCI_HOST_SUFFIX)(
> > +    void* opaque, PCI_ADDR_T addr, uint32_t val)
> >  {
> >      PCIHostState *s = opaque;
> >  #ifdef TARGET_WORDS_BIGENDIAN
> > @@ -77,7 +62,8 @@ static void pci_host_data_writel(void* opaque, pci_addr_t addr, uint32_t val)
> >          pci_data_write(s->bus, s->config_reg, val, 4);
> >  }
> >  
> > -static uint32_t pci_host_data_readb(void* opaque, pci_addr_t addr)
> > +static uint32_t glue(pci_host_data_readb, PCI_HOST_SUFFIX)(
> > +    void* opaque, PCI_ADDR_T addr)
> >  {
> >      PCIHostState *s = opaque;
> >      uint32_t val;
> > @@ -90,7 +76,8 @@ static uint32_t pci_host_data_readb(void* opaque, pci_addr_t addr)
> >      return val;
> >  }
> >  
> > -static uint32_t pci_host_data_readw(void* opaque, pci_addr_t addr)
> > +static uint32_t glue(pci_host_data_readw, PCI_HOST_SUFFIX)(
> > +    void* opaque, PCI_ADDR_T addr)
> >  {
> >      PCIHostState *s = opaque;
> >      uint32_t val;
> > @@ -105,7 +92,8 @@ static uint32_t pci_host_data_readw(void* opaque, pci_addr_t addr)
> >      return val;
> >  }
> >  
> > -static uint32_t pci_host_data_readl(void* opaque, pci_addr_t addr)
> > +static uint32_t glue(pci_host_data_readl, PCI_HOST_SUFFIX)(
> > +    void* opaque, PCI_ADDR_T addr)
> >  {
> >      PCIHostState *s = opaque;
> >      uint32_t val;
> > diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> > index ed036fe..866348d 100644
> > --- a/hw/piix_pci.c
> > +++ b/hw/piix_pci.c
> > @@ -25,12 +25,10 @@
> >  #include "hw.h"
> >  #include "pc.h"
> >  #include "pci.h"
> > +#include "pci_host.h"
> >  #include "isa.h"
> >  #include "sysbus.h"
> >  
> > -typedef uint32_t pci_addr_t;
> > -#include "pci_host.h"
> > -
> >  typedef PCIHostState I440FXState;
> >  
> >  typedef struct PIIX3State {
> > @@ -197,12 +195,7 @@ static int i440fx_pcihost_initfn(SysBusDevice *dev)
> >      register_ioport_write(0xcf8, 4, 4, i440fx_addr_writel, s);
> >      register_ioport_read(0xcf8, 4, 4, i440fx_addr_readl, s);
> >  
> > -    register_ioport_write(0xcfc, 4, 1, pci_host_data_writeb, s);
> > -    register_ioport_write(0xcfc, 4, 2, pci_host_data_writew, s);
> > -    register_ioport_write(0xcfc, 4, 4, pci_host_data_writel, s);
> > -    register_ioport_read(0xcfc, 4, 1, pci_host_data_readb, s);
> > -    register_ioport_read(0xcfc, 4, 2, pci_host_data_readw, s);
> > -    register_ioport_read(0xcfc, 4, 4, pci_host_data_readl, s);
> > +    pci_host_data_register_ioport(0xcfc, s);
> >      return 0;
> >  }
> >  
> > diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
> > index 655fe86..3aa7489 100644
> > --- a/hw/ppc4xx_pci.c
> > +++ b/hw/ppc4xx_pci.c
> > @@ -22,8 +22,6 @@
> >  #include "hw.h"
> >  #include "ppc.h"
> >  #include "ppc4xx.h"
> > -
> > -typedef target_phys_addr_t pci_addr_t;
> >  #include "pci.h"
> >  #include "pci_host.h"
> >  #include "bswap.h"
> > @@ -117,18 +115,6 @@ static CPUWriteMemoryFunc * const pci4xx_cfgaddr_write[] = {
> >      &pci4xx_cfgaddr_writel,
> >  };
> >  
> > -static CPUReadMemoryFunc * const pci4xx_cfgdata_read[] = {
> > -    &pci_host_data_readb,
> > -    &pci_host_data_readw,
> > -    &pci_host_data_readl,
> > -};
> > -
> > -static CPUWriteMemoryFunc * const pci4xx_cfgdata_write[] = {
> > -    &pci_host_data_writeb,
> > -    &pci_host_data_writew,
> > -    &pci_host_data_writel,
> > -};
> > -
> >  static void ppc4xx_pci_reg_write4(void *opaque, target_phys_addr_t offset,
> >                                    uint32_t value)
> >  {
> > @@ -392,9 +378,7 @@ PCIBus *ppc4xx_pci_init(CPUState *env, qemu_irq pci_irqs[4],
> >      cpu_register_physical_memory(config_space + PCIC0_CFGADDR, 4, index);
> >  
> >      /* CFGDATA */
> > -    index = cpu_register_io_memory(pci4xx_cfgdata_read,
> > -                                   pci4xx_cfgdata_write,
> > -                                   &controller->pci_state);
> > +    index = pci_host_data_register_io_memory(&controller->pci_state);
> >      if (index < 0)
> >          goto free;
> >      cpu_register_physical_memory(config_space + PCIC0_CFGDATA, 4, index);
> > diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> > index 64fccfd..7c8cdad 100644
> > --- a/hw/ppce500_pci.c
> > +++ b/hw/ppce500_pci.c
> > @@ -17,7 +17,6 @@
> >  #include "hw.h"
> >  #include "ppc.h"
> >  #include "ppce500.h"
> > -typedef target_phys_addr_t pci_addr_t;
> >  #include "pci.h"
> >  #include "pci_host.h"
> >  #include "bswap.h"
> > @@ -116,18 +115,6 @@ static CPUWriteMemoryFunc * const pcie500_cfgaddr_write[] = {
> >      &pcie500_cfgaddr_writel,
> >  };
> >  
> > -static CPUReadMemoryFunc * const pcie500_cfgdata_read[] = {
> > -    &pci_host_data_readb,
> > -    &pci_host_data_readw,
> > -    &pci_host_data_readl,
> > -};
> > -
> > -static CPUWriteMemoryFunc * const pcie500_cfgdata_write[] = {
> > -    &pci_host_data_writeb,
> > -    &pci_host_data_writew,
> > -    &pci_host_data_writel,
> > -};
> > -
> >  static uint32_t pci_reg_read4(void *opaque, target_phys_addr_t addr)
> >  {
> >      PPCE500PCIState *pci = opaque;
> > @@ -344,9 +331,7 @@ PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
> >      cpu_register_physical_memory(registers + PCIE500_CFGADDR, 4, index);
> >  
> >      /* CFGDATA */
> > -    index = cpu_register_io_memory(pcie500_cfgdata_read,
> > -                                   pcie500_cfgdata_write,
> > -                                   &controller->pci_state);
> > +    index = pci_host_data_register_io_memory(&controller->pci_state);
> >      if (index < 0)
> >          goto free;
> >      cpu_register_physical_memory(registers + PCIE500_CFGDATA, 4, index);
> > diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> > index 2d8a0fa..5a5b3da 100644
> > --- a/hw/prep_pci.c
> > +++ b/hw/prep_pci.c
> > @@ -24,8 +24,6 @@
> >  
> >  #include "hw.h"
> >  #include "pci.h"
> > -
> > -typedef uint32_t pci_addr_t;
> >  #include "pci_host.h"
> >  
> >  typedef PCIHostState PREPPCIState;
> > @@ -144,12 +142,7 @@ PCIBus *pci_prep_init(qemu_irq *pic)
> >      register_ioport_write(0xcf8, 4, 4, pci_prep_addr_writel, s);
> >      register_ioport_read(0xcf8, 4, 4, pci_prep_addr_readl, s);
> >  
> > -    register_ioport_write(0xcfc, 4, 1, pci_host_data_writeb, s);
> > -    register_ioport_write(0xcfc, 4, 2, pci_host_data_writew, s);
> > -    register_ioport_write(0xcfc, 4, 4, pci_host_data_writel, s);
> > -    register_ioport_read(0xcfc, 4, 1, pci_host_data_readb, s);
> > -    register_ioport_read(0xcfc, 4, 2, pci_host_data_readw, s);
> > -    register_ioport_read(0xcfc, 4, 4, pci_host_data_readl, s);
> > +    pci_host_data_register_ioport(0xcfc, s);
> >  
> >      PPC_io_memory = cpu_register_io_memory(PPC_PCIIO_read,
> >                                             PPC_PCIIO_write, s);
> > diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> > index 4abb5c8..6b8f98b 100644
> > --- a/hw/unin_pci.c
> > +++ b/hw/unin_pci.c
> > @@ -24,6 +24,7 @@
> >  #include "hw.h"
> >  #include "ppc_mac.h"
> >  #include "pci.h"
> > +#include "pci_host.h"
> >  
> >  /* debug UniNorth */
> >  //#define DEBUG_UNIN
> > @@ -35,9 +36,6 @@
> >  #define UNIN_DPRINTF(fmt, ...)
> >  #endif
> >  
> > -typedef target_phys_addr_t pci_addr_t;
> > -#include "pci_host.h"
> > -
> >  typedef struct UNINState {
> >      SysBusDevice busdev;
> >      PCIHostState host_state;
> > @@ -83,18 +81,6 @@ static CPUReadMemoryFunc * const pci_unin_main_config_read[] = {
> >      &pci_unin_main_config_readl,
> >  };
> >  
> > -static CPUWriteMemoryFunc * const pci_unin_main_write[] = {
> > -    &pci_host_data_writeb,
> > -    &pci_host_data_writew,
> > -    &pci_host_data_writel,
> > -};
> > -
> > -static CPUReadMemoryFunc * const pci_unin_main_read[] = {
> > -    &pci_host_data_readb,
> > -    &pci_host_data_readw,
> > -    &pci_host_data_readl,
> > -};
> > -
> >  static void pci_unin_config_writel (void *opaque, target_phys_addr_t addr,
> >                                      uint32_t val)
> >  {
> > @@ -123,18 +109,6 @@ static CPUReadMemoryFunc * const pci_unin_config_read[] = {
> >      &pci_unin_config_readl,
> >  };
> >  
> > -static CPUWriteMemoryFunc * const pci_unin_write[] = {
> > -    &pci_host_data_writeb,
> > -    &pci_host_data_writew,
> > -    &pci_host_data_writel,
> > -};
> > -
> > -static CPUReadMemoryFunc * const pci_unin_read[] = {
> > -    &pci_host_data_readb,
> > -    &pci_host_data_readw,
> > -    &pci_host_data_readl,
> > -};
> > -
> >  /* Don't know if this matches real hardware, but it agrees with OHW.  */
> >  static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num)
> >  {
> > @@ -180,8 +154,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
> >  
> >      pci_mem_config = cpu_register_io_memory(pci_unin_main_config_read,
> >                                              pci_unin_main_config_write, s);
> > -    pci_mem_data = cpu_register_io_memory(pci_unin_main_read,
> > -                                          pci_unin_main_write, &s->host_state);
> > +    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> >  
> >      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
> >      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
> > @@ -203,8 +176,7 @@ static int pci_dec_21154_init_device(SysBusDevice *dev)
> >      // XXX: s = &pci_bridge[2];
> >      pci_mem_config = cpu_register_io_memory(pci_unin_config_read,
> >                                              pci_unin_config_write, s);
> > -    pci_mem_data = cpu_register_io_memory(pci_unin_main_read,
> > -                                          pci_unin_main_write, &s->host_state);
> > +    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> >      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
> >      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
> >      return 0;
> > @@ -220,8 +192,7 @@ static int pci_unin_agp_init_device(SysBusDevice *dev)
> >  
> >      pci_mem_config = cpu_register_io_memory(pci_unin_config_read,
> >                                              pci_unin_config_write, s);
> > -    pci_mem_data = cpu_register_io_memory(pci_unin_main_read,
> > -                                          pci_unin_main_write, &s->host_state);
> > +    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> >      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
> >      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
> >      return 0;
> > @@ -237,8 +208,7 @@ static int pci_unin_internal_init_device(SysBusDevice *dev)
> >  
> >      pci_mem_config = cpu_register_io_memory(pci_unin_config_read,
> >                                              pci_unin_config_write, s);
> > -    pci_mem_data = cpu_register_io_memory(pci_unin_read,
> > -                                          pci_unin_write, s);
> > +    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> >      sysbus_init_mmio(dev, 0x1000, pci_mem_config);
> >      sysbus_init_mmio(dev, 0x1000, pci_mem_data);
> >      return 0;
> > -- 
> > 1.6.0.2
Isaku Yamahata - Nov. 4, 2009, 4:09 a.m.
On Tue, Nov 03, 2009 at 03:35:12PM +0200, Michael S. Tsirkin wrote:
> On Tue, Nov 03, 2009 at 03:31:51PM +0200, Michael S. Tsirkin wrote:
> > On Fri, Oct 30, 2009 at 09:21:06PM +0900, Isaku Yamahata wrote:
> > > split static functions in pci_host.h into pci_host.c and
> > > pci_host_template.h.
> > > Later a structures declared in pci_host.h, PCIHostState, will be used.
> > > However pci_host.h doesn't allow to include itself easily. This patches
> > > addresses it.
> > > 
> > > pci_host.h includes functions which are instantiated in .c by including
> > > pci_host.h with typedefing pci_addr_t.
> > > pci_addr_t is per pci host bridge and is typedef'ed to uint32_t for ioio
> > > or target_phys_addr_t for mmio in .c file.
> > > That prevents from including pci_host.h to use PCIHostState because of
> > > requiring type, pci_addr_t.
> > > 
> > > Its purpose to include is to instantiate io function for mmio or ioio
> > > depending on which pci host bridge requires ioio or mmio.
> > > To avoid including code, we always instantiate both version.
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > 
> > I think this is good cleanup.  Maybe names can be shorter:
> > pci_host_data_register_io_memory -> pci_host_register_io_memory
> > pci_host_data_register_ioport -> pci_host_register_ioport
> > 
> > What do you think?
> 
> Or maybe not - we have to avoid confusing this with config...

How about the followings?
pci_host_{conf, data}_register_{mmio, mmio_noswap, ioport}()

Patch

diff --git a/Makefile.target b/Makefile.target
index fefd7ac..db0d0ab 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -154,7 +154,7 @@  endif #CONFIG_BSD_USER
 # System emulator target
 ifdef CONFIG_SOFTMMU
 
-obj-y = vl.o async.o monitor.o pci.o machine.o gdbstub.o
+obj-y = vl.o async.o monitor.o pci.o pci_host.o machine.o gdbstub.o
 # virtio has to be here due to weird dependency between PCI and virtio-net.
 # need to fix this properly
 obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-console.o virtio-pci.o
diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index 1ea3e0d..560617a 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -28,6 +28,7 @@ 
 
 #include "sysbus.h"
 #include "pci.h"
+#include "pci_host.h"
 
 /* debug APB */
 //#define DEBUG_APB
@@ -48,9 +49,6 @@  do { printf("APB: " fmt , ## __VA_ARGS__); } while (0)
  * http://www.sun.com/processors/manuals/805-1251.pdf
  */
 
-typedef target_phys_addr_t pci_addr_t;
-#include "pci_host.h"
-
 typedef struct APBState {
     SysBusDevice busdev;
     PCIHostState host_state;
@@ -145,18 +143,6 @@  static CPUReadMemoryFunc * const apb_config_read[] = {
     &apb_config_readl,
 };
 
-static CPUWriteMemoryFunc * const pci_apb_write[] = {
-    &pci_host_data_writeb,
-    &pci_host_data_writew,
-    &pci_host_data_writel,
-};
-
-static CPUReadMemoryFunc * const pci_apb_read[] = {
-    &pci_host_data_readb,
-    &pci_host_data_readw,
-    &pci_host_data_readl,
-};
-
 static void pci_apb_iowriteb (void *opaque, target_phys_addr_t addr,
                                   uint32_t val)
 {
@@ -293,8 +279,7 @@  static int pci_pbm_init_device(SysBusDevice *dev)
                                             pci_apb_config_write, s);
     sysbus_init_mmio(dev, 0x10ULL, pci_mem_config);
     /* mem_data */
-    pci_mem_data = cpu_register_io_memory(pci_apb_read,
-                                          pci_apb_write, &s->host_state);
+    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
     sysbus_init_mmio(dev, 0x10000000ULL, pci_mem_data);
     return 0;
 }
diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
index b49cf1e..8407cd2 100644
--- a/hw/grackle_pci.c
+++ b/hw/grackle_pci.c
@@ -26,6 +26,7 @@ 
 #include "sysbus.h"
 #include "ppc_mac.h"
 #include "pci.h"
+#include "pci_host.h"
 
 /* debug Grackle */
 //#define DEBUG_GRACKLE
@@ -37,9 +38,6 @@ 
 #define GRACKLE_DPRINTF(fmt, ...)
 #endif
 
-typedef target_phys_addr_t pci_addr_t;
-#include "pci_host.h"
-
 typedef struct GrackleState {
     SysBusDevice busdev;
     PCIHostState host_state;
@@ -84,18 +82,6 @@  static CPUReadMemoryFunc * const pci_grackle_config_read[] = {
     &pci_grackle_config_readl,
 };
 
-static CPUWriteMemoryFunc * const pci_grackle_write[] = {
-    &pci_host_data_writeb,
-    &pci_host_data_writew,
-    &pci_host_data_writel,
-};
-
-static CPUReadMemoryFunc * const pci_grackle_read[] = {
-    &pci_host_data_readb,
-    &pci_host_data_readw,
-    &pci_host_data_readl,
-};
-
 /* Don't know if this matches real hardware, but it agrees with OHW.  */
 static int pci_grackle_map_irq(PCIDevice *pci_dev, int irq_num)
 {
@@ -163,9 +149,7 @@  static int pci_grackle_init_device(SysBusDevice *dev)
 
     pci_mem_config = cpu_register_io_memory(pci_grackle_config_read,
                                             pci_grackle_config_write, s);
-    pci_mem_data = cpu_register_io_memory(pci_grackle_read,
-                                          pci_grackle_write,
-                                          &s->host_state);
+    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
     sysbus_init_mmio(dev, 0x1000, pci_mem_config);
     sysbus_init_mmio(dev, 0x1000, pci_mem_data);
 
@@ -185,9 +169,7 @@  static int pci_dec_21154_init_device(SysBusDevice *dev)
 
     pci_mem_config = cpu_register_io_memory(pci_grackle_config_read,
                                             pci_grackle_config_write, s);
-    pci_mem_data = cpu_register_io_memory(pci_grackle_read,
-                                          pci_grackle_write,
-                                          &s->host_state);
+    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
     sysbus_init_mmio(dev, 0x1000, pci_mem_config);
     sysbus_init_mmio(dev, 0x1000, pci_mem_data);
     return 0;
diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
index 8f9ae4a..fb7f5bd 100644
--- a/hw/gt64xxx.c
+++ b/hw/gt64xxx.c
@@ -25,10 +25,8 @@ 
 #include "hw.h"
 #include "mips.h"
 #include "pci.h"
-#include "pc.h"
-
-typedef target_phys_addr_t pci_addr_t;
 #include "pci_host.h"
+#include "pc.h"
 
 //#define DEBUG
 
@@ -1119,13 +1117,6 @@  PCIBus *pci_gt64120_init(qemu_irq *pic)
     GT64120State *s;
     PCIDevice *d;
 
-    (void)&pci_host_data_writeb; /* avoid warning */
-    (void)&pci_host_data_writew; /* avoid warning */
-    (void)&pci_host_data_writel; /* avoid warning */
-    (void)&pci_host_data_readb; /* avoid warning */
-    (void)&pci_host_data_readw; /* avoid warning */
-    (void)&pci_host_data_readl; /* avoid warning */
-
     s = qemu_mallocz(sizeof(GT64120State));
     s->pci = qemu_mallocz(sizeof(GT64120PCIState));
 
diff --git a/hw/pci_host.c b/hw/pci_host.c
new file mode 100644
index 0000000..45da1e7
--- /dev/null
+++ b/hw/pci_host.c
@@ -0,0 +1,75 @@ 
+/*
+ * pci_host.c
+ *
+ * Copyright (c) 2009 Isaku Yamahata <yamahata at valinux co jp>
+ *                    VA Linux Systems Japan K.K.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include "pci.h"
+#include "pci_host.h"
+
+/* debug PCI */
+//#define DEBUG_PCI
+
+#ifdef DEBUG_PCI
+#define PCI_DPRINTF(fmt, ...) \
+do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define PCI_DPRINTF(fmt, ...)
+#endif
+
+#define PCI_ADDR_T      target_phys_addr_t
+#define PCI_HOST_SUFFIX _mmio
+
+#include "pci_host_template.h"
+
+static CPUWriteMemoryFunc * const pci_host_data_write_mmio[] = {
+    pci_host_data_writeb_mmio,
+    pci_host_data_writew_mmio,
+    pci_host_data_writel_mmio,
+};
+
+static CPUReadMemoryFunc * const pci_host_data_read_mmio[] = {
+    pci_host_data_readb_mmio,
+    pci_host_data_readw_mmio,
+    pci_host_data_readl_mmio,
+};
+
+int pci_host_data_register_io_memory(PCIHostState *s)
+{
+    return cpu_register_io_memory(pci_host_data_read_mmio,
+                                  pci_host_data_write_mmio,
+                                  s);
+}
+
+#undef PCI_ADDR_T
+#undef PCI_HOST_SUFFIX
+
+#define PCI_ADDR_T      uint32_t
+#define PCI_HOST_SUFFIX _ioport
+
+#include "pci_host_template.h"
+
+void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s)
+{
+    register_ioport_write(ioport, 4, 1, pci_host_data_writeb_ioport, s);
+    register_ioport_write(ioport, 4, 2, pci_host_data_writew_ioport, s);
+    register_ioport_write(ioport, 4, 4, pci_host_data_writel_ioport, s);
+    register_ioport_read(ioport, 4, 1, pci_host_data_readb_ioport, s);
+    register_ioport_read(ioport, 4, 2, pci_host_data_readw_ioport, s);
+    register_ioport_read(ioport, 4, 4, pci_host_data_readl_ioport, s);
+}
diff --git a/hw/pci_host.h b/hw/pci_host.h
index 48862b5..92a35f9 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host.h
@@ -25,97 +25,21 @@ 
 /* Worker routines for a PCI host controller that uses an {address,data}
    register pair to access PCI configuration space.  */
 
-/* debug PCI */
-//#define DEBUG_PCI
+#ifndef PCI_HOST_H
+#define PCI_HOST_H
 
 #include "sysbus.h"
 
-#ifdef DEBUG_PCI
-#define PCI_DPRINTF(fmt, ...) \
-do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define PCI_DPRINTF(fmt, ...)
-#endif
-
 typedef struct {
     SysBusDevice busdev;
     uint32_t config_reg;
     PCIBus *bus;
 } PCIHostState;
 
-static void pci_host_data_writeb(void* opaque, pci_addr_t addr, uint32_t val)
-{
-    PCIHostState *s = opaque;
-
-    PCI_DPRINTF("writeb addr " TARGET_FMT_plx " val %x\n",
-                (target_phys_addr_t)addr, val);
-    if (s->config_reg & (1u << 31))
-        pci_data_write(s->bus, s->config_reg | (addr & 3), val, 1);
-}
-
-static void pci_host_data_writew(void* opaque, pci_addr_t addr, uint32_t val)
-{
-    PCIHostState *s = opaque;
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap16(val);
-#endif
-    PCI_DPRINTF("writew addr " TARGET_FMT_plx " val %x\n",
-                (target_phys_addr_t)addr, val);
-    if (s->config_reg & (1u << 31))
-        pci_data_write(s->bus, s->config_reg | (addr & 3), val, 2);
-}
-
-static void pci_host_data_writel(void* opaque, pci_addr_t addr, uint32_t val)
-{
-    PCIHostState *s = opaque;
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap32(val);
-#endif
-    PCI_DPRINTF("writel addr " TARGET_FMT_plx " val %x\n",
-                (target_phys_addr_t)addr, val);
-    if (s->config_reg & (1u << 31))
-        pci_data_write(s->bus, s->config_reg, val, 4);
-}
-
-static uint32_t pci_host_data_readb(void* opaque, pci_addr_t addr)
-{
-    PCIHostState *s = opaque;
-    uint32_t val;
-
-    if (!(s->config_reg & (1 << 31)))
-        return 0xff;
-    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 1);
-    PCI_DPRINTF("readb addr " TARGET_FMT_plx " val %x\n",
-                (target_phys_addr_t)addr, val);
-    return val;
-}
+/* for mmio */
+int pci_host_data_register_io_memory(PCIHostState *s);
 
-static uint32_t pci_host_data_readw(void* opaque, pci_addr_t addr)
-{
-    PCIHostState *s = opaque;
-    uint32_t val;
-    if (!(s->config_reg & (1 << 31)))
-        return 0xffff;
-    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 2);
-    PCI_DPRINTF("readw addr " TARGET_FMT_plx " val %x\n",
-                (target_phys_addr_t)addr, val);
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap16(val);
-#endif
-    return val;
-}
+/* for ioio */
+void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s);
 
-static uint32_t pci_host_data_readl(void* opaque, pci_addr_t addr)
-{
-    PCIHostState *s = opaque;
-    uint32_t val;
-    if (!(s->config_reg & (1 << 31)))
-        return 0xffffffff;
-    val = pci_data_read(s->bus, s->config_reg | (addr & 3), 4);
-    PCI_DPRINTF("readl addr " TARGET_FMT_plx " val %x\n",
-                (target_phys_addr_t)addr, val);
-#ifdef TARGET_WORDS_BIGENDIAN
-    val = bswap32(val);
-#endif
-    return val;
-}
+#endif /* PCI_HOST_H */
diff --git a/hw/pci_host.h b/hw/pci_host_template.h
similarity index 80%
copy from hw/pci_host.h
copy to hw/pci_host_template.h
index 48862b5..11e6c88 100644
--- a/hw/pci_host.h
+++ b/hw/pci_host_template.h
@@ -25,25 +25,8 @@ 
 /* Worker routines for a PCI host controller that uses an {address,data}
    register pair to access PCI configuration space.  */
 
-/* debug PCI */
-//#define DEBUG_PCI
-
-#include "sysbus.h"
-
-#ifdef DEBUG_PCI
-#define PCI_DPRINTF(fmt, ...) \
-do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define PCI_DPRINTF(fmt, ...)
-#endif
-
-typedef struct {
-    SysBusDevice busdev;
-    uint32_t config_reg;
-    PCIBus *bus;
-} PCIHostState;
-
-static void pci_host_data_writeb(void* opaque, pci_addr_t addr, uint32_t val)
+static void glue(pci_host_data_writeb, PCI_HOST_SUFFIX)(
+    void* opaque, PCI_ADDR_T addr, uint32_t val)
 {
     PCIHostState *s = opaque;
 
@@ -53,7 +36,8 @@  static void pci_host_data_writeb(void* opaque, pci_addr_t addr, uint32_t val)
         pci_data_write(s->bus, s->config_reg | (addr & 3), val, 1);
 }
 
-static void pci_host_data_writew(void* opaque, pci_addr_t addr, uint32_t val)
+static void glue(pci_host_data_writew, PCI_HOST_SUFFIX)(
+    void* opaque, PCI_ADDR_T addr, uint32_t val)
 {
     PCIHostState *s = opaque;
 #ifdef TARGET_WORDS_BIGENDIAN
@@ -65,7 +49,8 @@  static void pci_host_data_writew(void* opaque, pci_addr_t addr, uint32_t val)
         pci_data_write(s->bus, s->config_reg | (addr & 3), val, 2);
 }
 
-static void pci_host_data_writel(void* opaque, pci_addr_t addr, uint32_t val)
+static void glue(pci_host_data_writel, PCI_HOST_SUFFIX)(
+    void* opaque, PCI_ADDR_T addr, uint32_t val)
 {
     PCIHostState *s = opaque;
 #ifdef TARGET_WORDS_BIGENDIAN
@@ -77,7 +62,8 @@  static void pci_host_data_writel(void* opaque, pci_addr_t addr, uint32_t val)
         pci_data_write(s->bus, s->config_reg, val, 4);
 }
 
-static uint32_t pci_host_data_readb(void* opaque, pci_addr_t addr)
+static uint32_t glue(pci_host_data_readb, PCI_HOST_SUFFIX)(
+    void* opaque, PCI_ADDR_T addr)
 {
     PCIHostState *s = opaque;
     uint32_t val;
@@ -90,7 +76,8 @@  static uint32_t pci_host_data_readb(void* opaque, pci_addr_t addr)
     return val;
 }
 
-static uint32_t pci_host_data_readw(void* opaque, pci_addr_t addr)
+static uint32_t glue(pci_host_data_readw, PCI_HOST_SUFFIX)(
+    void* opaque, PCI_ADDR_T addr)
 {
     PCIHostState *s = opaque;
     uint32_t val;
@@ -105,7 +92,8 @@  static uint32_t pci_host_data_readw(void* opaque, pci_addr_t addr)
     return val;
 }
 
-static uint32_t pci_host_data_readl(void* opaque, pci_addr_t addr)
+static uint32_t glue(pci_host_data_readl, PCI_HOST_SUFFIX)(
+    void* opaque, PCI_ADDR_T addr)
 {
     PCIHostState *s = opaque;
     uint32_t val;
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index ed036fe..866348d 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -25,12 +25,10 @@ 
 #include "hw.h"
 #include "pc.h"
 #include "pci.h"
+#include "pci_host.h"
 #include "isa.h"
 #include "sysbus.h"
 
-typedef uint32_t pci_addr_t;
-#include "pci_host.h"
-
 typedef PCIHostState I440FXState;
 
 typedef struct PIIX3State {
@@ -197,12 +195,7 @@  static int i440fx_pcihost_initfn(SysBusDevice *dev)
     register_ioport_write(0xcf8, 4, 4, i440fx_addr_writel, s);
     register_ioport_read(0xcf8, 4, 4, i440fx_addr_readl, s);
 
-    register_ioport_write(0xcfc, 4, 1, pci_host_data_writeb, s);
-    register_ioport_write(0xcfc, 4, 2, pci_host_data_writew, s);
-    register_ioport_write(0xcfc, 4, 4, pci_host_data_writel, s);
-    register_ioport_read(0xcfc, 4, 1, pci_host_data_readb, s);
-    register_ioport_read(0xcfc, 4, 2, pci_host_data_readw, s);
-    register_ioport_read(0xcfc, 4, 4, pci_host_data_readl, s);
+    pci_host_data_register_ioport(0xcfc, s);
     return 0;
 }
 
diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c
index 655fe86..3aa7489 100644
--- a/hw/ppc4xx_pci.c
+++ b/hw/ppc4xx_pci.c
@@ -22,8 +22,6 @@ 
 #include "hw.h"
 #include "ppc.h"
 #include "ppc4xx.h"
-
-typedef target_phys_addr_t pci_addr_t;
 #include "pci.h"
 #include "pci_host.h"
 #include "bswap.h"
@@ -117,18 +115,6 @@  static CPUWriteMemoryFunc * const pci4xx_cfgaddr_write[] = {
     &pci4xx_cfgaddr_writel,
 };
 
-static CPUReadMemoryFunc * const pci4xx_cfgdata_read[] = {
-    &pci_host_data_readb,
-    &pci_host_data_readw,
-    &pci_host_data_readl,
-};
-
-static CPUWriteMemoryFunc * const pci4xx_cfgdata_write[] = {
-    &pci_host_data_writeb,
-    &pci_host_data_writew,
-    &pci_host_data_writel,
-};
-
 static void ppc4xx_pci_reg_write4(void *opaque, target_phys_addr_t offset,
                                   uint32_t value)
 {
@@ -392,9 +378,7 @@  PCIBus *ppc4xx_pci_init(CPUState *env, qemu_irq pci_irqs[4],
     cpu_register_physical_memory(config_space + PCIC0_CFGADDR, 4, index);
 
     /* CFGDATA */
-    index = cpu_register_io_memory(pci4xx_cfgdata_read,
-                                   pci4xx_cfgdata_write,
-                                   &controller->pci_state);
+    index = pci_host_data_register_io_memory(&controller->pci_state);
     if (index < 0)
         goto free;
     cpu_register_physical_memory(config_space + PCIC0_CFGDATA, 4, index);
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 64fccfd..7c8cdad 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -17,7 +17,6 @@ 
 #include "hw.h"
 #include "ppc.h"
 #include "ppce500.h"
-typedef target_phys_addr_t pci_addr_t;
 #include "pci.h"
 #include "pci_host.h"
 #include "bswap.h"
@@ -116,18 +115,6 @@  static CPUWriteMemoryFunc * const pcie500_cfgaddr_write[] = {
     &pcie500_cfgaddr_writel,
 };
 
-static CPUReadMemoryFunc * const pcie500_cfgdata_read[] = {
-    &pci_host_data_readb,
-    &pci_host_data_readw,
-    &pci_host_data_readl,
-};
-
-static CPUWriteMemoryFunc * const pcie500_cfgdata_write[] = {
-    &pci_host_data_writeb,
-    &pci_host_data_writew,
-    &pci_host_data_writel,
-};
-
 static uint32_t pci_reg_read4(void *opaque, target_phys_addr_t addr)
 {
     PPCE500PCIState *pci = opaque;
@@ -344,9 +331,7 @@  PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
     cpu_register_physical_memory(registers + PCIE500_CFGADDR, 4, index);
 
     /* CFGDATA */
-    index = cpu_register_io_memory(pcie500_cfgdata_read,
-                                   pcie500_cfgdata_write,
-                                   &controller->pci_state);
+    index = pci_host_data_register_io_memory(&controller->pci_state);
     if (index < 0)
         goto free;
     cpu_register_physical_memory(registers + PCIE500_CFGDATA, 4, index);
diff --git a/hw/prep_pci.c b/hw/prep_pci.c
index 2d8a0fa..5a5b3da 100644
--- a/hw/prep_pci.c
+++ b/hw/prep_pci.c
@@ -24,8 +24,6 @@ 
 
 #include "hw.h"
 #include "pci.h"
-
-typedef uint32_t pci_addr_t;
 #include "pci_host.h"
 
 typedef PCIHostState PREPPCIState;
@@ -144,12 +142,7 @@  PCIBus *pci_prep_init(qemu_irq *pic)
     register_ioport_write(0xcf8, 4, 4, pci_prep_addr_writel, s);
     register_ioport_read(0xcf8, 4, 4, pci_prep_addr_readl, s);
 
-    register_ioport_write(0xcfc, 4, 1, pci_host_data_writeb, s);
-    register_ioport_write(0xcfc, 4, 2, pci_host_data_writew, s);
-    register_ioport_write(0xcfc, 4, 4, pci_host_data_writel, s);
-    register_ioport_read(0xcfc, 4, 1, pci_host_data_readb, s);
-    register_ioport_read(0xcfc, 4, 2, pci_host_data_readw, s);
-    register_ioport_read(0xcfc, 4, 4, pci_host_data_readl, s);
+    pci_host_data_register_ioport(0xcfc, s);
 
     PPC_io_memory = cpu_register_io_memory(PPC_PCIIO_read,
                                            PPC_PCIIO_write, s);
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index 4abb5c8..6b8f98b 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -24,6 +24,7 @@ 
 #include "hw.h"
 #include "ppc_mac.h"
 #include "pci.h"
+#include "pci_host.h"
 
 /* debug UniNorth */
 //#define DEBUG_UNIN
@@ -35,9 +36,6 @@ 
 #define UNIN_DPRINTF(fmt, ...)
 #endif
 
-typedef target_phys_addr_t pci_addr_t;
-#include "pci_host.h"
-
 typedef struct UNINState {
     SysBusDevice busdev;
     PCIHostState host_state;
@@ -83,18 +81,6 @@  static CPUReadMemoryFunc * const pci_unin_main_config_read[] = {
     &pci_unin_main_config_readl,
 };
 
-static CPUWriteMemoryFunc * const pci_unin_main_write[] = {
-    &pci_host_data_writeb,
-    &pci_host_data_writew,
-    &pci_host_data_writel,
-};
-
-static CPUReadMemoryFunc * const pci_unin_main_read[] = {
-    &pci_host_data_readb,
-    &pci_host_data_readw,
-    &pci_host_data_readl,
-};
-
 static void pci_unin_config_writel (void *opaque, target_phys_addr_t addr,
                                     uint32_t val)
 {
@@ -123,18 +109,6 @@  static CPUReadMemoryFunc * const pci_unin_config_read[] = {
     &pci_unin_config_readl,
 };
 
-static CPUWriteMemoryFunc * const pci_unin_write[] = {
-    &pci_host_data_writeb,
-    &pci_host_data_writew,
-    &pci_host_data_writel,
-};
-
-static CPUReadMemoryFunc * const pci_unin_read[] = {
-    &pci_host_data_readb,
-    &pci_host_data_readw,
-    &pci_host_data_readl,
-};
-
 /* Don't know if this matches real hardware, but it agrees with OHW.  */
 static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num)
 {
@@ -180,8 +154,7 @@  static int pci_unin_main_init_device(SysBusDevice *dev)
 
     pci_mem_config = cpu_register_io_memory(pci_unin_main_config_read,
                                             pci_unin_main_config_write, s);
-    pci_mem_data = cpu_register_io_memory(pci_unin_main_read,
-                                          pci_unin_main_write, &s->host_state);
+    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
 
     sysbus_init_mmio(dev, 0x1000, pci_mem_config);
     sysbus_init_mmio(dev, 0x1000, pci_mem_data);
@@ -203,8 +176,7 @@  static int pci_dec_21154_init_device(SysBusDevice *dev)
     // XXX: s = &pci_bridge[2];
     pci_mem_config = cpu_register_io_memory(pci_unin_config_read,
                                             pci_unin_config_write, s);
-    pci_mem_data = cpu_register_io_memory(pci_unin_main_read,
-                                          pci_unin_main_write, &s->host_state);
+    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
     sysbus_init_mmio(dev, 0x1000, pci_mem_config);
     sysbus_init_mmio(dev, 0x1000, pci_mem_data);
     return 0;
@@ -220,8 +192,7 @@  static int pci_unin_agp_init_device(SysBusDevice *dev)
 
     pci_mem_config = cpu_register_io_memory(pci_unin_config_read,
                                             pci_unin_config_write, s);
-    pci_mem_data = cpu_register_io_memory(pci_unin_main_read,
-                                          pci_unin_main_write, &s->host_state);
+    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
     sysbus_init_mmio(dev, 0x1000, pci_mem_config);
     sysbus_init_mmio(dev, 0x1000, pci_mem_data);
     return 0;
@@ -237,8 +208,7 @@  static int pci_unin_internal_init_device(SysBusDevice *dev)
 
     pci_mem_config = cpu_register_io_memory(pci_unin_config_read,
                                             pci_unin_config_write, s);
-    pci_mem_data = cpu_register_io_memory(pci_unin_read,
-                                          pci_unin_write, s);
+    pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
     sysbus_init_mmio(dev, 0x1000, pci_mem_config);
     sysbus_init_mmio(dev, 0x1000, pci_mem_data);
     return 0;