On Tue, Jan 12, 2010 at 11:36:58AM +0100, Alexander Graf wrote: > > On 05.01.2010, at 13:46, Isaku Yamahata wrote: > > > Basically it looks good. > > Some minor comments below. > > > > Although further clean up is possible, > > this is first small step. > > > > On Mon, Jan 04, 2010 at 08:32:50AM +0100, Alexander Graf wrote: > >> Different host buses may have different layouts for config space accessors. > >> > >> The Mac U3 for example uses the following define to access Type 0 (directly > >> attached) devices: > >> > >> #define MACRISC_CFA0(devfn, off) \ > >> ((1 << (unsigned int)PCI_SLOT(dev_fn)) \ > >> | (((unsigned int)PCI_FUNC(dev_fn)) << 8) \ > >> | (((unsigned int)(off)) & 0xFCUL)) > >> > >> Obviously, this is different from the encoding we interpret in qemu. In > >> order to let host controllers take some influence there, we can just > >> introduce a decoding function that takes whatever accessor we have and > >> decodes it into understandable fields. > >> > >> To not touch all different code paths in qemu, I added this on top of > >> the existing config_reg element. In the future, we should work on gradually > >> moving references to config_reg to config_reg_dec and then phase out > >> config_reg. > >> > >> This patch is the groundwork for Uninorth PCI config space fixes. > >> > >> Signed-off-by: Alexander Graf <ag...@suse.de> > >> CC: Michael S. Tsirkin <m...@redhat.com> > >> CC: Benjamin Herrenschmidt <b...@kernel.crashing.org> > >> > >> --- > >> > >> v1 -> v2: > >> > >> - merge data access functions > >> - use decoding function for config space bits > >> - introduce encoding function for x86 style encodings > >> > >> --- > >> hw/apb_pci.c | 1 + > >> hw/grackle_pci.c | 1 + > >> hw/gt64xxx.c | 1 + > >> hw/pci.h | 13 ++++++ > >> hw/pci_host.c | 48 ++++++++++++++++++----- > >> hw/pci_host.h | 16 ++++++++ > >> hw/pci_host_template.h | 90 > >> +++++++++++++------------------------------- > >> hw/pci_host_template_all.h | 23 +++++++++++ > >> hw/piix_pci.c | 1 + > >> hw/ppc4xx_pci.c | 1 + > >> hw/ppce500_pci.c | 1 + > >> hw/unin_pci.c | 1 + > >> 12 files changed, 123 insertions(+), 74 deletions(-) > >> create mode 100644 hw/pci_host_template_all.h > >> > >> diff --git a/hw/apb_pci.c b/hw/apb_pci.c > >> index f05308b..fedb84c 100644 > >> --- a/hw/apb_pci.c > >> +++ b/hw/apb_pci.c > >> @@ -222,6 +222,7 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base, > >> /* mem_data */ > >> sysbus_mmio_map(s, 3, mem_base); > >> d = FROM_SYSBUS(APBState, s); > >> + pci_host_init(&d->host_state); > >> d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci", > >> pci_apb_set_irq, pci_pbm_map_irq, > >> pic, > >> 0, 32); > >> diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c > >> index ee4fed5..7feba63 100644 > >> --- a/hw/grackle_pci.c > >> +++ b/hw/grackle_pci.c > >> @@ -88,6 +88,7 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic) > >> qdev_init_nofail(dev); > >> s = sysbus_from_qdev(dev); > >> d = FROM_SYSBUS(GrackleState, s); > >> + pci_host_init(&d->host_state); > >> d->host_state.bus = pci_register_bus(&d->busdev.qdev, "pci", > >> pci_grackle_set_irq, > >> pci_grackle_map_irq, > >> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c > >> index fb7f5bd..8cf93ca 100644 > >> --- a/hw/gt64xxx.c > >> +++ b/hw/gt64xxx.c > >> @@ -1120,6 +1120,7 @@ PCIBus *pci_gt64120_init(qemu_irq *pic) > >> s = qemu_mallocz(sizeof(GT64120State)); > >> s->pci = qemu_mallocz(sizeof(GT64120PCIState)); > >> > >> + pci_host_init(s->pci); > >> s->pci->bus = pci_register_bus(NULL, "pci", > >> pci_gt64120_set_irq, > >> pci_gt64120_map_irq, > >> pic, 144, 4); > >> diff --git a/hw/pci.h b/hw/pci.h > >> index fd16460..cfaa0a9 100644 > >> --- a/hw/pci.h > >> +++ b/hw/pci.h > >> @@ -10,10 +10,23 @@ > >> > >> /* PCI bus */ > >> > >> +typedef struct PCIAddress { > >> + PCIBus *domain; > >> + uint8_t bus; > >> + uint8_t slot; > >> + uint8_t fn; > >> +} PCIAddress; > >> + > >> #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07)) > >> #define PCI_SLOT(devfn) (((devfn) >> 3) & 0x1f) > >> #define PCI_FUNC(devfn) ((devfn) & 0x07) > >> > >> +static inline uint32_t pci_addr_to_devfn(PCIAddress *addr, uint32_t > >> offset) > >> +{ > >> + return ((addr->bus & 0xff) << 16) | (PCI_DEVFN(addr->slot, addr->fn) > >> << 8) | > >> + offset; > >> +} > >> + > > > > Hmm, this name doesn't seem good. > > devfn sounds something like encoded (slot, fn) pair (to me), > > but the function returns something else. > > I'm open for naming suggestions.
Does it encode to config_reg format? If so pci_addr_to_config_reg? > > > > This function is used for pci_data_{read, write}() > > which again decodes bus, slot, fn. > > So shouldn't they be changed to accept PCIAddress itself? > > > > > >> /* Class, Vendor and Device IDs from Linux's pci_ids.h */ > >> #include "pci_ids.h" > >> > >> diff --git a/hw/pci_host.c b/hw/pci_host.c > >> index eeb8dee..746ffc2 100644 > >> --- a/hw/pci_host.c > >> +++ b/hw/pci_host.c > >> @@ -32,13 +32,6 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } > >> while (0) > >> #define PCI_DPRINTF(fmt, ...) > >> #endif > >> > >> -/* > >> - * PCI address > >> - * bit 16 - 24: bus number > >> - * bit 8 - 15: devfun number > >> - * bit 0 - 7: offset in configuration space of a given pci device > >> - */ > >> - > >> /* the helper functio to get a PCIDeice* for a given pci address */ > >> static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr) > >> { > >> @@ -56,7 +49,7 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t > >> val, int len) > >> if (!pci_dev) > >> return; > >> > >> - PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRI32x" len=%d\n", > >> + PCI_DPRINTF("%s: %s: addr=%02"PRIx32" val=%08"PRIx32" len=%d\n", > > > > Good catch. This part should be split into another patch. > > Right :). > > > > >> __func__, pci_dev->name, config_addr, val, len); > >> pci_dev->config_write(pci_dev, config_addr, val, len); > >> } > >> @@ -89,7 +82,9 @@ static void pci_host_config_writel(void *opaque, > >> target_phys_addr_t addr, > >> #endif > >> PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n", > >> __func__, addr, val); > >> + > >> s->config_reg = val; > >> + s->decode_config_reg(s, val, &s->config_reg_dec); > >> } > >> > >> static uint32_t pci_host_config_readl(void *opaque, target_phys_addr_t > >> addr) > >> @@ -131,7 +126,9 @@ static void pci_host_config_writel_noswap(void *opaque, > >> > >> PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n", > >> __func__, addr, val); > >> + > >> s->config_reg = val; > >> + s->decode_config_reg(s, val, &s->config_reg_dec); > >> } > >> > >> static uint32_t pci_host_config_readl_noswap(void *opaque, > >> @@ -169,7 +166,9 @@ static void pci_host_config_writel_ioport(void *opaque, > >> PCIHostState *s = opaque; > >> > >> PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr, val); > >> + > >> s->config_reg = val; > >> + s->decode_config_reg(s, val, &s->config_reg_dec); > >> } > >> > >> static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t addr) > >> @@ -190,7 +189,7 @@ void pci_host_conf_register_ioport(pio_addr_t ioport, > >> PCIHostState *s) > >> #define PCI_ADDR_T target_phys_addr_t > >> #define PCI_HOST_SUFFIX _mmio > >> > >> -#include "pci_host_template.h" > >> +#include "pci_host_template_all.h" > >> > >> static CPUWriteMemoryFunc * const pci_host_data_write_mmio[] = { > >> pci_host_data_writeb_mmio, > >> @@ -217,7 +216,7 @@ int pci_host_data_register_mmio(PCIHostState *s) > >> #define PCI_ADDR_T uint32_t > >> #define PCI_HOST_SUFFIX _ioport > >> > >> -#include "pci_host_template.h" > >> +#include "pci_host_template_all.h" > >> > >> void pci_host_data_register_ioport(pio_addr_t ioport, PCIHostState *s) > >> { > >> @@ -228,3 +227,32 @@ void pci_host_data_register_ioport(pio_addr_t ioport, > >> PCIHostState *s) > >> register_ioport_read(ioport, 4, 2, pci_host_data_readw_ioport, s); > >> register_ioport_read(ioport, 4, 4, pci_host_data_readl_ioport, s); > >> } > >> + > >> +/* This implements the default x86 way of interpreting a PCI address > >> + * > >> + * PCI address > >> + * bit 16 - 24: bus number > >> + * bit 11 - 15: slot number > >> + * bit 8 - 10: function number > >> + * bit 0 - 7: offset in configuration space of a given pci device > >> + */ > >> + > >> +void pci_host_decode_config_reg(PCIHostState *s, uint32_t config_reg, > >> + PCIConfigAddress *decoded) > >> +{ > >> + uint32_t devfn; > >> + > >> + decoded->addr.domain = s->bus; > >> + decoded->addr.bus = (config_reg >> 16) & 0xff; > >> + devfn = config_reg >> 8; > >> + decoded->addr.slot = (config_reg >> 11) & 0x1f; > >> + decoded->addr.fn = (config_reg >> 8) & 0x7; > >> + decoded->offset = config_reg & 0xff; > >> + decoded->addr_mask = 3; > >> + decoded->valid = (config_reg & (1u << 31)) ? true : false; > >> +} > >> + > >> +void pci_host_init(PCIHostState *s) > >> +{ > >> + s->decode_config_reg = pci_host_decode_config_reg; > >> +} > >> diff --git a/hw/pci_host.h b/hw/pci_host.h > >> index a006687..0fcdf64 100644 > >> --- a/hw/pci_host.h > >> +++ b/hw/pci_host.h > >> @@ -30,14 +30,30 @@ > >> > >> #include "sysbus.h" > >> > >> +/* for config space access */ > >> +typedef struct PCIConfigAddress { > >> + PCIAddress addr; > >> + uint32_t addr_mask; > >> + uint16_t offset; > >> + bool valid; > >> +} PCIConfigAddress; > >> + > >> +typedef void (*pci_config_reg_fn)(PCIHostState *s, uint32_t config_reg, > >> + PCIConfigAddress *conf); > >> + > >> struct PCIHostState { > >> SysBusDevice busdev; > >> + pci_config_reg_fn decode_config_reg; > >> + PCIConfigAddress config_reg_dec; > >> uint32_t config_reg; > >> PCIBus *bus; > >> }; > >> > >> void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len); > >> uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len); > >> +void pci_host_init(PCIHostState *s); > >> +void pci_host_decode_config_reg(PCIHostState *s, uint32_t config_reg, > >> + PCIConfigAddress *decoded); > >> > >> /* for mmio */ > >> int pci_host_conf_register_mmio(PCIHostState *s); > >> diff --git a/hw/pci_host_template.h b/hw/pci_host_template.h > >> index 11e6c88..d989c25 100644 > >> --- a/hw/pci_host_template.h > >> +++ b/hw/pci_host_template.h > >> @@ -25,85 +25,47 @@ > >> /* Worker routines for a PCI host controller that uses an {address,data} > >> register pair to access PCI configuration space. */ > >> > >> -static void glue(pci_host_data_writeb, PCI_HOST_SUFFIX)( > >> +static void glue(glue(pci_host_data_write, PCI_HOST_BWL), > >> PCI_HOST_SUFFIX)( > >> void* opaque, PCI_ADDR_T addr, uint32_t val) > >> { > >> PCIHostState *s = opaque; > >> + PCIConfigAddress *config_reg = &s->config_reg_dec; > >> + int offset = config_reg->offset; > >> > >> - PCI_DPRINTF("writeb addr " TARGET_FMT_plx " val %x\n", > >> - (target_phys_addr_t)addr, val); > >> - if (s->config_reg & (1u << 31)) > >> - pci_data_write(s->bus, s->config_reg | (addr & 3), val, 1); > >> -} > >> - > >> -static void glue(pci_host_data_writew, PCI_HOST_SUFFIX)( > >> - void* opaque, PCI_ADDR_T addr, uint32_t val) > >> -{ > >> - PCIHostState *s = opaque; > >> -#ifdef TARGET_WORDS_BIGENDIAN > >> - val = bswap16(val); > >> +#if defined(TARGET_WORDS_BIGENDIAN) && (PCI_HOST_BITS > 8) > >> + val = glue(bswap, PCI_HOST_BITS)(val); > >> #endif > >> - PCI_DPRINTF("writew addr " TARGET_FMT_plx " val %x\n", > >> - (target_phys_addr_t)addr, val); > >> - if (s->config_reg & (1u << 31)) > >> - pci_data_write(s->bus, s->config_reg | (addr & 3), val, 2); > >> -} > >> > >> -static void glue(pci_host_data_writel, PCI_HOST_SUFFIX)( > >> - void* opaque, PCI_ADDR_T addr, uint32_t val) > >> -{ > >> - PCIHostState *s = opaque; > >> -#ifdef TARGET_WORDS_BIGENDIAN > >> - val = bswap32(val); > >> -#endif > >> - PCI_DPRINTF("writel addr " TARGET_FMT_plx " val %x\n", > >> - (target_phys_addr_t)addr, val); > >> - if (s->config_reg & (1u << 31)) > >> - pci_data_write(s->bus, s->config_reg, val, 4); > >> + offset |= (addr & config_reg->addr_mask); > >> + PCI_DPRINTF("%s: addr " TARGET_FMT_plx " val %x\n", > >> + __func__, (target_phys_addr_t)addr, val); > >> + if (config_reg->valid) { > >> + pci_data_write(s->bus, pci_addr_to_devfn(&config_reg->addr, > >> offset), val, > >> + sizeof(glue(glue(uint, PCI_HOST_BITS), _t))); > >> + } > >> } > >> > >> -static uint32_t glue(pci_host_data_readb, PCI_HOST_SUFFIX)( > >> +static uint32_t glue(glue(pci_host_data_read, PCI_HOST_BWL), > >> PCI_HOST_SUFFIX)( > >> void* opaque, PCI_ADDR_T addr) > >> { > >> PCIHostState *s = opaque; > >> + PCIConfigAddress *config_reg = &s->config_reg_dec; > >> + int offset = config_reg->offset; > >> uint32_t val; > >> > >> - if (!(s->config_reg & (1 << 31))) > >> - return 0xff; > >> - val = pci_data_read(s->bus, s->config_reg | (addr & 3), 1); > >> - PCI_DPRINTF("readb addr " TARGET_FMT_plx " val %x\n", > >> - (target_phys_addr_t)addr, val); > >> - return val; > >> -} > >> + if (!config_reg->valid) { > >> + return ( 1ULL << PCI_HOST_BITS ) - 1; > > > > Are you using intentionally 1ULL (64bit) not to overflow it? > > We are overflowing it for a short amount of time. > > 1 << PCI_HOST_BITS = 0x100000000 > 0x100000000 - 1 = 0xffffffff > > That's why we need the ULL. OK. However, please do not put space after ( and before ). I also suspect just return ~0x0 will do the trick as well (but not sure). > > > > > >> + } > >> > >> -static uint32_t glue(pci_host_data_readw, PCI_HOST_SUFFIX)( > >> - void* opaque, PCI_ADDR_T addr) > >> -{ > >> - PCIHostState *s = opaque; > >> - uint32_t val; > >> - if (!(s->config_reg & (1 << 31))) > >> - return 0xffff; > >> - val = pci_data_read(s->bus, s->config_reg | (addr & 3), 2); > >> - PCI_DPRINTF("readw addr " TARGET_FMT_plx " val %x\n", > >> - (target_phys_addr_t)addr, val); > >> -#ifdef TARGET_WORDS_BIGENDIAN > >> - val = bswap16(val); > >> -#endif > >> - return val; > >> -} > >> + offset |= (addr & config_reg->addr_mask); > >> + val = pci_data_read(s->bus, pci_addr_to_devfn(&config_reg->addr, > >> offset), > >> + sizeof(glue(glue(uint, PCI_HOST_BITS), _t))); > >> + PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %x\n", > >> + __func__, (target_phys_addr_t)addr, val); > >> > >> -static uint32_t glue(pci_host_data_readl, PCI_HOST_SUFFIX)( > >> - void* opaque, PCI_ADDR_T addr) > >> -{ > >> - PCIHostState *s = opaque; > >> - uint32_t val; > >> - if (!(s->config_reg & (1 << 31))) > >> - return 0xffffffff; > >> - val = pci_data_read(s->bus, s->config_reg | (addr & 3), 4); > >> - PCI_DPRINTF("readl addr " TARGET_FMT_plx " val %x\n", > >> - (target_phys_addr_t)addr, val); > >> -#ifdef TARGET_WORDS_BIGENDIAN > >> - val = bswap32(val); > >> +#if defined(TARGET_WORDS_BIGENDIAN) && (PCI_HOST_BITS > 8) > >> + val = glue(bswap, PCI_HOST_BITS)(val); > >> #endif > >> + > >> return val; > >> } > >> diff --git a/hw/pci_host_template_all.h b/hw/pci_host_template_all.h > >> new file mode 100644 > >> index 0000000..74b3e84 > >> --- /dev/null > >> +++ b/hw/pci_host_template_all.h > >> @@ -0,0 +1,23 @@ > >> +#define PCI_HOST_BWL b > >> +#define PCI_HOST_BITS 8 > >> + > >> +#include "pci_host_template.h" > >> + > >> +#undef PCI_HOST_BWL > >> +#undef PCI_HOST_BITS > >> + > >> +#define PCI_HOST_BWL w > >> +#define PCI_HOST_BITS 16 > >> + > >> +#include "pci_host_template.h" > >> + > >> +#undef PCI_HOST_BWL > >> +#undef PCI_HOST_BITS > >> + > >> +#define PCI_HOST_BWL l > >> +#define PCI_HOST_BITS 32 > >> + > >> +#include "pci_host_template.h" > >> + > >> +#undef PCI_HOST_BWL > >> +#undef PCI_HOST_BITS > > > > Oh, another new cpp tricks. > > I'm ok with this trick. However Michael may have his own idea. > > This trick would be split out into independent patch. > > Well I got fed up with having to change 10 lines of code that all look the > same ;-). > > Alex Yea, ok. Maybe split it out but it's not critical either.