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? Thanks, Shameer
