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

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

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