Re: [RFC PATCH 07/16] hw/arm/tegra241-cmdqv: Add read emulation support for registers

[Nicolin Chen <nicolinc@nvidia.com>]

Hi Eric,

On Mon, Jan 26, 2026 at 04:13:38PM +0100, Eric Auger wrote:
> > +/* Note that offset aligns down to 0x10000 */
> > +static uint64_t tegra241_cmdqv_read_vcmdq(Tegra241CMDQV *cmdqv, hwaddr offset,
> > +                                          int index)

> the offset is within the instance, right? Better to rename it
> differently from the main offset

In tegra241_cmdqv_read(), its offset is the register offset to the
entire CMDQV. That can be A_VCMDQ0_CONS_INDX, A_VCMDQ1_CONS_INDX,
until A_VCMDQ127_CONS_INDX.

Then, we align it down to 0x10000 (A_VCMDQ0_CONS_INDX) to pass it
into this function, so this function can handle any VCMDQ via the
A_VCMDQ0_* registers only, as "index" tells which VCMDQ instead.

Perhaps we could rename this "offset" to "offset0", and elaborate
this with a better function comments, e.g.:
/*
 * The caller striped away the VCMDQ index from its offset and passed that in
 * via @offset0 so any VCMDQ can fit into A_VCMDQ0_CONS_INDX for simplicity.
 *
 * On the other hand, @index will tell which VCMDQ is being accessed here.
 */

> > +{
> > +    uint32_t *ptr = NULL;
> > +    uint64_t off;
> better initialize it?

It's not used unless cmdqv->vcmdq_page0, so we should probably move
it to the if statement instead.

> > +
> > +    /*
> > +     * Each VCMDQ instance occupies a 128 byte region (0x80).
> > +     * The hardware layout is:
> > +     *   vcmdq_page0 + (index * 0x80) + (offset - 0x10000)
> > +     */
> > +    if (cmdqv->vcmdq_page0) {
> > +        off = (0x80 * index) + (offset - 0x10000);
> > +        ptr = (uint32_t *)(cmdqv->vcmdq_page0 + off);

> I am confused because I understood page0 was mmapped so that the guest
> could read it without VMM mediation?

Yes. I pointed it out too. This should be cleaned away:
https://lore.kernel.org/qemu-devel/aVLZv+nwUB7+ohuw@Asurada-Nvidia/

So, only return the cached regval unless !cmdqv_page0:

    if (!cmdqv->vcmdq_page0) {
        return cmdqv->vcmdq_cons_indx[index];
    }

> > +    case A_VCMDQ0_CONS_INDX ... A_VCMDQ127_GERRORN:
> > +        /*
> > +         * Align offset down to 0x10000 while extracting the index:
> this is a bit cryptic to me. Could you explain this in the commit msg
> > +         *   VCMDQ0_CONS_INDX  (0x10000) => 0x10000, 0
> > +         *   VCMDQ1_CONS_INDX  (0x10080) => 0x10000, 1
> > +         *   VCMDQ2_CONS_INDX  (0x10100) => 0x10000, 2
> > +         *   ...
> > +         *   VCMDQ127_CONS_INDX (0x13f80) => 0x10000, 127
> > +         */
> > +        index = (offset - 0x10000) / 0x80;
> > +        return tegra241_cmdqv_read_vcmdq(cmdqv, offset - 0x80 * index, index);

As I mentioned above, it is to ease the tegra241_cmdqv_read_vcmdq()
that only needs to handle A_VCMDQ0_* cases in the switch statement.

Yes, let's add in the commit message.

Thanks
Nicolin