Re: [PATCH v3 17/32] hw/arm/tegra241-cmdqv: mmap VINTF Page0 for CMDQV

[Eric Auger <eric.auger@redhat.com>]

On 3/11/26 1:34 PM, Shameer Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Sent: 11 March 2026 10:05
>> To: Shameer Kolothum Thodi <skolothumtho@nvidia.com>; qemu-
>> arm@nongnu.org; qemu-devel@nongnu.org
>> Cc: peter.maydell@linaro.org; clg@redhat.com; alex@shazbot.org; Nicolin
>> Chen <nicolinc@nvidia.com>; Nathan Chen <nathanc@nvidia.com>; Matt
>> Ochs <mochs@nvidia.com>; Jiandi An <jan@nvidia.com>; Jason Gunthorpe
>> <jgg@nvidia.com>; jonathan.cameron@huawei.com;
>> zhangfei.gao@linaro.org; zhenzhong.duan@intel.com; Krishnakant Jaju
>> <kjaju@nvidia.com>; phrdina@redhat.com
>> Subject: Re: [PATCH v3 17/32] hw/arm/tegra241-cmdqv: mmap VINTF Page0
>> for CMDQV
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 3/11/26 10:26 AM, Shameer Kolothum Thodi wrote:
>>>> -----Original Message-----
>>>> From: Eric Auger <eric.auger@redhat.com>
>>>> Sent: 11 March 2026 07:55
>>>> To: Shameer Kolothum Thodi <skolothumtho@nvidia.com>; qemu-
>>>> arm@nongnu.org; qemu-devel@nongnu.org
>>>> Cc: peter.maydell@linaro.org; clg@redhat.com; alex@shazbot.org; Nicolin
>>>> Chen <nicolinc@nvidia.com>; Nathan Chen <nathanc@nvidia.com>; Matt
>>>> Ochs <mochs@nvidia.com>; Jiandi An <jan@nvidia.com>; Jason Gunthorpe
>>>> <jgg@nvidia.com>; jonathan.cameron@huawei.com;
>>>> zhangfei.gao@linaro.org; zhenzhong.duan@intel.com; Krishnakant Jaju
>>>> <kjaju@nvidia.com>; phrdina@redhat.com
>>>> Subject: Re: [PATCH v3 17/32] hw/arm/tegra241-cmdqv: mmap VINTF
>> Page0
>>>> for CMDQV
>>>>
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 2/26/26 11:50 AM, Shameer Kolothum wrote:
>>>>> From: Nicolin Chen <nicolinc@nvidia.com>
>>>>>
>>>>> Global VCMDQ pages provide a VM wide view of all VCMDQs, while the
>>>>> VINTF pages expose a logical view local to a given VINTF. Although real
>>>>> hardware may support multiple VINTFs, the kernel currently exposes a
>>>>> single VINTF per VM.
>>>>>
>>>>> The kernel provides an mmap offset for the VINTF Page0 region during
>>>>> vIOMMU allocation. However, the logical-to-physical association between
>>>>> VCMDQs and a VINTF is only established after HW_QUEUE allocation. Prior
>>>>> to that, the mapped Page0 does not back any real VCMDQ state.
>>>>>
>>>>> When VINTF is enabled, mmap the kernel provided Page0 region and
>>>>> unmap it when VINTF is disabled. This prepares the VINTF mapping
>>>>> in advance of subsequent patches that add VCMDQ allocation support.
>>>> So at some point we transition from something that is purely emulated
>>>> (page 1 global cmdq) to something that is mmapped on a host page. How
>> do
>>>> we transfer the state of the cmdq from one to the other?
>>> Right. If a guest uses both the "Global VCMDQ registers Page0" and the
>>> "VINTF0 Logical VCMDQ registers Page0" interchangeably (and I see
>>> nothing in the spec that forbids this), then we need to keep the two
>>> views in sync.
>> Also assuming the global VCMDQs are accessible and thus used, I guess we
>> shall properly handle the actual commands (equivalent of
>> smmuv3_cmdq_consume()), no?
>> I don't see it done at the moment.
>>
>> By the way, do we want support of VCMDQs in fully emulated mode. I guess
>> it would be sensible?
> Yes, command handling is not implemented now.
>
> Since this is part of the accelerated SMMUv3 feature, I am not sure it
> makes sense to implement full command handling in QEMU and then forward
> the supported invalidation commands back to the host again.
>
> The idea here is to accelerate the CMDQ processing through the hardware.
> I am not sure a guest would have a real use case for VCMDQ in a fully
> emulated mode.

OK but really the whole user model is really confusing. One starts using
a "global" vcmdq and then switches to a logical one.
On real HW I guess they are synced. But on emulation, currently they are
not. Also when the guest is using the global vcmdq, it can
access the registers but they actually do not trigger any commands. This
really needs to be clarified.

Then I understand we want to focus on accel mode but above points need
to be clatified. I just wanted to mention that vcmdq should logically
also work in fully emulated mode (and you seemed to implement a mix
actually when you have emulated access to vi vcmdq) as it replaces the
SMMU standard cmdq, if I understand correctly

Eric


>
> Thanks,
> Shameer
>
>> Thanks
>>
>> Eric
>>> So when the mapping between a global VCMDQ and a logical VCMDQ is
>>> created through the HW_QUEUE ioctl, we should sync the state between
>>> the Global VCMDQ Page0 and the VINTF0 Logical VCMDQ Page0.
>>>
>>> I will double check this.
>>>
>>> Thanks,
>>> Shameer
>>>
>>>> Thanks
>>>>
>>>> Eric
>>>>> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
>>>>> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
>>>>> ---
>>>>>  hw/arm/tegra241-cmdqv.h |  3 +++
>>>>>  hw/arm/tegra241-cmdqv.c | 44
>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>>  2 files changed, 45 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/arm/tegra241-cmdqv.h b/hw/arm/tegra241-cmdqv.h
>>>>> index d379b8860c..3ce9f539ae 100644
>>>>> --- a/hw/arm/tegra241-cmdqv.h
>>>>> +++ b/hw/arm/tegra241-cmdqv.h
>>>>> @@ -18,6 +18,8 @@
>>>>>  #define TEGRA241_CMDQV_MAX_CMDQ            (1U <<
>>>> TEGRA241_CMDQV_NUM_CMDQ_LOG2)
>>>>>  #define TEGRA241_CMDQV_NUM_SID_PER_VM_LOG2 4
>>>>>
>>>>> +#define VINTF_PAGE_SIZE 0x10000
>>>>> +
>>>>>  /*
>>>>>   * Tegra241 CMDQV MMIO layout (64KB pages)
>>>>>   *
>>>>> @@ -34,6 +36,7 @@ typedef struct Tegra241CMDQV {
>>>>>      SMMUv3AccelState *s_accel;
>>>>>      MemoryRegion mmio_cmdqv;
>>>>>      qemu_irq irq;
>>>>> +    void *vintf_page0;
>>>>>
>>>>>      /* Register Cache */
>>>>>      uint32_t config;
>>>>> diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
>>>>> index e1f1562c44..a3767a85a3 100644
>>>>> --- a/hw/arm/tegra241-cmdqv.c
>>>>> +++ b/hw/arm/tegra241-cmdqv.c
>>>>> @@ -151,6 +151,39 @@ static uint64_t tegra241_cmdqv_read(void
>>>> *opaque, hwaddr offset, unsigned size)
>>>>>      }
>>>>>  }
>>>>>
>>>>> +static bool
>>>>> +tegra241_cmdqv_munmap_vintf_page0(Tegra241CMDQV *cmdqv,
>> Error
>>>> **errp)
>>>>> +{
>>>>> +    if (!cmdqv->vintf_page0) {
>>>>> +        return true;
>>>>> +    }
>>>>> +
>>>>> +    if (munmap(cmdqv->vintf_page0, VINTF_PAGE_SIZE) < 0) {
>>>>> +        error_setg_errno(errp, errno, "Failed to unmap VINTF page0");
>>>>> +        return false;
>>>>> +    }
>>>>> +    cmdqv->vintf_page0 = NULL;
>>>>> +    return true;
>>>>> +}
>>>>> +
>>>>> +static bool tegra241_cmdqv_mmap_vintf_page0(Tegra241CMDQV
>>>> *cmdqv, Error **errp)
>>>>> +{
>>>>> +    IOMMUFDViommu *viommu = cmdqv->s_accel->viommu;
>>>>> +
>>>>> +    if (cmdqv->vintf_page0) {
>>>>> +        return true;
>>>>> +    }
>>>>> +
>>>>> +    if (!iommufd_backend_viommu_mmap(viommu->iommufd, viommu-
>>>>> viommu_id,
>>>>> +                                     VINTF_PAGE_SIZE,
>>>>> +                                     cmdqv->cmdqv_data.out_vintf_mmap_offset,
>>>>> +                                     &cmdqv->vintf_page0, errp)) {
>>>>> +        return false;
>>>>> +    }
>>>>> +
>>>>> +    return true;
>>>>> +}
>>>>> +
>>>>>  /*
>>>>>   * Write a VCMDQ register using VCMDQ0_* offsets.
>>>>>   *
>>>>> @@ -216,7 +249,7 @@
>> tegra241_cmdqv_write_vcmdq(Tegra241CMDQV
>>>> *cmdqv, hwaddr offset0, int index,
>>>>>  }
>>>>>
>>>>>  static void tegra241_cmdqv_write_vintf(Tegra241CMDQV *cmdqv,
>> hwaddr
>>>> offset,
>>>>> -                                       uint64_t value)
>>>>> +                                       uint64_t value, Error **errp)
>>>>>  {
>>>>>      int i;
>>>>>
>>>>> @@ -227,8 +260,10 @@ static void
>>>> tegra241_cmdqv_write_vintf(Tegra241CMDQV *cmdqv, hwaddr offset,
>>>>>          cmdqv->vintf_config = value;
>>>>>          if (value & R_VINTF0_CONFIG_ENABLE_MASK) {
>>>>> +            tegra241_cmdqv_mmap_vintf_page0(cmdqv, errp);
>>>>>              cmdqv->vintf_status |= R_VINTF0_STATUS_ENABLE_OK_MASK;
>>>>>          } else {
>>>>> +            tegra241_cmdqv_munmap_vintf_page0(cmdqv, errp);
>>>>>              cmdqv->vintf_status &= ~R_VINTF0_STATUS_ENABLE_OK_MASK;
>>>>>          }
>>>>>          break;
>>>>> @@ -251,6 +286,7 @@ static void tegra241_cmdqv_write(void *opaque,
>>>> hwaddr offset, uint64_t value,
>>>>>                                   unsigned size)
>>>>>  {
>>>>>      Tegra241CMDQV *cmdqv = (Tegra241CMDQV *)opaque;
>>>>> +    Error *local_err = NULL;
>>>>>      int index;
>>>>>
>>>>>      if (offset >= TEGRA241_CMDQV_IO_LEN) {
>>>>> @@ -276,7 +312,7 @@ static void tegra241_cmdqv_write(void *opaque,
>>>> hwaddr offset, uint64_t value,
>>>>>          cmdqv->cmdq_alloc_map[(offset - A_CMDQ_ALLOC_MAP_0) / 4] =
>>>> value;
>>>>>          break;
>>>>>      case A_VINTF0_CONFIG ... A_VINTF0_LVCMDQ_ERR_MAP_3:
>>>>> -        tegra241_cmdqv_write_vintf(cmdqv, offset, value);
>>>>> +        tegra241_cmdqv_write_vintf(cmdqv, offset, value, &local_err);
>>>>>          break;
>>>>>      case A_VI_VCMDQ0_CONS_INDX ... A_VI_VCMDQ1_GERRORN:
>>>>>          /* Same decoding as read() case: See comments above */
>>>>> @@ -300,6 +336,10 @@ static void tegra241_cmdqv_write(void
>> *opaque,
>>>> hwaddr offset, uint64_t value,
>>>>>          qemu_log_mask(LOG_UNIMP, "%s unhandled write access at 0x%"
>>>> PRIx64 "\n",
>>>>>                        __func__, offset);
>>>>>      }
>>>>> +
>>>>> +    if (local_err) {
>>>>> +        error_report_err(local_err);
>>>>> +    }
>>>>>  }
>>>>>
>>>>>  static void tegra241_cmdqv_free_viommu(SMMUv3State *s)