On 3/9/26 12:31 PM, Shameer Kolothum Thodi wrote:
>
>> -----Original Message-----
>> From: Eric Auger <[email protected]>
>> Sent: 09 March 2026 11:09
>> 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 13/32] hw/arm/tegra241-cmdqv: Implement CMDQV
>> vIOMMU alloc/free
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 2/26/26 11:50 AM, Shameer Kolothum wrote:
>>> From: Nicolin Chen <[email protected]>
>>>
>>> Replace the stub implementation with real vIOMMU allocation for
>>> Tegra241 CMDQV.
>>>
>>> Free the vIOMMU ID on teardown.
>>>
>>> Signed-off-by: Nicolin Chen <[email protected]>
>>> Signed-off-by: Shameer Kolothum <[email protected]>
>>> ---
>>> hw/arm/tegra241-cmdqv.c | 19 +++++++++++++++++--
>>> 1 file changed, 17 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c index
>>> 6959766129..d487612ba2 100644
>>> --- a/hw/arm/tegra241-cmdqv.c
>>> +++ b/hw/arm/tegra241-cmdqv.c
>>> @@ -25,14 +25,29 @@ static void tegra241_cmdqv_write(void *opaque,
>>> hwaddr offset, uint64_t value,
>>>
>>> static void tegra241_cmdqv_free_viommu(SMMUv3State *s) {
>>> + SMMUv3AccelState *accel = s->s_accel;
>>> + IOMMUFDViommu *viommu = accel->viommu;
>>> +
>>> + if (!viommu) {
>>> + return;
>>> + }
>>> + iommufd_backend_free_id(viommu->iommufd, viommu->viommu_id);
>>> }
>>>
>>> static bool
>>> tegra241_cmdqv_alloc_viommu(SMMUv3State *s,
>> HostIOMMUDeviceIOMMUFD *idev,
>>> uint32_t *out_viommu_id, Error **errp) {
>>> - error_setg(errp, "NVIDIA Tegra241 CMDQV is unsupported");
>>> - return false;
>>> + Tegra241CMDQV *cmdqv = s->s_accel->cmdqv;
>>> +
>>> + if (!iommufd_backend_alloc_viommu(idev->iommufd, idev->devid,
>>> +
>>> + IOMMU_VIOMMU_TYPE_TEGRA241_CMDQV,
>> if the only think that differs compared to no cmdq is the type, ie.
>>
>> IOMMU_VIOMMU_TYPE_TEGRA241_CMDQV vs
>> IOMMU_VIOMMU_TYPE_TEGRA241_CMDQV
>>
>> + passing true args, maybe you can just record the type of the cmdqv
>> + that is being used in the smmu_accel device. Then you can get rid of
>> + alloc and free
> It is not just the type. Based on the type we also need to pass,
>
> * @data_len: Length of the type specific data
> * @__reserved: Must be 0
> * @data_uptr: User pointer to a driver-specific virtual IOMMU data
> *
>
> And the above is implementation specific.
>
> If our idea of the "ops" is to allow easier support for different
> implementations in future, it probably makes sense to keep this.
OK I missed the fact data_len and IOMMU data needed specialization.
Thanks
Eric
>
> Thanks,
> Shameer
>
>> Eric
>>
>>
>>> + idev->hwpt_id, &cmdqv->cmdqv_data,
>>> + sizeof(cmdqv->cmdqv_data),
>>> out_viommu_id,
>>> + errp)) {
>>> + return false;
>>> + }
>>> + return true;
>>> }
>>>
>>> static void tegra241_cmdqv_reset(SMMUv3State *s)
