Re: [Qemu-devel] [RFC v2 3/4] hw/pci-host: Add a generic PCI host controller for virtual platforms

[Alexander Graf]

On 21.11.14 19:07, Alvise Rigo wrote:
> Add a generic PCI host controller for virtual platforms, based on the
> previous work by Rob Herring:
> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03482.html
> 
> The controller creates a PCI bus for PCI devices; it is intended to
> receive from the platform all the needed information to initiate the
> memory regions expected by the PCI system. Due to this dependence, a
> header file is used to define the structure that the platform has to
> fill with the data needed by the controller. The device provides also a
> routine for the device tree node generation, which mostly has to take
> care of the interrupt-map property generation. This property is fetched
> by the guest operating system to map any PCI interrupt to the interrupt
> controller. For the time being, the device expects a GIC v2 to be used
> by the guest.
> Only mach-virt has been used to test the controller.
> 
> Signed-off-by: Alvise Rigo <address@hidden>
> ---
>  hw/pci-host/Makefile.objs         |   2 +-
>  hw/pci-host/generic-pci.c         | 280 
> ++++++++++++++++++++++++++++++++++++++
>  include/hw/pci-host/generic-pci.h |  74 ++++++++++
>  3 files changed, 355 insertions(+), 1 deletion(-)
>  create mode 100644 hw/pci-host/generic-pci.c
>  create mode 100644 include/hw/pci-host/generic-pci.h
> 
> diff --git a/hw/pci-host/Makefile.objs b/hw/pci-host/Makefile.objs
> index bb65f9c..8ef9fac 100644
> --- a/hw/pci-host/Makefile.objs
> +++ b/hw/pci-host/Makefile.objs
> @@ -1,4 +1,4 @@
> -common-obj-y += pam.o
> +common-obj-y += pam.o generic-pci.o
>  
>  # PPC devices
>  common-obj-$(CONFIG_PREP_PCI) += prep.o
> diff --git a/hw/pci-host/generic-pci.c b/hw/pci-host/generic-pci.c
> new file mode 100644
> index 0000000..9c90263
> --- /dev/null
> +++ b/hw/pci-host/generic-pci.c
> @@ -0,0 +1,280 @@
> +/*
> + * Generic PCI host controller
> + *
> + * Copyright (c) 2014 Linaro, Ltd.
> + * Author: Rob Herring <address@hidden>
> + *
> + * Based on ARM Versatile PCI controller (hw/pci-host/versatile.c):
> + * Copyright (c) 2006-2009 CodeSourcery.
> + * Written by Paul Brook
> + *
> + * This code is licensed under the LGPL.
> + */
> +
> +#include "hw/sysbus.h"
> +#include "hw/pci-host/generic-pci.h"
> +#include "exec/address-spaces.h"
> +#include "sysemu/device_tree.h"
> +
> +static const VMStateDescription pci_generic_host_vmstate = {
> +    .name = "generic-host-pci",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +};
> +
> +static void pci_cam_config_write(void *opaque, hwaddr addr,
> +                                 uint64_t val, unsigned size)
> +{
> +    PCIGenState *s = opaque;
> +    pci_data_write(&s->pci_bus, addr, val, size);
> +}
> +
> +static uint64_t pci_cam_config_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    PCIGenState *s = opaque;
> +    uint32_t val;
> +    val = pci_data_read(&s->pci_bus, addr, size);
> +    return val;
> +}
> +
> +static const MemoryRegionOps pci_vpb_config_ops = {
> +    .read = pci_cam_config_read,
> +    .write = pci_cam_config_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,

Never use NATIVE_ENDIAN unless you're 100% sure it's correct. In this
case, please make it LITTLE_ENDIAN.

> +};
> +
> +static void pci_generic_set_irq(void *opaque, int irq_num, int level)
> +{
> +    qemu_irq *pic = opaque;
> +    qemu_set_irq(pic[irq_num], level);
> +}
> +
> +static void pci_generic_host_init(Object *obj)
> +{
> +    PCIHostState *h = PCI_HOST_BRIDGE(obj);
> +    PCIGenState *s = PCI_GEN(obj);
> +    GenericPCIHostState *gps = &s->pci_gen;
> +
> +    memory_region_init(&s->pci_io_space, OBJECT(s), "pci_io", 1ULL << 32);

Why is IO space that big? It's only 64k usually, no?

> +    memory_region_init(&s->pci_mem_space, OBJECT(s), "pci_mem", 1ULL << 32);
> +
> +    pci_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), "pci",
> +                        &s->pci_mem_space, &s->pci_io_space,
> +                        PCI_DEVFN(0, 0), TYPE_PCIE_BUS);
> +    h->bus = &s->pci_bus;
> +
> +    object_initialize(gps, sizeof(*gps), TYPE_GENERIC_PCI_HOST);
> +    qdev_set_parent_bus(DEVICE(gps), BUS(&s->pci_bus));
> +    gps->devfns = 0;
> +}
> +
> +static int generic_pci_map_irq_fn(PCIDevice *pci_dev, int pin)
> +{
> +    BusState *bus = qdev_get_parent_bus(&pci_dev->qdev);
> +    PCIBus *pci_bus = PCI_BUS(bus);
> +    PCIDevice *pdev = pci_bus->devices[PCI_DEVFN(0, 0)];
> +    GenericPCIHostState *gps = PCI_GEN_HOST(pdev);
> +
> +    return gps->irqmap.devfn_idx_map[PCI_SLOT(pci_dev->devfn)]
> +                                    [PCI_FUNC(pci_dev->devfn)];
> +}
> +
> +static void pci_generic_host_realize(DeviceState *dev, Error **errp)
> +{
> +    PCIGenState *s = PCI_GEN(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    GenericPCIPlatformData *pdata = &s->plat_data;

Where does this come from? Why isn't it exposed as qdev parameters?

> +    int i;
> +
> +    if (!pdata->addr_map[PCI_CFG].size || !pdata->addr_map[PCI_IO].size ||
> +                           !pdata->addr_map[PCI_MEM].size || !pdata->irqs) {
> +        hw_error("generic_pci: PCI controller not fully configured.");
> +    }
> +
> +    for (i = 0; i < pdata->irqs; i++) {
> +        sysbus_init_irq(sbd, &s->irq[i]);
> +    }
> +
> +    pci_bus_irqs(&s->pci_bus, pci_generic_set_irq, generic_pci_map_irq_fn,
> +                 s->irq, pdata->irqs);
> +
> +    memory_region_init_io(&s->mem_config, OBJECT(s), &pci_vpb_config_ops, s,
> +                          "pci-config", pdata->addr_map[PCI_CFG].size);
> +    sysbus_init_mmio(sbd, &s->mem_config);
> +
> +    /* Depending on the available memory of the board using the controller, 
> we
> +       create a window on both the I/O and mememory space. */

meme?

> +    memory_region_init_alias(&s->pci_io_window, OBJECT(s), "pci-io-win",
> +                             &s->pci_io_space, 0,
> +                             pdata->addr_map[PCI_IO].size);
> +    sysbus_init_mmio(sbd, &s->pci_io_window);
> +
> +    memory_region_init_alias(&s->pci_mem_window, OBJECT(s), "pci-mem-win",
> +                             &s->pci_mem_space,
> +                             pdata->addr_map[PCI_MEM].addr,
> +                             pdata->addr_map[PCI_MEM].size);
> +    sysbus_init_mmio(sbd, &s->pci_mem_window);

What if we simply postpone the creation of those regions and the
creation of the PCIBus to the realize function? At that point we know
the size of the regions.

> +
> +    /* TODO Remove once realize propagates to child devices. */
> +    object_property_set_bool(OBJECT(&s->pci_gen), true, "realized", errp);

Is this still necessary? In fact, wouldn't the realize of our PHB and
the creation of child devices happen consecutively usually?

> +}
> +
> +static void pci_generic_host_class_init(ObjectClass *klass, void *data)
> +{
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    k->vendor_id = PCI_VENDOR_ID_REDHAT;
> +    k->device_id = 0x1234;

Is this a reserved ID?

> +    k->class_id = PCI_CLASS_PROCESSOR_CO;
> +    /*
> +     * 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;
> +}
> +
> +struct dt_irq_mapping {
> +        int irqs;
> +        uint32_t gic_phandle;
> +        int base_irq_num;
> +        uint64_t *data;
> +};
> +
> +/*  */
> +#define IRQ_MAP_ENTRY_DESC_SZ 14
> +static void gen_int_mapping_fn(PCIBus *b, PCIDevice *d, void *opaque)
> +{
> +    struct dt_irq_mapping *map_data;
> +    int pin;
> +
> +    PCIDevice *pdev = b->devices[PCI_DEVFN(0, 0)];
> +    GenericPCIHostState *gps = PCI_GEN_HOST(pdev);
> +    map_data = (struct dt_irq_mapping *)opaque;
> +
> +    /* Check if the platform has enough IRQs available. */
> +    if (gps->devfns > map_data->irqs) {
> +        hw_error("generic_pci: too many PCI devices.");
> +    }
> +
> +    pin = pci_get_byte(d->config + PCI_INTERRUPT_PIN);
> +    if (pin) {
> +        uint64_t *data_ptr = map_data->data + (IRQ_MAP_ENTRY_DESC_SZ * 
> gps->devfns);
> +        uint8_t pci_slot = PCI_SLOT(d->devfn);
> +        uint8_t pci_func = PCI_FUNC(d->devfn);
> +        uint8_t *devfn_idx = &gps->irqmap.devfn_idx_map[pci_slot][0];
> +        uint8_t *devfn_irq = gps->irqmap.devfn_irq_map;
> +
> +        devfn_idx[pci_func] = gps->devfns;
> +        devfn_irq[gps->devfns] = map_data->base_irq_num + gps->devfns;
> +
> +        uint64_t buffer[IRQ_MAP_ENTRY_DESC_SZ] =
> +        {1, (pci_slot << 11)|(pci_func << 8), 2, 0x00000000, 1, pin,
> +         1, map_data->gic_phandle, 1, 0x0, 1, devfn_irq[gps->devfns],
> +         1, 0x1};
> +
> +        memcpy(data_ptr, buffer, IRQ_MAP_ENTRY_DESC_SZ * sizeof(*buffer));
> +        gps->devfns++;
> +    }
> +}
> +
> +/* Generate the "interrup-map" node's data and store it in map_data */
> +static void generate_int_mapping(struct dt_irq_mapping *map_data,
> +                                 PCIGenState *s)
> +{
> +    pci_for_each_device(s->parent_obj.bus, 0, gen_int_mapping_fn, map_data);

The interrupt-map should describe interrupt mappings for every slot, not
every device. If you only describe the current state of affairs, hotplug
won't work.

> +}
> +
> +static void generate_dt_node(void *fdt, DeviceState *dev)
> +{
> +    PCIGenState *s = PCI_GEN(dev);
> +    char *nodename;
> +    uint32_t gic_phandle;
> +    uint32_t plat_acells;
> +    uint32_t plat_scells;
> +
> +    SysBusDevice *busdev;
> +    busdev = SYS_BUS_DEVICE(dev);
> +    MemoryRegion *cfg = sysbus_mmio_get_region(busdev, 0);
> +    MemoryRegion *io = sysbus_mmio_get_region(busdev, 1);
> +    MemoryRegion *mem = sysbus_mmio_get_region(busdev, 2);
> +
> +    nodename = g_strdup_printf("/address@hidden" PRIx64, cfg->addr);
> +    qemu_fdt_add_subnode(fdt, nodename);
> +    qemu_fdt_setprop_string(fdt, nodename, "compatible",
> +                            "pci-host-cam-generic");
> +    qemu_fdt_setprop_string(fdt, nodename, "device_type", "pci");
> +    qemu_fdt_setprop_cell(fdt, nodename, "#address-cells", 0x3);
> +    qemu_fdt_setprop_cell(fdt, nodename, "#size-cells", 0x2);
> +    qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 0x1);
> +
> +    plat_acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells");
> +    plat_scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells");
> +    qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", plat_acells, 
> cfg->addr,
> +                                        plat_scells, 
> memory_region_size(cfg));
> +
> +    qemu_fdt_setprop_sized_cells(fdt, nodename, "ranges",
> +                        1, 0x01000000, 2, 0x00000000, /* I/O space */
> +                        2, io->addr, 2, memory_region_size(io),
> +                        1, 0x02000000, 2, mem->addr, /* 32bit memory space */
> +                        2, mem->addr, 2, memory_region_size(mem));
> +
> +    gic_phandle = qemu_fdt_get_phandle(fdt, s->plat_data.gic_node_name);
> +    /* A mask covering bits [15,8] of phys.high allows to honor the
> +       function number when resolving a triggered PCI interrupt. */
> +    qemu_fdt_setprop_sized_cells(fdt, nodename, "interrupt-map-mask",
> +                                  1, 0xff00, 1, 0x0, 1, 0x0, 1, 0x7);
> +
> +    uint64_t *int_mapping_data = g_malloc0(IRQ_MAP_ENTRY_DESC_SZ *
> +                                          sizeof(uint64_t) * 
> s->plat_data.irqs);
> +    struct dt_irq_mapping dt_map_data = {
> +        .irqs = s->plat_data.irqs,
> +        .gic_phandle = gic_phandle,
> +        .base_irq_num = s->plat_data.base_irq,
> +        .data = int_mapping_data
> +    };
> +    /* Generate the interrupt mapping according to the devices attached
> +     * to the PCI bus of the controller. */
> +    generate_int_mapping(&dt_map_data, s);
> +    qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "interrupt-map",
> +                (s->pci_gen.devfns * IRQ_MAP_ENTRY_DESC_SZ)/2, 
> int_mapping_data);
> +
> +    g_free(int_mapping_data);
> +    g_free(nodename);
> +}
> +
> +void pci_controller_build_dt_node(void *fdt, DeviceState *dev)
> +{
> +    generate_dt_node(fdt, dev);

Device tree generation should *never* be part of device code. It's
machine / board specific. If one day someone can convince me that it's
safe to share device tree bits of one device with multiple boards I'm
happy to work with them to achieve that goal then, but for now, please
leave device tree bits in the machine logic.


Alex

> +}
> +
> +static const TypeInfo pci_generic_host_info = {
> +    .name          = TYPE_GENERIC_PCI_HOST,
> +    .parent        = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(GenericPCIHostState),
> +    .class_init    = pci_generic_host_class_init,
> +};
> +
> +static void pci_generic_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = pci_generic_host_realize;
> +    dc->vmsd = &pci_generic_host_vmstate;
> +}
> +
> +static const TypeInfo pci_generic_info = {
> +    .name          = TYPE_GENERIC_PCI,
> +    .parent        = TYPE_PCI_HOST_BRIDGE,
> +    .instance_size = sizeof(PCIGenState),
> +    .instance_init = pci_generic_host_init,
> +    .class_init    = pci_generic_class_init,
> +};
> +
> +static void generic_pci_host_register_types(void)
> +{
> +    type_register_static(&pci_generic_info);
> +    type_register_static(&pci_generic_host_info);
> +}
> +
> +type_init(generic_pci_host_register_types)
> \ No newline at end of file
> diff --git a/include/hw/pci-host/generic-pci.h 
> b/include/hw/pci-host/generic-pci.h
> new file mode 100644
> index 0000000..43c7a0f
> --- /dev/null
> +++ b/include/hw/pci-host/generic-pci.h
> @@ -0,0 +1,74 @@
> +#ifndef QEMU_GENERIC_PCI_H
> +#define QEMU_GENERIC_PCI_H
> +
> +#include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
> +#include "hw/pci/pci_host.h"
> +
> +#define MAX_PCI_DEVICES (PCI_SLOT_MAX * PCI_FUNC_MAX)
> +
> +enum {
> +    PCI_CFG = 0,
> +    PCI_IO,
> +    PCI_MEM,
> +};
> +
> +typedef struct {
> +    /* addr_map[PCI_CFG].addr = base address of configuration memory
> +       addr_map[PCI_CFG].size = configuration memory size
> +       ... */
> +    struct addr_map {
> +        hwaddr addr;
> +        hwaddr size;
> +    } addr_map[3];
> +
> +    const char *gic_node_name;
> +    uint32_t base_irq;
> +    uint32_t irqs;
> +} GenericPCIPlatformData;
> +
> +typedef struct {
> +    PCIDevice parent_obj;
> +
> +    struct irqmap {
> +        /* devfn_idx_map[i][j] = index inside slot_irq_map for device at 
> slot i,
> +           fn j */
> +        uint8_t devfn_idx_map[PCI_SLOT_MAX][PCI_FUNC_MAX];
> +        /* devfn_irq_map[i] = IRQ num. of the i-th device (devfn) attached to
> +           the bus */
> +        uint8_t devfn_irq_map[MAX_PCI_DEVICES];
> +    } irqmap;
> +    /* number of devices attached to the bus */
> +    uint32_t devfns;
> +} GenericPCIHostState;
> +
> +typedef struct {
> +    PCIHostState parent_obj;
> +
> +    qemu_irq irq[MAX_PCI_DEVICES];
> +    MemoryRegion mem_config;
> +    /* Containers representing the PCI address spaces */
> +    MemoryRegion pci_io_space;
> +    MemoryRegion pci_mem_space;
> +    /* Alias regions into PCI address spaces which we expose as sysbus 
> regions.
> +     * The offsets into pci_mem_space is fixed. */
> +    MemoryRegion pci_io_window;
> +    MemoryRegion pci_mem_window;
> +    PCIBus pci_bus;
> +    GenericPCIHostState pci_gen;
> +
> +    /* Platform dependent data */
> +    GenericPCIPlatformData plat_data;
> +} PCIGenState;
> +
> +#define TYPE_GENERIC_PCI "generic_pci"
> +#define PCI_GEN(obj) \
> +    OBJECT_CHECK(PCIGenState, (obj), TYPE_GENERIC_PCI)
> +
> +#define TYPE_GENERIC_PCI_HOST "generic_pci_host"
> +#define PCI_GEN_HOST(obj) \
> +    OBJECT_CHECK(GenericPCIHostState, (obj), TYPE_GENERIC_PCI_HOST)
> +
> +void pci_controller_build_dt_node(void *fdt, DeviceState *dev);
> +
> +#endif
> \ No newline at end of file
>