subject:"\[PATCH v5 08\/32\] hw\/arm\/smmuv3\-accel\: Wire CMDQV ops into accel lifecycle"

RE: [PATCH v5 08/32] hw/arm/smmuv3-accel: Wire CMDQV ops into accel lifecycle

2026-05-19 Thread Shameer Kolothum Thodi

Hi Eric,

> -Original Message-
> From: Eric Auger 
> Sent: 20 May 2026 06:27
> To: Shameer Kolothum Thodi ; qemu-
> [email protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected]; Nicolin
> Chen ; Nathan Chen ; Matt
> Ochs ; Jiandi An ; Jason Gunthorpe
> ; [email protected]; Krishnakant Jaju
> ; [email protected]
> Subject: Re: [PATCH v5 08/32] hw/arm/smmuv3-accel: Wire CMDQV ops into
> accel lifecycle
> 
> External email: Use caution opening links or attachments
> 
> 
> 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 
> > Reviewed-by: Nicolin Chen 
> > Signed-off-by: Shameer Kolothum 
> > ---
> >  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?

Yeah. You are right. Probe is mandatory if ops is non NULL. I will revise
the comments.

> > +return false;
> > +}
> > +} else {
> > +if (!iommufd_backend_alloc_viommu(hiodi->iommufd, hiodi->devid,
> > +  IOMMU_VIOMMU_TYPE_ARM_SMMUV3,
> > +  s2_hwpt_id, NULL, 0, &viommu_id,
> > +

Re: [PATCH v5 08/32] hw/arm/smmuv3-accel: Wire CMDQV ops into accel lifecycle

2026-05-19 Thread Eric Auger

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 
> Reviewed-by: Nicolin Chen 
> Signed-off-by: Shameer Kolothum 
> ---
>  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

[PATCH v5 08/32] hw/arm/smmuv3-accel: Wire CMDQV ops into accel lifecycle

2026-05-19 Thread Shameer Kolothum

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 
Reviewed-by: Nicolin Chen 
Signed-off-by: Shameer Kolothum 
---
 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)) {
+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);
+}
+
+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,

RE: [PATCH v5 08/32] hw/arm/smmuv3-accel: Wire CMDQV ops into accel lifecycle

Re: [PATCH v5 08/32] hw/arm/smmuv3-accel: Wire CMDQV ops into accel lifecycle

[PATCH v5 08/32] hw/arm/smmuv3-accel: Wire CMDQV ops into accel lifecycle

3 matches

Site Navigation

Mail list logo

Footer information