> -----Original Message-----
> From: Nicolin Chen <[email protected]>
> Sent: 29 December 2025 19:07
> To: Shameer Kolothum <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Nathan Chen
> <[email protected]>; Matt Ochs <[email protected]>; Jason Gunthorpe
> <[email protected]>; [email protected];
> [email protected]; [email protected]; Krishnakant Jaju
> <[email protected]>
> Subject: Re: [RFC PATCH 05/16] hw/arm/tegra241-cmdqv: Add initial
> Tegra241 CMDQ-Virtualisation support
>
> On Wed, Dec 10, 2025 at 01:37:26PM +0000, Shameer Kolothum wrote:
> > +void tegra241_cmdqv_init(SMMUv3State *s)
> > +{
> > + SysBusDevice *sbd = SYS_BUS_DEVICE(OBJECT(s));
> > + Tegra241CMDQV *cmdqv;
> > +
> > + if (!s->tegra241_cmdqv) {
> > + return;
> > + }
>
> Maybe g_assert?
Sure.
>
> > +typedef struct Tegra241CMDQV {
> > + struct iommu_viommu_tegra241_cmdqv cmdqv_data;
> > + SMMUv3State *smmu;
>
> I see all the cmdqv functions want "smmu->s_accel", so maybe store
> "s_accel" instead?
Yes. But see below.
> > +#ifdef CONFIG_TEGRA241_CMDQV
> > +bool tegra241_cmdqv_alloc_viommu(SMMUv3State *s,
> HostIOMMUDeviceIOMMUFD *idev,
> > + uint32_t *out_viommu_id, Error **errp);
> > +void tegra241_cmdqv_init(SMMUv3State *s);
> > +#else
> > +static inline void tegra241_cmdqv_init(SMMUv3State *s)
> > +{
> > +}
> > +static inline bool
> > +tegra241_cmdqv_alloc_viommu(SMMUv3State *s,
> HostIOMMUDeviceIOMMUFD *idev,
> > + uint32_t *out_viommu_id, Error **errp)
> > +{
> > + return true;
>
> Should it return false?
Hmm..it should be, given how it is invoked here.
>
> > index 2d4970fe19..8e56e480a0 100644
> > --- a/include/hw/arm/smmuv3.h
> > +++ b/include/hw/arm/smmuv3.h
> > @@ -73,6 +73,9 @@ struct SMMUv3State {
> > bool ats;
> > uint8_t oas;
> > bool pasid;
> > + /* Support for NVIDIA Tegra241 SMMU CMDQV extension */
> > + struct Tegra241CMDQV *cmdqv;
> > + bool tegra241_cmdqv;
>
> tegra241_cmdqv is a Property, so it has to stay with SMMUv3State.
>
> But "struct Tegra241CMDQV *cmdqv" might be in the SMMUv3AccelState?
I have considered this. Currently, SMMUv3AccelState is allocated in
smmuv3_accel_alloc_viommu(), which happens later than tegra241_cmdqv_init().
Changing this would require some rework. I will look into whether we can address
that as part of the next respin of the SMMUv3 accel series to make this cleaner.
Thanks,
Shameer
