On 2/10/26 9:46 AM, Shameer Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Nicolin Chen <[email protected]>
>> Sent: 09 February 2026 19:42
>> To: Shameer Kolothum Thodi <[email protected]>
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; Nathan Chen <[email protected]>; Matt Ochs
>> <[email protected]>; Jiandi An <[email protected]>; Jason Gunthorpe
>> <[email protected]>; [email protected];
>> [email protected]; [email protected]; Krishnakant Jaju
>> <[email protected]>
>> Subject: Re: [PATCH v2 05/24] hw/arm/smmuv3-accel: Introduce CMDQV ops
>> interface
>>
>> On Fri, Feb 06, 2026 at 02:48:04PM +0000, Shameer Kolothum wrote:
>>> CMDQ-Virtualization (CMDQV) is a hardware extension to SMMUv3 that
>> enables
>>> virtualization of multiple command queues (VCMDQs).
>> Let's mention "NVIDIA", noting it's a non-standard extension.
> The idea of this ops based callback is to make it more generic and not
> NVIDIA specific to start with. This leaves room in case ARM introduces
> a standardised version of something similar in the future, or if another
> vendor provides a similar extension.
>
>>> CMDQV support is a specialization of the IOMMUFD backed accelerated
>>> SMMUv3 path. Introduce an ops interface to factor CMDQV specific
>>> initialization and CMDQV vIOMMU/vEVENTQ allocation behavior out of
>>> the base implementation. The ops pointer and associated state are
>>> stored in the accelerated SMMUv3 state.
>>>
>>> No functional change
>>>
>>> Signed-off-by: Shameer Kolothum <[email protected]>
>>> ---
>>> hw/arm/smmuv3-accel.h | 19 +++++++++++++++++++
>>> 1 file changed, 19 insertions(+)
>>>
>>> diff --git a/hw/arm/smmuv3-accel.h b/hw/arm/smmuv3-accel.h
>>> index c9c10e55c3..ca087240e5 100644
>>> --- a/hw/arm/smmuv3-accel.h
>>> +++ b/hw/arm/smmuv3-accel.h
>>> @@ -16,6 +16,23 @@
>>> #endif
>>> #include CONFIG_DEVICES
>>>
>>> +/*
>>> + * CMDQ-Virtualization (CMDQV) hardware support, extends the SMMUv3
>> to
>>> + * support multiple VCMDQs with virtualization capabilities.
>> Ditto
>>
>>> + * CMDQV specific behavior is factored behind this ops interface.
>>> + */
>>> +typedef struct SMMUv3AccelCmdqvOps {
>>> + bool (*init)(SMMUv3State *s, Error **errp);
>>> + bool (*alloc_viommu)(SMMUv3State *s,
>>> + HostIOMMUDeviceIOMMUFD *idev,
>>> + uint32_t *out_viommu_id,
>>> + Error **errp);
>>> + void (*free_viommu)(SMMUv3State *s);
>> I don't see much value of having alloc/free_viommu..
>>
>> Type and data structure are the only difference from the standard
>> viommu routine, and both are already in the uAPI header.
> That is right. But again, this is mainly to keep the interface generic.
> IIUC, Eric had suggested, using an ops or object style abstraction
> to separate extension specific behaviour from the core code.
My initial thought was callbacks to specialize implementation for accel
smmu without vcmdq versus accel smmu with vcmdqs. Effectively we could
later imagine to have other implementations with vcmdqs but it is
difficult to guess the specialization needs at that stage. I think in
the original series you already add the first kind of differentiation.
At least reset and init need to be specialized.
Maybe alloc_viommu does not need to because the differentiation only
happens on the type arg. But originally you where also turning offÂ
s->tegra241_cmdqv if IOMMU_VIOMMU_TYPE_TEGRA241_CMDQV failed so I had
the impression you reverted to a fallback impl. If there is
specialization, I still think ops are relevant. Eric
>
>>> + bool (*alloc_veventq)(SMMUv3State *s, Error **errp);
>>> + void (*free_veventq)(SMMUv3State *s);
>> alloc_/free_veventq can be moved into alloc/free_viommu functions.
>>> + void (*reset)(SMMUv3State *s);
>>> +} SMMUv3AccelCmdqvOps;
>> Overall, an ops structure feels unnecessary to me. Maybe init and
>> reset are somewhat plausible. But nobody else would reuse this ops
>> structure that is exclusive to Tegra241CMDQV?
> That's true for now, as there is only one implementation today. If this
> abstraction feels over engineered at this stage, we can simplify or defer
> it (and fall back to calling the tegra241-specific functions directly, as in
> the RFC) and revisit once there is a clearer need.
>
> I'm open to suggestions, and let's wait to hear from others as well.
>
>>> /*
>>> * Represents an accelerated SMMU instance backed by an iommufd
>> vIOMMU object.
>>> * Holds bypass and abort proxy HWPT IDs used for device attachment.
>>> @@ -28,6 +45,8 @@ typedef struct SMMUv3AccelState {
>>> uint32_t bypass_hwpt_id;
>>> uint32_t abort_hwpt_id;
>>> QLIST_HEAD(, SMMUv3AccelDevice) device_list;
>>> + const SMMUv3AccelCmdqvOps *cmdqv_ops;
>>> + void *cmdqv;
>> Having two pointers for the same extension feels redundant. Maybe
>> merge them with:
>>
>> typedef struct SMMUv3AccelCmdqv {
>> void *private; // points to Tegra241CMDQV
>> struct iommu_viommu_tegra241_cmdqv viommu_data;
>> } SMMUv3AccelCmdqv;
>>
>> ...
>>
>> SMMUv3AccelState {
>> ...
>> SMMUv3AccelCmdqv *cmdqv;
>> };
>>
>> ?
> We could fold the private data and ops into a single struct and store that
> in SMMUv3AccelState. However, if we want to keep this generic, it may
> be better to keep them separate.
>
> Thanks,
> Shameer
>
