On 5/20/26 11:54 AM, Shameer Kolothum Thodi wrote:
>
>> -----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.
Yes this helps
>
>>> + 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.
also I would add a comment in the code
>
>>> + 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.
OK
Thanks
Eric
>
> Thanks,
> Shameer
