[Qemu-devel] Re: [PATCH 6/6] pci host: make pci_data_{write, read}() get PCIConfigAddress.

[Michael S. Tsirkin]

On Tue, Jan 12, 2010 at 05:52:58PM +0900, Isaku Yamahata wrote:
> Move out pci address decoding logic from pci_data_{write, read}()
> by making pci_data_{write, read}() get PCIConfigAddress.
> 
> Cc: Blue Swirl <address@hidden>
> Cc: Aurelien Jarno <address@hidden>
> Cc: Paul Brook <address@hidden>
> Signed-off-by: Isaku Yamahata <address@hidden>
> ---
>  hw/apb_pci.c           |   12 ++++++++----
>  hw/gt64xxx.c           |   20 ++++++++++++--------
>  hw/pci_host.c          |   25 ++++++++++++++-----------
>  hw/pci_host.h          |    5 +++--
>  hw/pci_host_template.h |   15 +++++++--------
>  hw/prep_pci.c          |   28 ++++++++++++++++++++++------
>  hw/sh_pci.c            |   24 ++++++++++++++++++++----
>  hw/versatile_pci.c     |   30 ++++++++++++++++++++++++------
>  8 files changed, 110 insertions(+), 49 deletions(-)
> 
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index c14e1c0..3c098d2 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -107,18 +107,22 @@ static CPUReadMemoryFunc * const apb_config_read[] = {
>  static void apb_pci_config_write(APBState *s, target_phys_addr_t addr,
>                                   uint32_t val, int size)
>  {
> +    PCIConfigAddress conf_addr;
>      APB_DPRINTF("%s: addr " TARGET_FMT_lx " val %x\n", __func__, addr, val);
> -    pci_data_write(s->host_state.bus, (addr & 0x00ffffff) | (1u << 31), val,
> -                   size);
> +    pci_host_decode_config_addr_valid(&s->host_state, (addr & 0x00ffffff),
> +                                      &conf_addr);
> +    pci_data_write(&conf_addr, 0, val, size);
>  }
>  
>  static uint32_t apb_pci_config_read(APBState *s, target_phys_addr_t addr,
>                                      int size)
>  {
> +    PCIConfigAddress conf_addr;
>      uint32_t ret;
>  
> -    ret = pci_data_read(s->host_state.bus, (addr & 0x00ffffff) | (1u << 31),
> -                        size);
> +    pci_host_decode_config_addr_valid(&s->host_state, (addr & 0x00ffffff),
> +                                      &conf_addr);
> +    ret = pci_data_read(&conf_addr, 0, size);
>      APB_DPRINTF("%s: addr " TARGET_FMT_lx " -> %x\n", __func__, addr, ret);
>      return ret;
>  }
> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> index c8034e2..2e135dc 100644
> --- a/hw/gt64xxx.c
> +++ b/hw/gt64xxx.c
> @@ -527,12 +527,15 @@ static void gt64120_writel (void *opaque, 
> target_phys_addr_t addr,
>      case GT_PCI0_CFGADDR:
>          s->pci->config_reg = val & 0x80fffffc;
>          break;
> -    case GT_PCI0_CFGDATA:
> +    case GT_PCI0_CFGDATA: {
> +        PCIConfigAddress conf_addr;
>          if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci->config_reg & 0x00fff800))
>              val = bswap32(val);
> -        if (s->pci->config_reg & (1u << 31))
> -            pci_data_write(s->pci->bus, s->pci->config_reg, val, 4);
> +        pci_host_decode_config_addr_cfge(s->pci, s->pci->config_reg,
> +                                         &conf_addr);
> +        pci_data_write(&conf_addr, 0, val, 4);
>          break;
> +    }
>  
>      /* Interrupts */
>      case GT_INTRCAUSE:
> @@ -767,14 +770,15 @@ static uint32_t gt64120_readl (void *opaque,
>      case GT_PCI0_CFGADDR:
>          val = s->pci->config_reg;
>          break;
> -    case GT_PCI0_CFGDATA:
> -        if (!(s->pci->config_reg & (1 << 31)))
> -            val = 0xffffffff;
> -        else
> -            val = pci_data_read(s->pci->bus, s->pci->config_reg, 4);
> +    case GT_PCI0_CFGDATA: {
> +        PCIConfigAddress conf_addr;
> +        pci_host_decode_config_addr_cfge(s->pci, s->pci->config_reg,
> +                                         &conf_addr);
> +        val = pci_data_read(&conf_addr, 0, 4);
>          if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci->config_reg & 0x00fff800))
>              val = bswap32(val);
>          break;
> +    }
>  
>      case GT_PCI0_CMD:
>      case GT_PCI0_TOR:
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index fa194e2..c837110 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -72,18 +72,21 @@ void pci_host_decode_config_addr_valid(const PCIHostState 
> *s,
>  }
>  
>  /* the helper functio to get a PCIDeice* for a given pci address */
> -static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
> +static inline PCIDevice *pci_dev_find_by_addr(
> +    const PCIConfigAddress *conf_addr)
>  {
> -    uint8_t bus_num = addr >> 16;
> -    uint8_t devfn = addr >> 8;
> -
> -    return pci_find_device(bus, bus_num, PCI_SLOT(devfn), PCI_FUNC(devfn));
> +    const PCIAddress *addr = &conf_addr->addr;
> +    if (!conf_addr->valid) {
> +        return NULL;
> +    }
> +    return pci_find_device(addr->domain, addr->bus, addr->slot, addr->fn);
>  }
>  
> -void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
> +void pci_data_write(PCIConfigAddress *conf_addr,
> +                    uint32_t addr, uint32_t val, int len)
>  {
> -    PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> -    uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> +    PCIDevice *pci_dev = pci_dev_find_by_addr(conf_addr);
> +    uint32_t config_addr = conf_addr->offset | (conf_addr->addr_mask & addr);
>  
>      if (!pci_dev)
>          return;
> @@ -93,10 +96,10 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t 
> val, int len)
>      pci_dev->config_write(pci_dev, config_addr, val, len);
>  }
>  
> -uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> +uint32_t pci_data_read(PCIConfigAddress *conf_addr, uint32_t addr, int len)
>  {
> -    PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> -    uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> +    PCIDevice *pci_dev = pci_dev_find_by_addr(conf_addr);
> +    uint32_t config_addr = conf_addr->offset | (conf_addr->addr_mask & addr);
>      uint32_t val;
>  
>      assert(len == 1 || len == 2 || len == 4);
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index ebc95f2..d2d558d 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -52,8 +52,9 @@ void pci_host_decode_config_addr_valid(const PCIHostState 
> *s,
>                                         uint32_t config_reg,
>                                         PCIConfigAddress *decoded);
>  
> -void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
> -uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
> +void pci_data_write(PCIConfigAddress *conf_addr,
> +                    uint32_t addr, uint32_t val, int len);
> +uint32_t pci_data_read(PCIConfigAddress *conf_addr, uint32_t addr, int len);
>  
>  /* for mmio */
>  int pci_host_conf_register_mmio(PCIHostState *s);
> diff --git a/hw/pci_host_template.h b/hw/pci_host_template.h
> index 2c508bf..02f0ff1 100644
> --- a/hw/pci_host_template.h
> +++ b/hw/pci_host_template.h
> @@ -29,28 +29,27 @@ static void glue(glue(pci_host_data_write, PCI_HOST_BWL), 
> PCI_HOST_SUFFIX)(
>      void* opaque, PCI_ADDR_T addr, uint32_t val)
>  {
>      PCIHostState *s = opaque;
> +    PCIConfigAddress conf_addr;
>  
>  #if defined(TARGET_WORDS_BIGENDIAN) && (PCI_HOST_BITS > 8)
>      val = glue(bswap, PCI_HOST_BITS)(val);
>  #endif
>      PCI_DPRINTF("%s: addr " TARGET_FMT_plx " val %x\n",
>                  __func__, (target_phys_addr_t)addr, val);
> -    if (s->config_reg & (1u << 31))
> -        pci_data_write(s->bus, s->config_reg | (addr & 3), val,
> -                       sizeof(glue(glue(uint, PCI_HOST_BITS), _t)));
> +    pci_host_decode_config_addr_cfge(s, s->config_reg, &conf_addr);
> +    pci_data_write(&conf_addr, addr, val,
> +                   sizeof(glue(glue(uint, PCI_HOST_BITS), _t)));
>  }
>  
>  static uint32_t glue(glue(pci_host_data_read, PCI_HOST_BWL), 
> PCI_HOST_SUFFIX)(
>      void* opaque, PCI_ADDR_T addr)
>  {
>      PCIHostState *s = opaque;
> +    PCIConfigAddress conf_addr;
>      uint32_t val;
>  
> -    if (!(s->config_reg & (1 << 31))) {
> -        return ( 1ULL << PCI_HOST_BITS ) - 1;
> -    }
> -
> -    val = pci_data_read(s->bus, s->config_reg | (addr & 3),
> +    pci_host_decode_config_addr_cfge(s, s->config_reg, &conf_addr);
> +    val = pci_data_read(&conf_addr, addr,
>                          sizeof(glue(glue(uint, PCI_HOST_BITS), _t)));
>      PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %x\n",
>                  __func__, (target_phys_addr_t)addr, val);
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 19f028c..53862d7 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -40,10 +40,26 @@ static inline uint32_t 
> PPC_PCIIO_config(target_phys_addr_t addr)
>      return (addr & 0x7ff) |  (i << 11);
>  }
>  
> +static void PPC_pci_data_write(PREPPCIState *s, target_phys_addr_t addr,
> +                               uint32_t val, int len)
> +{
> +    PCIConfigAddress conf_addr;
> +    pci_host_decode_config_addr_valid(s, PPC_PCIIO_config(addr), &conf_addr);
> +    pci_data_write(&conf_addr, 0, val, len);
> +}
> +
> +static uint32_t PPC_pci_data_read(PREPPCIState *s, target_phys_addr_t addr,
> +                                  int len)
> +{
> +    PCIConfigAddress conf_addr;
> +    pci_host_decode_config_addr_valid(s, PPC_PCIIO_config(addr), &conf_addr);
> +    return pci_data_read(&conf_addr, 0, len);
> +}
> +
>  static void PPC_PCIIO_writeb (void *opaque, target_phys_addr_t addr, 
> uint32_t val)
>  {
>      PREPPCIState *s = opaque;
> -    pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 1);
> +    PPC_pci_data_write(s, addr, val, 1);
>  }
>  
>  static void PPC_PCIIO_writew (void *opaque, target_phys_addr_t addr, 
> uint32_t val)
> @@ -52,7 +68,7 @@ static void PPC_PCIIO_writew (void *opaque, 
> target_phys_addr_t addr, uint32_t va
>  #ifdef TARGET_WORDS_BIGENDIAN
>      val = bswap16(val);
>  #endif
> -    pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 2);
> +    PPC_pci_data_write(s, addr, val, 2);
>  }
>  
>  static void PPC_PCIIO_writel (void *opaque, target_phys_addr_t addr, 
> uint32_t val)
> @@ -61,14 +77,14 @@ static void PPC_PCIIO_writel (void *opaque, 
> target_phys_addr_t addr, uint32_t va
>  #ifdef TARGET_WORDS_BIGENDIAN
>      val = bswap32(val);
>  #endif
> -    pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 4);
> +    PPC_pci_data_write(s, addr, val, 4);
>  }
>  
>  static uint32_t PPC_PCIIO_readb (void *opaque, target_phys_addr_t addr)
>  {
>      PREPPCIState *s = opaque;
>      uint32_t val;
> -    val = pci_data_read(s->bus, PPC_PCIIO_config(addr), 1);
> +    val = PPC_pci_data_read(s, addr, 1);
>      return val;
>  }
>  
> @@ -76,7 +92,7 @@ static uint32_t PPC_PCIIO_readw (void *opaque, 
> target_phys_addr_t addr)
>  {
>      PREPPCIState *s = opaque;
>      uint32_t val;
> -    val = pci_data_read(s->bus, PPC_PCIIO_config(addr), 2);
> +    val = PPC_pci_data_read(s, addr, 2);
>  #ifdef TARGET_WORDS_BIGENDIAN
>      val = bswap16(val);
>  #endif
> @@ -87,7 +103,7 @@ static uint32_t PPC_PCIIO_readl (void *opaque, 
> target_phys_addr_t addr)
>  {
>      PREPPCIState *s = opaque;
>      uint32_t val;
> -    val = pci_data_read(s->bus, PPC_PCIIO_config(addr), 4);
> +    val = PPC_pci_data_read(s, addr, 4);
>  #ifdef TARGET_WORDS_BIGENDIAN
>      val = bswap32(val);
>  #endif
> diff --git a/hw/sh_pci.c b/hw/sh_pci.c
> index 046db2e..2a8f132 100644
> --- a/hw/sh_pci.c
> +++ b/hw/sh_pci.c
> @@ -36,6 +36,22 @@ typedef struct {
>      uint32_t iobr;
>  } SHPCIC;
>  
> +static void sh_pci_data_write(SHPCIC *pcic, target_phys_addr_t addr,
> +                              uint32_t val, int size)
> +{
> +    PCIConfigAddress conf_addr;
> +    pci_host_decode_config_addr_valid(&pcic->s, addr, &conf_addr);
> +    pci_data_write(&conf_addr, 0, val, size);
> +}
> +
> +static uint32_t sh_pci_data_read(SHPCIC *pcic,
> +                                 target_phys_addr_t addr, int size)
> +{
> +    PCIConfigAddress conf_addr;
> +    pci_host_decode_config_addr_valid(&pcic->s, addr, &conf_addr);
> +    return pci_data_read(&conf_addr, 0, size);
> +}
> +
>  static void sh_pci_reg_write (void *p, target_phys_addr_t addr, uint32_t val)
>  {
>      SHPCIC *pcic = p;
> @@ -53,7 +69,7 @@ static void sh_pci_reg_write (void *p, target_phys_addr_t 
> addr, uint32_t val)
>          pcic->iobr = val;
>          break;
>      case 0x220:
> -        pci_data_write(pcic->s.bus, pcic->par, val, 4);
> +        sh_pci_data_write(pcic, pcic->par, val, 4);
>          break;
>      }
>  }
> @@ -67,7 +83,7 @@ static uint32_t sh_pci_reg_read (void *p, 
> target_phys_addr_t addr)
>      case 0x1c0:
>          return pcic->par;
>      case 0x220:
> -        return pci_data_read(pcic->s.bus, pcic->par, 4);
> +        return sh_pci_data_read(pcic, pcic->par, 4);
>      }
>      return 0;
>  }
> @@ -75,13 +91,13 @@ static uint32_t sh_pci_reg_read (void *p, 
> target_phys_addr_t addr)
>  static void sh_pci_mem_write (SHPCIC *pcic, target_phys_addr_t addr,
>                                 uint32_t val, int size)
>  {
> -    pci_data_write(pcic->s.bus, addr + pcic->mbr, val, size);
> +    sh_pci_data_write(pcic, addr + pcic->mbr, val, size);
>  }
>  
>  static uint32_t sh_pci_mem_read (SHPCIC *pcic, target_phys_addr_t addr,
>                                   int size)
>  {
> -    return pci_data_read(pcic->s.bus, addr + pcic->mbr, size);
> +    return sh_pci_data_read(pcic, addr + pcic->mbr, size);
>  }
>  
>  static void sh_pci_writeb (void *p, target_phys_addr_t addr, uint32_t val)
> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
> index d99b7fa..14a728a 100644
> --- a/hw/versatile_pci.c
> +++ b/hw/versatile_pci.c
> @@ -23,11 +23,29 @@ static inline uint32_t 
> vpb_pci_config_addr(target_phys_addr_t addr)
>      return addr & 0xffffff;
>  }
>  
> +static void vpb_pci_data_write(PCIHostState *s, target_phys_addr_t addr,
> +                               uint32_t val, int len)
> +{
> +    PCIConfigAddress conf_addr;
> +    pci_host_decode_config_addr_valid(s, vpb_pci_config_addr(addr),
> +                                      &conf_addr);
> +    pci_data_write(&conf_addr, 0, val, len);
> +}
> +
> +static uint32_t vpb_pci_data_read(PCIHostState *s, target_phys_addr_t addr,
> +                                  int len)
> +{
> +    PCIConfigAddress conf_addr;
> +    pci_host_decode_config_addr_valid(s, vpb_pci_config_addr(addr),
> +                                      &conf_addr);
> +    return pci_data_read(&conf_addr, 0, len);
> +}
> +


I thought we will get rid of vpb_pci_config_addr, and fill in
fields in PCIConfigAddress directly.  If we don't, and still
recode into PC format, this is not making code any prettier
so I don't really see what this buys us.

>  static void pci_vpb_config_writeb (void *opaque, target_phys_addr_t addr,
>                                     uint32_t val)
>  {
>      PCIHostState *s = opaque;
> -    pci_data_write(s->bus, vpb_pci_config_addr (addr), val, 1);
> +    vpb_pci_data_write(s, addr, val, 1);
>  }
>  
>  static void pci_vpb_config_writew (void *opaque, target_phys_addr_t addr,
> @@ -37,7 +55,7 @@ static void pci_vpb_config_writew (void *opaque, 
> target_phys_addr_t addr,
>  #ifdef TARGET_WORDS_BIGENDIAN
>      val = bswap16(val);
>  #endif
> -    pci_data_write(s->bus, vpb_pci_config_addr (addr), val, 2);
> +    vpb_pci_data_write(s, addr, val, 2);
>  }
>  
>  static void pci_vpb_config_writel (void *opaque, target_phys_addr_t addr,
> @@ -47,14 +65,14 @@ static void pci_vpb_config_writel (void *opaque, 
> target_phys_addr_t addr,
>  #ifdef TARGET_WORDS_BIGENDIAN
>      val = bswap32(val);
>  #endif
> -    pci_data_write(s->bus, vpb_pci_config_addr (addr), val, 4);
> +    vpb_pci_data_write(s, addr, val, 4);
>  }
>  
>  static uint32_t pci_vpb_config_readb (void *opaque, target_phys_addr_t addr)
>  {
>      PCIHostState *s = opaque;
>      uint32_t val;
> -    val = pci_data_read(s->bus, vpb_pci_config_addr (addr), 1);
> +    val = vpb_pci_data_read(s, addr, 1);
>      return val;
>  }
>  
> @@ -62,7 +80,7 @@ static uint32_t pci_vpb_config_readw (void *opaque, 
> target_phys_addr_t addr)
>  {
>      PCIHostState *s = opaque;
>      uint32_t val;
> -    val = pci_data_read(s->bus, vpb_pci_config_addr (addr), 2);
> +    val = vpb_pci_data_read(s, addr, 2);
>  #ifdef TARGET_WORDS_BIGENDIAN
>      val = bswap16(val);
>  #endif
> @@ -73,7 +91,7 @@ static uint32_t pci_vpb_config_readl (void *opaque, 
> target_phys_addr_t addr)
>  {
>      PCIHostState *s = opaque;
>      uint32_t val;
> -    val = pci_data_read(s->bus, vpb_pci_config_addr (addr), 4);
> +    val = vpb_pci_data_read(s, addr, 4);
>  #ifdef TARGET_WORDS_BIGENDIAN
>      val = bswap32(val);
>  #endif
> -- 
> 1.6.5.4