Re: [Qemu-devel] [RFC PATCH 6/8] generic_pci: generate dt node after devices init

[alvise rigo]

On Wed, Nov 5, 2014 at 1:26 PM, Claudio Fontana
<address@hidden> wrote:
> On 11.07.2014 09:21, Alvise Rigo wrote:
>> Keeping advantage of the finalize_dt QEMUMachine function, the mach-virt
>> machine now completes the device tree creation after that all the
>> generic devices have been instantiated. This allows to generate the
>> interrupt-map node according to the devices attached to the PCI bus.
>> These devices can be specified either as command line argument (like
>> -device lsi53c895a addr=0x5 ...) or explicitly inside the machine
>> definition with the pci_create_simple method.
>>
>> Fill also the generic_pci state with the offsets and sizes of the memory
>> regions needed by the PCI system. The offset in the machine address
>> space of these regions (config, IO and memory) is specified by the
>> mach-virt platform.
>
> If this is mach-virt specific, why is this called generic_pci..?
> Is it generic ARM pci?

The overall implementation shouldn't be mach-virt specific, but at the
moment I don't see any other use case for it.
Maybe in the future this controller could come in handy to support
other ARM platforms with PCI support.

>
>>
>> TODO:
>> - Part of the ranges device node is still hardcoded.
>
> Also this would benefit from extensive commenting.
> Especially the exact meaning of the interrupt mapping, and if it is ARM 
> specific,
> how the interrupts end up being mapped to gic irqs.
> [see below]
>
>>
>> Signed-off-by: Alvise Rigo <address@hidden>
>> ---
>>  hw/arm/virt.c                     | 80 +++++++++++++++++++--------------
>>  hw/pci-host/generic-pci.c         | 94 
>> +++++++++++++++++++++++++++++++++++++++
>>  include/hw/pci-host/pci_generic.h | 20 +++++++++
>>  3 files changed, 160 insertions(+), 34 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index a433902..e182282 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -358,44 +358,35 @@ static void create_pci_host(const VirtBoardInfo *vbi, 
>> qemu_irq *pic)
>>      PCIBus *pci_bus;
>>      DeviceState *dev;
>>      SysBusDevice *busdev;
>> -    uint32_t gic_phandle;
>> -    char *nodename;
>> -    int i;
>> +    PCIVPBState *ps;
>> +    int i, count = 0;
>>      hwaddr cfg_base = vbi->memmap[VIRT_PCI_CFG].base;
>> -    hwaddr cfg_size = vbi->memmap[VIRT_PCI_CFG].size;
>>      hwaddr io_base = vbi->memmap[VIRT_PCI_IO].base;
>> -    hwaddr io_size = vbi->memmap[VIRT_PCI_IO].size;
>>      hwaddr mem_base = vbi->memmap[VIRT_PCI_MEM].base;
>> -    hwaddr mem_size = vbi->memmap[VIRT_PCI_MEM].size;
>> -
>> -    nodename = g_strdup_printf("/address@hidden" PRIx64, cfg_base);
>> -    qemu_fdt_add_subnode(vbi->fdt, nodename);
>> -    qemu_fdt_setprop_string(vbi->fdt, nodename, "compatible",
>> -                            "pci-host-cam-generic");
>> -    qemu_fdt_setprop_string(vbi->fdt, nodename, "device_type", "pci");
>> -    qemu_fdt_setprop_cell(vbi->fdt, nodename, "#address-cells", 0x3);
>> -    qemu_fdt_setprop_cell(vbi->fdt, nodename, "#size-cells", 0x2);
>> -    qemu_fdt_setprop_cell(vbi->fdt, nodename, "#interrupt-cells", 0x1);
>> -
>> -    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg", 2, cfg_base,
>> -                                                           2, cfg_size);
>> -
>> -    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
>> -        1, 0x01000000, 2, 0x00000000, 2, io_base, 2, io_size,
>> -        1, 0x02000000, 2, 0x12000000, 2, mem_size, 2, mem_size);
>> -
>> -    gic_phandle = qemu_fdt_get_phandle(vbi->fdt, "/intc");
>> -    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "interrupt-map-mask",
>> -        1, 0xf800, 1, 0x0, 1, 0x0, 1, 0x7);
>> -    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "interrupt-map",
>> -        1, 0x0000, 2, 0x00000000, 1, 0x1, 1, gic_phandle, 1, 0, 1, 0x4, 1, 
>> 0x1,
>> -        1, 0x0800, 2, 0x00000000, 1, 0x1, 1, gic_phandle, 1, 0, 1, 0x5, 1, 
>> 0x1,
>> -        1, 0x1000, 2, 0x00000000, 1, 0x1, 1, gic_phandle, 1, 0, 1, 0x6, 1, 
>> 0x1,
>> -        1, 0x1800, 2, 0x00000000, 1, 0x1, 1, gic_phandle, 1, 0, 1, 0x7, 1, 
>> 0x1);
>>
>>      dev = qdev_create(NULL, "generic_pci");
>>      busdev = SYS_BUS_DEVICE(dev);
>> +    ps = PCI_GEN(dev);
>> +
>> +    /* Set the mapping data in the device structure where:
>> +     * ptr[i] = base address
>> +     * ptr[i+1] = size
>> +     * i = 0 => config
>> +     * i = 2 => I/O
>> +     * i = 4 => memory
>> +     * These values are needed by the specific device code.
>> +     * */
>> +    hwaddr *ptr = g_malloc(6 * sizeof(hwaddr));
>> +    for (i = VIRT_PCI_CFG; i <= VIRT_PCI_MEM; i++) {
>> +        ptr[count++] = vbi->memmap[i].base;
>> +        ptr[count++] = vbi->memmap[i].size;
>> +    }
>> +    ps->dt_data.fdt = vbi->fdt;
>> +    ps->dt_data.irq_base = vbi->irqmap[VIRT_PCI_CFG];
>> +    ps->dt_data.addr_mapping = ptr;
>> +
>>      qdev_init_nofail(dev);
>> +
>>      sysbus_mmio_map(busdev, 0, cfg_base); /* PCI config */
>>      sysbus_mmio_map(busdev, 1, io_base);  /* PCI I/O */
>>      sysbus_mmio_map(busdev, 2, mem_base); /* PCI memory window */
>> @@ -407,8 +398,6 @@ static void create_pci_host(const VirtBoardInfo *vbi, 
>> qemu_irq *pic)
>>      pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
>>      pci_create_simple(pci_bus, -1, "pci-ohci");
>>      pci_create_simple(pci_bus, -1, "lsi53c895a");
>> -
>> -    g_free(nodename);
>>  }
>>
>>  static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
>> @@ -454,6 +443,29 @@ static void *machvirt_dtb(const struct arm_boot_info 
>> *binfo, int *fdt_size)
>>      return board->fdt;
>>  }
>>
>> +static void machvirt_finalize_dt(MachineState *machine)
>> +{
>> +    BusState *bus;
>> +    BusChild *child;
>> +    VirtBoardInfo *vbi;
>> +    /* TODO store somewhere the CPU model used */
>> +    vbi = find_machine_info("cortex-a15");
>> +
>> +    /* If the device has been successfully created, create also its
>> +     * device node in the dt. */
>> +    bus = sysbus_get_default();
>> +    QTAILQ_FOREACH(child, &bus->children, sibling) {
>> +        DeviceState *dev = child->child;
>> +        if (!strcmp(object_get_typename(OBJECT(dev)), TYPE_GENERIC_PCI)) {
>> +            GenericPCIClass *gpci = GENERIC_PCI_GET_CLASS(OBJECT(dev));
>> +            /* create device tree node */
>> +            gpci->gen_dt_node(dev);
>> +        }
>> +    }
>> +
>> +    arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
>> +}
>> +
>>  static void machvirt_init(MachineState *machine)
>>  {
>>      MemoryRegion *sysmem = get_system_memory();
>> @@ -542,13 +554,13 @@ static void machvirt_init(MachineState *machine)
>>      vbi->bootinfo.board_id = -1;
>>      vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base;
>>      vbi->bootinfo.get_dtb = machvirt_dtb;
>> -    arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
>>  }
>>
>>  static QEMUMachine machvirt_a15_machine = {
>>      .name = "virt",
>>      .desc = "ARM Virtual Machine",
>>      .init = machvirt_init,
>> +    .finalize_dt = machvirt_finalize_dt,
>>      .max_cpus = 4,
>>  };
>>
>> diff --git a/hw/pci-host/generic-pci.c b/hw/pci-host/generic-pci.c
>> index 75aae45..2068079 100644
>> --- a/hw/pci-host/generic-pci.c
>> +++ b/hw/pci-host/generic-pci.c
>> @@ -14,6 +14,7 @@
>>  #include "hw/sysbus.h"
>>  #include "hw/pci-host/pci_generic.h"
>>  #include "exec/address-spaces.h"
>> +#include "sysemu/device_tree.h"
>>
>>  static const VMStateDescription pci_generic_host_vmstate = {
>>      .name = "generic-host-pci",
>> @@ -130,6 +131,96 @@ static void pci_generic_host_class_init(ObjectClass 
>> *klass, void *data)
>>      dc->cannot_instantiate_with_device_add_yet = true;
>>  }
>>
>> +struct dt_irq_mapping {
>> +        DeviceState *dev;
>> +        uint32_t gic_phandle;
>> +        int base_irq_num;
>> +        uint64_t *data;
>> +};
>> +
>> +#define IRQ_MAPPING_CELLS 14
>> +/* Generate the irq_mapping data and return the number of the device 
>> attached
>> + * to the device bus.
>> + * */
>> +static int generate_int_mapping(struct dt_irq_mapping *irq_map)
>> +{
>> +    BusState *inner_bus;
>> +    BusChild *inner;
>> +    int num_slots = 0;
>> +    uint64_t *data_ptr = irq_map->data;
>> +
>> +    QLIST_FOREACH(inner_bus, &irq_map->dev->child_bus, sibling) {
>> +        QTAILQ_FOREACH(inner, &inner_bus->children, sibling) {
>> +            DeviceState *dev = inner->child;
>> +            PCIDevice *pdev = PCI_DEVICE(dev);
>> +            int pci_slot = PCI_SLOT(pdev->devfn);
>> +
>> +            uint64_t buffer[IRQ_MAPPING_CELLS] =
>> +            {1, pci_slot << 11, 2, 0x00000000, 1, 0x1,
>> +             1, irq_map->gic_phandle, 1, 0, 1, irq_map->base_irq_num + 
>> pci_slot,
>> +             1, 0x1};
>
> Here. I think this needs way more commenting and an explanation about how 
> this works.

Yes, indeed. This interrupts mapping node is defined here:
http://www.unixag-kl.fh-kl.de/~jkunz/tmp/imap0_9d.pdf

The purpose of this mapping, in this specific case, is to map
interrupts from the PCI domain to the interrupt controller domain (the
GIC in our case).
This code produces a node like:
interrupt-map = <0x1800 0x0 0x0 0x1 0x8001 0x0 0x2f 0x1, ...>
where, for each tuple, the first four values specify the interrupt
according to the PCI domain: "0x1800 0x0 0x0" is the device's unit
address (PCI device slot number) while "0x1" is the PCI interrupt
number.
The fifth value (0x8001) represent the phandle of the interrupt
controller (the node that somehow defines the new domain).
The last three values define the interrupt in the new domain, and
follow the exact format required by the interrupt controller.
For sake of completeness, the GIC node is reported below (note phandle
address and #interrupt-cells).
intc {
        phandle = <0x8001>;
        reg = <0x0 0x8000000 0x0 0x10000 0x0 0x8010000 0x0 0x10000>;
        interrupt-controller;
        #interrupt-cells = <0x3>;
        compatible = "arm,cortex-a15-gic";
};

>
>> +
>> +            memcpy(data_ptr, buffer, IRQ_MAPPING_CELLS * sizeof(*buffer));
>> +            num_slots++;
>> +            data_ptr += IRQ_MAPPING_CELLS;
>> +        }
>> +    }
>> +
>> +    return num_slots;
>> +}
>> +
>> +static void generate_dt_node(DeviceState *dev)
>> +{
>> +    PCIVPBState *s = PCI_GEN(dev);
>> +    char *nodename;
>> +    uint32_t gic_phandle;
>> +    int num_dev;
>> +    hwaddr *map = s->dt_data.addr_mapping;
>> +    void *fdt = s->dt_data.fdt;
>> +
>> +    nodename = g_strdup_printf("/address@hidden" PRIx64, map[0]);
>> +    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);
>> +
>> +    /* config space */
>> +    qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", 2, map[0],
>> +                                                            2, map[1]);
>> +
>> +    qemu_fdt_setprop_sized_cells(fdt, nodename, "ranges",
>> +         1, 0x01000000, 2, 0x00000000, 2, map[2], 2, map[3],
>> +         1, 0x02000000, 2, 0x12000000, 2, map[4], 2, map[5]);
>
> hard coded mach-virt addresses in generic_pci: is it really generic pci, or 
> is it mach-virt pci?

The hardcoded values you see are related to the generic-pci device,
while the values passed inside the map array define how to map the
device regions to memory.
These values should be moved in the header file, since I don't expect
them to change.

>
>> +
>> +    gic_phandle = qemu_fdt_get_phandle(fdt, "/intc");
>> +    qemu_fdt_setprop_sized_cells(fdt, nodename, "interrupt-map-mask",
>> +                                  1, 0xf800, 1, 0x0, 1, 0x0, 1, 0x7);
>
> also here some clear comment and explanation would be beneficial.

The interrupt map property tells how to perform the lookup in the
interrupt-map table.
This mask is useful when not all the bits of a unit interrupt
specifier are relevant for the lookup. In the PCI case the value
0xf800 0x0 0x0 0x7 will keep only PCI slot number and interrupt number
(which in basically always 1).

Regards,
alvise

>
>> +
>> +    /* Generate the interrupt mapping according to the devices attached
>> +     * to the PCI bus of the device. */
>> +    uint64_t *int_mapping_data = g_malloc0(IRQ_MAPPING_CELLS * 
>> sizeof(uint64_t)
>> +                                                            * 
>> MAX_PCI_DEVICES);
>> +
>> +    struct dt_irq_mapping dt_map = {
>> +        .dev = dev,
>> +        .gic_phandle = gic_phandle,
>> +        .base_irq_num = s->dt_data.irq_base,
>> +        .data = int_mapping_data
>> +    };
>> +
>> +    num_dev = generate_int_mapping(&dt_map);
>> +    qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "interrupt-map",
>> +                        (num_dev * IRQ_MAPPING_CELLS)/2, int_mapping_data);
>> +
>> +    g_free(int_mapping_data);
>> +    g_free(nodename);
>> +    /* Once the dt node is created, this data is no longer necessary */
>> +    g_free(s->dt_data.addr_mapping);
>> +}
>> +
>>  static const TypeInfo pci_generic_host_info = {
>>      .name          = TYPE_GENERIC_PCI_HOST,
>>      .parent        = TYPE_PCI_DEVICE,
>> @@ -140,9 +231,11 @@ static const TypeInfo pci_generic_host_info = {
>>  static void pci_generic_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> +    GenericPCIClass *pc = GENERIC_PCI_CLASS(klass);
>>
>>      dc->realize = pci_generic_host_realize;
>>      dc->vmsd = &pci_generic_host_vmstate;
>> +    pc->gen_dt_node = generate_dt_node;
>>  }
>>
>>  static const TypeInfo pci_generic_info = {
>> @@ -151,6 +244,7 @@ static const TypeInfo pci_generic_info = {
>>      .instance_size = sizeof(PCIVPBState),
>>      .instance_init = pci_generic_host_init,
>>      .class_init    = pci_generic_class_init,
>> +    .class_size    = sizeof(GenericPCIClass),
>>  };
>>
>>  static void generic_pci_host_register_types(void)
>> diff --git a/include/hw/pci-host/pci_generic.h 
>> b/include/hw/pci-host/pci_generic.h
>> index 2367d80..039549f 100644
>> --- a/include/hw/pci-host/pci_generic.h
>> +++ b/include/hw/pci-host/pci_generic.h
>> @@ -7,6 +7,12 @@
>>
>>  #define MAX_PCI_DEVICES 10
>>
>> +struct dt_data {
>> +    void *fdt;
>> +    int irq_base;
>> +    hwaddr *addr_mapping;
>> +};
>> +
>>  typedef struct {
>>      PCIHostState parent_obj;
>>
>> @@ -22,8 +28,17 @@ typedef struct {
>>      MemoryRegion pci_mem_window;
>>      PCIBus pci_bus;
>>      PCIDevice pci_dev;
>> +    /* Device tree data set by the machine
>> +     */
>> +    struct dt_data dt_data;
>>  } PCIVPBState;
>>
>> +typedef struct GenericPCIClass {
>> +    PCIDeviceClass parent_class;
>> +
>> +    void (*gen_dt_node)(DeviceState *dev);
>> +} GenericPCIClass;
>> +
>>  #define TYPE_GENERIC_PCI "generic_pci"
>>  #define PCI_GEN(obj) \
>>      OBJECT_CHECK(PCIVPBState, (obj), TYPE_GENERIC_PCI)
>> @@ -32,4 +47,9 @@ typedef struct {
>>  #define PCI_GEN_HOST(obj) \
>>      OBJECT_CHECK(PCIDevice, (obj), TYPE_GENERIC_PCIHOST)
>>
>> +#define GENERIC_PCI_CLASS(klass) \
>> +     OBJECT_CLASS_CHECK(GenericPCIClass, (klass), TYPE_GENERIC_PCI)
>> +#define GENERIC_PCI_GET_CLASS(obj) \
>> +    OBJECT_GET_CLASS(GenericPCIClass, (obj), TYPE_GENERIC_PCI)
>> +
>>  #endif
>>
>
>
>
>