Hi SHameer,
On 5/19/26 12:36 PM, Shameer Kolothum wrote:
> Add support for selecting and initializing a CMDQV backend based on the
> cmdqv OnOffAuto property.
>
> If set to OFF, CMDQV is not used and the default IOMMUFD-backed allocation
> path is taken.
>
> If set to AUTO, QEMU attempts to probe a CMDQV backend during device setup.
> If probing succeeds, the selected ops are stored in the accelerated SMMUv3
> state and used. If probing fails, QEMU silently falls back to the default
> path.
>
> If set to ON, QEMU requires CMDQV support. Probing is performed during
> setup and failure results in an error.
>
> When a CMDQV backend is active, its callbacks are used for vIOMMU
> allocation, free, and reset handling. Otherwise, the base implementation
> is used.
>
> The current implementation wires up the Tegra241 CMDQV backend through the
> generic ops interface. Functional CMDQV behaviour is added in subsequent
> patches.
>
> No functional change.
>
> Reviewed-by: Eric Auger <[email protected]>
> Reviewed-by: Nicolin Chen <[email protected]>
> Signed-off-by: Shameer Kolothum <[email protected]>
> ---
> include/hw/arm/smmuv3.h | 2 +
> hw/arm/smmuv3-accel.c | 96 ++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 91 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
> index fe0493c1aa..aa6a79237a 100644
> --- a/include/hw/arm/smmuv3.h
> +++ b/include/hw/arm/smmuv3.h
> @@ -74,6 +74,8 @@ struct SMMUv3State {
> OnOffAuto ats;
> OasMode oas;
> SsidSizeMode ssidsize;
> + /* SMMU CMDQV extension */
> + OnOffAuto cmdqv;
>
> Notifier machine_done;
> };
> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> index 30c498ffcf..202b1aedd9 100644
> --- a/hw/arm/smmuv3-accel.c
> +++ b/hw/arm/smmuv3-accel.c
> @@ -19,6 +19,7 @@
> #include "smmuv3-internal.h"
> #include "smmuv3-accel.h"
> #include "system/system.h"
> +#include "tegra241-cmdqv.h"
>
> /*
> * The root region aliases the global system memory, and shared_as_sysmem
> @@ -570,6 +571,7 @@ smmuv3_accel_alloc_viommu(SMMUv3State *s,
> HostIOMMUDeviceIOMMUFD *hiodi,
> Error **errp)
> {
> SMMUv3AccelState *accel = s->s_accel;
> + const SMMUv3AccelCmdqvOps *cmdqv_ops = accel->cmdqv_ops;
> struct iommu_hwpt_arm_smmuv3 bypass_data = {
> .ste = { SMMU_STE_CFG_BYPASS | SMMU_STE_VALID, 0x0ULL },
> };
> @@ -580,10 +582,17 @@ smmuv3_accel_alloc_viommu(SMMUv3State *s,
> HostIOMMUDeviceIOMMUFD *hiodi,
> uint32_t viommu_id, hwpt_id;
> IOMMUFDViommu *viommu;
>
> - if (!iommufd_backend_alloc_viommu(hiodi->iommufd, hiodi->devid,
> - IOMMU_VIOMMU_TYPE_ARM_SMMUV3,
> s2_hwpt_id,
> - NULL, 0, &viommu_id, errp)) {
> - return false;
> + if (cmdqv_ops) {
> + if (!cmdqv_ops->alloc_viommu(s, hiodi, &viommu_id, errp)) {
the doc comment in 6/32 says that alloc_iommu is mandatory if probe is
implemented. So it looks there are situations where alloc_iommu may be null.
But actually given the implementation of smmuv3_accel_probe_cmdqv below,
the ops is only non NULL if probe has succeeded. So shouldn't probe be
also mandatory?
> + return false;
> + }
> + } else {
> + if (!iommufd_backend_alloc_viommu(hiodi->iommufd, hiodi->devid,
> + IOMMU_VIOMMU_TYPE_ARM_SMMUV3,
> + s2_hwpt_id, NULL, 0, &viommu_id,
> + errp)) {
> + return false;
> + }
> }
>
> viommu = g_new0(IOMMUFDViommu, 1);
> @@ -629,12 +638,72 @@ free_bypass_hwpt:
> free_abort_hwpt:
> iommufd_backend_free_id(hiodi->iommufd, accel->abort_hwpt_id);
> free_viommu:
> - iommufd_backend_free_id(hiodi->iommufd, viommu->viommu_id);
> + if (cmdqv_ops && cmdqv_ops->free_viommu) {
> + cmdqv_ops->free_viommu(s);
> + } else {
> + iommufd_backend_free_id(hiodi->iommufd, viommu->viommu_id);
> + }
> g_free(viommu);
> accel->viommu = NULL;
> return false;
> }
>
> +static const SMMUv3AccelCmdqvOps *
> +smmuv3_accel_probe_cmdqv(SMMUv3State *s, HostIOMMUDeviceIOMMUFD *idev,
> + Error **errp)
> +{
> + const SMMUv3AccelCmdqvOps *ops = tegra241_cmdqv_get_ops();
> +
> + if (!ops || !ops->probe) {
> + error_setg(errp, "No CMDQV ops found");
> + return NULL;
> + }
> +
> + if (!ops->probe(s, idev, errp)) {
> + return NULL;
> + }
> + return ops;
> +}
> +
> +static bool
> +smmuv3_accel_select_cmdqv(SMMUv3State *s, HostIOMMUDeviceIOMMUFD *idev,
> + Error **errp)
> +{
> + const SMMUv3AccelCmdqvOps *ops = NULL;
> +
> + if (s->s_accel->cmdqv_ops) {
> + return true;
> + }
> +
> + switch (s->cmdqv) {
> + case ON_OFF_AUTO_OFF:
> + s->s_accel->cmdqv_ops = NULL;
> + return true;
> + case ON_OFF_AUTO_AUTO:
> + ops = smmuv3_accel_probe_cmdqv(s, idev, NULL);
> + break;
> + case ON_OFF_AUTO_ON:
> + ops = smmuv3_accel_probe_cmdqv(s, idev, errp);
> + if (!ops) {
> + error_append_hint(errp, "CMDQV requested but not supported");
> + return false;
> + }
> + break;
> + default:
> + g_assert_not_reached();
> + }
> +
> + if (ops) {
> + g_assert(ops->alloc_viommu);
I think I would move the assert at the end of
smmuv3_accel_probe_cmdqv()
> + }
> +
> + if (ops && ops->init && !ops->init(s, errp)) {
> + return false;
> + }
> + s->s_accel->cmdqv_ops = ops;
> + return true;
> +}
> +
> static bool smmuv3_accel_set_iommu_device(PCIBus *bus, void *opaque, int
> devfn,
> HostIOMMUDevice *hiod, Error
> **errp)
> {
> @@ -669,6 +738,10 @@ static bool smmuv3_accel_set_iommu_device(PCIBus *bus,
> void *opaque, int devfn,
> goto done;
> }
>
> + if (!smmuv3_accel_select_cmdqv(s, hiodi, errp)) {
> + return false;
> + }
> +
> if (!smmuv3_accel_alloc_viommu(s, hiodi, errp)) {
> error_append_hint(errp, "Unable to alloc vIOMMU: hiodi devid 0x%x: ",
> hiodi->devid);
> @@ -946,8 +1019,17 @@ bool smmuv3_accel_attach_gbpa_hwpt(SMMUv3State *s,
> Error **errp)
>
> void smmuv3_accel_reset(SMMUv3State *s)
> {
> - /* Attach a HWPT based on GBPA reset value */
> - smmuv3_accel_attach_gbpa_hwpt(s, NULL);
> + SMMUv3AccelState *accel = s->s_accel;
> +
> + if (!accel) {
> + return;
> + }
> + /* Attach a HWPT based on GBPA reset value */
> + smmuv3_accel_attach_gbpa_hwpt(s, NULL);
> +
> + if (accel->cmdqv_ops && accel->cmdqv_ops->reset) {
> + accel->cmdqv_ops->reset(s);
> + }
> }
>
> static void smmuv3_accel_as_init(SMMUv3State *s)
Eric
