Re: [Qemu-devel] [RFC v3 2/2] hw/arm/virt: add generic-pci PCI host controller

[alvise rigo]

Hi Claudio,

On Wed, Jan 14, 2015 at 2:10 PM, Claudio Fontana
<address@hidden> wrote:
> On 14.01.2015 11:16, Alvise Rigo wrote:
>> The platform memory map has now three more memory ranges to map the
>> device's memory regions (Configuration region, I/O region and Memory
>> region).
>>
>> The dt node interrupt-map property tells how to route the PCI interrupts
>> to system interrupts. In the mach-virt case, four IRQs are swizzled
>> between all the possible interrupts coming from the PCI bus. In
>> particular, the mapping ensures that the first four devices will use a
>> system IRQ always different (supposing that only PIN_A of each device is
>> used).
>>
>> Now that a PCI bus is provided, the machine can be launched with
>> multiple PCI devices through the -device option (e.g., -device
>> virtio-blk-pci).
>>
>> Signed-off-by: Alvise Rigo <address@hidden>
>> ---
>>  hw/arm/virt.c | 112 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 111 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 2353440..7b0326f 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -44,6 +44,7 @@
>>  #include "qemu/error-report.h"
>>
>>  #define NUM_VIRTIO_TRANSPORTS 32
>> +#define NUM_PCI_IRQS 4
>>
>>  /* Number of external interrupt lines to configure the GIC with */
>>  #define NUM_IRQS 128
>> @@ -68,8 +69,12 @@ enum {
>>      VIRT_UART,
>>      VIRT_MMIO,
>>      VIRT_RTC,
>> +    VIRT_PCI_CFG,
>> +    VIRT_PCI_IO,
>> +    VIRT_PCI_MEM,
>>      VIRT_FW_CFG,
>>  };
>> +#define VIRT_PCI VIRT_PCI_CFG
>>
>>  typedef struct MemMapEntry {
>>      hwaddr base;
>> @@ -129,13 +134,17 @@ static const MemMapEntry a15memmap[] = {
>>      [VIRT_FW_CFG] =     { 0x09020000, 0x0000000a },
>>      [VIRT_MMIO] =       { 0x0a000000, 0x00000200 },
>>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size 
>> */
>> -    /* 0x10000000 .. 0x40000000 reserved for PCI */
>> +    /* PCI regions */
>> +    [VIRT_PCI_CFG] =    { 0x10000000, 0x01000000 },
>> +    [VIRT_PCI_IO] =     { 0x11000000, 0x00010000 },
>> +    [VIRT_PCI_MEM] =    { 0x12000000, 0x2e000000 },
>>      [VIRT_MEM] =        { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
>>  };
>>
>>  static const int a15irqmap[] = {
>>      [VIRT_UART] = 1,
>>      [VIRT_RTC] = 2,
>> +    [VIRT_PCI] = 3, /* ...to 3 + NUM_PCI_IRQS - 1 */
>>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>>  };
>>
>> @@ -436,6 +445,105 @@ static void create_rtc(const VirtBoardInfo *vbi, 
>> qemu_irq *pic)
>>      g_free(nodename);
>>  }
>>
>> +static void create_pci_host(const VirtBoardInfo *vbi, qemu_irq *pic)
>> +{
>> +    DeviceState *dev;
>> +    SysBusDevice *busdev;
>> +    int i, j, intmap_rows;
>> +    const MemMapEntry *mme = vbi->memmap;
>> +    uint32_t plat_acells;
>> +    uint32_t plat_scells;
>> +    uint32_t gic_phandle;
>> +    char *nodename;
>> +
>> +    dev = qdev_create(NULL, "generic_pci");
>> +    qdev_prop_set_uint64(dev, "mmio_win_size", mme[VIRT_PCI_MEM].size);
>> +    qdev_prop_set_uint64(dev, "mmio_win_addr", mme[VIRT_PCI_MEM].base);
>> +    qdev_prop_set_uint32(dev, "irqs", NUM_PCI_IRQS);
>> +    qdev_init_nofail(dev);
>> +
>> +    busdev = SYS_BUS_DEVICE(dev);
>> +    sysbus_mmio_map(busdev, 0, mme[VIRT_PCI_CFG].base); /* PCI config */
>> +    sysbus_mmio_map(busdev, 1, mme[VIRT_PCI_IO].base);  /* PCI I/O */
>> +    sysbus_mmio_map(busdev, 2, mme[VIRT_PCI_MEM].base); /* PCI memory 
>> window */
>> +
>> +    for ( i = 0; i < NUM_PCI_IRQS; i++) {
>> +        sysbus_connect_irq(busdev, i, pic[vbi->irqmap[VIRT_PCI] + i]);
>> +    }
>> +
>> +    /* add device tree node */
>> +    nodename = g_strdup_printf("/address@hidden" PRIx64, 
>> mme[VIRT_PCI_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);
>> +
>> +    plat_acells = qemu_fdt_getprop_cell(vbi->fdt, "/", "#address-cells");
>> +    plat_scells = qemu_fdt_getprop_cell(vbi->fdt, "/", "#size-cells");
>> +    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
>> +                                    plat_acells, mme[VIRT_PCI_CFG].base,
>> +                                    plat_scells, mme[VIRT_PCI_CFG].size);
>> +
>> +    /* Two regions (second and third of *busdev):
>> +       - I/O region, PCI addr = 0x0, CPU addr = mme[VIRT_PCI_IO].base,
>> +         size = mme[VIRT_PCI_IO].size,
>> +       - 32bit MMIO region, PCI addr = CPU addr = mme[VIRT_PCI_MEM].base,
>> +         size = mme[VIRT_PCI_MEM].size */
>> +    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
>> +            1, 0x01000000, 2, 0x00000000, /* I/O space */
>> +            2, mme[VIRT_PCI_IO].base, 2, mme[VIRT_PCI_IO].size,
>> +            1, 0x02000000, 2, mme[VIRT_PCI_MEM].base, /* 32bit memory space 
>> */
>> +            2, mme[VIRT_PCI_MEM].base, 2, mme[VIRT_PCI_MEM].size);
>> +
>> +    gic_phandle = qemu_fdt_get_phandle(vbi->fdt, "/intc");
>> +
>> +#define IRQ_MAP_ENTRY_DESC_SZ 14
>> +#define PHYSH_DEV_SHIFT 11
>> +    /* Any interrupt from the PCI bus is represented by four 32bit values 
>> <physH
>> +       physM physL pin#> (unit-interrupt-specifier), where bits physH[11:15]
>> +       identify the device number and pin# the pin number that triggered. 
>> We use
>> +       the two bits physH[14:15] and pin# to route all the PCI interrupts to
>> +       four different system interrupts. The following mask is used to 
>> honour
>> +       exactly those bits. */
>> +    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "interrupt-map-mask",
>> +                    1, 0x3 << PHYSH_DEV_SHIFT, 1, 0x0, 1, 0x0, 1, 0x7);
>> +
>> +    /* interrupt-map property
>> +       Two bits of physH[14:15] and four possible values of pin# give a 
>> maximum
>> +       of 16 different results of the mask operation:
>> +       <physH physM physL pin#> AND <interrupt-map-mask>
>> +       Cycling through the four system interrupts available, we assign to 
>> each
>> +       result an interrupt number (basically, 32 * 4 maximum PCI interrupts 
>> are
>> +       mapped to 4 system interrupts).
>> +       Each entry inside interrupt-map has the following form:
>> +       <PCI unit-interrupt-specifier, system unit-interrupt-specifier> */
>> +    intmap_rows = 16;
>> +    uint64_t *int_mapping_data = g_malloc0(IRQ_MAP_ENTRY_DESC_SZ *
>> +                                  sizeof(uint64_t) * intmap_rows);
>> +    uint64_t *int_map_data = int_mapping_data;
>> +    for (i = 0; i < 4; i++) {
>> +        for (j = 0; j < 4; j++) {
>> +           uint64_t physh_dev_nr = i << PHYSH_DEV_SHIFT;
>> +           uint64_t pci_pin_nr = j + 1;
>> +           uint64_t row[IRQ_MAP_ENTRY_DESC_SZ] =
>> +           {1, physh_dev_nr, 2, 0x0, 1, pci_pin_nr, // masked PCI 
>> interrupt...
>> +            1, gic_phandle, 1, GIC_FDT_IRQ_TYPE_SPI, // ...mapped to this 
>> IRQ
>> +            1, (vbi->irqmap[VIRT_PCI_CFG] + (j + i) % 4),
>> +            1, GIC_FDT_IRQ_FLAGS_LEVEL_HI};
>> +           int_map_data = mempcpy(int_map_data, row, sizeof(row));
>> +        }
>> +    }
>> +
>
> sorry for maybe being pedantic, but is this working as intended?

Yes, four interrupts are used for all the PCI IRQs.

> The same PHYS_HI is mapped multiple times to a different irq if I understand 
> this correctly, ending up in these mappings:
>
> B,D,F irqmap-mask   0x00001800
> PHYS_HI > SPI irq mappings
>
> 0x00000000 -> SPI irq 3
> 0x00000000 -> SPI irq 4
> 0x00000000 -> SPI irq 5
> 0x00000000 -> SPI irq 6
>
> 0x00000800 -> SPI irq 3
> 0x00000800 -> SPI irq 4
> 0x00000800 -> SPI irq 5
> 0x00000800 -> SPI irq 6
>
> 0x00001000 -> SPI irq 3
> 0x00001000 -> SPI irq 4
> 0x00001000 -> SPI irq 5
> 0x00001000 -> SPI irq 6
>
> 0x00001800 -> SPI irq 3
> 0x00001800 -> SPI irq 4
> 0x00001800 -> SPI irq 5
> 0x00001800 -> SPI irq 6

The interrupt-map generated is the following:

0x0    0x0 0x0 0x1 0x8001 0x0 0x3 0x4
0x0    0x0 0x0 0x2 0x8001 0x0 0x4 0x4
0x0    0x0 0x0 0x3 0x8001 0x0 0x5 0x4
0x0    0x0 0x0 0x4 0x8001 0x0 0x6 0x4
0x800  0x0 0x0 0x1 0x8001 0x0 0x4 0x4
0x800  0x0 0x0 0x2 0x8001 0x0 0x5 0x4
0x800  0x0 0x0 0x3 0x8001 0x0 0x6 0x4
0x800  0x0 0x0 0x4 0x8001 0x0 0x3 0x4
0x1000 0x0 0x0 0x1 0x8001 0x0 0x5 0x4
0x1000 0x0 0x0 0x2 0x8001 0x0 0x6 0x4
0x1000 0x0 0x0 0x3 0x8001 0x0 0x3 0x4
0x1000 0x0 0x0 0x4 0x8001 0x0 0x4 0x4
0x1800 0x0 0x0 0x1 0x8001 0x0 0x6 0x4
0x1800 0x0 0x0 0x2 0x8001 0x0 0x3 0x4
0x1800 0x0 0x0 0x3 0x8001 0x0 0x4 0x4
0x1800 0x0 0x0 0x4 0x8001 0x0 0x5 0x4

Now, removing the INT_{B,C,D} information we have:
0x0       0x0 0x0 0x1 0x8001 0x0 0x3 0x4
0x800   0x0 0x0 0x1 0x8001 0x0 0x4 0x4
0x1000 0x0 0x0 0x1 0x8001 0x0 0x5 0x4
0x1800 0x0 0x0 0x1 0x8001 0x0 0x6 0x4

Which means that, when the two lower bits of the device number are
"00" (and so the mask result is 0x0 0x0 0x0 0x1, as the first entry of
the interrupt-map), the assigned IRQ will be 3. If the two lower bit
are "01" (mask result 0x800 0x0 0x0 0x1), the assigned IRQ number will
be 4, and so on.

Regards,
alvise

>
> did you not intend to do
>
> B,D,F irqmap-mask   0x00001800
> PHYS_HI > SPI irq mappings
>
> 0x00000000 -> SPI irq 3
> 0x00000800 -> SPI irq 4
> 0x00001000 -> SPI irq 5
> 0x00001800 -> SPI irq 6
>
> ?
>
> Thanks,
>
> Claudio
>
>
>> +    qemu_fdt_setprop_sized_cells_from_array(vbi->fdt, nodename, 
>> "interrupt-map",
>> +                     (intmap_rows * IRQ_MAP_ENTRY_DESC_SZ)/2, 
>> int_mapping_data);
>> +
>> +    g_free(int_mapping_data);
>> +    g_free(nodename);
>> +}
>> +
>>  static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
>>  {
>>      int i;
>> @@ -640,6 +748,8 @@ static void machvirt_init(MachineState *machine)
>>
>>      create_rtc(vbi, pic);
>>
>> +    create_pci_host(vbi, pic);
>> +
>>      /* Create mmio transports, so the user can create virtio backends
>>       * (which will be automatically plugged in to the transports). If
>>       * no backend is created the transport will just sit harmlessly idle.
>>
>
>
> --
> Claudio Fontana
> Server Virtualization Architect
> Huawei Technologies Duesseldorf GmbH
> Riesstraße 25 - 80992 München
>
> office: +49 89 158834 4135
> mobile: +49 15253060158