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

[Claudio Fontana]

On 21.11.2014 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

Would it not be better to add rob herring's signoff, and then the explanation
of what you changed from his patch, followed by your signoff?
Or did you change so much that you had to redo the original work by Rob Herring
from scratch?

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

That's a pretty big limitation for a "generic" controller.
If it's only about the size of a parent interrupt cell or something like
that, why don't we provide it as part of the platform description that
you pass to the machinery anyway?

> 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)
> +{

I would add a comment:
/* the MemoryRegionOps require uint64_t val, but we can only do 32bit */

there are already asserts in pci_host.c, so that should be sufficient.

> +    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)

/* same here */

> +{
> +    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,
> +};
> +
> +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);
> +    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);

TYPE_PCI_BUS, until we actually support PCIE.

> +    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;
> +    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. */

s/mememory/memory/

> +    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);
> +
> +    /* TODO Remove once realize propagates to child devices. */
> +    object_property_set_bool(OBJECT(&s->pci_gen), true, "realized", errp);
> +}
> +
> +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;
> +    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;

cannot this be generic enough that we don't talk about gic here?

> +        int base_irq_num;
> +        uint64_t *data;
> +};
> +
> +/*  */

? Can you remove this empty comment ?


> +#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};

ah now I see, I think you forgot a comment above. But maybe here is a better 
place.
The above needs to be commented heavily, to make it obvious what each field is,
and which "spec" this is following. You have mentioned already in the previous 
discussion,
but it needs to be in the code.

> +
> +        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 */

interrupt-map

> +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);
> +}
> +
> +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 */

Is it commented somewhere sensible that the generic pci has this layout:

cfg = whatever base address
io = base + 0x1000000
mem = base + 0x2000000

> +                        2, mem->addr, 2, memory_region_size(mem));
> +
> +    gic_phandle = qemu_fdt_get_phandle(fdt, s->plat_data.gic_node_name);

hmm could it be gic-independent?

> +    /* 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);

and what about 0, 0, 7, what do they mean? How does the mapping work,
what do all the fields mean, and which paper/spec are you implementing?..

> +
> +    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,

could this be gic independent?

> +        .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);
> +}
> +
> +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)
> +

maybe this would be a good place to talk about what the address space
layout assumptions of generic pci are, like 

/* The Configuration space starts at the PCI base address passed in the virtual 
platform
 * information,
 * The I/O space starts at an offset of 0x1000000 from the PCI base address
 * The Mem space starts at an offset of 0x2000000 from the PCI base address
 */

these the assumptions right?

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