On 6/4/26 11:26 AM, Shameer Kolothum Thodi wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Eric Auger <[email protected]>
>> Sent: 04 June 2026 10:01
>> 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 v6 17/31] hw/arm/tegra241-cmdqv: mmap VINTF Page0
>> for CMDQV
>>
>> External email: Use caution opening links or attachments
>>
>>
>> Hi Shameer,
>>
>> On 6/1/26 1:42 PM, Shameer Kolothum wrote:
>>> From: Nicolin Chen <[email protected]>
>>>
>>> The kernel currently exposes a single VINTF per emulated SMMUv3
>>> instance. IOMMU_VIOMMU_ALLOC returns an mmap offset for the host
>>> VINTF Page0 allocated for this SMMU. However, VCMDQs only become
>>> bound to that VINTF after IOMMU_HW_QUEUE_ALLOC, so until then the
>>> mapped Page0 does not back any real VCMDQ state.
>>>
>>> When VINTF is enabled, mmap the kernel-provided Page0 region and set
>>> STATUS.ENABLE_OK only if the mmap succeeds; unmap it when VINTF is
>>> disabled. This prepares the VINTF mapping in advance of subsequent
>>> patches that add VCMDQ allocation and the guest MMIO install.
>>>
>>> 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.h | 3 +++
>>> hw/arm/tegra241-cmdqv.c | 50
>> ++++++++++++++++++++++++++++++++++++++---
>>> 2 files changed, 50 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/arm/tegra241-cmdqv.h b/hw/arm/tegra241-cmdqv.h
>>> index ad7fb8725f..c4d327a9a5 100644
>>> --- a/hw/arm/tegra241-cmdqv.h
>>> +++ b/hw/arm/tegra241-cmdqv.h
>>> @@ -37,6 +37,8 @@
>>> #define CMDQV_VINTF_PAGE1_BASE 0x40000
>>> #define CMDQV_VCMDQ_STRIDE 0x80
>>>
>>> +#define VINTF_PAGE_SIZE 0x10000
>>> +
>>> struct iommu_viommu_tegra241_cmdqv;
>>>
>>> typedef struct Tegra241CMDQV {
>>> @@ -45,6 +47,7 @@ typedef struct Tegra241CMDQV {
>>> MemoryRegion mmio_cmdqv;
>>> qemu_irq irq;
>>> IOMMUFDVeventq *veventq;
>>> + void *vintf_page0;
>>>
>>> /* CMDQ-V Config page register cache */
>>> uint32_t config;
>>> diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
>>> index 3cb1b45575..a52e153501 100644
>>> --- a/hw/arm/tegra241-cmdqv.c
>>> +++ b/hw/arm/tegra241-cmdqv.c
>>> @@ -9,6 +9,8 @@
>>>
>>> #include "qemu/osdep.h"
>>> #include "qemu/log.h"
>>> +#include "qemu/error-report.h"
>>> +#include "qapi/error.h"
>>>
>>> #include "hw/arm/smmuv3.h"
>>> #include "hw/arm/smmuv3-common.h"
>>> @@ -218,8 +220,42 @@ static void
>> tegra241_cmdqv_write_vcmdq_page1_64(Tegra241CMDQV *cmdqv,
>>> offset0, value);
>>> }
>>>
>>> +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;
>>> +}
>>> +
>>> static void tegra241_cmdqv_config_vintf_write(Tegra241CMDQV *cmdqv,
>>> - hwaddr offset, uint64_t
>>> value)
>>> + hwaddr offset, uint64_t
>>> value,
>>> + Error **errp)
>>> {
>>> int i;
>>>
>>> @@ -234,8 +270,11 @@ static void
>> tegra241_cmdqv_config_vintf_write(Tegra241CMDQV *cmdqv,
>>> cmdqv->vintf_config = value;
>>> if (value & R_VINTF0_CONFIG_ENABLE_MASK) {
>>> - cmdqv->vintf_status |= R_VINTF0_STATUS_ENABLE_OK_MASK;
>>> + if (tegra241_cmdqv_mmap_vintf_page0(cmdqv, errp)) {
>>> + cmdqv->vintf_status |= R_VINTF0_STATUS_ENABLE_OK_MASK;
>>> + }
>>> } else {
>>> + tegra241_cmdqv_munmap_vintf_page0(cmdqv, errp);
>> the munmap is only done upon guest action in this patch. Later on, it is
>> also done on reset (in smmuv3_accel_reset called on smmu_reset_exit
>> called in exit phase). So I think we are good overall but it is a bit
>> annoying to leave it done only upon guest action in this patch. What do
>> you think?
> Though I didn’t really get the concern here, there is a suggestion to
> move the mmap just after IOMMU_VIOMMU_ALLOC as it doesn't
> need to be tied to VINTF enable.
>
> https://lore.kernel.org/qemu-devel/[email protected]/T/#m972247a4652b09e3d86def921d9a8551b7620341
>
> If we do that it will keep mapped until VIOMMU is alive.
> Does that address the concern above?
my concern is the munmap shall be eventually done on exit even if the
guest does not issue the associated command, no? This is done later in
the patch that introduces the reset. My concern is about patch
sequencing. I think this is unrelated to Nicolin's suggestion.
Thanks
Eric
>
> Thanks,
> Shameer
>
>
>
>
