Re: [Qemu-devel] [PATCH 1/6] PCI config space access overhaul

["Michael S. Tsirkin" <mst@redhat.com>]

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.