Re: [PATCH v4 14/27] hw/arm/smmuv3-accel: Get host SMMUv3 hw info and validate

[Jonathan Cameron via <qemu-arm@nongnu.org>]

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;
> +}
> +