On 3/11/26 10:26 AM, Shameer Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Eric Auger <[email protected]>
>> Sent: 11 March 2026 07:55
>> 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];
>> [email protected]; [email protected]; Krishnakant Jaju
>> <[email protected]>; [email protected]
>> 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 <[email protected]>
>>>
>>> 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?
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 <[email protected]>
>>> Signed-off-by: Shameer Kolothum <[email protected]>
>>> ---
>>> 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)
