Hi Shameer,
On 10/1/25 2:56 PM, Jonathan Cameron wrote:
> On Mon, 29 Sep 2025 14:36:30 +0100
> Shameer Kolothum <[email protected]> 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 <[email protected]>
> 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
