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.
>
> 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
