| Message ID | 1254737223-16129-15-git-send-email-yamahata@valinux.co.jp |
|---|---|
| State | Superseded |
| Headers | show |
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 <yamahata@valinux.co.jp> > --- > 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. > { > - 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. > +} > + > +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 */ > > -- > 1.6.0.2
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 <yamahata@valinux.co.jp> > > --- > > 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 */ > > >
On Tue, Oct 06, 2009 at 06:50:05PM +0900, Isaku Yamahata wrote: > 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 <yamahata@valinux.co.jp> > > > --- > > > 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 The comment is basically that there's no common code: you don't need len checks that you added for mmconfig (they are there just for documentation purposes), and so all pci_data_write does is address parsing. So just call pci_dev->config_write for both mmconfig and config. > > > { > > > - 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. > Okay. I was misled by naming. What I am saying, here and elsewhere, is that a large part of pain is because of 256 byte/4K byte difference. Since in express you can hardware addresses > 256 to 0, I propose that we simply make mmconfig code do this: if (offset > 256) return 0; And then basically nothing needs to be changed in pci. > > > > +} > > > + > > > +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
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) { - 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 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); +} + +uint32_t pci_data_read(void *opaque, uint32_t addr, int len) +{ + PCIBus *s = opaque; + return pci_data_read_common(pci_addr_to_dev(s, addr), + pci_addr_to_config(addr), len); +} /***********************************************************/ /* generic PCI irq support */
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. Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/pci.c | 83 ++++++++++++++++++++++++++++++++++--------------------------- 1 files changed, 46 insertions(+), 37 deletions(-)