Re: [Qemu-devel] [PATCH 3/4] arm: Add PCIe host bridge in virt machine

[Alexander Graf]

On 08.01.15 13:55, Claudio Fontana wrote:
> (added cc: Alvise which I mistakenly assumed was in Cc: already)

He was in CC :). Now he's there twice.

> 
> On 07.01.2015 22:47, Alexander Graf wrote:
>>
>>
>> On 07.01.15 16:52, Claudio Fontana wrote:
>>> On 06.01.2015 17:03, Alexander Graf wrote:
>>>> Now that we have a working "generic" PCIe host bridge driver, we can plug
>>>> it into ARMs virt machine to always have PCIe available to normal ARM VMs.
>>>>
>>>> I've successfully managed to expose a Bochs VGA device, XHCI and an e1000
>>>> into an AArch64 VM with this and they all lived happily ever after.
>>>>
>>>> Signed-off-by: Alexander Graf <address@hidden>
>>>>
>>>> ---
>>>>
>>>> Linux 3.19 only supports the generic PCIe host bridge driver for 32bit ARM
>>>> systems. If you want to use it with AArch64 guests, please apply the 
>>>> following
>>>> patch or wait until upstream cleaned up the code properly:
>>>>
>>>>   http://csgraf.de/agraf/pci/pci-3.19.patch
>>>> ---
>>>>  default-configs/arm-softmmu.mak |  2 +
>>>>  hw/arm/virt.c                   | 83 
>>>> ++++++++++++++++++++++++++++++++++++++---
>>>>  2 files changed, 80 insertions(+), 5 deletions(-)
>>>>

[...]

>>>> +    dev = qdev_create(NULL, TYPE_GPEX_HOST);
>>>> +
>>>> +    qdev_prop_set_uint64(dev, "mmio_window_size", size_mmio);
>>>> +    qdev_init_nofail(dev);
>>>> +
>>>> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base_ecam);
>>>> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, base_ioport);
>>>> +
>>>> +    /* Map the MMIO window at the same spot in bus and cpu layouts */
>>>> +    mmio_alias = g_new0(MemoryRegion, 1);
>>>> +    mmio_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
>>>> +    memory_region_init_alias(mmio_alias, OBJECT(dev), "pcie-mmio",
>>>> +                             mmio_reg, base_mmio, size_mmio);
>>>> +    memory_region_add_subregion(get_system_memory(), base_mmio, 
>>>> mmio_alias);
>>>> +
>>>> +    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[irq]);
>>>> +
>>>> +    nodename = g_strdup_printf("/address@hidden" PRIx64, base);
>>>> +    qemu_fdt_add_subnode(vbi->fdt, nodename);
>>>> +    qemu_fdt_setprop_string(vbi->fdt, nodename,
>>>> +                            "compatible", "pci-host-ecam-generic");
>>>
>>> is this the only compatible string we should set here? Is this not legacy 
>>> pci compatible?
>>> In other device trees I see this mentioned as compatible = 
>>> "arm,versatile-pci-hostbridge", "pci" for example,
>>> would it be sensible to make it a list and include "pci" as well?
>>
>> I couldn't find anything that defines what an "pci" compatible should
>> look like. We definitely don't implement the legacy PCI config space
>> accessor registers.
> 
> I see, I assumed this patch would support both CAM and ECAM as configuration 
> methods, while now I understand
> Alvise's patches support only CAM, while these support only ECAM..
> So basically I should look at the compatible string and then choose 
> configuration method accordingly.
> I wonder if I should deal with the case where the compatible string contains 
> both ECAM and CAM.

Well, original PCI didn't even do CAM. You only had 2 32bit ioport
registers that you tunnel all config space access through.

> 
>>>
>>>> +    qemu_fdt_setprop_string(vbi->fdt, nodename, "device_type", "pci");
>>>> +    qemu_fdt_setprop_cell(vbi->fdt, nodename, "#address-cells", 3);
>>>> +    qemu_fdt_setprop_cell(vbi->fdt, nodename, "#size-cells", 2);
>>>> +    qemu_fdt_setprop_cells(vbi->fdt, nodename, "bus-range", 0, 1);
>>>> +
>>>> +    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
>>>> +                                 2, base_ecam, 2, size_ecam);
>>>> +    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
>>>> +                                 1, 0x01000000, 2, 0,
>>>> +                                 2, base_ioport, 2, size_ioport,
>>>> +
>>>> +                                 1, 0x02000000, 2, base_mmio,
>>>> +                                 2, base_mmio, 2, size_mmio);
>>>> +
>>>> +    qemu_fdt_setprop_cell(vbi->fdt, nodename, "#interrupt-cells", 1);
>>>> +    qemu_fdt_setprop_cells(vbi->fdt, nodename, "interrupt-map",
>>>> +                           0, 0, 0, /* device */
>>>> +                           0,       /* PCI irq */
>>>> +                           gic_phandle, GIC_FDT_IRQ_TYPE_SPI, irq,
>>>> +                             GIC_FDT_IRQ_FLAGS_LEVEL_HI /* system irq */);
>>>> +    qemu_fdt_setprop_cells(vbi->fdt, nodename, "interrupt-map-mask",
>>>> +                           0, 0, 0, /* device */
>>>> +                           0        /* PCI irq */);
>>>
>>> Interrupt map does not seem to work for me; incidentally this ends up being 
>>> the same kind of undocumented blob that Alvise posted in his series.
>>
>> How exactly is this undocumented? The "mask" is a mask over the first
>> fields of an interrupt-map row plus an IRQ offset. So the mask above
>> means "Any device with any function and any IRQ on it, map to device IRQ
>> 0" which maps to vbi->irqmap[VIRT_PCIE] (IRQ 3).
> 
> (see my answer to Peter below in thread)
> 
> this is a bit different to what Alvise's series is doing I think (see later).

Yes, but it's easier :).

> 
>>
>>> Can you add a good comment about what the ranges property contains (the 
>>> 0x01000000, 0x02000000 which I suspect means IO vs MMIO IIRC, but there is 
>>> no need to be cryptic about it).
>>
>> You're saying you'd prefer a define?
> 
> Yes that would be helpful :)
> 
>>
>>> How does your interrupt map implementation differ from the patchset posted 
>>> by Alvise? I ask because that one works for me (tm).
>>
>> His implementation explicitly listed every PCI slot, while mine actually
>> makes use of the mask and simply routes everything to a single IRQ line.
>>
>> The benefit of masking devfn out is that you don't need to worry about
>> the number of slots you support - and anything performance critical
>> should go via MSI-X anyway ;).
> 
> The benefit for me (but for me only probably..) is that with one IRQ per slot 
> I didn't have to implement shared irqs and msi / msi-x in the guest yet. But 
> that should be done eventually anyway..

You most likely wouldn't get one IRQ per slot anyway. Most PHBs expose 4
outgoing IRQ lines, so you'll need to deal with sharing IRQs regardless.

Also, sharing IRQ lines isn't incredibly black magic and quite a
fundamental PCI IRQ concept, so I'm glad I'm pushing you into the
direction of implementing it early on :).

> 
>>
>> So what exactly do you test it with? I've successfully received IRQs
>> with a Linux guest, so I'm slightly puzzled you're running into problems.
> 
> Of course, I am just implementing PCI guest support for OSv/AArch64.
> Works with the legacy pci support using Alvise's series to do virtio-pci with 
> block, net, rng etc,
> but I am now considering going for PCIE support directly although that means 
> more implementation work for me.

Both should look quite similar from your point of view. If you do the
level interrupt handling, irq masking and window handling correctly the
only difference should be CAM vs ECAM.

I'm glad to hear that it breaks your implementation though and not
something already existing (like Linux, BSD, etc). That means there's a
good chance my patches are correct and you just took some shortcuts
which can be easily fixed :).


Alex