Hi Eric,
> -----Original Message-----
> From: Eric Auger <[email protected]>
> Sent: 20 May 2026 06:27
> To: Shameer Kolothum Thodi <[email protected]>; qemu-
> [email protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected]; Nicolin
> Chen <[email protected]>; Nathan Chen <[email protected]>; Matt
> Ochs <[email protected]>; Jiandi An <[email protected]>; Jason Gunthorpe
> <[email protected]>; [email protected]; Krishnakant Jaju
> <[email protected]>; [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 <[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?
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,
> > + 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()
Agree. I will change it into:
smmuv3_accel_probe_cmdqv()
{
....
if (!ops) {
error_setg(errp, "No CMDQV ops found");
return NULL;
}
g_assert(ops->probe);
g_assert(ops->alloc_viommu);
if (!ops->probe(s, idev, errp)) {
return NULL;
}
return ops;
}
Thanks,
Shameer
