> -----Original Message-----
> From: Jonathan Cameron <[email protected]>
> Sent: 01 October 2025 13:56
> To: Shameer Kolothum <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Jason Gunthorpe
> <[email protected]>; Nicolin Chen <[email protected]>; [email protected];
> [email protected]; Nathan Chen <[email protected]>; Matt Ochs
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> 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 <[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.
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;
> > +}
> > +
>
