| Message ID | 1262590375-11431-2-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
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 <agraf@suse.de> > CC: Michael S. Tsirkin <mst@redhat.com> > CC: Benjamin Herrenschmidt <benh@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. 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. > __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? > + } > > -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. > diff --git a/hw/piix_pci.c b/hw/piix_pci.c > index 1b67475..97500e0 100644 > --- a/hw/piix_pci.c > +++ b/hw/piix_pci.c > @@ -211,6 +211,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn, qemu_irq * > > dev = qdev_create(NULL, "i440FX-pcihost"); > s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev)); > + pci_host_init(s); > b = pci_bus_new(&s->busdev.qdev, NULL, 0); > s->bus = b; > qdev_init_nofail(dev); > diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c > index 2d00b61..dd8e290 100644 > --- a/hw/ppc4xx_pci.c > +++ b/hw/ppc4xx_pci.c > @@ -357,6 +357,7 @@ PCIBus *ppc4xx_pci_init(CPUState *env, qemu_irq pci_irqs[4], > > controller = qemu_mallocz(sizeof(PPC4xxPCIState)); > > + pci_host_init(&controller->pci_state); > controller->pci_state.bus = pci_register_bus(NULL, "pci", > ppc4xx_pci_set_irq, > ppc4xx_pci_map_irq, > diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c > index a72fb86..5914183 100644 > --- a/hw/ppce500_pci.c > +++ b/hw/ppce500_pci.c > @@ -278,6 +278,7 @@ PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers) > > controller = qemu_mallocz(sizeof(PPCE500PCIState)); > > + pci_host_init(&controller->pci_state); > controller->pci_state.bus = pci_register_bus(NULL, "pci", > mpc85xx_pci_set_irq, > mpc85xx_pci_map_irq, > diff --git a/hw/unin_pci.c b/hw/unin_pci.c > index 3ae4e7a..fdb9401 100644 > --- a/hw/unin_pci.c > +++ b/hw/unin_pci.c > @@ -84,6 +84,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev) > /* Uninorth main bus */ > s = FROM_SYSBUS(UNINState, dev); > > + pci_host_init(&s->host_state); > pci_mem_config = pci_host_conf_register_mmio(&s->host_state); > pci_mem_data = pci_host_data_register_mmio(&s->host_state); > sysbus_init_mmio(dev, 0x1000, pci_mem_config); > -- > 1.6.0.2 > > >
On Tue, Jan 05, 2010 at 09:46:38PM +0900, Isaku Yamahata wrote: > > 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. I'm ok, yes. Though long term, we should think about switching to an API that does not result in all this horrible boilerplate code that we are then forced to work around with macros. And it need not be hard: we just want 1. bswap(addr, len) 2. wrapper around cpu_register_io_memory that gets length and passes it on. > This trick would be split out into independent patch. Yes.
On Mon, Jan 04, 2010 at 08:32:50AM +0100, Alexander Graf wrote: > +typedef void (*pci_config_reg_fn)(PCIHostState *s, uint32_t config_reg, > + PCIConfigAddress *conf); > + pci_decode_config_addr_fn would be a better name. > struct PCIHostState { > SysBusDevice busdev; > + pci_config_reg_fn decode_config_reg; > + PCIConfigAddress config_reg_dec; decode_config_addr and config_addr would be better names > 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); Shouldn't this be static? And again, pci_host_decode_config_addr would be a better name IMO.
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 <agraf@suse.de> >> CC: Michael S. Tsirkin <mst@redhat.com> >> CC: Benjamin Herrenschmidt <benh@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. > > 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. > > >> + } >> >> -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
On 05.01.2010, at 23:16, Michael S. Tsirkin wrote: > On Mon, Jan 04, 2010 at 08:32:50AM +0100, Alexander Graf wrote: >> +typedef void (*pci_config_reg_fn)(PCIHostState *s, uint32_t config_reg, >> + PCIConfigAddress *conf); >> + > > pci_decode_config_addr_fn would be a better name. > >> struct PCIHostState { >> SysBusDevice busdev; >> + pci_config_reg_fn decode_config_reg; >> + PCIConfigAddress config_reg_dec; > > decode_config_addr > and > config_addr > > would be better names > >> 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); > > Shouldn't this be static? No, since I want to call it from unin for the OpenBIOS compatibility call. We still use the x86 encoding for the BIOS (for now). > And again, pci_host_decode_config_addr would be a better name IMO. Alrighty. Alex
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 <agraf@suse.de> > >> CC: Michael S. Tsirkin <mst@redhat.com> > >> CC: Benjamin Herrenschmidt <benh@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.
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; +} + /* 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", __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; + } -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 diff --git a/hw/piix_pci.c b/hw/piix_pci.c index 1b67475..97500e0 100644 --- a/hw/piix_pci.c +++ b/hw/piix_pci.c @@ -211,6 +211,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn, qemu_irq * dev = qdev_create(NULL, "i440FX-pcihost"); s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev)); + pci_host_init(s); b = pci_bus_new(&s->busdev.qdev, NULL, 0); s->bus = b; qdev_init_nofail(dev); diff --git a/hw/ppc4xx_pci.c b/hw/ppc4xx_pci.c index 2d00b61..dd8e290 100644 --- a/hw/ppc4xx_pci.c +++ b/hw/ppc4xx_pci.c @@ -357,6 +357,7 @@ PCIBus *ppc4xx_pci_init(CPUState *env, qemu_irq pci_irqs[4], controller = qemu_mallocz(sizeof(PPC4xxPCIState)); + pci_host_init(&controller->pci_state); controller->pci_state.bus = pci_register_bus(NULL, "pci", ppc4xx_pci_set_irq, ppc4xx_pci_map_irq, diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c index a72fb86..5914183 100644 --- a/hw/ppce500_pci.c +++ b/hw/ppce500_pci.c @@ -278,6 +278,7 @@ PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t registers) controller = qemu_mallocz(sizeof(PPCE500PCIState)); + pci_host_init(&controller->pci_state); controller->pci_state.bus = pci_register_bus(NULL, "pci", mpc85xx_pci_set_irq, mpc85xx_pci_map_irq, diff --git a/hw/unin_pci.c b/hw/unin_pci.c index 3ae4e7a..fdb9401 100644 --- a/hw/unin_pci.c +++ b/hw/unin_pci.c @@ -84,6 +84,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev) /* Uninorth main bus */ s = FROM_SYSBUS(UNINState, dev); + pci_host_init(&s->host_state); pci_mem_config = pci_host_conf_register_mmio(&s->host_state); pci_mem_data = pci_host_data_register_mmio(&s->host_state); sysbus_init_mmio(dev, 0x1000, pci_mem_config);
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 <agraf@suse.de> CC: Michael S. Tsirkin <mst@redhat.com> CC: Benjamin Herrenschmidt <benh@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