Re: [Qemu-devel] [PATCH v2 7/8] hw: xilinx-pcie: Add support for Xilinx AXI PCIe Controller

[Alistair Francis]

On Thu, Sep 8, 2016 at 7:51 AM, Paul Burton <address@hidden> wrote:
> Add support for emulating the Xilinx AXI Root Port Bridge for PCI
> Express as described by Xilinx' PG055 document. This is a PCIe
> controller that can be used with certain series of Xilinx FPGAs, and is
> used on the MIPS Boston board which will make use of this code.
>
> Signed-off-by: Paul Burton <address@hidden>

Hey Paul,

Thanks for upstreaming Xilinx related devices. I had a go at reveiwing
your patch. I don't know a huge amount about PCIe and how it is all
connnected in QEMU, so if that is how it is usually done just let me
know.

> ---
>  hw/pci-host/Makefile.objs         |   1 +
>  hw/pci-host/xilinx-pcie.c         | 310 
> ++++++++++++++++++++++++++++++++++++++
>  include/hw/pci-host/xilinx-pcie.h | 102 +++++++++++++
>  3 files changed, 413 insertions(+)
>  create mode 100644 hw/pci-host/xilinx-pcie.c
>  create mode 100644 include/hw/pci-host/xilinx-pcie.h
>
> diff --git a/hw/pci-host/Makefile.objs b/hw/pci-host/Makefile.objs
> index 45f1f0e..9c7909c 100644
> --- a/hw/pci-host/Makefile.objs
> +++ b/hw/pci-host/Makefile.objs
> @@ -16,3 +16,4 @@ common-obj-$(CONFIG_FULONG) += bonito.o
>  common-obj-$(CONFIG_PCI_PIIX) += piix.o
>  common-obj-$(CONFIG_PCI_Q35) += q35.o
>  common-obj-$(CONFIG_PCI_GENERIC) += gpex.o
> +common-obj-$(CONFIG_PCI_XILINX) += xilinx-pcie.o

Where does this config option get added?

I think it should be added in this patch.

> diff --git a/hw/pci-host/xilinx-pcie.c b/hw/pci-host/xilinx-pcie.c
> new file mode 100644
> index 0000000..2f3a712
> --- /dev/null
> +++ b/hw/pci-host/xilinx-pcie.c
> @@ -0,0 +1,310 @@
> +/*
> + * Xilinx PCIe host controller emulation.
> + *
> + * Copyright (c) 2016 Imagination Technologies
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/pci/pci_bridge.h"
> +#include "hw/pci-host/xilinx-pcie.h"
> +
> +enum root_cfg_reg {
> +    ROOTCFG_INTDEC              = 0x138,
> +
> +    ROOTCFG_INTMASK             = 0x13c,
> +#define ROOTCFG_INTMASK_INTX    (1 << 16)
> +#define ROOTCFG_INTMASK_MSI     (1 << 17)
> +
> +    ROOTCFG_PSCR                = 0x144,
> +#define ROOTCFG_PSCR_LINK       (1 << 11)
> +
> +    ROOTCFG_RPSCR               = 0x148,
> +#define ROOTCFG_RPSCR_BRIDGEEN  (1 << 0)
> +#define ROOTCFG_RPSCR_INTNEMPTY (1 << 18)
> +#define ROOTCFG_RPSCR_INTOVF    (1 << 19)
> +
> +    ROOTCFG_RPIFR1              = 0x158,
> +    ROOTCFG_RPIFR2              = 0x15c,
> +};

Do you not need any debug prints in here?

> +
> +static void xilinx_pcie_update_intr(XilinxPCIEHost *s,
> +                                    uint32_t set, uint32_t clear)
> +{
> +    int level;
> +
> +    s->intr |= set;
> +    s->intr &= ~clear;
> +
> +    if (s->intr_fifo_r != s->intr_fifo_w) {
> +        s->intr |= ROOTCFG_INTMASK_INTX;
> +    }
> +
> +    level = !!(s->intr & s->intr_mask);
> +    qemu_set_irq(s->irq, level);
> +}
> +
> +static void xilinx_pcie_queue_intr(XilinxPCIEHost *s,
> +                                   uint32_t fifo_reg1, uint32_t fifo_reg2)
> +{
> +    XilinxPCIEInt *intr;
> +    unsigned int new_w;
> +
> +    new_w = (s->intr_fifo_w + 1) % ARRAY_SIZE(s->intr_fifo);
> +    if (new_w == s->intr_fifo_r) {
> +        s->rpscr |= ROOTCFG_RPSCR_INTOVF;
> +        return;
> +    }
> +
> +    intr = &s->intr_fifo[s->intr_fifo_w];
> +    s->intr_fifo_w = new_w;
> +
> +    intr->fifo_reg1 = fifo_reg1;
> +    intr->fifo_reg2 = fifo_reg2;
> +
> +    xilinx_pcie_update_intr(s, ROOTCFG_INTMASK_INTX, 0);
> +}
> +
> +static void xilinx_pcie_set_irq(void *opaque, int irq_num, int level)
> +{
> +    XilinxPCIEHost *s = XILINX_PCIE_HOST(opaque);
> +
> +    if (!level) {
> +        return;
> +    }

How is the interrupt ever cleared?

> +
> +    xilinx_pcie_queue_intr(s, (irq_num << 27) | (level << 29) | (1 << 31), 
> 0);
> +}
> +
> +static void xilinx_pcie_host_realize(DeviceState *dev, Error **errp)
> +{
> +    PCIHostState *pci = PCI_HOST_BRIDGE(dev);
> +    XilinxPCIEHost *s = XILINX_PCIE_HOST(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    PCIExpressHost *pex = PCIE_HOST_BRIDGE(dev);
> +
> +    snprintf(s->name, sizeof(s->name), "pcie%u", s->bus_nr);
> +
> +    /* PCI configuration space */
> +    pcie_host_mmcfg_init(pex, s->cfg_size);
> +
> +    /* MMIO region */
> +    memory_region_init(&s->mmio, OBJECT(s), "mmio", UINT64_MAX);
> +    memory_region_set_enabled(&s->mmio, false);
> +
> +    /* dummy I/O region */
> +    memory_region_init_ram(&s->io, OBJECT(s), "io", 16, NULL);
> +    memory_region_set_enabled(&s->io, false);
> +
> +    sysbus_init_mmio(sbd, &pex->mmio);
> +    sysbus_init_mmio(sbd, &s->mmio);
> +
> +    pci->bus = pci_register_bus(dev, s->name, xilinx_pcie_set_irq,
> +                                pci_swizzle_map_irq_fn, s, &s->mmio,
> +                                &s->io, 0, 4, TYPE_PCIE_BUS);
> +
> +    qdev_set_parent_bus(DEVICE(&s->root), BUS(pci->bus));
> +    qdev_init_nofail(DEVICE(&s->root));
> +}
> +
> +static const char *xilinx_pcie_host_root_bus_path(PCIHostState *host_bridge,
> +                                                  PCIBus *rootbus)
> +{
> +    return "0000:00";
> +}
> +
> +static void xilinx_pcie_host_init(Object *obj)
> +{
> +    XilinxPCIEHost *s = XILINX_PCIE_HOST(obj);
> +    XilinxPCIERoot *root = &s->root;
> +
> +    object_initialize(root, sizeof(*root), TYPE_XILINX_PCIE_ROOT);
> +    object_property_add_child(obj, "root", OBJECT(root), NULL);
> +    qdev_prop_set_uint32(DEVICE(root), "addr", PCI_DEVFN(0, 0));
> +    qdev_prop_set_bit(DEVICE(root), "multifunction", false);
> +}
> +
> +static Property xilinx_pcie_host_props[] = {
> +    DEFINE_PROP_UINT32("bus_nr", XilinxPCIEHost, bus_nr, 0),
> +    DEFINE_PROP_SIZE("cfg_base", XilinxPCIEHost, cfg_base, 0),
> +    DEFINE_PROP_SIZE("cfg_size", XilinxPCIEHost, cfg_size, 32 << 20),
> +    DEFINE_PROP_SIZE("mmio_base", XilinxPCIEHost, mmio_base, 0),
> +    DEFINE_PROP_SIZE("mmio_size", XilinxPCIEHost, mmio_size, 1 << 20),
> +    DEFINE_PROP_PTR("irq", XilinxPCIEHost, irq_void),

This seems like a really strange way to connec the IRQ from how we
usually do it for other Xilinx related things. Is this what PCI
devices normally do? Is there any reason not to use the GPIO logic?

> +    DEFINE_PROP_BOOL("link_up", XilinxPCIEHost, link_up, true),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void xilinx_pcie_host_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
> +
> +    hc->root_bus_path = xilinx_pcie_host_root_bus_path;
> +    dc->realize = xilinx_pcie_host_realize;
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +    dc->fw_name = "pci";
> +    dc->props = xilinx_pcie_host_props;
> +}
> +
> +static const TypeInfo xilinx_pcie_host_info = {
> +    .name       = TYPE_XILINX_PCIE_HOST,
> +    .parent     = TYPE_PCIE_HOST_BRIDGE,
> +    .instance_size = sizeof(XilinxPCIEHost),
> +    .instance_init = xilinx_pcie_host_init,
> +    .class_init = xilinx_pcie_host_class_init,
> +};
> +
> +static uint32_t xilinx_pcie_root_config_read(PCIDevice *d,
> +                                             uint32_t address, int len)
> +{
> +    XilinxPCIEHost *s = XILINX_PCIE_HOST(OBJECT(d)->parent);
> +    uint32_t val;
> +
> +    switch (address) {
> +    case ROOTCFG_INTDEC:
> +        return s->intr;
> +
> +    case ROOTCFG_INTMASK:
> +        return s->intr_mask;
> +
> +    case ROOTCFG_PSCR:
> +        val = s->link_up ? ROOTCFG_PSCR_LINK : 0;
> +        return val;
> +
> +    case ROOTCFG_RPSCR:
> +        if (s->intr_fifo_r != s->intr_fifo_w) {
> +            s->rpscr &= ~ROOTCFG_RPSCR_INTNEMPTY;
> +        } else {
> +            s->rpscr |= ROOTCFG_RPSCR_INTNEMPTY;
> +        }
> +        return s->rpscr;
> +
> +    case ROOTCFG_RPIFR1:
> +        if (s->intr_fifo_w == s->intr_fifo_r) {
> +            /* FIFO empty */
> +            return 0;
> +        }
> +        return s->intr_fifo[s->intr_fifo_r].fifo_reg1;
> +
> +    case ROOTCFG_RPIFR2:
> +        if (s->intr_fifo_w == s->intr_fifo_r) {
> +            /* FIFO empty */
> +            return 0;
> +        }
> +        return s->intr_fifo[s->intr_fifo_r].fifo_reg2;
> +
> +    default:
> +        return pci_default_read_config(d, address, len);
> +    }
> +}
> +
> +static void xilinx_pcie_root_config_write(PCIDevice *d, uint32_t address,
> +                                          uint32_t val, int len)
> +{
> +    XilinxPCIEHost *s = XILINX_PCIE_HOST(OBJECT(d)->parent);
> +
> +    switch (address) {
> +    case ROOTCFG_INTDEC:
> +        xilinx_pcie_update_intr(s, 0, val);
> +        break;
> +
> +    case ROOTCFG_INTMASK:
> +        s->intr_mask = val;
> +        xilinx_pcie_update_intr(s, 0, 0);
> +        break;
> +
> +    case ROOTCFG_RPSCR:
> +        s->rpscr &= ~ROOTCFG_RPSCR_BRIDGEEN;
> +        s->rpscr |= val & ROOTCFG_RPSCR_BRIDGEEN;
> +        memory_region_set_enabled(&s->mmio, val & ROOTCFG_RPSCR_BRIDGEEN);
> +
> +        if (val & ROOTCFG_INTMASK_INTX) {
> +            s->rpscr &= ~ROOTCFG_INTMASK_INTX;
> +        }
> +        break;
> +
> +    case ROOTCFG_RPIFR1:
> +    case ROOTCFG_RPIFR2:
> +        if (s->intr_fifo_w == s->intr_fifo_r) {
> +            /* FIFO empty */
> +            return;
> +        }
> +        s->intr_fifo_r = (s->intr_fifo_r + 1) % ARRAY_SIZE(s->intr_fifo);
> +        break;
> +
> +    default:
> +        pci_default_write_config(d, address, val, len);
> +        break;
> +    }
> +}
> +
> +static int xilinx_pcie_root_init(PCIDevice *dev)
> +{
> +    BusState *bus = qdev_get_parent_bus(DEVICE(dev));
> +    XilinxPCIEHost *s = XILINX_PCIE_HOST(bus->parent);
> +
> +    dev->config[PCI_COMMAND] = PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER;
> +    stw_le_p(&dev->config[PCI_MEMORY_BASE], s->mmio_base >> 16);
> +    stw_le_p(&dev->config[PCI_MEMORY_LIMIT],
> +             ((s->mmio_base + s->mmio_size - 1) >> 16) & 0xfff0);
> +
> +    pci_bridge_initfn(dev, TYPE_PCI_BUS);
> +
> +    if (pcie_endpoint_cap_v1_init(dev, 0x80) < 0) {
> +        hw_error("Failed to initialize PCIe capability");
> +    }
> +
> +    return 0;
> +}
> +
> +static void xilinx_pcie_root_class_init(ObjectClass *klass, void *data)
> +{
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +    dc->desc = "Xilinx AXI-PCIe Host Bridge";
> +    k->vendor_id = 0x10ee;
> +    k->device_id = 0x7021;
> +    k->revision = 0;
> +    k->class_id = PCI_CLASS_BRIDGE_HOST;
> +    k->is_express = true;
> +    k->is_bridge = true;
> +    k->init = xilinx_pcie_root_init;
> +    k->exit = pci_bridge_exitfn;
> +    dc->reset = pci_bridge_reset;
> +    k->config_read = xilinx_pcie_root_config_read;
> +    k->config_write = xilinx_pcie_root_config_write;
> +    /*
> +     * PCI-facing part of the host bridge, not usable without the
> +     * host-facing part, which can't be device_add'ed, yet.
> +     */
> +    dc->cannot_instantiate_with_device_add_yet = true;
> +}
> +
> +static const TypeInfo xilinx_pcie_root_info = {
> +    .name = TYPE_XILINX_PCIE_ROOT,
> +    .parent = TYPE_PCI_BRIDGE,
> +    .instance_size = sizeof(XilinxPCIERoot),
> +    .class_init = xilinx_pcie_root_class_init,
> +};
> +
> +static void xilinx_pcie_register(void)
> +{
> +    type_register_static(&xilinx_pcie_root_info);
> +    type_register_static(&xilinx_pcie_host_info);
> +}

Newline.

> +type_init(xilinx_pcie_register)
> diff --git a/include/hw/pci-host/xilinx-pcie.h 
> b/include/hw/pci-host/xilinx-pcie.h
> new file mode 100644
> index 0000000..443c1a6
> --- /dev/null
> +++ b/include/hw/pci-host/xilinx-pcie.h
> @@ -0,0 +1,102 @@
> +/*
> + * Xilinx PCIe host controller emulation.
> + *
> + * Copyright (c) 2016 Imagination Technologies
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef HW_XILINX_PCIE_H
> +#define HW_XILINX_PCIE_H
> +
> +#include "hw/hw.h"
> +#include "hw/sysbus.h"
> +#include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
> +#include "hw/pci/pcie_host.h"
> +
> +#define TYPE_XILINX_PCIE_HOST "xilinx-pcie-host"
> +#define XILINX_PCIE_HOST(obj) \
> +     OBJECT_CHECK(XilinxPCIEHost, (obj), TYPE_XILINX_PCIE_HOST)
> +
> +#define TYPE_XILINX_PCIE_ROOT "xilinx-pcie-root"
> +#define XILINX_PCIE_ROOT(obj) \
> +     OBJECT_CHECK(XilinxPCIERoot, (obj), TYPE_XILINX_PCIE_ROOT)
> +
> +typedef struct XilinxPCIERoot {
> +    PCIBridge parent_obj;
> +} XilinxPCIERoot;
> +
> +typedef struct XilinxPCIEInt {
> +    uint32_t fifo_reg1;
> +    uint32_t fifo_reg2;
> +} XilinxPCIEInt;
> +
> +typedef struct XilinxPCIEHost {
> +    PCIExpressHost parent_obj;
> +
> +    char name[16];
> +
> +    uint32_t bus_nr;
> +    uint64_t cfg_base, cfg_size;
> +    uint64_t mmio_base, mmio_size;
> +    bool link_up;
> +
> +    union {
> +        qemu_irq irq;
> +        void *irq_void;
> +    };
> +
> +    MemoryRegion mmio, io;
> +
> +    XilinxPCIERoot root;
> +
> +    uint32_t intr;
> +    uint32_t intr_mask;
> +    XilinxPCIEInt intr_fifo[16];
> +    unsigned int intr_fifo_r, intr_fifo_w;
> +    uint32_t rpscr;
> +} XilinxPCIEHost;
> +
> +static inline XilinxPCIEHost *
> +xilinx_pcie_init(MemoryRegion *sys_mem, uint32_t bus_nr,
> +                 hwaddr cfg_base, uint64_t cfg_size,
> +                 hwaddr mmio_base, uint64_t mmio_size,
> +                 qemu_irq irq, bool link_up)
> +{
> +    DeviceState *dev;
> +    MemoryRegion *cfg, *mmio;
> +
> +    dev = qdev_create(NULL, TYPE_XILINX_PCIE_HOST);
> +
> +    qdev_prop_set_uint32(dev, "bus_nr", bus_nr);
> +    qdev_prop_set_uint64(dev, "cfg_base", cfg_base);
> +    qdev_prop_set_uint64(dev, "cfg_size", cfg_size);
> +    qdev_prop_set_uint64(dev, "mmio_base", mmio_base);
> +    qdev_prop_set_uint64(dev, "mmio_size", mmio_size);
> +    qdev_prop_set_ptr(dev, "irq", irq);
> +    qdev_prop_set_bit(dev, "link_up", link_up);
> +
> +    qdev_init_nofail(dev);
> +
> +    cfg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> +    memory_region_add_subregion_overlap(sys_mem, cfg_base, cfg, 0);
> +
> +    mmio = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
> +    memory_region_add_subregion_overlap(sys_mem, 0, mmio, 0);
> +
> +    return XILINX_PCIE_HOST(dev);
> +}

I don't like this function being used to create the device. I think it
should all be done in the machine.

I feel like it breaks the whole QOMification effort by creating
specific functions to init devices.

In saying that I don't know enough about what other PCI devices do for
this, so if this is usual then that's fine.

Thanks,

Alistair

> +
> +#endif /* HW_XILINX_PCIE_H */
> --
> 2.9.3
>
>