Re: [Qemu-devel] [PATCH 7/9] pci: factor out the logic to get pci device from address.

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

On Wed, Jul 15, 2009 at 08:15:07PM +0900, Isaku Yamahata wrote:
> factor out conversion logic from io port address into bus+dev+func
> with bit shift operation and conversion bus+dev+func into pci device.
> They will be used later.
> This patch also eliminates the logic duplication.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |  105 ++++++++++++++++++++++++++++++++++---------------------------
>  1 files changed, 58 insertions(+), 47 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index f06e1da..3b19c3d 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -584,71 +584,82 @@ static PCIBus *pci_find_bus_from(PCIBus *from, int bus_num)
>      return s;
>  }
>  
> -void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> +static PCIDevice *pci_bdf_to_dev(PCIBus *s, int bus_num, unsigned int devfn)

This name is pretty bad.

>  {
> -    PCIBus *s = opaque;
> -    PCIDevice *pci_dev;
> -    int config_addr, bus_num;
> -
> -#if 0
> -    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> -                addr, val, len);
> -#endif
> -    bus_num = (addr >> 16) & 0xff;
>      s = pci_find_bus_from(s, bus_num);
>      if (!s)
> -        return;
> -    pci_dev = s->devices[(addr >> 8) & 0xff];
> +        return NULL;
> +
> +    return s->devices[devfn];
> +}
> +static void pci_dev_data_write(PCIDevice *pci_dev,
> +                               uint32_t config_addr, uint32_t val, int len)


There seems to be 1 caller, and it's confusing to
have 2 functions with basically the same nhame.
Please open-code it.

> +{
> +    assert(len == 1 || len == 2 || len == 4);
>      if (!pci_dev)
>          return;
> -    config_addr = addr & 0xff;
> -    PCI_DPRINTF("pci_config_write: %s: "
> -                "addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
> -                pci_dev->name, config_addr, val, len);
> +
> +    PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRI32x" len=%d\n",
> +                __func__, pci_dev->name, config_addr, val, len);
>      pci_dev->config_write(pci_dev, config_addr, val, len);
>  }
>  
> -uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> +static uint32_t pci_dev_data_read(PCIDevice *pci_dev,
> +                                  uint32_t config_addr, int len)
>  {
> -    PCIBus *s = opaque;
> -    PCIDevice *pci_dev;
> -    int config_addr, bus_num;
>      uint32_t val;
>  
> -    bus_num = (addr >> 16) & 0xff;
> -    s = pci_find_bus_from(s, bus_num);
> -    if (!s)
> -        goto fail;
> -    pci_dev = s->devices[(addr >> 8) & 0xff];
> +    assert(len == 1 || len == 2 || len == 4);
>      if (!pci_dev) {
> -    fail:
> -        switch(len) {
> -        case 1:
> -            val = 0xff;
> -            break;
> -        case 2:
> -            val = 0xffff;
> -            break;
> -        default:
> -        case 4:
> -            val = 0xffffffff;
> -            break;
> -        }
> -        goto the_end;
> +        val = (1 << (len * 8)) - 1;

You can't do this: len * 8 will be 32 if len is 4,
and shift by 32 is undefined in C.

> +    } else {
> +        val = pci_dev->config_read(pci_dev, config_addr, len);
> +        PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> +                    __func__, pci_dev->name, config_addr, val, len);
>      }
> -    config_addr = addr & 0xff;
> -    val = pci_dev->config_read(pci_dev, config_addr, len);
> -    PCI_DPRINTF("pci_config_read: %s: "
> -                "addr=%02"PRIx32" val=%08"PRIx32" len=%d\n",
> -                pci_dev->name, config_addr, val, len);
> - the_end:
> +
>  #if 0
> -    PCI_DPRINTF("pci_data_read: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> -                addr, val, len);
> +    PCI_DPRINTF("%s: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> +                __func__, addr, val, len);
>  #endif
>      return val;
>  }
>  
> +static void pci_addr_to_dev(PCIBus *s, uint32_t addr,
> +                            PCIDevice **pci_dev, uint32_t *config_addr)
> +{
> +    int bus_num = (addr >> 16) & 0xff;
> +    unsigned int devfn = (addr >> 8) & 0xff;
> +
> +    *pci_dev = pci_bdf_to_dev(s, bus_num, devfn);
> +    *config_addr = addr & 0xff;
> +}

Just open-code this function.
If you really must, add ADDR_TO_DEVFN and ADDR_TO_BUS macros.

> +void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> +{
> +    PCIBus *s = opaque;
> +    PCIDevice *pci_dev;
> +    uint32_t config_addr;
> +
> +#if 0
> +    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" len=%d\n",
> +                addr, val, len);
> +#endif
> +
> +    pci_addr_to_dev(s, addr, &pci_dev, &config_addr);

It's better to have 2 incline functions that return a single
value each here.

> +    pci_dev_data_write(pci_dev, config_addr, val, len);
> +}
> +
> +uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> +{
> +    PCIBus *s = opaque;
> +    PCIDevice *pci_dev;
> +    uint32_t config_addr;
> +
> +    pci_addr_to_dev(s, addr, &pci_dev, &config_addr);
> +    return pci_dev_data_read(pci_dev, config_addr, len);
> +}
>  /***********************************************************/
>  /* generic PCI irq support */
>  
> -- 
> 1.6.0.2
> 
>