[Qemu-devel] Re: [PATCH V5 19/29] pci: split out ioport address parsing from pci configuration access logic.

[Michael S. Tsirkin]

On Tue, Oct 13, 2009 at 11:14:11PM +0900, Isaku Yamahata wrote:
> On Sat, Oct 10, 2009 at 09:45:00PM +0200, Michael S. Tsirkin wrote:
> > On Fri, Oct 09, 2009 at 03:28:52PM +0900, Isaku Yamahata wrote:
> > > Split out pci_data_{write, read} into two part, one is the code which
> > > convert ioport address into pci device and the other is the code
> > > which access PCI configuration space.
> > > 
> > > Later PCI express code will use the access code.
> > > PCI express addressing scheme is different so that the address
> > > must be parsed differently.
> > > 
> > > PCI:
> > >  0- 7 bit: offset in the configuration space (256bytes)
> > >  7-15 bit: devfn
> > > 16-24 bit: bus
> > > 
> > > PCI express:
> > >  0-11 bit: offset in the configuration space (4KBytes)
> > > 12-19 bit: devfn
> > > 20-28 bit: bus
> > > 
> > > Signed-off-by: Isaku Yamahata <address@hidden>
> > 
> > So I think I said this already: the only useful thing that
> > pci_data_read/write does is address conversion which is different for
> > express.  PCI Express should do its own thing and then call
> > pci_dev->config_read / pci_dev->config_write directly.
> > 
> > I think this will even make it possible to put express code in a
> > separate file from pci.c.
> 
> Yes, I did it. And with the next patch, they will moved to pci_host.c
> too as you noticed it.
> Hmm, you seem to be saying that pci_dev_{write, read}_config() aren't
> needed, however they are used by both pci_host.c and pcie_host.c
> as follows.

Yes, but if you look at them they do not do antyhing useful,
just call dev->config_write. So open-code and be done with it.

> So introducing pci_dev_{write, read}_config() makes sense.
> 
> 
> void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
> {
>     pci_dev_write_config(pci_addr_to_dev(s, addr), pci_addr_to_config(addr),
>                          val, len);
> }
> It parses pci address and pass it to lower function.
> 
> 
> With the patch of "pci: pcie host and mmcfg support.",
> pcie counter part is as follows.
> static void pcie_mmcfg_data_write(PCIBus *s,
>                                   uint32_t mmcfg_addr, uint32_t val, int len)
> {
>     pci_dev_write_config(pcie_mmcfg_addr_to_dev(s, mmcfg_addr),
>                          PCIE_MMCFG_CONFOFFSET(mmcfg_addr),
>                          val, len);
> }
> It parses pcie mmconfig address and pass it to lower function.

Yes but the lower function does nothing, so just opencode it.

So that will be
void pci_data_write(PCIBus *s,
                    uint32_t addr, uint32_t val, int len)
{
        PCIDevice *dev = pci_addr_to_dev(s, addr);
        if (!dev)
                return;
        dev->config_write(dev, pci_addr_to_config(addr), val, len);
}

and separately
static void pcie_mmcfg_data_write(PCIBus *s,
                                  uint32_t mmcfg_addr, uint32_t val, int len)
{
        PCIDevice *dev = pcie_mmcfg_addr_to_dev(s, mmcfg_addr);
        if (!dev)
                return;
        dev->config_write(dev, PCIE_MMCFG_CONFOFFSET(mmcfg_addr), val, len);
}

> > 
> > > ---
> > >  hw/pci.c |   80 
> > > ++++++++++++++++++++++++++++++-------------------------------
> > >  1 files changed, 39 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index 0021c96..d472b58 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -654,46 +654,24 @@ void pci_default_write_config(PCIDevice *d, 
> > > uint32_t addr, uint32_t val, int l)
> > >          pci_update_mappings(d);
> > >  }
> > >  
> > > -static inline PCIDevice *pci_addr_to_dev(PCIBus *bus, uint32_t addr)
> > > -{
> > > -    uint8_t bus_num = (addr >> 16) & 0xff;
> > > -    uint8_t devfn = (addr >> 8) & 0xff;
> > > -    return pci_find_device(bus, bus_num, PCI_SLOT(devfn), 
> > > PCI_FUNC(devfn));
> > > -}
> > > -
> > > -static inline int pci_addr_to_config(uint32_t addr)
> > > -{
> > > -    return addr & 0xff;
> > > -}
> > > -
> > > -void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > > +static void pci_dev_write_config(PCIDevice *pci_dev,
> > > +                                 uint32_t config_addr, uint32_t val, int 
> > > len)
> > >  {
> > > -    PCIBus *s = opaque;
> > > -    PCIDevice *pci_dev;
> > > -    int config_addr;
> > > -
> > > -#if 0
> > > -    PCI_DPRINTF("pci_data_write: addr=%08x val=%08x len=%d\n",
> > > -                addr, val, len);
> > > -#endif
> > > -    pci_dev = pci_addr_to_dev(s, addr);
> > > +    assert(len == 1 || len == 2 || len == 4);
> > >      if (!pci_dev)
> > >          return;
> > > -    config_addr = addr & 0xff;
> > > -    config_addr = pci_addr_to_config(addr);
> > > -    PCI_DPRINTF("pci_config_write: %s: addr=%02x val=%08x 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_read_config(PCIDevice *pci_dev,
> > > +                                    uint32_t config_addr, int len)
> > >  {
> > > -    PCIBus *s = opaque;
> > > -    PCIDevice *pci_dev;
> > > -    int config_addr;
> > >      uint32_t val;
> > >  
> > > -    pci_dev = pci_addr_to_dev(s, addr);
> > > +    assert(len == 1 || len == 2 || len == 4);
> > >      if (!pci_dev) {
> > >          switch(len) {
> > >          case 1:
> > > @@ -707,20 +685,40 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, 
> > > int len)
> > >              val = 0xffffffff;
> > >              break;
> > >          }
> > > -        goto the_end;
> > > +    } 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 = pci_addr_to_config(addr);
> > > -    val = pci_dev->config_read(pci_dev, config_addr, len);
> > > -    PCI_DPRINTF("pci_config_read: %s: addr=%02x val=%08x len=%d\n",
> > > -                pci_dev->name, config_addr, val, len);
> > > - the_end:
> > > -#if 0
> > > -    PCI_DPRINTF("pci_data_read: addr=%08x val=%08x len=%d\n",
> > > -                addr, val, len);
> > > -#endif
> > >      return val;
> > >  }
> > >  
> > > +static inline PCIDevice *pci_addr_to_dev(PCIBus *bus, uint32_t addr)
> > > +{
> > > +    uint8_t bus_num = (addr >> 16) & 0xff;
> > > +    uint8_t devfn = (addr >> 8) & 0xff;
> > > +    return pci_find_device(bus, bus_num, PCI_SLOT(devfn), 
> > > PCI_FUNC(devfn));
> > > +}
> > > +
> > > +static inline int pci_addr_to_config(uint32_t addr)
> > > +{
> > > +    return addr & 0xff;
> > > +}
> > > +
> > > +void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > > +{
> > > +    PCIBus *s = opaque;
> > > +    pci_dev_write_config(pci_addr_to_dev(s, addr), 
> > > pci_addr_to_config(addr),
> > > +                         val, len);
> > > +}
> > > +
> > > +uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> > > +{
> > > +    PCIBus *s = opaque;
> > > +    return pci_dev_read_config(pci_addr_to_dev(s, addr),
> > > +                               pci_addr_to_config(addr), len);
> > > +}
> > > +
> > >  /***********************************************************/
> > >  /* generic PCI irq support */
> > >  
> > 
> 
> -- 
> yamahata