RE: [RFC PATCH v3 14/15] Read and validate host SMMUv3 feature bits

[Shameerali Kolothum Thodi via <qemu-devel@nongnu.org>]

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Wednesday, July 16, 2025 3:58 AM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; nathanc@nvidia.com;
> mochs@nvidia.com; smostafa@google.com; Linuxarm
> <linuxarm@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>;
> jiangkunkun <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org;
> zhenzhong.duan@intel.com; shameerkolothum@gmail.com
> Subject: Re: [RFC PATCH v3 14/15] Read and validate host SMMUv3 feature
> bits
> 
> On Mon, Jul 14, 2025 at 04:59:40PM +0100, Shameer Kolothum wrote:
> > From: Nicolin Chen <nicolinc@nvidia.com>
> >
> > Not all fields in the SMMU 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
> 
> But half of these fields are not validated in the patch :-/

That’s why I said " Use the relevant fields from these to check.." .
But sorry, I was conservative ☹ and not sure the SSIDSIZE/STALL mattered
for non pasid cases.

> My vSMMU didn't work until I added entries like SIDSIZE, SSIDSIZE,
> TERM_MODEL, STALL_MODEL, and RIL.

How come your vSMMU not working? Or you meant the assigned
dev is not working?

The emulation supports SIDSIZE = 16 and RIL. Could you please
share the difference between these values w.r.t host SMMUv3.

> I think IDR5.OAS should be also added in the list. Maybe we should
> update the kernel uAPI meanwhile.

Ok.
 
> > +    val = FIELD_EX32(s_accel->info.idr[5], IDR5, GRAN4K);
> > +    if (val < FIELD_EX32(s->idr[5], IDR5, GRAN4K)) {
> > +        s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, val);
> > +    }
> > +    val = FIELD_EX32(s_accel->info.idr[5], IDR5, GRAN16K);
> > +    if (val < FIELD_EX32(s->idr[5], IDR5, GRAN16K)) {
> > +        s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, val);
> > +    }
> > +    val = FIELD_EX32(s_accel->info.idr[5], IDR5, GRAN64K);
> > +    if (val < FIELD_EX32(s->idr[5], IDR5, GRAN64K)) {
> > +        s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, val);
> 
> Unless there is some conflicts between the QEMU emulation and the
> SMMU HW, I think we should probably just override these fields to
> the HW values, instead of running comparisons. The justification
> could be that these fields are unlikely going to be controlled by
> the QEMU but supported directly by the real HW.
> 
> For example, if HW supports SSIDSIZE=5, there seems to be no good
> reason to limit it to SSIDSIZE=4? Even if the default SSIDSIZE in
> the smmuv3_init_regs() is 4.
> 
> > @@ -1903,6 +1904,9 @@ static void smmu_reset_exit(Object *obj,
> ResetType type)
> >      }
> >
> >      smmuv3_init_regs(s);
> > +    if (sys->accel) {
> > +        smmuv3_accel_init_regs(s);
> > +    }
> 
> I feel that we should likely do an if-else instead, i.e.
> 
>     if (sys->accel) {
>         smmuv3_accel_init_regs(s);
>     } else {
>         smmuv3_init_regs(s);
>     }
> 
> The smmuv3_init_regs() enables certain bits that really should be
> set by the returned IDRs from hw_info in smmuv3_accel_init_regs().
> 
> Doing an overriding call can potentially give us some trouble in
> the future if there are new bits being introduced and enabled in
> smmuv3_init_regs() but missed in smmuv3_accel_init_regs().
> 
> So, it can be simpler in the long run if smmuv3_accel_init_regs()
> initializes in its own way, IMHO.

Ok. Are you suggesting we simply override the IDR values from Host?
I don't think that is a good idea as it is not just the IDR values that
determines the host features. And we had a discussion on this 
in v2 and the suggestion was " vmm should not be copying IDR fields
blindly..."

https://lore.kernel.org/qemu-devel/Z+VNA+hFu0LJn19l@nvidia.com/

Probably we should take a look at Intel vtd implementation mentioned
by Zhenzhong in the other thread where it looks like there seems to be
a property for each capability they care about.

Probably something like,
-device arm-smmuv3,accel=on,pasid_cap=on,

And then enabling all features related to pasid and on later when
we retrieve the HW_INFO on device plug, compare and fail if not?

But I think on ARM, we still we have limitations in knowing the actual
host supported features through IDR. In that case, we can only assume
that user is making an informed decision while enabling these features.

Thanks,
Shameer