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

[Eric Auger <eric.auger@redhat.com>]

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