Just before the device gets attached to the SMMUv3, make sure QEMU SMMUv3
features are compatible with the host SMMUv3.

Not all fields in the host SMMUv3 IDR registers are meaningful for userspace.
Only the following fields can be used:

  - IDR0: ST_LEVEL, TERM_MODEL, STALL_MODEL, TTENDIAN, CD2L, ASID16, TTF
  - IDR1: SIDSIZE, SSIDSIZE
  - IDR3: BBML, RIL
  - IDR5: VAX, GRAN64K, GRAN16K, GRAN4K

For now, the check is to make sure the features are in sync to enable
basic accelerated SMMUv3 support.

One other related change is, move the smmuv3_init_regs() to smmu_realize()
so that we do have that early enough for the check mentioned above.

Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 hw/arm/smmuv3-accel.c | 98 +++++++++++++++++++++++++++++++++++++++++++
 hw/arm/smmuv3.c       |  4 +-
 2 files changed, 100 insertions(+), 2 deletions(-)

diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
index 9ad8595ce2..defeddbd8c 100644
--- a/hw/arm/smmuv3-accel.c
+++ b/hw/arm/smmuv3-accel.c
@@ -39,6 +39,96 @@
 #define STE1_MASK     (STE1_ETS | STE1_S1STALLD | STE1_S1CSH | STE1_S1COR | \
                        STE1_S1CIR | STE1_S1DSS)
 
+static bool
+smmuv3_accel_check_hw_compatible(SMMUv3State *s,
+                                 struct iommu_hw_info_arm_smmuv3 *info,
+                                 Error **errp)
+{
+    uint32_t val;
+
+    /*
+     * QEMU SMMUv3 supports both linear and 2-level stream tables.
+     */
+    val = FIELD_EX32(info->idr[0], IDR0, STLEVEL);
+    if (val != FIELD_EX32(s->idr[0], IDR0, STLEVEL)) {
+        s->idr[0] = FIELD_DP32(s->idr[0], IDR0, STLEVEL, val);
+        error_setg(errp, "Host SUMMUv3 differs in Stream Table format");
+        return false;
+    }
+
+    /* QEMU SMMUv3 supports only little-endian translation table walks */
+    val = FIELD_EX32(info->idr[0], IDR0, TTENDIAN);
+    if (!val && val > FIELD_EX32(s->idr[0], IDR0, TTENDIAN)) {
+        error_setg(errp, "Host SUMMUv3 doesn't support Little-endian "
+                   "translation table");
+        return false;
+    }
+
+    /* QEMU SMMUv3 supports only AArch64 translation table format */
+    val = FIELD_EX32(info->idr[0], IDR0, TTF);
+    if (val < FIELD_EX32(s->idr[0], IDR0, TTF)) {
+        error_setg(errp, "Host SUMMUv3 deosn't support Arch64 Translation "
+                   "table format");
+        return false;
+    }
+
+    /* QEMU SMMUv3 supports SIDSIZE 16 */
+    val = FIELD_EX32(info->idr[1], IDR1, SIDSIZE);
+    if (val < FIELD_EX32(s->idr[1], IDR1, SIDSIZE)) {
+        error_setg(errp, "Host SUMMUv3 SIDSIZE not compatible");
+        return false;
+    }
+
+    /* QEMU SMMUv3 supports Range Invalidation by default */
+    val = FIELD_EX32(info->idr[3], IDR3, RIL);
+    if (val != FIELD_EX32(s->idr[3], IDR3, RIL)) {
+        error_setg(errp, "Host SUMMUv3 deosn't support Range Invalidation");
+        return false;
+    }
+
+    val = FIELD_EX32(info->idr[5], IDR5, GRAN4K);
+    if (val != FIELD_EX32(s->idr[5], IDR5, GRAN4K)) {
+        error_setg(errp, "Host SMMUv3 doesn't support 64K translation granule");
+        return false;
+    }
+    val = FIELD_EX32(info->idr[5], IDR5, GRAN16K);
+    if (val != FIELD_EX32(s->idr[5], IDR5, GRAN16K)) {
+        error_setg(errp, "Host SMMUv3 doesn't support 16K translation granule");
+        return false;
+    }
+    val = FIELD_EX32(info->idr[5], IDR5, GRAN64K);
+    if (val != FIELD_EX32(s->idr[5], IDR5, GRAN64K)) {
+        error_setg(errp, "Host SMMUv3 doesn't support 16K translation granule");
+        return false;
+    }
+    return true;
+}
+
+static bool
+smmuv3_accel_hw_compatible(SMMUv3State *s, HostIOMMUDeviceIOMMUFD *idev,
+                           Error **errp)
+{
+    struct iommu_hw_info_arm_smmuv3 info;
+    uint32_t data_type;
+    uint64_t caps;
+
+    if (!iommufd_backend_get_device_info(idev->iommufd, idev->devid, &data_type,
+                                         &info, sizeof(info), &caps, errp)) {
+        return false;
+    }
+
+    if (data_type != IOMMU_HW_INFO_TYPE_ARM_SMMUV3) {
+        error_setg(errp, "Wrong data type (%d) for Host SMMUv3 device info",
+                     data_type);
+        return false;
+    }
+
+    if (!smmuv3_accel_check_hw_compatible(s, &info, errp)) {
+        return false;
+    }
+    return true;
+}
+
 static bool
 smmuv3_accel_alloc_vdev(SMMUv3AccelDevice *accel_dev, int sid, Error **errp)
 {
@@ -363,6 +453,14 @@ static bool smmuv3_accel_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
         return true;
     }
 
+    /*
+     * Check the host SMMUv3 associated with the dev is compatible with the
+     * QEMU SMMUv3 accel.
+     */
+    if (!smmuv3_accel_hw_compatible(s, idev, errp)) {
+        return false;
+    }
+
     if (!smmuv3_accel_dev_alloc_viommu(accel_dev, idev, errp)) {
         error_setg(errp, "Device 0x%x: Unable to alloc viommu", sid);
         return false;
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 3963bdc87f..5830cf5a03 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1913,8 +1913,6 @@ static void smmu_reset_exit(Object *obj, ResetType type)
     if (c->parent_phases.exit) {
         c->parent_phases.exit(obj, type);
     }
-
-    smmuv3_init_regs(s);
 }
 
 static void smmu_realize(DeviceState *d, Error **errp)
@@ -1945,6 +1943,8 @@ static void smmu_realize(DeviceState *d, Error **errp)
     sysbus_init_mmio(dev, &sys->iomem);
 
     smmu_init_irq(s, dev);
+
+    smmuv3_init_regs(s);
 }
 
 static const VMStateDescription vmstate_smmuv3_queue = {
-- 
2.43.0

On 9/29/25 3:36 PM, Shameer Kolothum wrote:
> Just before the device gets attached to the SMMUv3, make sure QEMU SMMUv3
> features are compatible with the host SMMUv3.
>
> Not all fields in the host SMMUv3 IDR registers are meaningful for userspace.
> Only the following fields can be used:
>
>   - IDR0: ST_LEVEL, TERM_MODEL, STALL_MODEL, TTENDIAN, CD2L, ASID16, TTF
>   - IDR1: SIDSIZE, SSIDSIZE
>   - IDR3: BBML, RIL
>   - IDR5: VAX, GRAN64K, GRAN16K, GRAN4K
>
> For now, the check is to make sure the features are in sync to enable
> basic accelerated SMMUv3 support.
>
> One other related change is, move the smmuv3_init_regs() to smmu_realize()
> so that we do have that early enough for the check mentioned above.
>
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
> ---
>  hw/arm/smmuv3-accel.c | 98 +++++++++++++++++++++++++++++++++++++++++++
>  hw/arm/smmuv3.c       |  4 +-
>  2 files changed, 100 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> index 9ad8595ce2..defeddbd8c 100644
> --- a/hw/arm/smmuv3-accel.c
> +++ b/hw/arm/smmuv3-accel.c
> @@ -39,6 +39,96 @@
>  #define STE1_MASK     (STE1_ETS | STE1_S1STALLD | STE1_S1CSH | STE1_S1COR | \
>                         STE1_S1CIR | STE1_S1DSS)
>  
> +static bool
> +smmuv3_accel_check_hw_compatible(SMMUv3State *s,
> +                                 struct iommu_hw_info_arm_smmuv3 *info,
> +                                 Error **errp)
> +{
> +    uint32_t val;
> +
> +    /*
> +     * QEMU SMMUv3 supports both linear and 2-level stream tables.
> +     */
> +    val = FIELD_EX32(info->idr[0], IDR0, STLEVEL);
> +    if (val != FIELD_EX32(s->idr[0], IDR0, STLEVEL)) {
> +        s->idr[0] = FIELD_DP32(s->idr[0], IDR0, STLEVEL, val);
> +        error_setg(errp, "Host SUMMUv3 differs in Stream Table format");
> +        return false;
> +    }
> +
> +    /* QEMU SMMUv3 supports only little-endian translation table walks */
> +    val = FIELD_EX32(info->idr[0], IDR0, TTENDIAN);
> +    if (!val && val > FIELD_EX32(s->idr[0], IDR0, TTENDIAN)) {
> +        error_setg(errp, "Host SUMMUv3 doesn't support Little-endian "
> +                   "translation table");
> +        return false;
> +    }
> +
> +    /* QEMU SMMUv3 supports only AArch64 translation table format */
> +    val = FIELD_EX32(info->idr[0], IDR0, TTF);
> +    if (val < FIELD_EX32(s->idr[0], IDR0, TTF)) {
> +        error_setg(errp, "Host SUMMUv3 deosn't support Arch64 Translation "
> +                   "table format");
> +        return false;
> +    }
> +
> +    /* QEMU SMMUv3 supports SIDSIZE 16 */
> +    val = FIELD_EX32(info->idr[1], IDR1, SIDSIZE);
> +    if (val < FIELD_EX32(s->idr[1], IDR1, SIDSIZE)) {
> +        error_setg(errp, "Host SUMMUv3 SIDSIZE not compatible");
> +        return false;
> +    }
> +
> +    /* QEMU SMMUv3 supports Range Invalidation by default */
> +    val = FIELD_EX32(info->idr[3], IDR3, RIL);
> +    if (val != FIELD_EX32(s->idr[3], IDR3, RIL)) {
> +        error_setg(errp, "Host SUMMUv3 deosn't support Range Invalidation");
> +        return false;
> +    }
> +
> +    val = FIELD_EX32(info->idr[5], IDR5, GRAN4K);
> +    if (val != FIELD_EX32(s->idr[5], IDR5, GRAN4K)) {
> +        error_setg(errp, "Host SMMUv3 doesn't support 64K translation granule");
> +        return false;
> +    }
> +    val = FIELD_EX32(info->idr[5], IDR5, GRAN16K);
> +    if (val != FIELD_EX32(s->idr[5], IDR5, GRAN16K)) {
> +        error_setg(errp, "Host SMMUv3 doesn't support 16K translation granule");
> +        return false;
> +    }
> +    val = FIELD_EX32(info->idr[5], IDR5, GRAN64K);
> +    if (val != FIELD_EX32(s->idr[5], IDR5, GRAN64K)) {
> +        error_setg(errp, "Host SMMUv3 doesn't support 16K translation granule");
> +        return false;
> +    }
> +    return true;
> +}
> +
> +static bool
> +smmuv3_accel_hw_compatible(SMMUv3State *s, HostIOMMUDeviceIOMMUFD *idev,
> +                           Error **errp)
> +{
> +    struct iommu_hw_info_arm_smmuv3 info;
> +    uint32_t data_type;
> +    uint64_t caps;
> +
> +    if (!iommufd_backend_get_device_info(idev->iommufd, idev->devid, &data_type,
> +                                         &info, sizeof(info), &caps, errp)) {
> +        return false;
> +    }
> +
> +    if (data_type != IOMMU_HW_INFO_TYPE_ARM_SMMUV3) {
> +        error_setg(errp, "Wrong data type (%d) for Host SMMUv3 device info",
> +                     data_type);
> +        return false;
> +    }
> +
> +    if (!smmuv3_accel_check_hw_compatible(s, &info, errp)) {
> +        return false;
> +    }
> +    return true;
> +}
> +
>  static bool
>  smmuv3_accel_alloc_vdev(SMMUv3AccelDevice *accel_dev, int sid, Error **errp)
>  {
> @@ -363,6 +453,14 @@ static bool smmuv3_accel_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>          return true;
>      }
>  
> +    /*
> +     * Check the host SMMUv3 associated with the dev is compatible with the
> +     * QEMU SMMUv3 accel.
> +     */
> +    if (!smmuv3_accel_hw_compatible(s, idev, errp)) {
> +        return false;
> +    }
> +
>      if (!smmuv3_accel_dev_alloc_viommu(accel_dev, idev, errp)) {
>          error_setg(errp, "Device 0x%x: Unable to alloc viommu", sid);
>          return false;
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 3963bdc87f..5830cf5a03 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1913,8 +1913,6 @@ static void smmu_reset_exit(Object *obj, ResetType type)
>      if (c->parent_phases.exit) {
>          c->parent_phases.exit(obj, type);
>      }
> -
> -    smmuv3_init_regs(s);
does that work on reset()? Besides setting idr regs, smmuv3_init_regs() also
resets the cmdq and eventq plus a bunch of dynamic registers. That needs
to happen on reset I think.

Eric
>  }
>  
>  static void smmu_realize(DeviceState *d, Error **errp)
> @@ -1945,6 +1943,8 @@ static void smmu_realize(DeviceState *d, Error **errp)
>      sysbus_init_mmio(dev, &sys->iomem);
>  
>      smmu_init_irq(s, dev);
> +
> +    smmuv3_init_regs(s);
>  }
>  
>  static const VMStateDescription vmstate_smmuv3_queue = {

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: 27 October 2025 10:47
> To: Shameer Kolothum <skolothumtho@nvidia.com>; qemu-
> arm@nongnu.org; qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; Jason Gunthorpe <jgg@nvidia.com>; Nicolin
> Chen <nicolinc@nvidia.com>; ddutile@redhat.com; berrange@redhat.com;
> Nathan Chen <nathanc@nvidia.com>; Matt Ochs <mochs@nvidia.com>;
> smostafa@google.com; wangzhou1@hisilicon.com;
> jiangkunkun@huawei.com; jonathan.cameron@huawei.com;
> zhangfei.gao@linaro.org; zhenzhong.duan@intel.com; yi.l.liu@intel.com;
> shameerkolothum@gmail.com
> Subject: Re: [PATCH v4 14/27] hw/arm/smmuv3-accel: Get host SMMUv3 hw
> info and validate
 
> ResetType type)
> >      if (c->parent_phases.exit) {
> >          c->parent_phases.exit(obj, type);
> >      }
> > -
> > -    smmuv3_init_regs(s);
> does that work on reset()? Besides setting idr regs, smmuv3_init_regs() also
> resets the cmdq and eventq plus a bunch of dynamic registers. That needs
> to happen on reset I think.

Yes. I noted that and is taken care for next.

Thanks,
Shameer

On Mon, 29 Sep 2025 14:36:30 +0100
Shameer Kolothum <skolothumtho@nvidia.com> wrote:

> Just before the device gets attached to the SMMUv3, make sure QEMU SMMUv3
> features are compatible with the host SMMUv3.
> 
> Not all fields in the host SMMUv3 IDR registers are meaningful for userspace.
> Only the following fields can be used:
> 
>   - IDR0: ST_LEVEL, TERM_MODEL, STALL_MODEL, TTENDIAN, CD2L, ASID16, TTF
>   - IDR1: SIDSIZE, SSIDSIZE
>   - IDR3: BBML, RIL
>   - IDR5: VAX, GRAN64K, GRAN16K, GRAN4K
> 
> For now, the check is to make sure the features are in sync to enable
> basic accelerated SMMUv3 support.
> 
> One other related change is, move the smmuv3_init_regs() to smmu_realize()
> so that we do have that early enough for the check mentioned above.
> 
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>

Hi Shameer,

Back to this series...

Various things in the checks in here.

Jonathan

> ---
>  hw/arm/smmuv3-accel.c | 98 +++++++++++++++++++++++++++++++++++++++++++
>  hw/arm/smmuv3.c       |  4 +-
>  2 files changed, 100 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> index 9ad8595ce2..defeddbd8c 100644
> --- a/hw/arm/smmuv3-accel.c
> +++ b/hw/arm/smmuv3-accel.c
> @@ -39,6 +39,96 @@
>  #define STE1_MASK     (STE1_ETS | STE1_S1STALLD | STE1_S1CSH | STE1_S1COR | \
>                         STE1_S1CIR | STE1_S1DSS)
>  
> +static bool
> +smmuv3_accel_check_hw_compatible(SMMUv3State *s,
> +                                 struct iommu_hw_info_arm_smmuv3 *info,
> +                                 Error **errp)
> +{
> +    uint32_t val;
> +
> +    /*
> +     * QEMU SMMUv3 supports both linear and 2-level stream tables.
> +     */

Single line comment would be more consistent with below and looks to be under 80 chars.

> +    val = FIELD_EX32(info->idr[0], IDR0, STLEVEL);
> +    if (val != FIELD_EX32(s->idr[0], IDR0, STLEVEL)) {
> +        s->idr[0] = FIELD_DP32(s->idr[0], IDR0, STLEVEL, val);

This seems a rather odd side effect to have.  Perhaps a comment on why
in error path it make sense to change s->idr[0]?

> +        error_setg(errp, "Host SUMMUv3 differs in Stream Table format");
> +        return false;
> +    }
> +
> +    /* QEMU SMMUv3 supports only little-endian translation table walks */
> +    val = FIELD_EX32(info->idr[0], IDR0, TTENDIAN);
> +    if (!val && val > FIELD_EX32(s->idr[0], IDR0, TTENDIAN)) {

This is a weird check.  || maybe?

Otherwise if !val is true, then val is not likely to be greater than anything.

> +        error_setg(errp, "Host SUMMUv3 doesn't support Little-endian "
> +                   "translation table");
> +        return false;
> +    }
> +
> +    /* QEMU SMMUv3 supports only AArch64 translation table format */
> +    val = FIELD_EX32(info->idr[0], IDR0, TTF);
> +    if (val < FIELD_EX32(s->idr[0], IDR0, TTF)) {
> +        error_setg(errp, "Host SUMMUv3 deosn't support Arch64 Translation "

Spell check the messages. doesn't.

> +                   "table format");
> +        return false;
> +    }
> +
> +    /* QEMU SMMUv3 supports SIDSIZE 16 */
> +    val = FIELD_EX32(info->idr[1], IDR1, SIDSIZE);
> +    if (val < FIELD_EX32(s->idr[1], IDR1, SIDSIZE)) {

Why not
    if (FIELD_EX32(info->idr[1], IDR1, SIDSIZE) <
	FIELD_EX32(s->idr[1], IDR1, SIDSIZE))
I.e. why does the local variable make sense in cases where the value is
only used once.  To me if anything this is slightly easier to read.   You could
even align the variables so it's obvious it's comparing one field.

    if (FIELD_EX32(info->idr[1], IDR1, SIDSIZE) <
	FIELD_EX32(s->idr[1],    IDR1, SIDSIZE))
  
> +        error_setg(errp, "Host SUMMUv3 SIDSIZE not compatible");
> +        return false;
> +    }
> +
> +    /* QEMU SMMUv3 supports Range Invalidation by default */
> +    val = FIELD_EX32(info->idr[3], IDR3, RIL);
> +    if (val != FIELD_EX32(s->idr[3], IDR3, RIL)) {
> +        error_setg(errp, "Host SUMMUv3 deosn't support Range Invalidation");

doesn't.

> +        return false;
> +    }
> +
> +    val = FIELD_EX32(info->idr[5], IDR5, GRAN4K);
> +    if (val != FIELD_EX32(s->idr[5], IDR5, GRAN4K)) {
> +        error_setg(errp, "Host SMMUv3 doesn't support 64K translation granule");
That doesn't smell like it's checking 64K
> +        return false;
> +    }
> +    val = FIELD_EX32(info->idr[5], IDR5, GRAN16K);
> +    if (val != FIELD_EX32(s->idr[5], IDR5, GRAN16K)) {
> +        error_setg(errp, "Host SMMUv3 doesn't support 16K translation granule");
> +        return false;
> +    }
> +    val = FIELD_EX32(info->idr[5], IDR5, GRAN64K);
> +    if (val != FIELD_EX32(s->idr[5], IDR5, GRAN64K)) {
> +        error_setg(errp, "Host SMMUv3 doesn't support 16K translation granule");
Nor is this one seem likely to be checking 16K. 
> +        return false;
> +    }
> +    return true;
> +}
> +
> +static bool
> +smmuv3_accel_hw_compatible(SMMUv3State *s, HostIOMMUDeviceIOMMUFD *idev,
> +                           Error **errp)
> +{
> +    struct iommu_hw_info_arm_smmuv3 info;
> +    uint32_t data_type;
> +    uint64_t caps;
> +
> +    if (!iommufd_backend_get_device_info(idev->iommufd, idev->devid, &data_type,
> +                                         &info, sizeof(info), &caps, errp)) {
> +        return false;
> +    }
> +
> +    if (data_type != IOMMU_HW_INFO_TYPE_ARM_SMMUV3) {
> +        error_setg(errp, "Wrong data type (%d) for Host SMMUv3 device info",
> +                     data_type);

Alignment looks off.

> +        return false;
> +    }
> +
> +    if (!smmuv3_accel_check_hw_compatible(s, &info, errp)) {
> +        return false;
> +    }
> +    return true;
> +}
> +

Hi Shameer,

On 10/1/25 2:56 PM, Jonathan Cameron wrote:
> On Mon, 29 Sep 2025 14:36:30 +0100
> Shameer Kolothum <skolothumtho@nvidia.com> wrote:
>
>> Just before the device gets attached to the SMMUv3, make sure QEMU SMMUv3
>> features are compatible with the host SMMUv3.
>>
>> Not all fields in the host SMMUv3 IDR registers are meaningful for userspace.
>> Only the following fields can be used:
>>
>>   - IDR0: ST_LEVEL, TERM_MODEL, STALL_MODEL, TTENDIAN, CD2L, ASID16, TTF
>>   - IDR1: SIDSIZE, SSIDSIZE
>>   - IDR3: BBML, RIL
>>   - IDR5: VAX, GRAN64K, GRAN16K, GRAN4K
>>
>> For now, the check is to make sure the features are in sync to enable
>> basic accelerated SMMUv3 support.
>>
>> One other related change is, move the smmuv3_init_regs() to smmu_realize()
>> so that we do have that early enough for the check mentioned above.
>>
>> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
> Hi Shameer,
>
> Back to this series...
>
> Various things in the checks in here.
>
> Jonathan
>
>> ---
>>  hw/arm/smmuv3-accel.c | 98 +++++++++++++++++++++++++++++++++++++++++++
>>  hw/arm/smmuv3.c       |  4 +-
>>  2 files changed, 100 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
>> index 9ad8595ce2..defeddbd8c 100644
>> --- a/hw/arm/smmuv3-accel.c
>> +++ b/hw/arm/smmuv3-accel.c
>> @@ -39,6 +39,96 @@
>>  #define STE1_MASK     (STE1_ETS | STE1_S1STALLD | STE1_S1CSH | STE1_S1COR | \
>>                         STE1_S1CIR | STE1_S1DSS)
>>  
>> +static bool
>> +smmuv3_accel_check_hw_compatible(SMMUv3State *s,
>> +                                 struct iommu_hw_info_arm_smmuv3 *info,
>> +                                 Error **errp)
>> +{
>> +    uint32_t val;
>> +
>> +    /*
>> +     * QEMU SMMUv3 supports both linear and 2-level stream tables.
>> +     */
> Single line comment would be more consistent with below and looks to be under 80 chars.
>
>> +    val = FIELD_EX32(info->idr[0], IDR0, STLEVEL);
>> +    if (val != FIELD_EX32(s->idr[0], IDR0, STLEVEL)) {
>> +        s->idr[0] = FIELD_DP32(s->idr[0], IDR0, STLEVEL, val);
> This seems a rather odd side effect to have.  Perhaps a comment on why
> in error path it make sense to change s->idr[0]?
also consider s->idr[] are not migrated. I know there is a migration
blocker set in 20/27 though. I would simply return an error here.
>
>> +        error_setg(errp, "Host SUMMUv3 differs in Stream Table format");
>> +        return false;
>> +    }
>> +
>> +    /* QEMU SMMUv3 supports only little-endian translation table walks */
>> +    val = FIELD_EX32(info->idr[0], IDR0, TTENDIAN);
>> +    if (!val && val > FIELD_EX32(s->idr[0], IDR0, TTENDIAN)) {
> This is a weird check.  || maybe?
>
> Otherwise if !val is true, then val is not likely to be greater than anything.
> imply check info idr0 ttendian == s->idr[0] one (=2)?
>
>> +        error_setg(errp, "Host SUMMUv3 doesn't support Little-endian "
>> +                   "translation table");
>> +        return false;
>> +    }
>> +
>> +    /* QEMU SMMUv3 supports only AArch64 translation table format */
>> +    val = FIELD_EX32(info->idr[0], IDR0, TTF);
>> +    if (val < FIELD_EX32(s->idr[0], IDR0, TTF)) {
>> +        error_setg(errp, "Host SUMMUv3 deosn't support Arch64 Translation "
> Spell check the messages. doesn't.
>
>> +                   "table format");
>> +        return false;
>> +    }
>> +
>> +    /* QEMU SMMUv3 supports SIDSIZE 16 */
>> +    val = FIELD_EX32(info->idr[1], IDR1, SIDSIZE);
>> +    if (val < FIELD_EX32(s->idr[1], IDR1, SIDSIZE)) {
> Why not
>     if (FIELD_EX32(info->idr[1], IDR1, SIDSIZE) <
> 	FIELD_EX32(s->idr[1], IDR1, SIDSIZE))
> I.e. why does the local variable make sense in cases where the value is
> only used once.  To me if anything this is slightly easier to read.   You could
> even align the variables so it's obvious it's comparing one field.
>
>     if (FIELD_EX32(info->idr[1], IDR1, SIDSIZE) <
> 	FIELD_EX32(s->idr[1],    IDR1, SIDSIZE))
>   
>> +        error_setg(errp, "Host SUMMUv3 SIDSIZE not compatible");
>> +        return false;
>> +    }
>> +
>> +    /* QEMU SMMUv3 supports Range Invalidation by default */
>> +    val = FIELD_EX32(info->idr[3], IDR3, RIL);
>> +    if (val != FIELD_EX32(s->idr[3], IDR3, RIL)) {
>> +        error_setg(errp, "Host SUMMUv3 deosn't support Range Invalidation");
> doesn't.
as Jonathan suggested you might avoid using the local var here too and below
>
>> +        return false;
>> +    }
>> +
>> +    val = FIELD_EX32(info->idr[5], IDR5, GRAN4K);
>> +    if (val != FIELD_EX32(s->idr[5], IDR5, GRAN4K)) {
>> +        error_setg(errp, "Host SMMUv3 doesn't support 64K translation granule");
> That doesn't smell like it's checking 64K
>> +        return false;
>> +    }
>> +    val = FIELD_EX32(info->idr[5], IDR5, GRAN16K);
>> +    if (val != FIELD_EX32(s->idr[5], IDR5, GRAN16K)) {
>> +        error_setg(errp, "Host SMMUv3 doesn't support 16K translation granule");
>> +        return false;
>> +    }
>> +    val = FIELD_EX32(info->idr[5], IDR5, GRAN64K);
>> +    if (val != FIELD_EX32(s->idr[5], IDR5, GRAN64K)) {
>> +        error_setg(errp, "Host SMMUv3 doesn't support 16K translation granule");
> Nor is this one seem likely to be checking 16K. 
>> +        return false;
>> +    }
>> +    return true;
>> +}
>> +
>> +static bool
>> +smmuv3_accel_hw_compatible(SMMUv3State *s, HostIOMMUDeviceIOMMUFD *idev,
>> +                           Error **errp)
>> +{
>> +    struct iommu_hw_info_arm_smmuv3 info;
>> +    uint32_t data_type;
>> +    uint64_t caps;
>> +
>> +    if (!iommufd_backend_get_device_info(idev->iommufd, idev->devid, &data_type,
>> +                                         &info, sizeof(info), &caps, errp)) {
>> +        return false;
>> +    }
>> +
>> +    if (data_type != IOMMU_HW_INFO_TYPE_ARM_SMMUV3) {
>> +        error_setg(errp, "Wrong data type (%d) for Host SMMUv3 device info",
>> +                     data_type);
> Alignment looks off.
>
>> +        return false;
>> +    }
>> +
>> +    if (!smmuv3_accel_check_hw_compatible(s, &info, errp)) {
>> +        return false;
>> +    }
>> +    return true;
return  smmuv3_accel_check_hw_compatible(s, &info, errp)
>> +}
>> +
>
Eric

> -----Original Message-----
> From: Eric Auger <eric.auger@redhat.com>
> Sent: 27 October 2025 10:42
> To: Jonathan Cameron <jonathan.cameron@huawei.com>; Shameer
> Kolothum <skolothumtho@nvidia.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> peter.maydell@linaro.org; Jason Gunthorpe <jgg@nvidia.com>; Nicolin Chen
> <nicolinc@nvidia.com>; ddutile@redhat.com; berrange@redhat.com; Nathan
> Chen <nathanc@nvidia.com>; Matt Ochs <mochs@nvidia.com>;
> smostafa@google.com; wangzhou1@hisilicon.com;
> jiangkunkun@huawei.com; zhangfei.gao@linaro.org;
> zhenzhong.duan@intel.com; yi.l.liu@intel.com;
> shameerkolothum@gmail.com
> Subject: Re: [PATCH v4 14/27] hw/arm/smmuv3-accel: Get host SMMUv3 hw
> info and validate
> 

[...]

> >> +    val = FIELD_EX32(info->idr[0], IDR0, STLEVEL);
> >> +    if (val != FIELD_EX32(s->idr[0], IDR0, STLEVEL)) {
> >> +        s->idr[0] = FIELD_DP32(s->idr[0], IDR0, STLEVEL, val);
> > This seems a rather odd side effect to have.  Perhaps a comment on why
> > in error path it make sense to change s->idr[0]?
> also consider s->idr[] are not migrated. I know there is a migration blocker set
> in 20/27 though. I would simply return an error here.

That idr[0] update is a mistake from rework/rebase and is not required.

[...]

> >> +    /* QEMU SMMUv3 supports Range Invalidation by default */
> >> +    val = FIELD_EX32(info->idr[3], IDR3, RIL);
> >> +    if (val != FIELD_EX32(s->idr[3], IDR3, RIL)) {
> >> +        error_setg(errp, "Host SUMMUv3 deosn't support Range
> >> + Invalidation");
> > doesn't.
> as Jonathan suggested you might avoid using the local var here too and below

Done.

Thanks,
Shameer

> -----Original Message-----
> From: Jonathan Cameron <jonathan.cameron@huawei.com>
> Sent: 01 October 2025 13:56
> To: Shameer Kolothum <skolothumtho@nvidia.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; Jason Gunthorpe
> <jgg@nvidia.com>; Nicolin Chen <nicolinc@nvidia.com>; ddutile@redhat.com;
> berrange@redhat.com; Nathan Chen <nathanc@nvidia.com>; Matt Ochs
> <mochs@nvidia.com>; smostafa@google.com; wangzhou1@hisilicon.com;
> jiangkunkun@huawei.com; zhangfei.gao@linaro.org;
> zhenzhong.duan@intel.com; yi.l.liu@intel.com;
> shameerkolothum@gmail.com
> Subject: Re: [PATCH v4 14/27] hw/arm/smmuv3-accel: Get host SMMUv3 hw
> info and validate
> 
> External email: Use caution opening links or attachments
> 
> 
> On Mon, 29 Sep 2025 14:36:30 +0100
> Shameer Kolothum <skolothumtho@nvidia.com> wrote:
> 
> > Just before the device gets attached to the SMMUv3, make sure QEMU
> SMMUv3
> > features are compatible with the host SMMUv3.
> >
> > Not all fields in the host SMMUv3 IDR registers are meaningful for userspace.
> > Only the following fields can be used:
> >
> >   - IDR0: ST_LEVEL, TERM_MODEL, STALL_MODEL, TTENDIAN, CD2L, ASID16,
> TTF
> >   - IDR1: SIDSIZE, SSIDSIZE
> >   - IDR3: BBML, RIL
> >   - IDR5: VAX, GRAN64K, GRAN16K, GRAN4K
> >
> > For now, the check is to make sure the features are in sync to enable
> > basic accelerated SMMUv3 support.
> >
> > One other related change is, move the smmuv3_init_regs() to
> smmu_realize()
> > so that we do have that early enough for the check mentioned above.
> >
> > Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
> 
> Hi Shameer,
> 
> Back to this series...
> 
> Various things in the checks in here.

Thanks for going through the entire series. I will address the comments
in the next revision.

Between, you seem to have missed patch #20 though. 

Thanks,
Shameer
 


> 
> > ---
> >  hw/arm/smmuv3-accel.c | 98
> +++++++++++++++++++++++++++++++++++++++++++
> >  hw/arm/smmuv3.c       |  4 +-
> >  2 files changed, 100 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> > index 9ad8595ce2..defeddbd8c 100644
> > --- a/hw/arm/smmuv3-accel.c
> > +++ b/hw/arm/smmuv3-accel.c
> > @@ -39,6 +39,96 @@
> >  #define STE1_MASK     (STE1_ETS | STE1_S1STALLD | STE1_S1CSH |
> STE1_S1COR | \
> >                         STE1_S1CIR | STE1_S1DSS)
> >
> > +static bool
> > +smmuv3_accel_check_hw_compatible(SMMUv3State *s,
> > +                                 struct iommu_hw_info_arm_smmuv3 *info,
> > +                                 Error **errp)
> > +{
> > +    uint32_t val;
> > +
> > +    /*
> > +     * QEMU SMMUv3 supports both linear and 2-level stream tables.
> > +     */
> 
> Single line comment would be more consistent with below and looks to be
> under 80 chars.
> 
> > +    val = FIELD_EX32(info->idr[0], IDR0, STLEVEL);
> > +    if (val != FIELD_EX32(s->idr[0], IDR0, STLEVEL)) {
> > +        s->idr[0] = FIELD_DP32(s->idr[0], IDR0, STLEVEL, val);
> 
> This seems a rather odd side effect to have.  Perhaps a comment on why
> in error path it make sense to change s->idr[0]?
> 
> > +        error_setg(errp, "Host SUMMUv3 differs in Stream Table format");
> > +        return false;
> > +    }
> > +
> > +    /* QEMU SMMUv3 supports only little-endian translation table walks */
> > +    val = FIELD_EX32(info->idr[0], IDR0, TTENDIAN);
> > +    if (!val && val > FIELD_EX32(s->idr[0], IDR0, TTENDIAN)) {
> 
> This is a weird check.  || maybe?
> 
> Otherwise if !val is true, then val is not likely to be greater than anything.
> 
> > +        error_setg(errp, "Host SUMMUv3 doesn't support Little-endian "
> > +                   "translation table");
> > +        return false;
> > +    }
> > +
> > +    /* QEMU SMMUv3 supports only AArch64 translation table format */
> > +    val = FIELD_EX32(info->idr[0], IDR0, TTF);
> > +    if (val < FIELD_EX32(s->idr[0], IDR0, TTF)) {
> > +        error_setg(errp, "Host SUMMUv3 deosn't support Arch64 Translation "
> 
> Spell check the messages. doesn't.
> 
> > +                   "table format");
> > +        return false;
> > +    }
> > +
> > +    /* QEMU SMMUv3 supports SIDSIZE 16 */
> > +    val = FIELD_EX32(info->idr[1], IDR1, SIDSIZE);
> > +    if (val < FIELD_EX32(s->idr[1], IDR1, SIDSIZE)) {
> 
> Why not
>     if (FIELD_EX32(info->idr[1], IDR1, SIDSIZE) <
>         FIELD_EX32(s->idr[1], IDR1, SIDSIZE))
> I.e. why does the local variable make sense in cases where the value is
> only used once.  To me if anything this is slightly easier to read.   You could
> even align the variables so it's obvious it's comparing one field.
> 
>     if (FIELD_EX32(info->idr[1], IDR1, SIDSIZE) <
>         FIELD_EX32(s->idr[1],    IDR1, SIDSIZE))
> 
> > +        error_setg(errp, "Host SUMMUv3 SIDSIZE not compatible");
> > +        return false;
> > +    }
> > +
> > +    /* QEMU SMMUv3 supports Range Invalidation by default */
> > +    val = FIELD_EX32(info->idr[3], IDR3, RIL);
> > +    if (val != FIELD_EX32(s->idr[3], IDR3, RIL)) {
> > +        error_setg(errp, "Host SUMMUv3 deosn't support Range
> Invalidation");
> 
> doesn't.
> 
> > +        return false;
> > +    }
> > +
> > +    val = FIELD_EX32(info->idr[5], IDR5, GRAN4K);
> > +    if (val != FIELD_EX32(s->idr[5], IDR5, GRAN4K)) {
> > +        error_setg(errp, "Host SMMUv3 doesn't support 64K translation
> granule");
> That doesn't smell like it's checking 64K
> > +        return false;
> > +    }
> > +    val = FIELD_EX32(info->idr[5], IDR5, GRAN16K);
> > +    if (val != FIELD_EX32(s->idr[5], IDR5, GRAN16K)) {
> > +        error_setg(errp, "Host SMMUv3 doesn't support 16K translation
> granule");
> > +        return false;
> > +    }
> > +    val = FIELD_EX32(info->idr[5], IDR5, GRAN64K);
> > +    if (val != FIELD_EX32(s->idr[5], IDR5, GRAN64K)) {
> > +        error_setg(errp, "Host SMMUv3 doesn't support 16K translation
> granule");
> Nor is this one seem likely to be checking 16K.
> > +        return false;
> > +    }
> > +    return true;
> > +}
> > +
> > +static bool
> > +smmuv3_accel_hw_compatible(SMMUv3State *s,
> HostIOMMUDeviceIOMMUFD *idev,
> > +                           Error **errp)
> > +{
> > +    struct iommu_hw_info_arm_smmuv3 info;
> > +    uint32_t data_type;
> > +    uint64_t caps;
> > +
> > +    if (!iommufd_backend_get_device_info(idev->iommufd, idev->devid,
> &data_type,
> > +                                         &info, sizeof(info), &caps, errp)) {
> > +        return false;
> > +    }
> > +
> > +    if (data_type != IOMMU_HW_INFO_TYPE_ARM_SMMUV3) {
> > +        error_setg(errp, "Wrong data type (%d) for Host SMMUv3 device info",
> > +                     data_type);
> 
> Alignment looks off.
> 
> > +        return false;
> > +    }
> > +
> > +    if (!smmuv3_accel_check_hw_compatible(s, &info, errp)) {
> > +        return false;
> > +    }
> > +    return true;
> > +}
> > +
> 

On Thu, 2 Oct 2025 07:37:46 +0000
Shameer Kolothum <skolothumtho@nvidia.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Sent: 01 October 2025 13:56
> > To: Shameer Kolothum <skolothumtho@nvidia.com>
> > Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> > eric.auger@redhat.com; peter.maydell@linaro.org; Jason Gunthorpe
> > <jgg@nvidia.com>; Nicolin Chen <nicolinc@nvidia.com>; ddutile@redhat.com;
> > berrange@redhat.com; Nathan Chen <nathanc@nvidia.com>; Matt Ochs
> > <mochs@nvidia.com>; smostafa@google.com; wangzhou1@hisilicon.com;
> > jiangkunkun@huawei.com; zhangfei.gao@linaro.org;
> > zhenzhong.duan@intel.com; yi.l.liu@intel.com;
> > shameerkolothum@gmail.com
> > Subject: Re: [PATCH v4 14/27] hw/arm/smmuv3-accel: Get host SMMUv3 hw
> > info and validate
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Mon, 29 Sep 2025 14:36:30 +0100
> > Shameer Kolothum <skolothumtho@nvidia.com> wrote:
> >   
> > > Just before the device gets attached to the SMMUv3, make sure QEMU  
> > SMMUv3  
> > > features are compatible with the host SMMUv3.
> > >
> > > Not all fields in the host SMMUv3 IDR registers are meaningful for userspace.
> > > Only the following fields can be used:
> > >
> > >   - IDR0: ST_LEVEL, TERM_MODEL, STALL_MODEL, TTENDIAN, CD2L, ASID16,  
> > TTF  
> > >   - IDR1: SIDSIZE, SSIDSIZE
> > >   - IDR3: BBML, RIL
> > >   - IDR5: VAX, GRAN64K, GRAN16K, GRAN4K
> > >
> > > For now, the check is to make sure the features are in sync to enable
> > > basic accelerated SMMUv3 support.
> > >
> > > One other related change is, move the smmuv3_init_regs() to  
> > smmu_realize()  
> > > so that we do have that early enough for the check mentioned above.
> > >
> > > Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>  
> > 
> > Hi Shameer,
> > 
> > Back to this series...
> > 
> > Various things in the checks in here.  
> 
> Thanks for going through the entire series. I will address the comments
> in the next revision.
> 
> Between, you seem to have missed patch #20 though. 
I should, but don't, know enough about migration blockers to comment on that one!

> 
> Thanks,
> Shameer