> -----Original Message-----
> From: Eric Auger <[email protected]>
> Sent: 04 June 2026 10:37
> 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 v6 18/31] hw/arm/tegra241-cmdqv: Allocate HW
> VCMDQs once configured
>
> External email: Use caution opening links or attachments
>
>
> Hi Shameer,
>
> On 6/1/26 1:42 PM, Shameer Kolothum wrote:
> > From: Nicolin Chen <[email protected]>
> >
> > Add support for allocating IOMMUFD hardware queues when the guest
> > programs the VCMDQ BASE registers.
> >
> > VCMDQ_EN lives in VCMDQ_CONFIG, which is on the VINTF Page0 region
> > that a later patch installs into guest MMIO - so QEMU won't trap its
> > writes. Allocate the hardware queue instead once all of these are
> > set: BASE programmed, CMDQ_ALLOC_MAP.ALLOC, and CMDQV / VINTF
> > enabled. Each precondition write retries the allocation, so the
> > guest may program them in any order.
> >
> > iommufd_backend_alloc_hw_queue() needs the guest physical address of
> > the VCMDQ ring buffer, so allocation is deferred until the guest has
> > populated BASE.
> >
> > If a hardware queue was previously allocated for the same VCMDQ,
> > free it before reallocation. All allocated VCMDQs are freed when
> > CMDQV or VINTF is disabled, or when the ALLOC bit is cleared.
> >
> > On allocation failure, set CMDQ_INIT_ERR and clear CMDQ_EN_OK in the
> > cache so trapped guest reads see the failure rather than a queue
> > that looks live. Clear them on a later successful allocation.
> >
> > Signed-off-by: Nicolin Chen <[email protected]>
> > Co-developed-by: Shameer Kolothum <[email protected]>
> > Signed-off-by: Shameer Kolothum <[email protected]>
> > ---
> > hw/arm/tegra241-cmdqv.h | 11 +++
> > hw/arm/tegra241-cmdqv.c | 154
> +++++++++++++++++++++++++++++++++++++---
> > 2 files changed, 156 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/arm/tegra241-cmdqv.h b/hw/arm/tegra241-cmdqv.h
> > index c4d327a9a5..84499b840d 100644
> > --- a/hw/arm/tegra241-cmdqv.h
> > +++ b/hw/arm/tegra241-cmdqv.h
> > @@ -47,6 +47,7 @@ typedef struct Tegra241CMDQV {
> > MemoryRegion mmio_cmdqv;
> > qemu_irq irq;
> > IOMMUFDVeventq *veventq;
> > + IOMMUFDHWqueue *vcmdq[TEGRA241_CMDQV_MAX_CMDQ];
> > void *vintf_page0;
> >
> > /* CMDQ-V Config page register cache */
> > @@ -364,6 +365,16 @@ SMMU_CMDQV_VI_VCMDQi_BASE_H_(1)
> > SMMU_CMDQV_VI_VCMDQi_CONS_INDX_BASE_DRAM_L_(1)
> > SMMU_CMDQV_VI_VCMDQi_CONS_INDX_BASE_DRAM_H_(1)
> >
> > +static inline bool tegra241_cmdqv_enabled(Tegra241CMDQV *cmdqv)
> > +{
> > + return cmdqv->status & R_STATUS_CMDQV_ENABLED_MASK;
> > +}
> > +
> > +static inline bool tegra241_vintf_enabled(Tegra241CMDQV *cmdqv)
> > +{
> > + return cmdqv->vintf_status & R_VINTF0_STATUS_ENABLE_OK_MASK;
> > +}
> > +
> > const SMMUv3AccelCmdqvOps *tegra241_cmdqv_get_ops(void);
> >
> > #endif /* HW_ARM_TEGRA241_CMDQV_H */
> > diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
> > index a52e153501..54d95f4e4e 100644
> > --- a/hw/arm/tegra241-cmdqv.c
> > +++ b/hw/arm/tegra241-cmdqv.c
> > @@ -18,6 +18,96 @@
> > #include "tegra241-cmdqv.h"
> > #include "trace.h"
> >
> > +static void tegra241_cmdqv_free_vcmdq(Tegra241CMDQV *cmdqv, int
> index)
> > +{
> > + IOMMUFDViommu *viommu = cmdqv->s_accel->viommu;
> > + IOMMUFDHWqueue *vcmdq = cmdqv->vcmdq[index];
> > +
> > + if (!vcmdq) {
> > + return;
> > + }
> > + iommufd_backend_free_id(viommu->iommufd, vcmdq->hw_queue_id);
> > + g_free(vcmdq);
> > + cmdqv->vcmdq[index] = NULL;
> > +}
> > +
> > +static void tegra241_cmdqv_free_all_vcmdq(Tegra241CMDQV *cmdqv)
> > +{
> > + /* uapi/linux/iommufd.h: hw_queue destroy must be in descending
> @index. */
> > + for (int i = (TEGRA241_CMDQV_MAX_CMDQ - 1); i >= 0; i--) {
> > + tegra241_cmdqv_free_vcmdq(cmdqv, i);
> > + }
> basically the same comment as previous patch. I would add in the exit
> phase reset at this stage too.
Yes. I get that now. Will populate the rest handler.
> > +}
> > +
> > +/*
> > + * Allocate a host HW VCMDQ from the current cached BASE / size for
> @index.
> > + *
> > + * Setup is a no-op (returns true) when any of the following preconditions
> > + * isn't met yet:
> > + * - BASE not prgrammed yet.
> > + * - VCMDQ is not mapped to a VINTF (CMDQ_ALLOC_MAP.ALLOC == 0)
> > + * - BASE / size don't resolve to a RAM region
> > + * - CMDQV global enable or VINTF enable is not yet asserted
> > + */
> > +static bool tegra241_cmdqv_setup_vcmdq(Tegra241CMDQV *cmdqv, int
> index,
> > + Error **errp)
> > +{
> > + SMMUv3AccelState *accel = cmdqv->s_accel;
> > + uint64_t base_mask = (uint64_t)R_VCMDQ0_BASE_L_ADDR_MASK |
> > + (uint64_t)R_VCMDQ0_BASE_H_ADDR_MASK << 32;
> > + uint64_t addr = cmdqv->vcmdq_base[index] & base_mask;
> > + uint64_t log2 = cmdqv->vcmdq_base[index] &
> R_VCMDQ0_BASE_L_LOG2SIZE_MASK;
> > + uint64_t size = 1ULL << (log2 + 4);
> > + IOMMUFDViommu *viommu = accel->viommu;
> > + IOMMUFDHWqueue *hw_queue;
> > + uint32_t hw_queue_id;
> > +
> > + /* BASE not programmed yet. */
> > + if (!cmdqv->vcmdq_base[index]) {
> > + return true;
> > + }
> > +
> > + /* VCMDQ not yet mapped to a VINTF. */
> > + if (!(cmdqv->cmdq_alloc_map[index] &
> R_CMDQ_ALLOC_MAP_0_ALLOC_MASK)) {
> > + return true;
> > + }
> > +
> > + if (!tegra241_cmdqv_enabled(cmdqv) ||
> !tegra241_vintf_enabled(cmdqv)) {
> > + return true;
> > + }
> nit: I think I would introduce an helper
> vcmdq_hw_queue_ready_to_be_allocated(cmdqv, index)
> where you could put a proper doc comment.
Sure.
>
> Not that once flag the base is set it returns true. ie. you do not check
> that both L & H are set.
>
> But anyway the queue will be reallocated
That's true. I will capture it in comments.
> > +
> > + tegra241_cmdqv_free_vcmdq(cmdqv, index);
> > +
> > + if (!iommufd_backend_alloc_hw_queue(viommu->iommufd, viommu-
> >viommu_id,
> > + IOMMU_HW_QUEUE_TYPE_TEGRA241_CMDQV,
> > + index, addr, size, &hw_queue_id,
> > + errp)) {
> > + /* Record the failure in the cache. */
> > + cmdqv->vcmdq_gerror[index] |=
> R_VCMDQ0_GERROR_CMDQ_INIT_ERR_MASK;
> > + cmdqv->vcmdq_status[index] &=
> ~R_VCMDQ0_STATUS_CMDQ_EN_OK_MASK;
> > + return false;
> > + }
> > + hw_queue = g_new(IOMMUFDHWqueue, 1);
> > + hw_queue->hw_queue_id = hw_queue_id;
> > + hw_queue->viommu = viommu;
> > + cmdqv->vcmdq[index] = hw_queue;
> > +
> > + cmdqv->vcmdq_gerror[index] &=
> ~R_VCMDQ0_GERROR_CMDQ_INIT_ERR_MASK;
> > + cmdqv->vcmdq_status[index] |=
> R_VCMDQ0_STATUS_CMDQ_EN_OK_MASK;
> > +
> > + return true;
> > +}
> > +
> > +static void tegra241_cmdqv_setup_all_vcmdq(Tegra241CMDQV *cmdqv,
> > + Error **errp)
> > +{
> > + for (int i = 0; i < TEGRA241_CMDQV_MAX_CMDQ; i++) {
> > + if (!tegra241_cmdqv_setup_vcmdq(cmdqv, i, errp)) {
> > + return;
> > + }
> > + }
> nit: you you could after the tegra241_cmdqv_free_all_vcmdq helper
> > +}
> > +
> > /*
> > * Read a VCMDQ Page 0 register (control/status) using VCMDQ0_* offsets.
> > *
> > @@ -145,7 +235,12 @@ static void
> tegra241_cmdqv_write_vcmdq_page0(Tegra241CMDQV *cmdqv,
> > break;
> > case A_VCMDQ0_CONFIG:
> > if (value & R_VCMDQ0_CONFIG_CMDQ_EN_MASK) {
> > - cmdqv->vcmdq_status[index] |=
> R_VCMDQ0_STATUS_CMDQ_EN_OK_MASK;
> > + /* Report init error if any. */
> > + if (!(cmdqv->vcmdq_gerror[index] &
> > + R_VCMDQ0_GERROR_CMDQ_INIT_ERR_MASK)) {
> > + cmdqv->vcmdq_status[index] |=
> > + R_VCMDQ0_STATUS_CMDQ_EN_OK_MASK;
> > + }
> does it really belong to this patch?
I put it here because this is the patch that sets that error in the
first place. Commit log kind of mentions it. I could explain it better in
commit log if that's enough. The other option is splitting the error
handling into a separate patch, but THB don't see a need for that.
Thanks,
Shameer
