[PATCH v5 0/4] hw/arm/smmuv3: Support non PCI/PCIe devices

[PATCH v5 0/4] hw/arm/smmuv3: Support non PCI/PCIe devices

[chunming]

From: chunming <chunming.li@verisilicon.com>

The current SMMU v3 model only support PCI/PCIe devices, so we update it for 
non-PCI/PCIe devices.
  . Add independent IOMMU memory regions for non-PCI/PCIe devices
  . Add SID value property setting for non-PCI/PCIe devices
  . Add PL330 DMA controller into "virt" machine and connect with SMMU v3
  . Test PL330 DMA controller and PCIe e1000 network with SMMU v3 enabled

Notes:
  You need apply PL330 memory region patch before compile "virt" machine:
  https://patchew.org/QEMU/4C23C17B8E87E74E906A25A3254A03F4FA1FEC31@SHASXM03.verisilicon.com/

  The old PL330 model cannot configure its memory region manually. 
  So we update it and provide path.
  The patch was reviewed and will be merged in target-arm.next for 6.2.


chunming (4):
  hw/arm/smmuv3: Support non PCI/PCIe device connect with SMMU v3
  hw/arm/smmuv3: Update implementation of CFGI commands based on device
    SID
  hw/arm/virt: Update SMMU v3 creation to support non PCI/PCIe device
    connection
  hw/arm/virt: Add PL330 DMA controller and connect with SMMU v3

 hw/arm/smmuv3.c              |  81 ++++++++++++++++++--------
 hw/arm/virt.c                | 108 ++++++++++++++++++++++++++++++++++-
 include/hw/arm/smmu-common.h |  12 +++-
 include/hw/arm/smmuv3.h      |   2 +
 include/hw/arm/virt.h        |   3 +
 5 files changed, 178 insertions(+), 28 deletions(-)

-- 
2.30.2

[PATCH v5 1/4] hw/arm/smmuv3: Support non PCI/PCIe device connect with SMMU v3

[chunming]

From: chunming <chunming.li@verisilicon.com>

  . Add sid-map property to store non PCI/PCIe devices SID
  . Create IOMMU memory regions for non PCI/PCIe devices based on their SID
  . Update SID getting strategy for PCI/PCIe and non PCI/PCIe devices

Signed-off-by: chunming <chunming.li@verisilicon.com>
---
 hw/arm/smmuv3.c              | 46 ++++++++++++++++++++++++++++++++++++
 include/hw/arm/smmu-common.h |  7 +++++-
 include/hw/arm/smmuv3.h      |  2 ++
 3 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 01b60bee49..11d7fe8423 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
@@ -1430,6 +1431,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 +1451,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 +1471,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 +1545,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 +1560,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/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 706be3c6d0..95cd12a4b5 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 == NULL) {
+        return sdev->devfn;
+    } else {
+        return PCI_BUILD_BDF(pci_bus_num(sdev->bus), sdev->devfn);
+    }
 }
 
 /**
diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
index c641e60735..32ba84990d 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;
-- 
2.30.2

Re: [PATCH v5 1/4] hw/arm/smmuv3: Support non PCI/PCIe device connect with SMMU v3

[Eric Auger]

Hi Chunming,

On 8/25/21 10:08 AM, chunming wrote:
> From: chunming <chunming.li@verisilicon.com>
>
>   . Add sid-map property to store non PCI/PCIe devices SID
>   . Create IOMMU memory regions for non PCI/PCIe devices based on their SID
>   . Update SID getting strategy for PCI/PCIe and non PCI/PCIe devices
>
> Signed-off-by: chunming <chunming.li@verisilicon.com>
> ---
>  hw/arm/smmuv3.c              | 46 ++++++++++++++++++++++++++++++++++++
>  include/hw/arm/smmu-common.h |  7 +++++-
>  include/hw/arm/smmuv3.h      |  2 ++
>  3 files changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 01b60bee49..11d7fe8423 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
> @@ -1430,6 +1431,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 +1451,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 +1471,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 */
s/peripheral devices/platform devices? I would suggest to rename the
fields contained 'peri' too.
> +    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 +1545,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 +1560,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/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 706be3c6d0..95cd12a4b5 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 == NULL) {
> +        return sdev->devfn;
> +    } else {
> +        return PCI_BUILD_BDF(pci_bus_num(sdev->bus), sdev->devfn);
> +    }
>  }
>  
>  /**
> diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
> index c641e60735..32ba84990d 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;

peri_sdev_list is a field of the SMMUState. Why don't you put those fields in the parent class too. If we were to support another other model of SMMU, platform device support would be meaningful there too so I would sugeest to put the fields and peorperty and this level.

>  
>      uint32_t idr[6];
>      uint32_t iidr;
Thanks

Eric

RE: [PATCH v5 1/4] hw/arm/smmuv3: Support non PCI/PCIe device connect with SMMU v3

[Li, Chunming]

> -----Original Message-----
> From: Eric Auger [mailto:eric.auger@redhat.com]
> Sent: Tuesday, August 31, 2021 10:02 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 1/4] hw/arm/smmuv3: Support non PCI/PCIe device
> connect with SMMU v3
> 
> Hi Chunming,
> 
> On 8/25/21 10:08 AM, chunming wrote:
> > From: chunming <chunming.li@verisilicon.com>
> >
> >   . Add sid-map property to store non PCI/PCIe devices SID
> >   . Create IOMMU memory regions for non PCI/PCIe devices based on
> their SID
> >   . Update SID getting strategy for PCI/PCIe and non PCI/PCIe devices
> >
> > Signed-off-by: chunming <chunming.li@verisilicon.com>
> > ---
> >  hw/arm/smmuv3.c              | 46
> ++++++++++++++++++++++++++++++++++++
> >  include/hw/arm/smmu-common.h |  7 +++++-
> >  include/hw/arm/smmuv3.h      |  2 ++
> >  3 files changed, 54 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > index 01b60bee49..11d7fe8423 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
> > @@ -1430,6 +1431,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 +1451,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 +1471,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 */
> s/peripheral devices/platform devices? I would suggest to rename the
> fields contained 'peri' too.

Got it. Will rename sid_map -> peri_sid_map, num_sid -> peri_num_sid in next version.

> > +    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 +1545,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 +1560,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/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-
> common.h
> > index 706be3c6d0..95cd12a4b5 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 == NULL) {
> > +        return sdev->devfn;
> > +    } else {
> > +        return PCI_BUILD_BDF(pci_bus_num(sdev->bus), sdev->devfn);
> > +    }
> >  }
> >
> >  /**
> > diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
> > index c641e60735..32ba84990d 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;
> 
> peri_sdev_list is a field of the SMMUState. Why don't you put those
> fields in the parent class too. If we were to support another other
> model of SMMU, platform device support would be meaningful there too so
> I would sugeest to put the fields and peorperty and this level.

Got it. Will move the fields and property into SMMUState in next version.

> 
> >
> >      uint32_t idr[6];
> >      uint32_t iidr;
> Thanks
> 
> Eric

[PATCH v5 2/4] hw/arm/smmuv3: Update implementation of CFGI commands based on device SID

[chunming]

From: chunming <chunming.li@verisilicon.com>

Replace "smmuv3_flush_config" with "g_hash_table_foreach_remove".
"smmu_iommu_mr" function can't get MR according to SID for non PCI/PCIe devices.

Signed-off-by: chunming <chunming.li@verisilicon.com>
---
 hw/arm/smmuv3.c              | 35 ++++++++++-------------------------
 include/hw/arm/smmu-common.h |  5 ++++-
 2 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 11d7fe8423..9f3f13fb8e 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -613,14 +613,6 @@ static SMMUTransCfg *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event)
     return cfg;
 }
 
-static void smmuv3_flush_config(SMMUDevice *sdev)
-{
-    SMMUv3State *s = sdev->smmu;
-    SMMUState *bc = &s->smmu_state;
-
-    trace_smmuv3_config_cache_inv(smmu_get_sid(sdev));
-    g_hash_table_remove(bc->configs, sdev);
-}
 
 static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
                                       IOMMUAccessFlags flag, int iommu_idx)
@@ -964,22 +956,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 */
@@ -1006,21 +994,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:
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 95cd12a4b5..d016455d80 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -159,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 non PCI/PCIe device
+ */
 IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid);
 
 #define SMMU_IOTLB_MAX_SIZE 256
-- 
2.30.2

Re: [PATCH v5 2/4] hw/arm/smmuv3: Update implementation of CFGI commands based on device SID

[Eric Auger]

Hi Chunming

On 8/25/21 10:08 AM, chunming wrote:
> From: chunming <chunming.li@verisilicon.com>
>
> Replace "smmuv3_flush_config" with "g_hash_table_foreach_remove".
this replacement may have a potential negative impact on the performance
for PCIe support, which is the main use case: a unique
g_hash_table_remove() is replaced by an iteration over all the config
hash keys.

I wonder if you couldn't just adapt smmu_iommu_mr() and it case this
latter returns NULL for the current PCIe search, look up in the platform
device list:

peri_sdev_list?

Thanks

Eric



> "smmu_iommu_mr" function can't get MR according to SID for non PCI/PCIe devices.
>
> Signed-off-by: chunming <chunming.li@verisilicon.com>
> ---
>  hw/arm/smmuv3.c              | 35 ++++++++++-------------------------
>  include/hw/arm/smmu-common.h |  5 ++++-
>  2 files changed, 14 insertions(+), 26 deletions(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 11d7fe8423..9f3f13fb8e 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -613,14 +613,6 @@ static SMMUTransCfg *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event)
>      return cfg;
>  }
>  
> -static void smmuv3_flush_config(SMMUDevice *sdev)
> -{
> -    SMMUv3State *s = sdev->smmu;
> -    SMMUState *bc = &s->smmu_state;
> -
> -    trace_smmuv3_config_cache_inv(smmu_get_sid(sdev));
> -    g_hash_table_remove(bc->configs, sdev);
> -}
>  
>  static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
>                                        IOMMUAccessFlags flag, int iommu_idx)
> @@ -964,22 +956,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 */
> @@ -1006,21 +994,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:
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 95cd12a4b5..d016455d80 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -159,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 non PCI/PCIe device
> + */
>  IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid);
>  
>  #define SMMU_IOTLB_MAX_SIZE 256

RE: [PATCH v5 2/4] hw/arm/smmuv3: Update implementation of CFGI commands based on device SID

[Li, Chunming]

> -----Original Message-----
> From: Eric Auger [mailto:eric.auger@redhat.com]
> Sent: Tuesday, August 31, 2021 10:02 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 2/4] hw/arm/smmuv3: Update implementation of
> CFGI commands based on device SID
> 
> Hi Chunming
> 
> On 8/25/21 10:08 AM, chunming wrote:
> > From: chunming <chunming.li@verisilicon.com>
> >
> > Replace "smmuv3_flush_config" with "g_hash_table_foreach_remove".
> this replacement may have a potential negative impact on the
> performance
> for PCIe support, which is the main use case: a unique
> g_hash_table_remove() is replaced by an iteration over all the config
> hash keys.
> 
> I wonder if you couldn't just adapt smmu_iommu_mr() and it case this
> latter returns NULL for the current PCIe search, look up in the
> platform
> device list:
> 
> peri_sdev_list?

I think there are 2 scenes:
	1.  PCIe devices share same SID with peripheral devices.
      2.  Multi peripheral devices share same SID.
If we search PCIe 1st then search peri_sdev_list, there are 2 problems:
      1.  The code is complex.
      2.  We may need to search peri_sdev_list multi times. It may has performance impact.
        
The CFGI commands are only called when the SMMU device is removed.
So we think there is no big performance impact.

> 
> Thanks
> 
> Eric
> 
> 
> 
> > "smmu_iommu_mr" function can't get MR according to SID for non
> PCI/PCIe devices.
> >
> > Signed-off-by: chunming <chunming.li@verisilicon.com>
> > ---
> >  hw/arm/smmuv3.c              | 35 ++++++++++------------------------
> -
> >  include/hw/arm/smmu-common.h |  5 ++++-
> >  2 files changed, 14 insertions(+), 26 deletions(-)
> >
> > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> > index 11d7fe8423..9f3f13fb8e 100644
> > --- a/hw/arm/smmuv3.c
> > +++ b/hw/arm/smmuv3.c
> > @@ -613,14 +613,6 @@ static SMMUTransCfg
> *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event)
> >      return cfg;
> >  }
> >
> > -static void smmuv3_flush_config(SMMUDevice *sdev)
> > -{
> > -    SMMUv3State *s = sdev->smmu;
> > -    SMMUState *bc = &s->smmu_state;
> > -
> > -    trace_smmuv3_config_cache_inv(smmu_get_sid(sdev));
> > -    g_hash_table_remove(bc->configs, sdev);
> > -}
> >
> >  static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr
> addr,
> >                                        IOMMUAccessFlags flag, int
> iommu_idx)
> > @@ -964,22 +956,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
> */
> > @@ -1006,21 +994,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:
> > diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-
> common.h
> > index 95cd12a4b5..d016455d80 100644
> > --- a/include/hw/arm/smmu-common.h
> > +++ b/include/hw/arm/smmu-common.h
> > @@ -159,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 non PCI/PCIe
> device
> > + */
> >  IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid);
> >
> >  #define SMMU_IOTLB_MAX_SIZE 256

Re: [PATCH v5 2/4] hw/arm/smmuv3: Update implementation of CFGI commands based on device SID

[Eric Auger]

Hi,

On 9/1/21 8:51 AM, Li, Chunming wrote:
>
>> -----Original Message-----
>> From: Eric Auger [mailto:eric.auger@redhat.com]
>> Sent: Tuesday, August 31, 2021 10:02 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 2/4] hw/arm/smmuv3: Update implementation of
>> CFGI commands based on device SID
>>
>> Hi Chunming
>>
>> On 8/25/21 10:08 AM, chunming wrote:
>>> From: chunming <chunming.li@verisilicon.com>
>>>
>>> Replace "smmuv3_flush_config" with "g_hash_table_foreach_remove".
>> this replacement may have a potential negative impact on the
>> performance
>> for PCIe support, which is the main use case: a unique
>> g_hash_table_remove() is replaced by an iteration over all the config
>> hash keys.
>>
>> I wonder if you couldn't just adapt smmu_iommu_mr() and it case this
>> latter returns NULL for the current PCIe search, look up in the
>> platform
>> device list:
>>
>> peri_sdev_list?
> I think there are 2 scenes:
> 	1.  PCIe devices share same SID with peripheral devices.
>       2.  Multi peripheral devices share same SID.
> If we search PCIe 1st then search peri_sdev_list, there are 2 problems:
>       1.  The code is complex.
>       2.  We may need to search peri_sdev_list multi times. It may has performance impact.
why multiple times? 1st you look for a PCIe RID and then you look for
platform devices? Still I am dubious about the duplicate streamid case.
what I wanted to emphasize is at the moment I do not have a clear view
about your use case and I don't want to degrade the perf of the main use
case to support a yet to be defined one ;-)
>         
> The CFGI commands are only called when the SMMU device is removed.
> So we think there is no big performance impact.

Nevertheless I think this is not a major issue indeed.

Thanks

Eric
>
>> Thanks
>>
>> Eric
>>
>>
>>
>>> "smmu_iommu_mr" function can't get MR according to SID for non
>> PCI/PCIe devices.
>>> Signed-off-by: chunming <chunming.li@verisilicon.com>
>>> ---
>>>  hw/arm/smmuv3.c              | 35 ++++++++++------------------------
>> -
>>>  include/hw/arm/smmu-common.h |  5 ++++-
>>>  2 files changed, 14 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>>> index 11d7fe8423..9f3f13fb8e 100644
>>> --- a/hw/arm/smmuv3.c
>>> +++ b/hw/arm/smmuv3.c
>>> @@ -613,14 +613,6 @@ static SMMUTransCfg
>> *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event)
>>>      return cfg;
>>>  }
>>>
>>> -static void smmuv3_flush_config(SMMUDevice *sdev)
>>> -{
>>> -    SMMUv3State *s = sdev->smmu;
>>> -    SMMUState *bc = &s->smmu_state;
>>> -
>>> -    trace_smmuv3_config_cache_inv(smmu_get_sid(sdev));
>>> -    g_hash_table_remove(bc->configs, sdev);
>>> -}
>>>
>>>  static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr
>> addr,
>>>                                        IOMMUAccessFlags flag, int
>> iommu_idx)
>>> @@ -964,22 +956,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
>> */
>>> @@ -1006,21 +994,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:
>>> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-
>> common.h
>>> index 95cd12a4b5..d016455d80 100644
>>> --- a/include/hw/arm/smmu-common.h
>>> +++ b/include/hw/arm/smmu-common.h
>>> @@ -159,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 non PCI/PCIe
>> device
>>> + */
>>>  IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid);
>>>
>>>  #define SMMU_IOTLB_MAX_SIZE 256

RE: [PATCH v5 2/4] hw/arm/smmuv3: Update implementation of CFGI commands based on device SID

[Li, Chunming]

Hi,

> -----Original Message-----
> From: Eric Auger [mailto:eric.auger@redhat.com]
> Sent: Wednesday, September 01, 2021 8:05 PM
> To: Li, Chunming; chunming; peter.maydell@linaro.org
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org; Wen, Jianxian; Liu,
> Renwei
> Subject: Re: [PATCH v5 2/4] hw/arm/smmuv3: Update implementation of
> CFGI commands based on device SID
> 
> Hi,
> 
> On 9/1/21 8:51 AM, Li, Chunming wrote:
> >
> >> -----Original Message-----
> >> From: Eric Auger [mailto:eric.auger@redhat.com]
> >> Sent: Tuesday, August 31, 2021 10:02 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 2/4] hw/arm/smmuv3: Update implementation of
> >> CFGI commands based on device SID
> >>
> >> Hi Chunming
> >>
> >> On 8/25/21 10:08 AM, chunming wrote:
> >>> From: chunming <chunming.li@verisilicon.com>
> >>>
> >>> Replace "smmuv3_flush_config" with "g_hash_table_foreach_remove".
> >> this replacement may have a potential negative impact on the
> >> performance
> >> for PCIe support, which is the main use case: a unique
> >> g_hash_table_remove() is replaced by an iteration over all the
> config
> >> hash keys.
> >>
> >> I wonder if you couldn't just adapt smmu_iommu_mr() and it case this
> >> latter returns NULL for the current PCIe search, look up in the
> >> platform
> >> device list:
> >>
> >> peri_sdev_list?
> > I think there are 2 scenes:
> > 	1.  PCIe devices share same SID with peripheral devices.
> >       2.  Multi peripheral devices share same SID.
> > If we search PCIe 1st then search peri_sdev_list, there are 2
> problems:
> >       1.  The code is complex.
> >       2.  We may need to search peri_sdev_list multi times. It may
> has performance impact.
> why multiple times? 1st you look for a PCIe RID and then you look for
> platform devices? Still I am dubious about the duplicate streamid case.
> what I wanted to emphasize is at the moment I do not have a clear view
> about your use case and I don't want to degrade the perf of the main
> use
> case to support a yet to be defined one ;-)

Yes, you are right. I agree with you after re-checking the code.
I will update it in next version.

> >
> > The CFGI commands are only called when the SMMU device is removed.
> > So we think there is no big performance impact.
> 
> Nevertheless I think this is not a major issue indeed.
> 
> Thanks
> 
> Eric
> >
> >> Thanks
> >>
> >> Eric
> >>
> >>
> >>
> >>> "smmu_iommu_mr" function can't get MR according to SID for non
> >> PCI/PCIe devices.
> >>> Signed-off-by: chunming <chunming.li@verisilicon.com>
> >>> ---
> >>>  hw/arm/smmuv3.c              | 35 ++++++++++----------------------
> --
> >> -
> >>>  include/hw/arm/smmu-common.h |  5 ++++-
> >>>  2 files changed, 14 insertions(+), 26 deletions(-)
> >>>
> >>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> >>> index 11d7fe8423..9f3f13fb8e 100644
> >>> --- a/hw/arm/smmuv3.c
> >>> +++ b/hw/arm/smmuv3.c
> >>> @@ -613,14 +613,6 @@ static SMMUTransCfg
> >> *smmuv3_get_config(SMMUDevice *sdev, SMMUEventInfo *event)
> >>>      return cfg;
> >>>  }
> >>>
> >>> -static void smmuv3_flush_config(SMMUDevice *sdev)
> >>> -{
> >>> -    SMMUv3State *s = sdev->smmu;
> >>> -    SMMUState *bc = &s->smmu_state;
> >>> -
> >>> -    trace_smmuv3_config_cache_inv(smmu_get_sid(sdev));
> >>> -    g_hash_table_remove(bc->configs, sdev);
> >>> -}
> >>>
> >>>  static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr,
> hwaddr
> >> addr,
> >>>                                        IOMMUAccessFlags flag, int
> >> iommu_idx)
> >>> @@ -964,22 +956,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
> >> */
> >>> @@ -1006,21 +994,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:
> >>> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-
> >> common.h
> >>> index 95cd12a4b5..d016455d80 100644
> >>> --- a/include/hw/arm/smmu-common.h
> >>> +++ b/include/hw/arm/smmu-common.h
> >>> @@ -159,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 non PCI/PCIe
> >> device
> >>> + */
> >>>  IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid);
> >>>
> >>>  #define SMMU_IOTLB_MAX_SIZE 256

[PATCH v5 3/4] hw/arm/virt: Update SMMU v3 creation to support non PCI/PCIe device connection

[chunming]

From: chunming <chunming.li@verisilicon.com>

  . Add "smmuv3_sidmap" to set non PCI/PCIe devices SID value
  . Pass non PCI/PCIe devices SID value to SMMU v3 model creation
  . Store SMMU v3 device in virtual machine then non PCI/PCIe can get its memory region later

Signed-off-by: chunming <chunming.li@verisilicon.com>
---
 hw/arm/virt.c         | 18 +++++++++++++++++-
 include/hw/arm/virt.h |  2 ++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 81eda46b0b..c3fd30e071 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -204,6 +204,10 @@ static const char *valid_cpus[] = {
     ARM_CPU_TYPE_NAME("max"),
 };
 
+static const uint16_t smmuv3_sidmap[] = {
+
+};
+
 static bool cpu_type_valid(const char *cpu)
 {
     int i;
@@ -1223,7 +1227,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 +1248,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++) {
@@ -2762,6 +2776,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/virt.h b/include/hw/arm/virt.h
index 9661c46699..d3402a43dd 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -167,6 +167,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)
-- 
2.30.2

Re: [PATCH v5 3/4] hw/arm/virt: Update SMMU v3 creation to support non PCI/PCIe device connection

[Eric Auger]

Hi Chunming,

On 8/25/21 10:08 AM, chunming wrote:
> From: chunming <chunming.li@verisilicon.com>
>
>   . Add "smmuv3_sidmap" to set non PCI/PCIe devices SID value
>   . Pass non PCI/PCIe devices SID value to SMMU v3 model creation
>   . Store SMMU v3 device in virtual machine then non PCI/PCIe can get its memory region later
>
> Signed-off-by: chunming <chunming.li@verisilicon.com>
> ---
>  hw/arm/virt.c         | 18 +++++++++++++++++-
>  include/hw/arm/virt.h |  2 ++
>  2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 81eda46b0b..c3fd30e071 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -204,6 +204,10 @@ static const char *valid_cpus[] = {
>      ARM_CPU_TYPE_NAME("max"),
>  };
>  
> +static const uint16_t smmuv3_sidmap[] = {
> +
> +};
> +
>  static bool cpu_type_valid(const char *cpu)
>  {
>      int i;
> @@ -1223,7 +1227,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 +1248,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++) {
> @@ -2762,6 +2776,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/virt.h b/include/hw/arm/virt.h
> index 9661c46699..d3402a43dd 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -167,6 +167,8 @@ struct VirtMachineState {
>      PCIBus *bus;
>      char *oem_id;
>      char *oem_table_id;
> +    DeviceState *smmuv3;
at this stage it is not obvious why you need the smmuv3 DeviceState in
the VirtMachineState (there is no user). You may squash this change in
subsequent patch instead

Thanks

Eric
> +    const uint16_t *sidmap;
>  };
>  
>  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)

RE: [PATCH v5 3/4] hw/arm/virt: Update SMMU v3 creation to support non PCI/PCIe device connection

[Li, Chunming]

> -----Original Message-----
> From: Eric Auger [mailto:eric.auger@redhat.com]
> Sent: Tuesday, August 31, 2021 10:13 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 3/4] hw/arm/virt: Update SMMU v3 creation to
> support non PCI/PCIe device connection
> 
> Hi Chunming,
> 
> On 8/25/21 10:08 AM, chunming wrote:
> > From: chunming <chunming.li@verisilicon.com>
> >
> >   . Add "smmuv3_sidmap" to set non PCI/PCIe devices SID value
> >   . Pass non PCI/PCIe devices SID value to SMMU v3 model creation
> >   . Store SMMU v3 device in virtual machine then non PCI/PCIe can get
> its memory region later
> >
> > Signed-off-by: chunming <chunming.li@verisilicon.com>
> > ---
> >  hw/arm/virt.c         | 18 +++++++++++++++++-
> >  include/hw/arm/virt.h |  2 ++
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 81eda46b0b..c3fd30e071 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -204,6 +204,10 @@ static const char *valid_cpus[] = {
> >      ARM_CPU_TYPE_NAME("max"),
> >  };
> >
> > +static const uint16_t smmuv3_sidmap[] = {
> > +
> > +};
> > +
> >  static bool cpu_type_valid(const char *cpu)
> >  {
> >      int i;
> > @@ -1223,7 +1227,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 +1248,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++) {
> > @@ -2762,6 +2776,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/virt.h b/include/hw/arm/virt.h
> > index 9661c46699..d3402a43dd 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -167,6 +167,8 @@ struct VirtMachineState {
> >      PCIBus *bus;
> >      char *oem_id;
> >      char *oem_table_id;
> > +    DeviceState *smmuv3;
> at this stage it is not obvious why you need the smmuv3 DeviceState in
> the VirtMachineState (there is no user). You may squash this change in
> subsequent patch instead

Got it. Move this change in subsequent patch instead in next version.

> 
> Thanks
> 
> Eric
> > +    const uint16_t *sidmap;
> >  };
> >
> >  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> VIRT_PCIE_ECAM)

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

[chunming]

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.

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,
-- 
2.30.2

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

[Eric Auger]

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?

This static SID allocation may not work in general as it may collide
with PCIe RID space?

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.
I guess this should work with virtio platform devices and VFIO platform
devices. How would you extend that work to those devices?

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,

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

[Li, Chunming]

> -----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.

> 
> 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.

> 
> 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?

> 
> 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,

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,

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

[Li, Chunming]

Hi,

> -----Original Message-----
> From: Eric Auger [mailto:eric.auger@redhat.com]
> Sent: Wednesday, September 01, 2021 6:23 PM
> To: Li, Chunming; chunming; peter.maydell@linaro.org
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org; Wen, Jianxian; Liu,
> Renwei
> Subject: Re: [PATCH v5 4/4] hw/arm/virt: Add PL330 DMA controller and
> connect with SMMU v3
> 
> 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?

The actual use case is this:
1. We will have a SoC which has SMMUv3 connected with our owned platform 
   Video Encoder/Decoder and other IPs
2. We plan to use SMMUv3 stage 1 for continuous memory allocation
   and stage 2 for memory protection
3. We are developing our own IP QEMU models now
4. These models will be connected with SMMUv3 in QEMU
5. We will use the QEMU board to development IP driver and ensure the driver 
   can work well with Linux SMMU and IOMMU framework

> 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).

I get your point but I cannot give you a clear answer now. I didn't consider
virtio-iommu before because our current use case doesn't need virtio-iommu.

> 
> 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,

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

[Eric Auger]

Hi,

On 9/2/21 8:46 AM, Li, Chunming wrote:
> Hi,
>
>> -----Original Message-----
>> From: Eric Auger [mailto:eric.auger@redhat.com]
>> Sent: Wednesday, September 01, 2021 6:23 PM
>> To: Li, Chunming; chunming; peter.maydell@linaro.org
>> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org; Wen, Jianxian; Liu,
>> Renwei
>> Subject: Re: [PATCH v5 4/4] hw/arm/virt: Add PL330 DMA controller and
>> connect with SMMU v3
>>
>> 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?
> The actual use case is this:
> 1. We will have a SoC which has SMMUv3 connected with our owned platform 
>    Video Encoder/Decoder and other IPs
> 2. We plan to use SMMUv3 stage 1 for continuous memory allocation
>    and stage 2 for memory protection
> 3. We are developing our own IP QEMU models now
> 4. These models will be connected with SMMUv3 in QEMU
> 5. We will use the QEMU board to development IP driver and ensure the driver 
>    can work well with Linux SMMU and IOMMU framework
I see and I understand your use case for system modeling purpose.

This raises few questions/comments though.
- supporting platform device protection from the vIOMMU/ARM makes sense
to me globally. But above use case does not justify (to me) the
introduction of PL330 in machvirt because it would be just for testing
purpose. Peter may validate/invalidate though. Instead I think you
should try to illustrate this feature with DMA capable platform devices
such virtio-net and virtio-block sysbus devices as a counterpart of
their PCIe flavour.
>
>> 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).
> I get your point but I cannot give you a clear answer now. I didn't consider
> virtio-iommu before because our current use case doesn't need virtio-iommu.

I think you need to have consistency in the machvirt topology: with
current series the PL330 would be protected with vSMMUv3 and not with
virtio-iommu which does not seem acceptable to me. Either we need to
devise a way to individually specify which sysbus device is protected
and potentially also specify its streamid or all/none of them are.

Thanks

Eric
>
>> 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,

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

[Li, Chunming]

Hi,

> -----Original Message-----
> From: Eric Auger [mailto:eric.auger@redhat.com]
> Sent: Thursday, September 02, 2021 3:46 PM
> To: Li, Chunming; chunming; peter.maydell@linaro.org
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org; Wen, Jianxian; Liu,
> Renwei
> Subject: Re: [PATCH v5 4/4] hw/arm/virt: Add PL330 DMA controller and
> connect with SMMU v3
> 
> Hi,
> 
> On 9/2/21 8:46 AM, Li, Chunming wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Eric Auger [mailto:eric.auger@redhat.com]
> >> Sent: Wednesday, September 01, 2021 6:23 PM
> >> To: Li, Chunming; chunming; peter.maydell@linaro.org
> >> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org; Wen, Jianxian; Liu,
> >> Renwei
> >> Subject: Re: [PATCH v5 4/4] hw/arm/virt: Add PL330 DMA controller
> and
> >> connect with SMMU v3
> >>
> >> 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?
> > The actual use case is this:
> > 1. We will have a SoC which has SMMUv3 connected with our owned
> platform
> >    Video Encoder/Decoder and other IPs
> > 2. We plan to use SMMUv3 stage 1 for continuous memory allocation
> >    and stage 2 for memory protection
> > 3. We are developing our own IP QEMU models now
> > 4. These models will be connected with SMMUv3 in QEMU
> > 5. We will use the QEMU board to development IP driver and ensure the
> driver
> >    can work well with Linux SMMU and IOMMU framework
> I see and I understand your use case for system modeling purpose.
> 
> This raises few questions/comments though.
> - supporting platform device protection from the vIOMMU/ARM makes sense
> to me globally. But above use case does not justify (to me) the
> introduction of PL330 in machvirt because it would be just for testing
> purpose. Peter may validate/invalidate though. Instead I think you
> should try to illustrate this feature with DMA capable platform devices
> such virtio-net and virtio-block sysbus devices as a counterpart of
> their PCIe flavour.

Thanks for your suggestion. I will try virtio-net and virtio-block sysbus 
devices in next step.
But I hope to keep PL330 because it's not just for testing purpose. 
It's a good example to show how to connect platform devices with SMMUv3 based
on this patch.
I assume other developer may have same requirement.

> >
> >> 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).
> > I get your point but I cannot give you a clear answer now. I didn't
> consider
> > virtio-iommu before because our current use case doesn't need virtio-
> iommu.
> 
> I think you need to have consistency in the machvirt topology: with
> current series the PL330 would be protected with vSMMUv3 and not with
> virtio-iommu which does not seem acceptable to me. Either we need to
> devise a way to individually specify which sysbus device is protected
> and potentially also specify its streamid or all/none of them are.
> 
> Thanks
> 
> Eric

Thanks for your suggestion. I created a v6 patch fixed your other feedback.
Let us continue the virtio-iommu discussion and I will try it in next step.

> >
> >> 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,

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

[Peter Maydell]

On Thu, 2 Sept 2021 at 09:23, Li, Chunming <Chunming.Li@verisilicon.com> wrote:
> Eric Auger wrote:
>> On 9/2/21 8:46 AM, Li, Chunming wrote:
>>> Eric Auger wrote:

>>>> Then I think you need to bring a proper motivation behind adding the
>>>> PL330 in machvirt besides a testing purpose.

>>>> After this series you would get a single platform device connected to
>>>> the SMMU, the PL330. What is the actual use case?

>>> The actual use case is this:
>>> 1. We will have a SoC which has SMMUv3 connected with our owned platform
>>>    Video Encoder/Decoder and other IPs
>>> 2. We plan to use SMMUv3 stage 1 for continuous memory allocation
>>>    and stage 2 for memory protection
>>> 3. We are developing our own IP QEMU models now
>>> 4. These models will be connected with SMMUv3 in QEMU
>>> 5. We will use the QEMU board to development IP driver and ensure the driver
>>>    can work well with Linux SMMU and IOMMU framework

>> I see and I understand your use case for system modeling purpose.
>>
>> This raises few questions/comments though.
>> - supporting platform device protection from the vIOMMU/ARM makes sense
>> to me globally. But above use case does not justify (to me) the
>> introduction of PL330 in machvirt because it would be just for testing
>> purpose. Peter may validate/invalidate though. Instead I think you
>> should try to illustrate this feature with DMA capable platform devices
>> such virtio-net and virtio-block sysbus devices as a counterpart of
>> their PCIe flavour.
>
> Thanks for your suggestion. I will try virtio-net and virtio-block sysbus
> devices in next step.
> But I hope to keep PL330 because it's not just for testing purpose.
> It's a good example to show how to connect platform devices with SMMUv3 based
> on this patch.
> I assume other developer may have same requirement.

I agree with Eric here that the 'virt' board is not the right place
to put this PL330. The 'virt' board is not intended as a dumping
ground or experimental testbed for every possible arm device or
system configuration -- we try to keep it at least reasonably
targeted towards the "generic virtual platform or VM" usecase.

The "sbsa-ref" platform is intended to look more like "real hardware".
It's possible that "memory mapped hardware that sits behind an SMMU"
is something that you would see on the sort of system that sbsa-ref
is trying to model. I've cc'd Leif who would have a better idea about
that.

thanks
-- PMM

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

[Leif Lindholm]

Hi Peter,

On Tue, Sep 07, 2021 at 12:00:45 +0100, Peter Maydell wrote:
> On Thu, 2 Sept 2021 at 09:23, Li, Chunming <Chunming.Li@verisilicon.com> wrote:
> > Eric Auger wrote:
> >> On 9/2/21 8:46 AM, Li, Chunming wrote:
> >>> Eric Auger wrote:
> 
> >>>> Then I think you need to bring a proper motivation behind adding the
> >>>> PL330 in machvirt besides a testing purpose.
> 
> >>>> After this series you would get a single platform device connected to
> >>>> the SMMU, the PL330. What is the actual use case?
> 
> >>> The actual use case is this:
> >>> 1. We will have a SoC which has SMMUv3 connected with our owned platform
> >>>    Video Encoder/Decoder and other IPs
> >>> 2. We plan to use SMMUv3 stage 1 for continuous memory allocation
> >>>    and stage 2 for memory protection
> >>> 3. We are developing our own IP QEMU models now
> >>> 4. These models will be connected with SMMUv3 in QEMU
> >>> 5. We will use the QEMU board to development IP driver and ensure the driver
> >>>    can work well with Linux SMMU and IOMMU framework
> 
> >> I see and I understand your use case for system modeling purpose.
> >>
> >> This raises few questions/comments though.
> >> - supporting platform device protection from the vIOMMU/ARM makes sense
> >> to me globally. But above use case does not justify (to me) the
> >> introduction of PL330 in machvirt because it would be just for testing
> >> purpose. Peter may validate/invalidate though. Instead I think you
> >> should try to illustrate this feature with DMA capable platform devices
> >> such virtio-net and virtio-block sysbus devices as a counterpart of
> >> their PCIe flavour.
> >
> > Thanks for your suggestion. I will try virtio-net and virtio-block sysbus
> > devices in next step.
> > But I hope to keep PL330 because it's not just for testing purpose.
> > It's a good example to show how to connect platform devices with SMMUv3 based
> > on this patch.
> > I assume other developer may have same requirement.
>
> I agree with Eric here that the 'virt' board is not the right place
> to put this PL330. The 'virt' board is not intended as a dumping
> ground or experimental testbed for every possible arm device or
> system configuration -- we try to keep it at least reasonably
> targeted towards the "generic virtual platform or VM" usecase.

Agreed.

> The "sbsa-ref" platform is intended to look more like "real hardware".
> It's possible that "memory mapped hardware that sits behind an SMMU"
> is something that you would see on the sort of system that sbsa-ref
> is trying to model. I've cc'd Leif who would have a better idea about
> that.

I don't know if there's a glovelike fit there, but I agree it's the
closest fit of any platform in QEMU today.

Most DMA on an SBSA system would normally be expected to come from
PCIe, but some have certainly been released with random platform
devices, and it's certainly conceivable that some platform devices
might wants to make use of DMA.

The potential conflict I can see with what this patchset is the use of
a qemu-generated DT to describe the hw to the OS, whereas sbbr-ref
uses ACPI, generated in-guest with a few hints from QEMU, to abstract
it.

It looks to me like the hand-in-glove fit for this use-case would be
an ebbr-ref platform. (Or I guess SystemReady-ES-ref this week.)

But if the ACPI thing isn't a showstopper, I don't particularly mind
carrying an extra peripheral or two in the address space.

/
    Leif

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

[Li, Chunming]

Hi,

> -----Original Message-----
> From: Li, Chunming
> Sent: Thursday, September 02, 2021 4:23 PM
> To: 'eric.auger@redhat.com'; chunming; peter.maydell@linaro.org
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org; Wen, Jianxian; Liu,
> Renwei
> Subject: RE: [PATCH v5 4/4] hw/arm/virt: Add PL330 DMA controller and
> connect with SMMU v3
> 
> Hi,
> 
> > -----Original Message-----
> > From: Eric Auger [mailto:eric.auger@redhat.com]
> > Sent: Thursday, September 02, 2021 3:46 PM
> > To: Li, Chunming; chunming; peter.maydell@linaro.org
> > Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org; Wen, Jianxian; Liu,
> > Renwei
> > Subject: Re: [PATCH v5 4/4] hw/arm/virt: Add PL330 DMA controller and
> > connect with SMMU v3
> >
> > Hi,
> >
> > On 9/2/21 8:46 AM, Li, Chunming wrote:
> > > Hi,
> > >
> > >> -----Original Message-----
> > >> From: Eric Auger [mailto:eric.auger@redhat.com]
> > >> Sent: Wednesday, September 01, 2021 6:23 PM
> > >> To: Li, Chunming; chunming; peter.maydell@linaro.org
> > >> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org; Wen, Jianxian;
> Liu,
> > >> Renwei
> > >> Subject: Re: [PATCH v5 4/4] hw/arm/virt: Add PL330 DMA controller
> > and
> > >> connect with SMMU v3
> > >>
> > >> 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?
> > > The actual use case is this:
> > > 1. We will have a SoC which has SMMUv3 connected with our owned
> > platform
> > >    Video Encoder/Decoder and other IPs
> > > 2. We plan to use SMMUv3 stage 1 for continuous memory allocation
> > >    and stage 2 for memory protection
> > > 3. We are developing our own IP QEMU models now
> > > 4. These models will be connected with SMMUv3 in QEMU
> > > 5. We will use the QEMU board to development IP driver and ensure
> the
> > driver
> > >    can work well with Linux SMMU and IOMMU framework
> > I see and I understand your use case for system modeling purpose.
> >
> > This raises few questions/comments though.
> > - supporting platform device protection from the vIOMMU/ARM makes
> sense
> > to me globally. But above use case does not justify (to me) the
> > introduction of PL330 in machvirt because it would be just for
> testing
> > purpose. Peter may validate/invalidate though. Instead I think you
> > should try to illustrate this feature with DMA capable platform
> devices
> > such virtio-net and virtio-block sysbus devices as a counterpart of
> > their PCIe flavour.
> 
> Thanks for your suggestion. I will try virtio-net and virtio-block
> sysbus
> devices in next step.
> But I hope to keep PL330 because it's not just for testing purpose.
> It's a good example to show how to connect platform devices with SMMUv3
> based
> on this patch.
> I assume other developer may have same requirement.
> 

I tried virtio-net-device but looks like it doesn't match the 
application scenario of my patch series: 
1. virtio-net-device is a unique parairtulization QEMU device 
   which connected with virtio mmio bus like this:
   	(qemu) info qom-tree
		/machine (virt-5.2-machine)
			/peripheral-anon (container)
				/device[2] (virtio-net-device)
	(qemu) qom-get /machine/peripheral-anon/device[2] parent_bus
		"/machine/unattached/device[43]/virtio-mmio-bus.30"
2. There is no virtio mmio bus or virtio-net-device IP in real HW SoC
3. ARM SMMUv3 IP support platform devices IP like PL330 but current SMMUv3
   model cannot do this.
4. My patch series target to solve the #3 issue.
5. I agree we should think about how to update the current SMMUv3 model
   to support virtio-net-device but I do not think mix these 2 different
   use cases together is a good idea.

> > >
> > >> 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).
> > > I get your point but I cannot give you a clear answer now. I didn't
> > consider
> > > virtio-iommu before because our current use case doesn't need
> virtio-
> > iommu.
> >
> > I think you need to have consistency in the machvirt topology: with
> > current series the PL330 would be protected with vSMMUv3 and not with
> > virtio-iommu which does not seem acceptable to me. Either we need to
> > devise a way to individually specify which sysbus device is protected
> > and potentially also specify its streamid or all/none of them are.
> >
> > Thanks
> >
> > Eric
> 
> Thanks for your suggestion. I created a v6 patch fixed your other
> feedback.
> Let us continue the virtio-iommu discussion and I will try it in next
> step.
> 

I think virtio-iommu is same as virtio-net-device. My patch target to solve
the SMMUv3 connected with platform IPs issue. In the real HW SoC, all the platform
device IP should be protected by SMMUv3, there is no virtio-iommu IP at all.

I assume virtio-iommu and virtio-net-device should be focus on QEMU/KVM virtualization
but my target is focus on real HW virtualization.

> > >
> > >> 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,