Re: [Qemu-devel] [PATCH v9 12/14] hw/arm/virt: Add SMMUv3 to the virt board
On 12 March 2018 at 15:01, Auger Eric wrote: > Hi Peter, > > On 12/03/18 13:46, Peter Maydell wrote: >> On 17 February 2018 at 18:46, Eric Auger wrote: >>> +qemu_fdt_setprop(vms->fdt, node, "dma-coherent", NULL, 0); >> >> Is this definitely the right setting for dma-coherent? My (possibly >> naive) thought is that for an emulated SMMU our page-table walks are >> going to be coherent with the CPU's view. > > Indeed we add the dma-capable property to make the DMA ops of the SMMU > cache coherent with the CPU. We pass NULL/O since there is just > dma-coherent in the dt property. What am I missing? My mistake, I thought this was the equivalent of "dma-coherent = 0;". thanks -- PMM
Re: [Qemu-devel] [PATCH v9 12/14] hw/arm/virt: Add SMMUv3 to the virt board
Hi Peter, On 12/03/18 13:46, Peter Maydell wrote: > On 17 February 2018 at 18:46, Eric Auger wrote: >> From: Prem Mallappa >> >> Add code to instantiate an smmuv3 in virt machine. A new iommu >> integer member is introduced in VirtMachineState to store the type >> of the iommu in use. >> >> Signed-off-by: Prem Mallappa >> Signed-off-by: Eric Auger >> >> --- >> v7 -> v8: >> - integer iommu member >> - add primary-bus property >> >> v4 -> v5: >> - add dma-coherent property >> >> v2 -> v3: >> - vbi was removed. Use vms instead >> - migrate to new smmu binding format (iommu-map) >> - don't use appendprop anymore >> - add vms->smmu and guard instantiation with this latter >> - interrupts type changed to edge >> >> Conflicts: >> hw/arm/smmuv3.c >> --- >> hw/arm/virt.c | 64 >> ++- >> include/hw/arm/virt.h | 10 >> 2 files changed, 73 insertions(+), 1 deletion(-) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index dbb3c80..e9dca0d 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -58,6 +58,7 @@ >> #include "hw/smbios/smbios.h" >> #include "qapi/visitor.h" >> #include "standard-headers/linux/input.h" >> +#include "hw/arm/smmuv3.h" >> >> #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \ >> static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ >> @@ -141,6 +142,7 @@ static const MemMapEntry a15memmap[] = { >> [VIRT_FW_CFG] = { 0x0902, 0x0018 }, >> [VIRT_GPIO] = { 0x0903, 0x1000 }, >> [VIRT_SECURE_UART] ={ 0x0904, 0x1000 }, >> +[VIRT_SMMU] = { 0x0905, 0x0002 }, /* 128K, needed >> */ > > What does the "needed" comment mean here? removed > >> [VIRT_MMIO] = { 0x0a00, 0x0200 }, >> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size >> */ >> [VIRT_PLATFORM_BUS] = { 0x0c00, 0x0200 }, >> @@ -161,6 +163,7 @@ static const int a15irqmap[] = { >> [VIRT_SECURE_UART] = 8, >> [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */ >> [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */ >> +[VIRT_SMMU] = 74,/* ...to 74 + NUM_SMMU_IRQS - 1 */ >> [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */ >> }; >> >> @@ -941,7 +944,57 @@ static void create_pcie_irq_map(const VirtMachineState >> *vms, >> 0x7 /* PCI irq */); >> } >> >> -static void create_pcie(const VirtMachineState *vms, qemu_irq *pic) >> +static void create_smmu(const VirtMachineState *vms, qemu_irq *pic, >> +PCIBus *bus) > > Side suggestion: if you add "algorithm = histogram" to the [diff] section > of your .gitconfig, you will probably find that it doesn't generate patches > with this sort of silly patch hunk any more. I only discovered this > config setting recently but I think it's better than the default. ah OK thanks for the advice. > >> +{ >> +char *node; >> +const char compat[] = "arm,smmu-v3"; >> +int irq = vms->irqmap[VIRT_SMMU]; >> +int i; >> +hwaddr base = vms->memmap[VIRT_SMMU].base; >> +hwaddr size = vms->memmap[VIRT_SMMU].size; >> +const char irq_names[] = "eventq\0priq\0cmdq-sync\0gerror"; >> +DeviceState *dev; >> + >> +if (vms->iommu != VIRT_IOMMU_SMMUV3 || !vms->iommu_phandle) { >> +return; >> +} >> + >> +dev = qdev_create(NULL, "arm-smmuv3"); >> + >> +object_property_set_link(OBJECT(dev), OBJECT(bus), "primary-bus", >> + &error_abort); >> +qdev_init_nofail(dev); >> +sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base); >> +for (i = 0; i < NUM_SMMU_IRQS; i++) { >> +sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, pic[irq + i]); >> +} >> + >> +node = g_strdup_printf("/smmuv3@%" PRIx64, base); >> +qemu_fdt_add_subnode(vms->fdt, node); >> +qemu_fdt_setprop(vms->fdt, node, "compatible", compat, sizeof(compat)); >> +qemu_fdt_setprop_sized_cells(vms->fdt, node, "reg", 2, base, 2, size); >> + >> +qemu_fdt_setprop_cells(vms->fdt, node, "interrupts", >> +GIC_FDT_IRQ_TYPE_SPI, irq, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI, >> +GIC_FDT_IRQ_TYPE_SPI, irq + 1, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI, >> +GIC_FDT_IRQ_TYPE_SPI, irq + 2, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI, >> +GIC_FDT_IRQ_TYPE_SPI, irq + 3, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI); >> + >> +qemu_fdt_setprop(vms->fdt, node, "interrupt-names", irq_names, >> + sizeof(irq_names)); >> + >> +qemu_fdt_setprop_cell(vms->fdt, node, "clocks", vms->clock_phandle); >> +qemu_fdt_setprop_string(vms->fdt, node, "clock-names", "apb_pclk"); >> +qemu_fdt_setprop(vms->fdt, node, "dma-coherent", NULL, 0); > > Is this definitely the right setting for dma-coherent? My (possibly > naive) thought is that for an emulated SMMU our page-table walks are > g
Re: [Qemu-devel] [PATCH v9 12/14] hw/arm/virt: Add SMMUv3 to the virt board
On 17 February 2018 at 18:46, Eric Auger wrote: > From: Prem Mallappa > > Add code to instantiate an smmuv3 in virt machine. A new iommu > integer member is introduced in VirtMachineState to store the type > of the iommu in use. > > Signed-off-by: Prem Mallappa > Signed-off-by: Eric Auger > > --- > v7 -> v8: > - integer iommu member > - add primary-bus property > > v4 -> v5: > - add dma-coherent property > > v2 -> v3: > - vbi was removed. Use vms instead > - migrate to new smmu binding format (iommu-map) > - don't use appendprop anymore > - add vms->smmu and guard instantiation with this latter > - interrupts type changed to edge > > Conflicts: > hw/arm/smmuv3.c > --- > hw/arm/virt.c | 64 > ++- > include/hw/arm/virt.h | 10 > 2 files changed, 73 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index dbb3c80..e9dca0d 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -58,6 +58,7 @@ > #include "hw/smbios/smbios.h" > #include "qapi/visitor.h" > #include "standard-headers/linux/input.h" > +#include "hw/arm/smmuv3.h" > > #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \ > static void virt_##major##_##minor##_class_init(ObjectClass *oc, \ > @@ -141,6 +142,7 @@ static const MemMapEntry a15memmap[] = { > [VIRT_FW_CFG] = { 0x0902, 0x0018 }, > [VIRT_GPIO] = { 0x0903, 0x1000 }, > [VIRT_SECURE_UART] ={ 0x0904, 0x1000 }, > +[VIRT_SMMU] = { 0x0905, 0x0002 }, /* 128K, needed > */ What does the "needed" comment mean here? > [VIRT_MMIO] = { 0x0a00, 0x0200 }, > /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size > */ > [VIRT_PLATFORM_BUS] = { 0x0c00, 0x0200 }, > @@ -161,6 +163,7 @@ static const int a15irqmap[] = { > [VIRT_SECURE_UART] = 8, > [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */ > [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */ > +[VIRT_SMMU] = 74,/* ...to 74 + NUM_SMMU_IRQS - 1 */ > [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */ > }; > > @@ -941,7 +944,57 @@ static void create_pcie_irq_map(const VirtMachineState > *vms, > 0x7 /* PCI irq */); > } > > -static void create_pcie(const VirtMachineState *vms, qemu_irq *pic) > +static void create_smmu(const VirtMachineState *vms, qemu_irq *pic, > +PCIBus *bus) Side suggestion: if you add "algorithm = histogram" to the [diff] section of your .gitconfig, you will probably find that it doesn't generate patches with this sort of silly patch hunk any more. I only discovered this config setting recently but I think it's better than the default. > +{ > +char *node; > +const char compat[] = "arm,smmu-v3"; > +int irq = vms->irqmap[VIRT_SMMU]; > +int i; > +hwaddr base = vms->memmap[VIRT_SMMU].base; > +hwaddr size = vms->memmap[VIRT_SMMU].size; > +const char irq_names[] = "eventq\0priq\0cmdq-sync\0gerror"; > +DeviceState *dev; > + > +if (vms->iommu != VIRT_IOMMU_SMMUV3 || !vms->iommu_phandle) { > +return; > +} > + > +dev = qdev_create(NULL, "arm-smmuv3"); > + > +object_property_set_link(OBJECT(dev), OBJECT(bus), "primary-bus", > + &error_abort); > +qdev_init_nofail(dev); > +sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base); > +for (i = 0; i < NUM_SMMU_IRQS; i++) { > +sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, pic[irq + i]); > +} > + > +node = g_strdup_printf("/smmuv3@%" PRIx64, base); > +qemu_fdt_add_subnode(vms->fdt, node); > +qemu_fdt_setprop(vms->fdt, node, "compatible", compat, sizeof(compat)); > +qemu_fdt_setprop_sized_cells(vms->fdt, node, "reg", 2, base, 2, size); > + > +qemu_fdt_setprop_cells(vms->fdt, node, "interrupts", > +GIC_FDT_IRQ_TYPE_SPI, irq, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI, > +GIC_FDT_IRQ_TYPE_SPI, irq + 1, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI, > +GIC_FDT_IRQ_TYPE_SPI, irq + 2, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI, > +GIC_FDT_IRQ_TYPE_SPI, irq + 3, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI); > + > +qemu_fdt_setprop(vms->fdt, node, "interrupt-names", irq_names, > + sizeof(irq_names)); > + > +qemu_fdt_setprop_cell(vms->fdt, node, "clocks", vms->clock_phandle); > +qemu_fdt_setprop_string(vms->fdt, node, "clock-names", "apb_pclk"); > +qemu_fdt_setprop(vms->fdt, node, "dma-coherent", NULL, 0); Is this definitely the right setting for dma-coherent? My (possibly naive) thought is that for an emulated SMMU our page-table walks are going to be coherent with the CPU's view. > + > +qemu_fdt_setprop_cell(vms->fdt, node, "#iommu-cells", 1); > + > +qemu_fdt_setprop_cell(vms->fdt, node, "phandle", vms->iommu_phandle); > +g_free(node); > +} > +enum