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

[Isaku Yamahata]

On Wed, Sep 30, 2009 at 01:30:01PM +0200, Michael S. Tsirkin wrote:
> 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 <address@hidden>
> > ---
> >  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.

pci_find_device() is almost same function.
Will replace it with pci_find_device().

> 
> >  {
> > -    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.

OK


> > +{
> > +    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.

Oh silly me. Nice catch.


> > +    } 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.

Will introduce 2 static inline functions.


> > +    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 */
> >  
> 

-- 
yamahata