[PATCH] hw/arm/smmuv3: Support non-PCI/PCIe devices connection

[PATCH] hw/arm/smmuv3: Support non-PCI/PCIe devices connection

[Li, Chunming]

The current SMMU V3 device model only support PCI/PCIe devices,
so we update it to support non-PCI/PCIe devices.

    hw/arm/smmuv3:
        . Create IOMMU memory regions for non-PCI/PCIe devices based on their SID
        . Add sid-map property to store non-PCI/PCIe devices SID
        . Update implementation of CFGI commands based on device SID
    hw/arm/smmu-common:
        . Differentiate PCI/PCIe and non-PCI/PCIe devices SID getting strategy
    hw/arm/virt:
        . Add PL330 DMA controller and connect with SMMUv3 for testing
        . Add smmuv3_sidmap for non-PCI/PCIe devices SID setting

Signed-off-by: Chunming Li <chunming.li@verisilicon.com>
Signed-off-by: Renwei Liu <renwei.liu@verisilicon.com>
---
This patch depends on PL330 memory region connection patch:
https://patchew.org/QEMU/4C23C17B8E87E74E906A25A3254A03F4FA1FEC31@SHASXM03.verisilicon.com/

 hw/arm/smmuv3.c              |  75 ++++++++++++++++++------
 hw/arm/virt.c                | 110 ++++++++++++++++++++++++++++++++++-
 include/hw/arm/smmu-common.h |  12 +++-
 include/hw/arm/smmuv3.h      |   2 +
 include/hw/arm/virt.h        |   3 +
 5 files changed, 181 insertions(+), 21 deletions(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 01b60bee4..c4da05d8b 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -32,6 +32,7 @@
 #include "hw/arm/smmuv3.h"
 #include "smmuv3-internal.h"
 #include "smmu-internal.h"
+#include "hw/qdev-properties.h"
 
 /**
  * smmuv3_trigger_irq - pulse @irq if enabled and update
@@ -612,7 +613,7 @@ static SMMUTransCfg *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event)
     return cfg;
 }
 
-static void smmuv3_flush_config(SMMUDevice *sdev)
+static void __attribute__((unused)) smmuv3_flush_config(SMMUDevice *sdev)
 {
     SMMUv3State *s = sdev->smmu;
     SMMUState *bc = &s->smmu_state;
@@ -963,22 +964,18 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
         case SMMU_CMD_CFGI_STE:
         {
             uint32_t sid = CMD_SID(&cmd);
-            IOMMUMemoryRegion *mr = smmu_iommu_mr(bs, sid);
-            SMMUDevice *sdev;
+            SMMUSIDRange sid_range;
 
             if (CMD_SSEC(&cmd)) {
                 cmd_error = SMMU_CERROR_ILL;
                 break;
             }
 
-            if (!mr) {
-                break;
-            }
-
+            sid_range.start = sid;
+            sid_range.end = sid;
             trace_smmuv3_cmdq_cfgi_ste(sid);
-            sdev = container_of(mr, SMMUDevice, iommu);
-            smmuv3_flush_config(sdev);
-
+            g_hash_table_foreach_remove(bs->configs, smmuv3_invalidate_ste,
+                                        &sid_range);
             break;
         }
         case SMMU_CMD_CFGI_STE_RANGE: /* same as SMMU_CMD_CFGI_ALL */
@@ -1005,21 +1002,18 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
         case SMMU_CMD_CFGI_CD_ALL:
         {
             uint32_t sid = CMD_SID(&cmd);
-            IOMMUMemoryRegion *mr = smmu_iommu_mr(bs, sid);
-            SMMUDevice *sdev;
+            SMMUSIDRange sid_range;
 
             if (CMD_SSEC(&cmd)) {
                 cmd_error = SMMU_CERROR_ILL;
                 break;
             }
 
-            if (!mr) {
-                break;
-            }
-
+            sid_range.start = sid;
+            sid_range.end = sid;
             trace_smmuv3_cmdq_cfgi_cd(sid);
-            sdev = container_of(mr, SMMUDevice, iommu);
-            smmuv3_flush_config(sdev);
+            g_hash_table_foreach_remove(bs->configs, smmuv3_invalidate_ste,
+                                        &sid_range);
             break;
         }
         case SMMU_CMD_TLBI_NH_ASID:
@@ -1430,6 +1424,19 @@ static void smmu_reset(DeviceState *dev)
     smmuv3_init_regs(s);
 }
 
+static SMMUDevice *smmu_find_peri_sdev(SMMUState *s, uint16_t sid)
+{
+    SMMUDevice *sdev;
+
+    QLIST_FOREACH(sdev, &s->peri_sdev_list, next) {
+        if (smmu_get_sid(sdev) == sid) {
+            return sdev;
+        }
+    }
+
+    return NULL;
+}
+
 static void smmu_realize(DeviceState *d, Error **errp)
 {
     SMMUState *sys = ARM_SMMU(d);
@@ -1437,6 +1444,9 @@ static void smmu_realize(DeviceState *d, Error **errp)
     SMMUv3Class *c = ARM_SMMUV3_GET_CLASS(s);
     SysBusDevice *dev = SYS_BUS_DEVICE(d);
     Error *local_err = NULL;
+    SMMUDevice *sdev;
+    char *name = NULL;
+    uint16_t sid = 0;
 
     c->parent_realize(d, &local_err);
     if (local_err) {
@@ -1454,6 +1464,28 @@ static void smmu_realize(DeviceState *d, Error **errp)
     sysbus_init_mmio(dev, &sys->iomem);
 
     smmu_init_irq(s, dev);
+
+    /* Create IOMMU memory region for peripheral devices based on their SID */
+    for (int i = 0; i < s->num_sid; i++) {
+        sid = s->sid_map[i];
+        sdev = smmu_find_peri_sdev(sys, sid);
+        if (sdev) {
+            continue;
+        }
+
+        sdev = g_new0(SMMUDevice, 1);
+        sdev->smmu = sys;
+        sdev->bus = NULL;
+        sdev->devfn = sid;
+
+        name = g_strdup_printf("%s-peri-%d", sys->mrtypename, sid);
+        memory_region_init_iommu(&sdev->iommu, sizeof(sdev->iommu),
+                                 sys->mrtypename,
+                                 OBJECT(sys), name, 1ULL << SMMU_MAX_VA_BITS);
+
+        QLIST_INSERT_HEAD(&sys->peri_sdev_list, sdev, next);
+        g_free(name);
+    }
 }
 
 static const VMStateDescription vmstate_smmuv3_queue = {
@@ -1506,6 +1538,12 @@ static void smmuv3_instance_init(Object *obj)
     /* Nothing much to do here as of now */
 }
 
+static Property smmuv3_properties[] = {
+    DEFINE_PROP_ARRAY("sid-map", SMMUv3State, num_sid, sid_map,
+                      qdev_prop_uint16, uint16_t),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void smmuv3_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -1515,6 +1553,7 @@ static void smmuv3_class_init(ObjectClass *klass, void *data)
     device_class_set_parent_reset(dc, smmu_reset, &c->parent_reset);
     c->parent_realize = dc->realize;
     dc->realize = smmu_realize;
+    device_class_set_props(dc, smmuv3_properties);
 }
 
 static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 81eda46b0..4879be986 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 */
@@ -204,6 +206,10 @@ static const char *valid_cpus[] = {
     ARM_CPU_TYPE_NAME("max"),
 };
 
+static const uint16_t smmuv3_sidmap[] = {
+    [VIRT_DMA] = 1,
+};
+
 static bool cpu_type_valid(const char *cpu)
 {
     int i;
@@ -789,6 +795,94 @@ 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;
@@ -1223,7 +1317,7 @@ static void create_pcie_irq_map(const MachineState *ms,
                            0x7           /* PCI irq */);
 }
 
-static void create_smmu(const VirtMachineState *vms,
+static void create_smmu(VirtMachineState *vms,
                         PCIBus *bus)
 {
     char *node;
@@ -1244,6 +1338,16 @@ static void create_smmu(const VirtMachineState *vms,
 
     object_property_set_link(OBJECT(dev), "primary-bus", OBJECT(bus),
                              &error_abort);
+
+    vms->smmuv3 = dev;
+
+    qdev_prop_set_uint32(dev, "len-sid-map", ARRAY_SIZE(smmuv3_sidmap));
+
+    for (i = 0; i < ARRAY_SIZE(smmuv3_sidmap); i++) {
+        g_autofree char *propname = g_strdup_printf("sid-map[%d]", i);
+        qdev_prop_set_uint16(dev, propname, smmuv3_sidmap[i]);
+    }
+
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
     for (i = 0; i < NUM_SMMU_IRQS; i++) {
@@ -2067,6 +2171,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 {
@@ -2762,6 +2868,8 @@ static void virt_instance_init(Object *obj)
 
     vms->irqmap = a15irqmap;
 
+    vms->sidmap = smmuv3_sidmap;
+
     virt_flash_create(vms);
 
     vms->oem_id = g_strndup(ACPI_BUILD_APPNAME6, 6);
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 706be3c6d..96cc563b5 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -117,6 +117,7 @@ struct SMMUState {
     QLIST_HEAD(, SMMUDevice) devices_with_notifiers;
     uint8_t bus_num;
     PCIBus *primary_bus;
+    QLIST_HEAD(, SMMUDevice) peri_sdev_list;
 };
 
 struct SMMUBaseClass {
@@ -138,7 +139,11 @@ SMMUPciBus *smmu_find_smmu_pcibus(SMMUState *s, uint8_t bus_num);
 /* Return the stream ID of an SMMU device */
 static inline uint16_t smmu_get_sid(SMMUDevice *sdev)
 {
-    return PCI_BUILD_BDF(pci_bus_num(sdev->bus), sdev->devfn);
+    if (!sdev->bus) {
+        return sdev->devfn;
+    } else {
+        return PCI_BUILD_BDF(pci_bus_num(sdev->bus), sdev->devfn);
+    }
 }
 
 /**
@@ -154,7 +159,10 @@ int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
  */
 SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova);
 
-/* Return the iommu mr associated to @sid, or NULL if none */
+/**
+ * Return the iommu mr associated to @sid, or NULL if none
+ * Only for PCI device, check smmu_find_peri_sdev for peripheral device
+ */
 IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid);
 
 #define SMMU_IOTLB_MAX_SIZE 256
diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
index c641e6073..32ba84990 100644
--- a/include/hw/arm/smmuv3.h
+++ b/include/hw/arm/smmuv3.h
@@ -39,6 +39,8 @@ struct SMMUv3State {
     uint32_t features;
     uint8_t sid_size;
     uint8_t sid_split;
+    uint32_t num_sid;
+    uint16_t *sid_map;
 
     uint32_t idr[6];
     uint32_t iidr;
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 9661c4669..f307b2658 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,
@@ -167,6 +168,8 @@ struct VirtMachineState {
     PCIBus *bus;
     char *oem_id;
     char *oem_table_id;
+    DeviceState *smmuv3;
+    const uint16_t *sidmap;
 };
 
 #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
--

Re: [PATCH] hw/arm/smmuv3: Support non-PCI/PCIe devices connection

[Philippe Mathieu-Daudé]

On 8/20/21 4:36 AM, Li, Chunming wrote:
> The current SMMU V3 device model only support PCI/PCIe devices,
> so we update it to support non-PCI/PCIe devices.
> 
>     hw/arm/smmuv3:
>         . Create IOMMU memory regions for non-PCI/PCIe devices based on their SID
>         . Add sid-map property to store non-PCI/PCIe devices SID
>         . Update implementation of CFGI commands based on device SID
>     hw/arm/smmu-common:
>         . Differentiate PCI/PCIe and non-PCI/PCIe devices SID getting strategy
>     hw/arm/virt:
>         . Add PL330 DMA controller and connect with SMMUv3 for testing
>         . Add smmuv3_sidmap for non-PCI/PCIe devices SID setting
> 
> Signed-off-by: Chunming Li <chunming.li@verisilicon.com>
> Signed-off-by: Renwei Liu <renwei.liu@verisilicon.com>
> ---
> This patch depends on PL330 memory region connection patch:
> https://patchew.org/QEMU/4C23C17B8E87E74E906A25A3254A03F4FA1FEC31@SHASXM03.verisilicon.com/
> 
>  hw/arm/smmuv3.c              |  75 ++++++++++++++++++------
>  hw/arm/virt.c                | 110 ++++++++++++++++++++++++++++++++++-
>  include/hw/arm/smmu-common.h |  12 +++-
>  include/hw/arm/smmuv3.h      |   2 +
>  include/hw/arm/virt.h        |   3 +
>  5 files changed, 181 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 01b60bee4..c4da05d8b 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -32,6 +32,7 @@
>  #include "hw/arm/smmuv3.h"
>  #include "smmuv3-internal.h"
>  #include "smmu-internal.h"
> +#include "hw/qdev-properties.h"
>  
>  /**
>   * smmuv3_trigger_irq - pulse @irq if enabled and update
> @@ -612,7 +613,7 @@ static SMMUTransCfg *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event)
>      return cfg;
>  }
>  
> -static void smmuv3_flush_config(SMMUDevice *sdev)
> +static void __attribute__((unused)) smmuv3_flush_config(SMMUDevice *sdev)
>  {

Why keep this function if unused?

RE: [PATCH] hw/arm/smmuv3: Support non-PCI/PCIe devices connection

[Li, Chunming]

> On 8/20/21 4:36 AM, Li, Chunming wrote:
> > The current SMMU V3 device model only support PCI/PCIe devices,
> > so we update it to support non-PCI/PCIe devices.
> >
> >     hw/arm/smmuv3:
> >         . Create IOMMU memory regions for non-PCI/PCIe devices based
> on their SID
> >         . Add sid-map property to store non-PCI/PCIe devices SID
> >         . Update implementation of CFGI commands based on device SID
> >     hw/arm/smmu-common:
> >         . Differentiate PCI/PCIe and non-PCI/PCIe devices SID getting
> strategy
> >     hw/arm/virt:
> >         . Add PL330 DMA controller and connect with SMMUv3 for
> testing
> >         . Add smmuv3_sidmap for non-PCI/PCIe devices SID setting
> >
> > Signed-off-by: Chunming Li <chunming.li@verisilicon.com>
> > Signed-off-by: Renwei Liu <renwei.liu@verisilicon.com>
> > ---
> > This patch depends on PL330 memory region connection patch:
> >
> https://patchew.org/QEMU/4C23C17B8E87E74E906A25A3254A03F4FA1FEC31@SHASX
> M03.verisilicon.com/
> >
> >  hw/arm/smmuv3.c              |  75 ++++++++++++++++++------
> >  hw/arm/virt.c                | 110
> ++++++++++++++++++++++++++++++++++-
> >  include/hw/arm/smmu-common.h |  12 +++-
> >  include/hw/arm/smmuv3.h      |   2 +
> >  include/hw/arm/virt.h        |   3 +
> >  5 files changed, 181 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > index 01b60bee4..c4da05d8b 100644
> > --- a/hw/arm/smmuv3.c
> > +++ b/hw/arm/smmuv3.c
> > @@ -32,6 +32,7 @@
> >  #include "hw/arm/smmuv3.h"
> >  #include "smmuv3-internal.h"
> >  #include "smmu-internal.h"
> > +#include "hw/qdev-properties.h"
> >
> >  /**
> >   * smmuv3_trigger_irq - pulse @irq if enabled and update
> > @@ -612,7 +613,7 @@ static SMMUTransCfg *smmuv3_get_config(SMMUDevice
> *sdev, SMMUEventInfo *event)
> >      return cfg;
> >  }
> >
> > -static void smmuv3_flush_config(SMMUDevice *sdev)
> > +static void __attribute__((unused)) smmuv3_flush_config(SMMUDevice
> *sdev)
> >  {
> 
> Why keep this function if unused?
"smmuv3_flush_config" is useless after our modification. The modification is verified by PL330 DMA and PCIe network.
But we are not sure if it has potential risk. So we hope maintainer can help to check, then we can remove it.

Re: [PATCH] hw/arm/smmuv3: Support non-PCI/PCIe devices connection

[Philippe Mathieu-Daudé]

On 8/20/21 9:42 AM, Li, Chunming wrote:
> 
>> On 8/20/21 4:36 AM, Li, Chunming wrote:
>>> The current SMMU V3 device model only support PCI/PCIe devices,
>>> so we update it to support non-PCI/PCIe devices.
>>>
>>>     hw/arm/smmuv3:
>>>         . Create IOMMU memory regions for non-PCI/PCIe devices based
>> on their SID
>>>         . Add sid-map property to store non-PCI/PCIe devices SID
>>>         . Update implementation of CFGI commands based on device SID
>>>     hw/arm/smmu-common:
>>>         . Differentiate PCI/PCIe and non-PCI/PCIe devices SID getting
>> strategy
>>>     hw/arm/virt:
>>>         . Add PL330 DMA controller and connect with SMMUv3 for
>> testing
>>>         . Add smmuv3_sidmap for non-PCI/PCIe devices SID setting
>>>
>>> Signed-off-by: Chunming Li <chunming.li@verisilicon.com>
>>> Signed-off-by: Renwei Liu <renwei.liu@verisilicon.com>
>>> ---
>>> This patch depends on PL330 memory region connection patch:
>>>
>> https://patchew.org/QEMU/4C23C17B8E87E74E906A25A3254A03F4FA1FEC31@SHASX
>> M03.verisilicon.com/
>>>
>>>  hw/arm/smmuv3.c              |  75 ++++++++++++++++++------
>>>  hw/arm/virt.c                | 110
>> ++++++++++++++++++++++++++++++++++-
>>>  include/hw/arm/smmu-common.h |  12 +++-
>>>  include/hw/arm/smmuv3.h      |   2 +
>>>  include/hw/arm/virt.h        |   3 +
>>>  5 files changed, 181 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>>> index 01b60bee4..c4da05d8b 100644
>>> --- a/hw/arm/smmuv3.c
>>> +++ b/hw/arm/smmuv3.c
>>> @@ -32,6 +32,7 @@
>>>  #include "hw/arm/smmuv3.h"
>>>  #include "smmuv3-internal.h"
>>>  #include "smmu-internal.h"
>>> +#include "hw/qdev-properties.h"
>>>
>>>  /**
>>>   * smmuv3_trigger_irq - pulse @irq if enabled and update
>>> @@ -612,7 +613,7 @@ static SMMUTransCfg *smmuv3_get_config(SMMUDevice
>> *sdev, SMMUEventInfo *event)
>>>      return cfg;
>>>  }
>>>
>>> -static void smmuv3_flush_config(SMMUDevice *sdev)
>>> +static void __attribute__((unused)) smmuv3_flush_config(SMMUDevice
>> *sdev)
>>>  {
>>
>> Why keep this function if unused?
> "smmuv3_flush_config" is useless after our modification. The modification is verified by PL330 DMA and PCIe network.
> But we are not sure if it has potential risk. So we hope maintainer can help to check, then we can remove it.

Either remove it in your patch, or your patch is incorrect and we
still need it. But don't let it as dead code and don't postpone the
removal please.

Re: [PATCH] hw/arm/smmuv3: Support non-PCI/PCIe devices connection

[Peter Maydell]

On Fri, 20 Aug 2021 at 03:36, Li, Chunming <Chunming.Li@verisilicon.com> wrote:
>
> The current SMMU V3 device model only support PCI/PCIe devices,
> so we update it to support non-PCI/PCIe devices.
>
>     hw/arm/smmuv3:
>         . Create IOMMU memory regions for non-PCI/PCIe devices based on their SID
>         . Add sid-map property to store non-PCI/PCIe devices SID
>         . Update implementation of CFGI commands based on device SID
>     hw/arm/smmu-common:
>         . Differentiate PCI/PCIe and non-PCI/PCIe devices SID getting strategy
>     hw/arm/virt:
>         . Add PL330 DMA controller and connect with SMMUv3 for testing
>         . Add smmuv3_sidmap for non-PCI/PCIe devices SID setting

Please don't try to do all these things in one big patch --
put together a patchseries with several smaller patches,
each of which does one self-contained thing.


> Signed-off-by: Chunming Li <chunming.li@verisilicon.com>
> Signed-off-by: Renwei Liu <renwei.liu@verisilicon.com>
> ---
> This patch depends on PL330 memory region connection patch:
> https://patchew.org/QEMU/4C23C17B8E87E74E906A25A3254A03F4FA1FEC31@SHASXM03.verisilicon.com/

If you have a patch that depends on another, it's usually better to
send them as a patchseries. I was wondering what the reason for
that PL330 patch was...

thanks
-- PMM

RE: [PATCH] hw/arm/smmuv3: Support non-PCI/PCIe devices connection

[Li, Chunming]

> On Fri, 20 Aug 2021 at 03:36, Li, Chunming
> <Chunming.Li@verisilicon.com> wrote:
> >
> > The current SMMU V3 device model only support PCI/PCIe devices,
> > so we update it to support non-PCI/PCIe devices.
> >
> >     hw/arm/smmuv3:
> >         . Create IOMMU memory regions for non-PCI/PCIe devices based
> on their SID
> >         . Add sid-map property to store non-PCI/PCIe devices SID
> >         . Update implementation of CFGI commands based on device SID
> >     hw/arm/smmu-common:
> >         . Differentiate PCI/PCIe and non-PCI/PCIe devices SID getting
> strategy
> >     hw/arm/virt:
> >         . Add PL330 DMA controller and connect with SMMUv3 for
> testing
> >         . Add smmuv3_sidmap for non-PCI/PCIe devices SID setting
> 
> Please don't try to do all these things in one big patch --
> put together a patchseries with several smaller patches,
> each of which does one self-contained thing.
> 
  Got it. Thanks for your comments.
  Let me try to split them into several smaller patches.
  But all these updates work for 1 thing. They are depend on each other.
> 
> > Signed-off-by: Chunming Li <chunming.li@verisilicon.com>
> > Signed-off-by: Renwei Liu <renwei.liu@verisilicon.com>
> > ---
> > This patch depends on PL330 memory region connection patch:
> >
> https://patchew.org/QEMU/4C23C17B8E87E74E906A25A3254A03F4FA1FEC31@SHASX
> M03.verisilicon.com/
> 
> If you have a patch that depends on another, it's usually better to
> send them as a patchseries. I was wondering what the reason for
> that PL330 patch was...
  I need a non-PCI/PCIe device to do the verification. The old PL330 DMA controller
  cannot configure its memory region manually. So we update it and provide path.
  PL330 patch was reviewed and will be merged in target-arm.next for 6.2.
  The virt.c compile will fail if there is no PL330 device patch.
> 
> thanks
> -- PMM

Thanks.

Chunming Li

Re: [PATCH] hw/arm/smmuv3: Support non-PCI/PCIe devices connection

[Peter Maydell]

On Fri, 20 Aug 2021 at 10:04, Li, Chunming <Chunming.Li@verisilicon.com> wrote:
>
> > On Fri, 20 Aug 2021 at 03:36, Li, Chunming
> > <Chunming.Li@verisilicon.com> wrote:
> > >
> > > The current SMMU V3 device model only support PCI/PCIe devices,
> > > so we update it to support non-PCI/PCIe devices.
> > >
> > >     hw/arm/smmuv3:
> > >         . Create IOMMU memory regions for non-PCI/PCIe devices based
> > on their SID
> > >         . Add sid-map property to store non-PCI/PCIe devices SID
> > >         . Update implementation of CFGI commands based on device SID
> > >     hw/arm/smmu-common:
> > >         . Differentiate PCI/PCIe and non-PCI/PCIe devices SID getting
> > strategy
> > >     hw/arm/virt:
> > >         . Add PL330 DMA controller and connect with SMMUv3 for
> > testing
> > >         . Add smmuv3_sidmap for non-PCI/PCIe devices SID setting
> >
> > Please don't try to do all these things in one big patch --
> > put together a patchseries with several smaller patches,
> > each of which does one self-contained thing.
> >
>   Got it. Thanks for your comments.
>   Let me try to split them into several smaller patches.
>   But all these updates work for 1 thing. They are depend on each other.

Yes, that's why you send a single patch *series*, which has
a cover letter that explains the overall purpose. Then each
individual patch in the series has a commit message that
describes what that specific patch is doing. As emails, the
patches are all sent as follow-ups to the coverletter.
"git format-patch" and "git send-email" should handle this for you.

https://wiki.qemu.org/Contribute/SubmitAPatch#Split_up_long_patches
has a little more discussion of this.

> > If you have a patch that depends on another, it's usually better to
> > send them as a patchseries. I was wondering what the reason for
> > that PL330 patch was...

>   I need a non-PCI/PCIe device to do the verification. The old PL330 DMA controller
>   cannot configure its memory region manually. So we update it and provide path.
>   PL330 patch was reviewed and will be merged in target-arm.next for 6.2.
>   The virt.c compile will fail if there is no PL330 device patch.

Yeah, I accepted it because it is a sensible cleanup on its own;
but it would have been a bit easier for me to understand why you
were doing that if I'd had the wider context.

thanks
-- PMM

RE: [PATCH] hw/arm/smmuv3: Support non-PCI/PCIe devices connection

[Li, Chunming]

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Friday, August 20, 2021 5:15 PM
> To: Li, Chunming
> Cc: eric.auger@redhat.com; qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> Wen, Jianxian; Liu, Renwei
> Subject: Re: [PATCH] hw/arm/smmuv3: Support non-PCI/PCIe devices
> connection
> 
> On Fri, 20 Aug 2021 at 10:04, Li, Chunming
> <Chunming.Li@verisilicon.com> wrote:
> >
> > > On Fri, 20 Aug 2021 at 03:36, Li, Chunming
> > > <Chunming.Li@verisilicon.com> wrote:
> > > >
> > > > The current SMMU V3 device model only support PCI/PCIe devices,
> > > > so we update it to support non-PCI/PCIe devices.
> > > >
> > > >     hw/arm/smmuv3:
> > > >         . Create IOMMU memory regions for non-PCI/PCIe devices
> based
> > > on their SID
> > > >         . Add sid-map property to store non-PCI/PCIe devices SID
> > > >         . Update implementation of CFGI commands based on device
> SID
> > > >     hw/arm/smmu-common:
> > > >         . Differentiate PCI/PCIe and non-PCI/PCIe devices SID
> getting
> > > strategy
> > > >     hw/arm/virt:
> > > >         . Add PL330 DMA controller and connect with SMMUv3 for
> > > testing
> > > >         . Add smmuv3_sidmap for non-PCI/PCIe devices SID setting
> > >
> > > Please don't try to do all these things in one big patch --
> > > put together a patchseries with several smaller patches,
> > > each of which does one self-contained thing.
> > >
> >   Got it. Thanks for your comments.
> >   Let me try to split them into several smaller patches.
> >   But all these updates work for 1 thing. They are depend on each
> other.
> 
> Yes, that's why you send a single patch *series*, which has
> a cover letter that explains the overall purpose. Then each
> individual patch in the series has a commit message that
> describes what that specific patch is doing. As emails, the
> patches are all sent as follow-ups to the coverletter.
> "git format-patch" and "git send-email" should handle this for you.
> 

Sorry for interrupt you. 
Could you help to check why my series patch cannot be listed by https://patchew.org/?
I split the patch into 4 commits with 1 coverletter and send them out with v4 tag.
I can see all 5 emails in https://www.mail-archive.com/qemu-devel@nongnu.org/ 
But https://patchew.org/QEMU/49C79B700B5D8F45B8EF0861B4EF3B3B01142FABD6@SHASXM03.verisilicon.com/# 
show " Only 0 patches received! ".
It is great helpful If you can help to check and tell me what mistake I made.
Thanks very much!

> https://wiki.qemu.org/Contribute/SubmitAPatch#Split_up_long_patches
> has a little more discussion of this.
> 
> > > If you have a patch that depends on another, it's usually better to
> > > send them as a patchseries. I was wondering what the reason for
> > > that PL330 patch was...
> 
> >   I need a non-PCI/PCIe device to do the verification. The old PL330
> DMA controller
> >   cannot configure its memory region manually. So we update it and
> provide path.
> >   PL330 patch was reviewed and will be merged in target-arm.next for
> 6.2.
> >   The virt.c compile will fail if there is no PL330 device patch.
> 
> Yeah, I accepted it because it is a sensible cleanup on its own;
> but it would have been a bit easier for me to understand why you
> were doing that if I'd had the wider context.
> 
> thanks
> -- PMM

Re: [PATCH] hw/arm/smmuv3: Support non-PCI/PCIe devices connection

[Peter Maydell]

On Tue, 24 Aug 2021 at 09:22, Li, Chunming <Chunming.Li@verisilicon.com> wrote:
> Sorry for interrupt you.
> Could you help to check why my series patch cannot be listed by https://patchew.org/?
> I split the patch into 4 commits with 1 coverletter and send them out with v4 tag.
> I can see all 5 emails in https://www.mail-archive.com/qemu-devel@nongnu.org/
> But https://patchew.org/QEMU/49C79B700B5D8F45B8EF0861B4EF3B3B01142FABD6@SHASXM03.verisilicon.com/#
> show " Only 0 patches received! ".
> It is great helpful If you can help to check and tell me what mistake I made.
> Thanks very much!

I guess you figured out the problem already, because v5 has them all:
https://patchew.org/QEMU/1629878922-173270-1-git-send-email-chunming_li1234@163.com/
But the problem was that for v4 the patch emails were not sent as
threaded emails following up to the cover letter (that is, they were
missing appropriate References: headers).

-- PMM

RE: [PATCH] hw/arm/smmuv3: Support non-PCI/PCIe devices connection

[Li, Chunming]

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Thursday, August 26, 2021 11:57 PM
> To: Li, Chunming
> Cc: eric.auger@redhat.com; Liu, Renwei; qemu-arm@nongnu.org; Wen,
> Jianxian; qemu-devel@nongnu.org
> Subject: Re: [PATCH] hw/arm/smmuv3: Support non-PCI/PCIe devices
> connection
> 
> On Tue, 24 Aug 2021 at 09:22, Li, Chunming
> <Chunming.Li@verisilicon.com> wrote:
> > Sorry for interrupt you.
> > Could you help to check why my series patch cannot be listed by
> https://patchew.org/?
> > I split the patch into 4 commits with 1 coverletter and send them out
> with v4 tag.
> > I can see all 5 emails in https://www.mail-archive.com/qemu-
> devel@nongnu.org/
> > But
> https://patchew.org/QEMU/49C79B700B5D8F45B8EF0861B4EF3B3B01142FABD6@SHA
> SXM03.verisilicon.com/#
> > show " Only 0 patches received! ".
> > It is great helpful If you can help to check and tell me what mistake
> I made.
> > Thanks very much!
> 
> I guess you figured out the problem already, because v5 has them all:
> https://patchew.org/QEMU/1629878922-173270-1-git-send-email-
> chunming_li1234@163.com/
> But the problem was that for v4 the patch emails were not sent as
> threaded emails following up to the cover letter (that is, they were
> missing appropriate References: headers).
> 
> -- PMM

Yes, it is. Thanks for your help. I found the problem and fixed it in v5.
Please help to check the v5 patch, looking forward to your review feedback.

Chunming