| Message ID | 1255069742-15724-20-git-send-email-yamahata@valinux.co.jp |
|---|---|
| State | Under Review |
| Headers | show |
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 <yamahata@valinux.co.jp> 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. > --- > 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 */ > > -- > 1.6.0.2
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 <yamahata@valinux.co.jp> > > 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. 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. > > > --- > > 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 */ > > >
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 <yamahata@valinux.co.jp> > > > > 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
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 */
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 <yamahata@valinux.co.jp> --- hw/pci.c | 80 ++++++++++++++++++++++++++++++------------------------------- 1 files changed, 39 insertions(+), 41 deletions(-)