> -----Original Message-----
> From: Eric Auger <[email protected]>
> Sent: 20 May 2026 08:54
> 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 16/32] hw/arm/tegra241-cmdqv: Emulate VCMDQ
> register writes
>
> External email: Use caution opening links or attachments
>
>
> Hi Shameer,
>
> On 5/19/26 12:36 PM, Shameer Kolothum wrote:
> > From: Nicolin Chen <[email protected]>
> >
> > This is the write side counterpart of the VCMDQ read emulation. Add
> > write handling for both the direct VCMDQ aperture and the VINTF
> > logical aperture using the same index decoding and VINTF-to-VCMDQ
> > translation logic as the read path.
> >
> > VINTF aperture writes are translated to their direct-aperture
> > equivalent and update the same cached state. Page 1 registers
> > (BASE, CONS_INDX_BASE) always update the cache. Once
> > IOMMU_HW_QUEUE_ALLOC and viommu_mmap are wired up in a
> subsequent
> > patch, Page 0 register writes will be forwarded to the hardware-
> > backed mmap'd page.
> >
> > Ignore VCMDQ BASE writes if the VCMDQ is already enabled.
> >
> > 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.c | 164
> +++++++++++++++++++++++++++++++++++++++-
> > hw/arm/trace-events | 2 +
> > 2 files changed, 164 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
> > index f859126ad6..5d2d9d2e26 100644
> > --- a/hw/arm/tegra241-cmdqv.c
> > +++ b/hw/arm/tegra241-cmdqv.c
> > @@ -16,6 +16,16 @@
> > #include "tegra241-cmdqv.h"
> > #include "trace.h"
> >
> > +/*
> > + * Returns true if the per-VCMDQ CMDQ_EN_OK bit is set.
> > + */
> > +static bool tegra241_vcmdq_enabled(Tegra241CMDQV *cmdqv, int index)
> > +{
> > + uint32_t status = cmdqv->vcmdq_status[index];
> > +
> > + return status & R_VCMDQ0_STATUS_CMDQ_EN_OK_MASK;
> > +}
> > +
> > /*
> > * Read a VCMDQ Page 0 register (control/status) using VCMDQ0_* offsets.
> > *
> > @@ -116,6 +126,108 @@ static uint64_t
> tegra241_cmdqv_config_vintf_read(Tegra241CMDQV *cmdqv,
> > }
> > }
> >
> > +/*
> > + * Write a VCMDQ Page 0 register (control/status) using VCMDQ0_* offsets.
> > + *
> > + * The caller normalizes the MMIO offset such that @offset0 always refers
> > + * to a VCMDQ0_* register, while @index selects the VCMDQ instance.
> > + *
> > + * Page 0 registers are all 32-bit; this helper is only called for 4-byte
> > + * writes.
> > + */
> > +static void tegra241_cmdqv_write_vcmdq_page0(Tegra241CMDQV
> *cmdqv,
> > + hwaddr offset0, int index,
> > + uint32_t value)
> > +{
> > + switch (offset0) {
> > + case A_VCMDQ0_CONS_INDX:
> > + cmdqv->vcmdq_cons_indx[index] = value;
> > + break;
> > + case A_VCMDQ0_PROD_INDX:
> > + cmdqv->vcmdq_prod_indx[index] = value;
> so there is no cmd consumption triggered by an increment of the producer
> index as opposed to std smmu emulation. This shall at least be explained
> in the commit msg and maybe in the code, + the justification. It is a
> bit strange to emulate the MMIO accesses while not doing anything
> functional out of them.
There is indeed a difference here w.r.t. std SMMU emulation. CMDQV
is a hardware-accelerated feature, and there isn't much value in emulating
the queue functionality without the HW backing.
From Spec, p.176:
"While the software can program the Virtual CMDQ(s) directly using the direct
VCMDQ aperture (and not through the Virtual Interface), it is required that the
VCMDQ be allocated to a Virtual Interface before it is used to send commands
to the SMMU"
So no actual command processing can happen until the VCMDQ is allocated to
a Virtual Interface. Until then, reads/writes only update the register cache.
I will make this clear in the commit log.
> > + break;
> > + case A_VCMDQ0_CONFIG:
> > + if (value & R_VCMDQ0_CONFIG_CMDQ_EN_MASK) {
> > + cmdqv->vcmdq_status[index] |=
> R_VCMDQ0_STATUS_CMDQ_EN_OK_MASK;
> > + } else {
> > + cmdqv->vcmdq_status[index] &=
> ~R_VCMDQ0_STATUS_CMDQ_EN_OK_MASK;
> > + }
> > + cmdqv->vcmdq_config[index] = value;
> > + break;
> > + case A_VCMDQ0_GERRORN:
> > + cmdqv->vcmdq_gerrorn[index] = value;
> shouldn't it do something functional besides storing the value
> on std SMMU we resume consuming cmds. interaction with interrupts,
> CONS_INDX?
Same rationale as above. In the pre-allocation state there is no real queue
function to drive, so there is nothing functional for QEMU to do on a GERRORN
write.
> > + break;
> > + default:
> > + qemu_log_mask(LOG_UNIMP,
> > + "%s unhandled write access at 0x%" PRIx64 "\n",
> > + __func__, offset0);
> > + }
> > + trace_tegra241_cmdqv_write_vcmdq_page0(index, offset0, value);
> > +}
> > +
> > +/*
> > + * Write a VCMDQ Page 1 register (base / DRAM address) - 4-byte access.
> > + */
> > +static void tegra241_cmdqv_write_vcmdq_page1(Tegra241CMDQV
> *cmdqv,
> > + hwaddr offset0, int index,
> > + uint32_t value)
> > +{
> > + switch (offset0) {
> > + case A_VCMDQ0_BASE_L:
> > + if (tegra241_vcmdq_enabled(cmdqv, index)) {
> > + return;
> > + }
> > + cmdqv->vcmdq_base[index] =
> > + deposit64(cmdqv->vcmdq_base[index], 0, 32, value);
> > + break;
> > + case A_VCMDQ0_BASE_H:
> > + if (tegra241_vcmdq_enabled(cmdqv, index)) {
> where is it documented in the spec? I don't see this restriction in the
> reg doc. Then "Enabling the virtual CMDQ" paragraph describes a std
> setup but does not say - afaics- that it should be ignored -
> worth commenting
Right. I don’t think it is explicitly there in the spec.
However, Nicolin mentioned about such a restriction during v4 discussion:
https://lore.kernel.org/qemu-devel/[email protected]/
AFAICS, it looks like an implied assumption based on the "Enabling the virtual
CMDQ" sequence or I missed it.
I will double check.
Thanks,
Shameer
