On 08.01.2015 16:01, Claudio Fontana wrote: > On 08.01.2015 14:26, Alexander Graf wrote: >> >> >> 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 <ag...@suse.de> >>>>>> >>>>>> --- >>>>>> >>>>>> 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("/pcie@%" 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 :).
Ok I have tentatively implemented this and I tested both Alvise's series and yours and both work ok for my use case. Note that I did not test any MSI/MSI-X, only the INTx method. >> > > damn open source making me work lol :) > > It's actually good to have these two implementations, so I can validate my > guest support against both series and ensure that I don't hardcode too many > shortcuts. > > Thanks, > > Claudio > >>> >>>> >>>> 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 >> > > -- Claudio Fontana Server Virtualization Architect Huawei Technologies Duesseldorf GmbH Riesstraße 25 - 80992 München office: +49 89 158834 4135 mobile: +49 15253060158