Re: [PATCH v5 4/4] hw/arm/virt: Add PL330 DMA controller and connect with SMMU v3

[Eric Auger]

Hi,

On 9/1/21 8:53 AM, Li, Chunming wrote:
>
>> -----Original Message-----
>> From: Eric Auger [mailto:eric.auger@redhat.com]
>> Sent: Tuesday, August 31, 2021 10:37 PM
>> To: chunming; peter.maydell@linaro.org
>> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org; Wen, Jianxian; Liu,
>> Renwei; Li, Chunming
>> Subject: Re: [PATCH v5 4/4] hw/arm/virt: Add PL330 DMA controller and
>> connect with SMMU v3
>>
>> Hi Chunming,
>>
>> On 8/25/21 10:08 AM, chunming wrote:
>>> From: chunming <chunming.li@verisilicon.com>
>>>
>>> Add PL330 DMA controller to test SMMU v3 connection and function.
>>> The default SID for PL330 is 1 but we test other values, it works
>> well.
>> Is it just a patch for testing or would you want this to be applied
>> upstream too?
> I want this to be applied upstream.
Then I think you need to bring a proper motivation behind adding the
PL330 in machvirt besides a testing purpose.
>
>> This static SID allocation may not work in general as it may collide
>> with PCIe RID space?
> I think SMMU support different devices connected with 1 SMMU and share same
> SID, even one is PCIe device and another one is peripheral platform device.
> They can share 1 SMMU page table and get right data translation.

Indeed I cannot find any statement that a streamid couldn't be used by
more than 1 device behing the same smmu. However the 2 devices would be
associated to the same context.
I think this kind of mapping would be really platform specific and in
general it does not make sense to me. what the point using the same
context for the PL330 and a virtio-net-pci device for instance?
>
>> My feeling is if we want to enable platform device support in the
>> SMMUv3
>> this should work for all platform devices doing DMA accesses and not
>> only for this PL330.
> Yes, these patches could support other platform devices connected with SMMUv3.
> They only should follow PL330 example to connect their memory region with 
> SMMUv3
> peripheral IOMMU memory region.
>
>> I guess this should work with virtio platform devices and VFIO platform
>> devices. How would you extend that work to those devices?
> I didn't get your point. 
> I think virtio platform device should be Linux kernel SW part.
> These patches fixed the HW platform devices connection with SMMUv3.
> Could you help to list one virtio platform device then I can check?
After this series you would get a single platform device connected to
the SMMU, the PL330. What is the actual use case?
By the way what about the virtio-iommu which is also supported in DT
mode at the moment?

Besides I meant virtio-net-pci and virtio-block-pci are protected by the
SMMU. What does happen with their virtio-net-device and
virtio-block-device sysbus device counterparts? Then possibly you can
assign a VFIO platform device. You may want this latter to protected by
the SMMU. How would you handle that case (SMMU is not yet integrated
with VFIO but the virtio-iommu is).

Thanks

Eric
>
>> Thanks
>>
>> Eric
>>> Signed-off-by: chunming <chunming.li@verisilicon.com>
>>> ---
>>>  hw/arm/virt.c         | 92
>> ++++++++++++++++++++++++++++++++++++++++++-
>>>  include/hw/arm/virt.h |  1 +
>>>  2 files changed, 92 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index c3fd30e071..8180e4a331 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -143,6 +143,7 @@ static const MemMapEntry base_memmap[] = {
>>>      [VIRT_GIC_REDIST] =         { 0x080A0000, 0x00F60000 },
>>>      [VIRT_UART] =               { 0x09000000, 0x00001000 },
>>>      [VIRT_RTC] =                { 0x09010000, 0x00001000 },
>>> +    [VIRT_DMA] =                { 0x09011000, 0x00001000 },
>>>      [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
>>>      [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
>>>      [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
>>> @@ -188,6 +189,7 @@ static const int a15irqmap[] = {
>>>      [VIRT_GPIO] = 7,
>>>      [VIRT_SECURE_UART] = 8,
>>>      [VIRT_ACPI_GED] = 9,
>>> +    [VIRT_DMA] = 10,
>>>      [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 */
>>> @@ -205,7 +207,7 @@ static const char *valid_cpus[] = {
>>>  };
>>>
>>>  static const uint16_t smmuv3_sidmap[] = {
>>> -
>>> +    [VIRT_DMA] = 1,
>>>  };
>>>
>>>  static bool cpu_type_valid(const char *cpu)
>>> @@ -793,6 +795,92 @@ static void create_uart(const VirtMachineState
>> *vms, int uart,
>>>      g_free(nodename);
>>>  }
>>>
>>> +static void create_dma(const VirtMachineState *vms)
>>> +{
>>> +    int i;
>>> +    char *nodename;
>>> +    hwaddr base = vms->memmap[VIRT_DMA].base;
>>> +    hwaddr size = vms->memmap[VIRT_DMA].size;
>>> +    int irq = vms->irqmap[VIRT_DMA];
>>> +    int sid = vms->sidmap[VIRT_DMA];
>>> +    const char compat[] = "arm,pl330\0arm,primecell";
>>> +    const char irq_names[] =
>> "abort\0dma0\0dma1\0dma2\0dma3\0dma4\0dma5\0dma6\0dma7";
>>> +    DeviceState *dev;
>>> +    MachineState *ms = MACHINE(vms);
>>> +    SysBusDevice *busdev;
>>> +    DeviceState *smmuv3_dev;
>>> +    SMMUState *smmuv3_sys;
>>> +    Object *smmuv3_memory;
>>> +
>>> +    dev = qdev_new("pl330");
>>> +
>>> +    if (vms->iommu == VIRT_IOMMU_SMMUV3 && vms->iommu_phandle) {
>>> +        smmuv3_dev = vms->smmuv3;
>>> +        smmuv3_sys = ARM_SMMU(smmuv3_dev);
>>> +        g_autofree char *memname = g_strdup_printf("%s-peri-%d[0]",
>>> +                                                   smmuv3_sys-
>>> mrtypename,
>>> +                                                   sid);
>>> +
>>> +        smmuv3_memory = object_property_get_link(OBJECT(smmuv3_dev),
>>> +                                memname, &error_abort);
>>> +
>>> +        object_property_set_link(OBJECT(dev), "memory",
>>> +                                 OBJECT(smmuv3_memory),
>>> +                                 &error_fatal);
>>> +    } else {
>>> +        object_property_set_link(OBJECT(dev), "memory",
>>> +                                 OBJECT(get_system_memory()),
>>> +                                 &error_fatal);
>>> +    }
>>> +
>>> +    qdev_prop_set_uint8(dev, "num_chnls",  8);
>>> +    qdev_prop_set_uint8(dev, "num_periph_req",  4);
>>> +    qdev_prop_set_uint8(dev, "num_events",  16);
>>> +    qdev_prop_set_uint8(dev, "data_width",  64);
>>> +    qdev_prop_set_uint8(dev, "wr_cap",  8);
>>> +    qdev_prop_set_uint8(dev, "wr_q_dep",  16);
>>> +    qdev_prop_set_uint8(dev, "rd_cap",  8);
>>> +    qdev_prop_set_uint8(dev, "rd_q_dep",  16);
>>> +    qdev_prop_set_uint16(dev, "data_buffer_dep",  256);
>>> +
>>> +    busdev = SYS_BUS_DEVICE(dev);
>>> +    sysbus_realize_and_unref(busdev, &error_fatal);
>>> +    sysbus_mmio_map(busdev, 0, base);
>>> +
>>> +    for (i = 0; i < 9; ++i) {
>>> +        sysbus_connect_irq(busdev, i, qdev_get_gpio_in(vms->gic, irq
>> + i));
>>> +    }
>>> +
>>> +    nodename = g_strdup_printf("/pl330@%" PRIx64, base);
>>> +    qemu_fdt_add_subnode(ms->fdt, nodename);
>>> +    qemu_fdt_setprop(ms->fdt, nodename, "compatible", compat,
>> sizeof(compat));
>>> +    qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
>>> +                                 2, base, 2, size);
>>> +    qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",
>>> +                    GIC_FDT_IRQ_TYPE_SPI, irq,
>> GIC_FDT_IRQ_FLAGS_LEVEL_HI,
>>> +                    GIC_FDT_IRQ_TYPE_SPI, irq + 1,
>> GIC_FDT_IRQ_FLAGS_LEVEL_HI,
>>> +                    GIC_FDT_IRQ_TYPE_SPI, irq + 2,
>> GIC_FDT_IRQ_FLAGS_LEVEL_HI,
>>> +                    GIC_FDT_IRQ_TYPE_SPI, irq + 3,
>> GIC_FDT_IRQ_FLAGS_LEVEL_HI,
>>> +                    GIC_FDT_IRQ_TYPE_SPI, irq + 4,
>> GIC_FDT_IRQ_FLAGS_LEVEL_HI,
>>> +                    GIC_FDT_IRQ_TYPE_SPI, irq + 5,
>> GIC_FDT_IRQ_FLAGS_LEVEL_HI,
>>> +                    GIC_FDT_IRQ_TYPE_SPI, irq + 6,
>> GIC_FDT_IRQ_FLAGS_LEVEL_HI,
>>> +                    GIC_FDT_IRQ_TYPE_SPI, irq + 7,
>> GIC_FDT_IRQ_FLAGS_LEVEL_HI,
>>> +                    GIC_FDT_IRQ_TYPE_SPI, irq + 8,
>> GIC_FDT_IRQ_FLAGS_LEVEL_HI);
>>> +
>>> +    qemu_fdt_setprop(ms->fdt, nodename, "interrupt-names",
>> irq_names,
>>> +                     sizeof(irq_names));
>>> +
>>> +    qemu_fdt_setprop_cell(ms->fdt, nodename, "clocks", vms-
>>> clock_phandle);
>>> +    qemu_fdt_setprop_string(ms->fdt, nodename, "clock-names",
>> "apb_pclk");
>>> +
>>> +    if (vms->iommu == VIRT_IOMMU_SMMUV3 && vms->iommu_phandle) {
>>> +        qemu_fdt_setprop_cells(ms->fdt, nodename, "iommus",
>>> +                               vms->iommu_phandle, sid);
>>> +        qemu_fdt_setprop(ms->fdt, nodename, "dma-coherent", NULL,
>> 0);
>>> +    }
>>> +
>>> +    g_free(nodename);
>>> +}
>>>  static void create_rtc(const VirtMachineState *vms)
>>>  {
>>>      char *nodename;
>>> @@ -2081,6 +2169,8 @@ static void machvirt_init(MachineState
>> *machine)
>>>      create_pcie(vms);
>>>
>>> +    create_dma(vms);
>>> +
>>>      if (has_ged && aarch64 && firmware_loaded &&
>> virt_is_acpi_enabled(vms)) {
>>>          vms->acpi_dev = create_acpi_ged(vms);
>>>      } else {
>>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>>> index d3402a43dd..f307b26587 100644
>>> --- a/include/hw/arm/virt.h
>>> +++ b/include/hw/arm/virt.h
>>> @@ -72,6 +72,7 @@ enum {
>>>      VIRT_UART,
>>>      VIRT_MMIO,
>>>      VIRT_RTC,
>>> +    VIRT_DMA,
>>>      VIRT_FW_CFG,
>>>      VIRT_PCIE,
>>>      VIRT_PCIE_MMIO,