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

[Isaku Yamahata]

On Mon, Oct 05, 2009 at 02:45:33PM +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 05, 2009 at 07:06:54PM +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 by pcie support.
> > This patch also eliminates the logic duplication.
> 
> Please split removing logic duplication into separate patch.
> It might be applicable already, while express stuff needs more work IMO.
> 
> > Signed-off-by: Isaku Yamahata <address@hidden>
> > ---
> >  hw/pci.c |   83 
> > ++++++++++++++++++++++++++++++++++---------------------------
> >  1 files changed, 46 insertions(+), 37 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 6ddd256..5f808ff 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -629,44 +629,25 @@ void pci_default_write_config(PCIDevice *d, uint32_t 
> > addr, uint32_t val, int l)
> >          pci_update_mappings(d);
> >  }
> >  
> > -void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len)
> > +static void pci_data_write_common(PCIDevice *pci_dev,
> > +                                  uint32_t config_addr, uint32_t val, int 
> > len)
> 
> So as I said in the previous comment, pci express should just be able to
> use pci code directly, instead of reusing this low-level stuff.
> All you need to do is range-check address and ignore extended registers.

Maybe my patch description isn't good enough,
I'm afraid that you misunderstood the patch purpose.

PCI configuration and PCI express MMCONFIG uses different address scheme
as follows.
This patch purpose is to separate out its address parsing logic
from pci config space update logic.

PCI:
 0- 7 bit: offset in the configuration space (256bytes)
 7-15 bit: devfn
16-14 bit: bus

PCI express:
 0-11 bit: offset in the configuration space (4KBytes)
12-19 bit: devfn
20-28 bit: bus



> >  {
> > -    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(s, bus_num);
> > -    if (!s)
> > -        return;
> > -    pci_dev = s->devices[(addr >> 8) & 0xff];
> > +    assert(len == 1 || len == 2 || len == 4);
> >      if (!pci_dev)
> >          return;
> > -    config_addr = addr & 0xff;
> > -    PCI_DPRINTF("pci_config_write: %s: "
> > -                "addr=%02x 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_data_read_common(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(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;
> > @@ -679,21 +660,49 @@ 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 = addr & 0xff;
> > -    val = pci_dev->config_read(pci_dev, config_addr, len);
> > -    PCI_DPRINTF("pci_config_read: %s: "
> > -                "addr=%02x 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
> 
> move to real pci_data_read?
> 
> >      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 uint32_t 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;
> > +#if 0
> > +    PCI_DPRINTF("pci_data_write: addr=%08"PRIx32" val=%08"PRIx32" 
> > len=%d\n",
> > +                addr, val, len);
> > +#endif
> > +
> > +    pci_data_write_common(pci_addr_to_dev(s, addr), 
> > pci_addr_to_config(addr),
> > +                          val, len);
> 
> For pci express, there's extended register number in bits 11:8
> so what we should do here IMO is check these bits and
> ignore if they are not 0.

I don't understand what you suggest.
pci_data_{write, read}() are glue for ioio or mmio to
access pci configuration space. And 'addr' parameter is used
to specify configuration space access.
It includes not only the offset in configuration space, but also
devfn and bus.


> > +}
> > +
> > +uint32_t pci_data_read(void *opaque, uint32_t addr, int len)
> > +{
> > +    PCIBus *s = opaque;
> 
> For pci express, there's extended register number in bits 11:8
> so what we should do here IMO is check these bits and
> return 0 if not 0.
> 
> > +    return pci_data_read_common(pci_addr_to_dev(s, addr),
> > +                                pci_addr_to_config(addr), len);
> > +}
> >  /***********************************************************/
> >  /* generic PCI irq support */
> >  
> 

-- 
yamahata