[Qemu-devel] Re: [PATCH V6 13/32] pci_host: consolidate pci config address access.

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Oct 30, 2009 at 09:21:07PM +0900, Isaku Yamahata wrote:
> consolidate pci_config address access into pci_host.c
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/apb_pci.c     |   43 +---------------------
>  hw/grackle_pci.c |   45 +---------------------
>  hw/pci_host.c    |  108 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pci_host.h    |    3 +
>  hw/piix_pci.c    |   15 +-------
>  hw/ppce500_pci.c |   34 +----------------
>  hw/prep_pci.c    |   15 +-------
>  hw/unin_pci.c    |   81 ++--------------------------------------
>  8 files changed, 121 insertions(+), 223 deletions(-)
> 
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 560617a..3999879 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -54,46 +54,6 @@ typedef struct APBState {
>      PCIHostState host_state;
>  } APBState;
>  
> -static void pci_apb_config_writel (void *opaque, target_phys_addr_t addr,
> -                                         uint32_t val)
> -{
> -    APBState *s = opaque;
> -
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -    APB_DPRINTF("config_writel addr " TARGET_FMT_plx " val %x\n", addr,
> -                val);
> -    s->host_state.config_reg = val;
> -}
> -
> -static uint32_t pci_apb_config_readl (void *opaque,
> -                                            target_phys_addr_t addr)
> -{
> -    APBState *s = opaque;
> -    uint32_t val;
> -
> -    val = s->host_state.config_reg;
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -    APB_DPRINTF("config_readl addr " TARGET_FMT_plx " val %x\n", addr,
> -                val);
> -    return val;
> -}
> -
> -static CPUWriteMemoryFunc * const pci_apb_config_write[] = {
> -    &pci_apb_config_writel,
> -    &pci_apb_config_writel,
> -    &pci_apb_config_writel,
> -};
> -
> -static CPUReadMemoryFunc * const pci_apb_config_read[] = {
> -    &pci_apb_config_readl,
> -    &pci_apb_config_readl,
> -    &pci_apb_config_readl,
> -};
> -
>  static void apb_config_writel (void *opaque, target_phys_addr_t addr,
>                                 uint32_t val)
>  {
> @@ -275,8 +235,7 @@ static int pci_pbm_init_device(SysBusDevice *dev)
>                                            pci_apb_iowrite, s);
>      sysbus_init_mmio(dev, 0x10000ULL, pci_ioport);
>      /* mem_config  */
> -    pci_mem_config = cpu_register_io_memory(pci_apb_config_read,
> -                                            pci_apb_config_write, s);
> +    pci_mem_config = pci_host_config_register_io_memory(&s->host_state);
>      sysbus_init_mmio(dev, 0x10ULL, pci_mem_config);
>      /* mem_data */
>      pci_mem_data = pci_host_data_register_io_memory(&s->host_state);
> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
> index 8407cd2..58dcd11 100644
> --- a/hw/grackle_pci.c
> +++ b/hw/grackle_pci.c
> @@ -43,45 +43,6 @@ typedef struct GrackleState {
>      PCIHostState host_state;
>  } GrackleState;
>  
> -static void pci_grackle_config_writel (void *opaque, target_phys_addr_t addr,
> -                                       uint32_t val)
> -{
> -    GrackleState *s = opaque;
> -
> -    GRACKLE_DPRINTF("config_writel addr " TARGET_FMT_plx " val %x\n", addr,
> -                    val);
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -    s->host_state.config_reg = val;
> -}
> -
> -static uint32_t pci_grackle_config_readl (void *opaque, target_phys_addr_t addr)
> -{
> -    GrackleState *s = opaque;
> -    uint32_t val;
> -
> -    val = s->host_state.config_reg;
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -    GRACKLE_DPRINTF("config_readl addr " TARGET_FMT_plx " val %x\n", addr,
> -                    val);
> -    return val;
> -}
> -
> -static CPUWriteMemoryFunc * const pci_grackle_config_write[] = {
> -    &pci_grackle_config_writel,
> -    &pci_grackle_config_writel,
> -    &pci_grackle_config_writel,
> -};
> -
> -static CPUReadMemoryFunc * const pci_grackle_config_read[] = {
> -    &pci_grackle_config_readl,
> -    &pci_grackle_config_readl,
> -    &pci_grackle_config_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)
>  {
> @@ -147,8 +108,7 @@ static int pci_grackle_init_device(SysBusDevice *dev)
>  
>      s = FROM_SYSBUS(GrackleState, dev);
>  
> -    pci_mem_config = cpu_register_io_memory(pci_grackle_config_read,
> -                                            pci_grackle_config_write, s);
> +    pci_mem_config = pci_host_config_register_io_memory(&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);
> @@ -167,8 +127,7 @@ static int pci_dec_21154_init_device(SysBusDevice *dev)
>  
>      s = FROM_SYSBUS(GrackleState, dev);
>  
> -    pci_mem_config = cpu_register_io_memory(pci_grackle_config_read,
> -                                            pci_grackle_config_write, s);
> +    pci_mem_config = pci_host_config_register_io_memory(&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);
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index 45da1e7..6009e37 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -32,6 +32,114 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } while (0)
>  #define PCI_DPRINTF(fmt, ...)
>  #endif
>  
> +static void pci_host_config_writel(void *opaque, target_phys_addr_t addr,
> +                                   uint32_t val)
> +{
> +    PCIHostState *s = opaque;
> +
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    val = bswap32(val);
> +#endif

I know you just copied it, but isn't this just
	val = le32_to_cpu(val);

?

> +    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> +                __func__, addr, val);
> +    s->config_reg = val;
> +}
> +
> +static uint32_t pci_host_config_readl(void *opaque, target_phys_addr_t addr)
> +{
> +    PCIHostState *s = opaque;
> +    uint32_t val = s->config_reg;
> +
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    val = bswap32(val);
> +#endif

same here.

> +    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> +                __func__, addr, val);
> +    return val;
> +}
> +
> +static CPUWriteMemoryFunc * const pci_host_config_write[] = {
> +    &pci_host_config_writel,
> +    &pci_host_config_writel,
> +    &pci_host_config_writel,
> +};
> +
> +static CPUReadMemoryFunc * const pci_host_config_read[] = {
> +    &pci_host_config_readl,
> +    &pci_host_config_readl,
> +    &pci_host_config_readl,
> +};
> +
> +int pci_host_config_register_io_memory(PCIHostState *s)
> +{
> +    return cpu_register_io_memory(pci_host_config_read,
> +                                  pci_host_config_write, s);
> +}
> +
> +static void pci_host_config_writel_noswap(void *opaque,
> +                                          target_phys_addr_t addr,
> +                                          uint32_t val)
> +{
> +    PCIHostState *s = opaque;
> +
> +    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> +                __func__, addr, val);
> +    s->config_reg = val;
> +}
> +
> +static uint32_t pci_host_config_readl_noswap(void *opaque,
> +                                             target_phys_addr_t addr)
> +{
> +    PCIHostState *s = opaque;
> +    uint32_t val = s->config_reg;
> +
> +    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> +                __func__, addr, val);
> +    return val;
> +}
> +
> +static CPUWriteMemoryFunc * const pci_host_config_write_noswap[] = {
> +    &pci_host_config_writel_noswap,
> +    &pci_host_config_writel_noswap,
> +    &pci_host_config_writel_noswap,
> +};
> +
> +static CPUReadMemoryFunc * const pci_host_config_read_noswap[] = {
> +    &pci_host_config_readl_noswap,
> +    &pci_host_config_readl_noswap,
> +    &pci_host_config_readl_noswap,
> +};
> +
> +int pci_host_config_register_io_memory_noswap(PCIHostState *s)

This name is clearly too long,
as a result when you use it you run over the 80 character
limit. Let's not fix it, let's make name shorter.
io_memory -> memory?

> +{
> +    return cpu_register_io_memory(pci_host_config_read_noswap,
> +                                  pci_host_config_write_noswap, s);
> +}

Do you happen to know whether no swap is really needed?  I suspect some
of these places really only happen to work because everyone uses intel,
so they simply would not work on ppc host.

> +
> +static void pci_host_config_writel_ioport(void *opaque,
> +                                          uint32_t addr, uint32_t val)
> +{
> +    PCIHostState *s = opaque;
> +
> +    PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr, val);
> +    s->config_reg = val;
> +}
> +
> +static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t addr)
> +{
> +    PCIHostState *s = opaque;
> +    uint32_t val = s->config_reg;
> +
> +    PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr, val);
> +    return val;
> +}
> +
> +void pci_host_config_register_ioport(pio_addr_t ioport, PCIHostState *s)
> +{
> +    register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport, s);
> +    register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s);
> +}
> +
>  #define PCI_ADDR_T      target_phys_addr_t
>  #define PCI_HOST_SUFFIX _mmio
>  
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index 92a35f9..e5e877f 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -37,9 +37,12 @@ typedef struct {
>  } PCIHostState;
>  
>  /* for mmio */
> +int pci_host_config_register_io_memory(PCIHostState *s);
> +int pci_host_config_register_io_memory_noswap(PCIHostState *s);
>  int pci_host_data_register_io_memory(PCIHostState *s);
>  
>  /* for ioio */
> +void pci_host_config_register_ioport(pio_addr_t ioport, PCIHostState *s);
>  void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s);
>  
>  #endif /* PCI_HOST_H */
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 866348d..bf0005e 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -44,18 +44,6 @@ struct PCII440FXState {
>      PIIX3State *piix3;
>  };
>  
> -static void i440fx_addr_writel(void* opaque, uint32_t addr, uint32_t val)
> -{
> -    I440FXState *s = opaque;
> -    s->config_reg = val;
> -}
> -
> -static uint32_t i440fx_addr_readl(void* opaque, uint32_t addr)
> -{
> -    I440FXState *s = opaque;
> -    return s->config_reg;
> -}
> -
>  static void piix3_set_irq(void *opaque, int irq_num, int level);
>  
>  /* return the global irq number corresponding to a given device irq
> @@ -192,8 +180,7 @@ static int i440fx_pcihost_initfn(SysBusDevice *dev)
>  {
>      I440FXState *s = FROM_SYSBUS(I440FXState, dev);
>  
> -    register_ioport_write(0xcf8, 4, 4, i440fx_addr_writel, s);
> -    register_ioport_read(0xcf8, 4, 4, i440fx_addr_readl, s);
> +    pci_host_config_register_ioport(0xcf8, s);
>  
>      pci_host_data_register_ioport(0xcfc, s);
>      return 0;
> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 7c8cdad..223de3a 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -84,37 +84,6 @@ struct PPCE500PCIState {
>  
>  typedef struct PPCE500PCIState PPCE500PCIState;
>  
> -static uint32_t pcie500_cfgaddr_readl(void *opaque, target_phys_addr_t addr)
> -{
> -    PPCE500PCIState *pci = opaque;
> -
> -    pci_debug("%s: (addr:" TARGET_FMT_plx ") -> value:%x\n", __func__, addr,
> -              pci->pci_state.config_reg);
> -    return pci->pci_state.config_reg;
> -}
> -
> -static CPUReadMemoryFunc * const pcie500_cfgaddr_read[] = {
> -    &pcie500_cfgaddr_readl,
> -    &pcie500_cfgaddr_readl,
> -    &pcie500_cfgaddr_readl,
> -};
> -
> -static void pcie500_cfgaddr_writel(void *opaque, target_phys_addr_t addr,
> -                                  uint32_t value)
> -{
> -    PPCE500PCIState *controller = opaque;
> -
> -    pci_debug("%s: value:%x -> (addr:" TARGET_FMT_plx ")\n", __func__, value,
> -              addr);
> -    controller->pci_state.config_reg = value & ~0x3;
> -}
> -
> -static CPUWriteMemoryFunc * const pcie500_cfgaddr_write[] = {
> -    &pcie500_cfgaddr_writel,
> -    &pcie500_cfgaddr_writel,
> -    &pcie500_cfgaddr_writel,
> -};
> -
>  static uint32_t pci_reg_read4(void *opaque, target_phys_addr_t addr)
>  {
>      PPCE500PCIState *pci = opaque;
> @@ -324,8 +293,7 @@ PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers)
>      controller->pci_dev = d;
>  
>      /* CFGADDR */
> -    index = cpu_register_io_memory(pcie500_cfgaddr_read,
> -                                   pcie500_cfgaddr_write, controller);
> +    index = pci_host_config_register_io_memory_noswap(&controller->pci_state);
>      if (index < 0)
>          goto free;
>      cpu_register_physical_memory(registers + PCIE500_CFGADDR, 4, index);
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 5a5b3da..a338f81 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -28,18 +28,6 @@
>  
>  typedef PCIHostState PREPPCIState;
>  
> -static void pci_prep_addr_writel(void* opaque, uint32_t addr, uint32_t val)
> -{
> -    PREPPCIState *s = opaque;
> -    s->config_reg = val;
> -}
> -
> -static uint32_t pci_prep_addr_readl(void* opaque, uint32_t addr)
> -{
> -    PREPPCIState *s = opaque;
> -    return s->config_reg;
> -}
> -
>  static inline uint32_t PPC_PCIIO_config(target_phys_addr_t addr)
>  {
>      int i;
> @@ -139,8 +127,7 @@ PCIBus *pci_prep_init(qemu_irq *pic)
>      s->bus = pci_register_bus(NULL, "pci",
>                                prep_set_irq, prep_map_irq, pic, 0, 4);
>  
> -    register_ioport_write(0xcf8, 4, 4, pci_prep_addr_writel, s);
> -    register_ioport_read(0xcf8, 4, 4, pci_prep_addr_readl, s);
> +    pci_host_config_register_ioport(0xcf8, s);
>  
>      pci_host_data_register_ioport(0xcfc, s);
>  
> diff --git a/hw/unin_pci.c b/hw/unin_pci.c
> index 6b8f98b..a9a62fd 100644
> --- a/hw/unin_pci.c
> +++ b/hw/unin_pci.c
> @@ -41,74 +41,6 @@ typedef struct UNINState {
>      PCIHostState host_state;
>  } UNINState;
>  
> -static void pci_unin_main_config_writel (void *opaque, target_phys_addr_t addr,
> -                                         uint32_t val)
> -{
> -    UNINState *s = opaque;
> -
> -    UNIN_DPRINTF("config_writel addr " TARGET_FMT_plx " val %x\n", addr, val);
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -
> -    s->host_state.config_reg = val;
> -}
> -
> -static uint32_t pci_unin_main_config_readl (void *opaque,
> -                                            target_phys_addr_t addr)
> -{
> -    UNINState *s = opaque;
> -    uint32_t val;
> -
> -    val = s->host_state.config_reg;
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
> -    UNIN_DPRINTF("config_readl addr " TARGET_FMT_plx " val %x\n", addr, val);
> -
> -    return val;
> -}
> -
> -static CPUWriteMemoryFunc * const pci_unin_main_config_write[] = {
> -    &pci_unin_main_config_writel,
> -    &pci_unin_main_config_writel,
> -    &pci_unin_main_config_writel,
> -};
> -
> -static CPUReadMemoryFunc * const pci_unin_main_config_read[] = {
> -    &pci_unin_main_config_readl,
> -    &pci_unin_main_config_readl,
> -    &pci_unin_main_config_readl,
> -};
> -
> -static void pci_unin_config_writel (void *opaque, target_phys_addr_t addr,
> -                                    uint32_t val)
> -{
> -    UNINState *s = opaque;
> -
> -    s->host_state.config_reg = val;
> -}
> -
> -static uint32_t pci_unin_config_readl (void *opaque,
> -                                       target_phys_addr_t addr)
> -{
> -    UNINState *s = opaque;
> -
> -    return s->host_state.config_reg;
> -}
> -
> -static CPUWriteMemoryFunc * const pci_unin_config_write[] = {
> -    &pci_unin_config_writel,
> -    &pci_unin_config_writel,
> -    &pci_unin_config_writel,
> -};
> -
> -static CPUReadMemoryFunc * const pci_unin_config_read[] = {
> -    &pci_unin_config_readl,
> -    &pci_unin_config_readl,
> -    &pci_unin_config_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)
>  {
> @@ -152,10 +84,8 @@ static int pci_unin_main_init_device(SysBusDevice *dev)
>      /* Uninorth main bus */
>      s = FROM_SYSBUS(UNINState, dev);
>  
> -    pci_mem_config = cpu_register_io_memory(pci_unin_main_config_read,
> -                                            pci_unin_main_config_write, s);
> +    pci_mem_config = pci_host_config_register_io_memory(&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);
>  
> @@ -174,8 +104,7 @@ static int pci_dec_21154_init_device(SysBusDevice *dev)
>      s = FROM_SYSBUS(UNINState, dev);
>  
>      // XXX: s = &pci_bridge[2];
> -    pci_mem_config = cpu_register_io_memory(pci_unin_config_read,
> -                                            pci_unin_config_write, s);
> +    pci_mem_config = pci_host_config_register_io_memory_noswap(&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);
> @@ -190,8 +119,7 @@ static int pci_unin_agp_init_device(SysBusDevice *dev)
>      /* Uninorth AGP bus */
>      s = FROM_SYSBUS(UNINState, dev);
>  
> -    pci_mem_config = cpu_register_io_memory(pci_unin_config_read,
> -                                            pci_unin_config_write, s);
> +    pci_mem_config = pci_host_config_register_io_memory_noswap(&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);
> @@ -206,8 +134,7 @@ static int pci_unin_internal_init_device(SysBusDevice *dev)
>      /* Uninorth internal bus */
>      s = FROM_SYSBUS(UNINState, dev);
>  
> -    pci_mem_config = cpu_register_io_memory(pci_unin_config_read,
> -                                            pci_unin_config_write, s);
> +    pci_mem_config = pci_host_config_register_io_memory_noswap(&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);
> -- 
> 1.6.0.2