Hi Eric,
> -----Original Message-----
> From: Eric Auger <[email protected]>
> Sent: 12 January 2026 15:29
> To: Shameer Kolothum <[email protected]>; qemu-
> [email protected]; [email protected]
> Cc: [email protected]; Nicolin Chen <[email protected]>; Nathan
> Chen <[email protected]>; Matt Ochs <[email protected]>; Jason
> Gunthorpe <[email protected]>; [email protected];
> [email protected]; [email protected]; Krishnakant Jaju
> <[email protected]>
> Subject: Re: [RFC PATCH 05/16] hw/arm/tegra241-cmdqv: Add initial
> Tegra241 CMDQ-Virtualisation support
>
> External email: Use caution opening links or attachments
>
>
> On 12/10/25 2:37 PM, Shameer Kolothum wrote:
> > From: Nicolin Chen <[email protected]>
> >
> > Introduce initial support for NVIDIA Tegra241 CMDQ-Virtualisation
> (CMDQV),
> > an extension to SMMUv3 providing virtualizable hardware command
> queues.
> > This adds the basic MMIO handling, and integration hooks in the SMMUv3
> > accelerated path. When enabled, the SMMUv3 backend allocates a Tegra241
> > specific vIOMMU object via IOMMUFD and exposes a CMDQV MMIO region
> and
> > IRQ to the guest.
> >
> > The "tegra241-cmdqv" property isn't user visible yet and it will be
> > introduced in a later patch once all the supporting pieces are ready.
> >
> > Signed-off-by: Nicolin Chen <[email protected]>
> > Signed-off-by: Shameer Kolothum <[email protected]>
> > ---
> > hw/arm/Kconfig | 5 ++++
> > hw/arm/meson.build | 1 +
> > hw/arm/smmuv3-accel.c | 10 +++++--
> > hw/arm/smmuv3.c | 4 +++
> > hw/arm/tegra241-cmdqv.c | 65
> +++++++++++++++++++++++++++++++++++++++++
> > hw/arm/tegra241-cmdqv.h | 40 +++++++++++++++++++++++++
> > include/hw/arm/smmuv3.h | 3 ++
> > 7 files changed, 126 insertions(+), 2 deletions(-)
> > create mode 100644 hw/arm/tegra241-cmdqv.c
> > create mode 100644 hw/arm/tegra241-cmdqv.h
> >
> > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > index 702b79a02b..42b6b95285 100644
> > --- a/hw/arm/Kconfig
> > +++ b/hw/arm/Kconfig
> > @@ -37,6 +37,7 @@ config ARM_VIRT
> > select VIRTIO_MEM_SUPPORTED
> > select ACPI_CXL
> > select ACPI_HMAT
> > + select TEGRA241_CMDQV
> >
> > config CUBIEBOARD
> > bool
> > @@ -634,6 +635,10 @@ config ARM_SMMUV3_ACCEL
> > bool
> > depends on ARM_SMMUV3 && IOMMUFD
> >
> > +config TEGRA241_CMDQV
> > + bool
> > + depends on ARM_SMMUV3_ACCEL
> > +
> > config FSL_IMX6UL
> > bool
> > default y
> > diff --git a/hw/arm/meson.build b/hw/arm/meson.build
> > index c250487e64..4ec91db50a 100644
> > --- a/hw/arm/meson.build
> > +++ b/hw/arm/meson.build
> > @@ -86,6 +86,7 @@ arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP',
> if_true: files('fsl-imx8mp.c'))
> > arm_common_ss.add(when: 'CONFIG_FSL_IMX8MP_EVK', if_true:
> files('imx8mp-evk.c'))
> > arm_ss.add(when: 'CONFIG_ARM_SMMUV3', if_true: files('smmuv3.c'))
> > arm_ss.add(when: 'CONFIG_ARM_SMMUV3_ACCEL', if_true:
> files('smmuv3-accel.c'))
> > +arm_ss.add(when: 'CONFIG_TEGRA241_CMDQV', if_true: files('tegra241-
> cmdqv.c'))
> > arm_common_ss.add(when: 'CONFIG_FSL_IMX6UL', if_true: files('fsl-
> imx6ul.c', 'mcimx6ul-evk.c'))
> > arm_common_ss.add(when: 'CONFIG_NRF51_SOC', if_true:
> files('nrf51_soc.c'))
> > arm_common_ss.add(when: 'CONFIG_XEN', if_true: files(
> > diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> > index 939898c9b0..e50c4b3bb7 100644
> > --- a/hw/arm/smmuv3-accel.c
> > +++ b/hw/arm/smmuv3-accel.c
> > @@ -18,6 +18,7 @@
> >
> > #include "smmuv3-internal.h"
> > #include "smmuv3-accel.h"
> > +#include "tegra241-cmdqv.h"
> >
> > /*
> > * The root region aliases the global system memory, and
> shared_as_sysmem
> > @@ -499,10 +500,15 @@ smmuv3_accel_alloc_viommu(SMMUv3State *s,
> HostIOMMUDeviceIOMMUFD *idev,
> > .ste = { SMMU_STE_VALID, 0x0ULL },
> > };
> > uint32_t s2_hwpt_id = idev->hwpt_id;
> > - uint32_t viommu_id, hwpt_id;
> > + uint32_t viommu_id = 0, hwpt_id;
> > SMMUv3AccelState *accel;
> >
> > - if (!iommufd_backend_alloc_viommu(idev->iommufd, idev->devid,
> > + if (s->tegra241_cmdqv && !tegra241_cmdqv_alloc_viommu(s, idev,
> &viommu_id,
> > + errp)) {
> > + return false;
> I am confused. In tegra241_cmdqv_alloc_viommu() it returns false if
> alloc_viommu fails. but you seem to reset s->tegra241_cmdqv as if you
> would fall back to non cmdqv setup. What do you try do, fallback or
> execute either tegra241 code or default code. Or maybe I misunderstand
> the uapi call sequence?
No fallback intended. Currently, if the user has enabled tegra241_cmdqv and
tegra241_cmdqv_alloc_viommu() fails, we fail the device init. Sorry about
that to reset s->tegra241_cmdqv and !viommu_id logic, , that was a left
over logic from previous internal branch I had.
@Nicolin, is there any such requirement for a fallback in this case?
> I would not reset a property value in general under the hood. If the end
> user has set up this option, I guess he expects it to be enabled when he
> queries it back, no?
Yes, that’s the idea.
> > + }
> > +
> > + if (!viommu_id && !iommufd_backend_alloc_viommu(idev->iommufd,
> idev->devid,
> > IOMMU_VIOMMU_TYPE_ARM_SMMUV3,
> > s2_hwpt_id,
> > NULL, 0, &viommu_id, errp)) {
>
> If this is a specialization of alloc_viommu code, it generally points to
> a class or ops interface. You may face such kind of comments later on so
> better to justify that choice or adopt a new one ;-)
> same for init which is overloaded compared to original implementation.
I agree. I will take another look at this. Also, this is where any help would
be really appreciated 😊. Please let me know if you have some ideas on
how we can improve this.
Thanks,
Shameer
