Re: [PATCH v2 05/24] hw/arm/smmuv3-accel: Introduce CMDQV ops interface

[Nicolin Chen <nicolinc@nvidia.com>]

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.

> 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 <skolothumtho@nvidia.com>
> ---
>  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. 

> +    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 defined exclusively for Tegra241CMDQV?

>  /*
>   * 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;
};

?

Nicolin