Hi Shameer, On 2/6/26 3:48 PM, Shameer Kolothum wrote: > From: Nicolin Chen <[email protected]> > > Tegra241 CMDQV extends SMMUv3 with support for virtual command queues > (VCMDQs) exposed via a CMDQV MMIO region. The CMDQV MMIO space is split > into 64KB pages as follows: > > 0x00000: Global CMDQV registers > 0x10000: Global VCMDQ registers, Page0 (control/status) > 0x20000: Global VCMDQ registers, Page1 (configuration) > 0x30000: VINTF0 logical VCMDQ registers, Page0 (control/status) > 0x40000: VINTF0 logical VCMDQ registers, Page1 (configuration) this shall rather be introduced along with the whole mmio region. In this patch you focus on rather VINTF0 page0 > > 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. Maybe you shall simply name it VINTF > > 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. > > Prepare the VINTF0 Page0 mapping by mmapping the kernel provided region > when CMDQV is accessed, while keeping all CMDQV MMIO accesses trapped > in QEMU. The mapping is not yet exposed to the guest address space. > A subsequent patch will install the RAM backed MMIO subregion for VINTF > page0 once HW_QUEUE allocation completes.
So we are likely to mmap a page whose content is undefined until HW_QUEUE alloc, is that correct? In that case, why don't we do that only after HW_QUEUE alloc? Please can you clarify? > > Signed-off-by: Nicolin Chen <[email protected]> > Signed-off-by: Shameer Kolothum <[email protected]> > --- > hw/arm/tegra241-cmdqv.c | 38 ++++++++++++++++++++++++++++++++++++++ > hw/arm/tegra241-cmdqv.h | 3 +++ > 2 files changed, 41 insertions(+) > > diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c > index 596b1c5595..97c9b9c8dc 100644 > --- a/hw/arm/tegra241-cmdqv.c > +++ b/hw/arm/tegra241-cmdqv.c > @@ -13,14 +13,52 @@ > #include "smmuv3-accel.h" > #include "tegra241-cmdqv.h" > > +static bool tegra241_cmdqv_mmap_vintf_page0(Tegra241CMDQV *cmdqv, Error > **errp) > +{ > + IOMMUFDViommu *viommu = cmdqv->s_accel->viommu; > + > + if (!viommu) { > + return true; > + } > + > + g_assert(!cmdqv->vintf_page0); > + if (!iommufd_backend_viommu_mmap(viommu->iommufd, viommu->viommu_id, > + VINTF_REG_PAGE_SIZE, > + cmdqv->cmdqv_data.out_vintf_mmap_offset, > + &cmdqv->vintf_page0, errp)) { So I would rather directly implement iommufd_backend_viommu_mmap() here. > + return false; > + } > + > + return true; > +} > + > static uint64_t tegra241_cmdqv_read(void *opaque, hwaddr offset, unsigned > size) > { > + Tegra241CMDQV *cmdqv = (Tegra241CMDQV *)opaque; > + Error *local_err = NULL; init is not needed since you don't test it. > + > + if (!cmdqv->vintf_page0) { > + if (!tegra241_cmdqv_mmap_vintf_page0(cmdqv, &local_err)) { > + error_report_err(local_err); > + local_err = NULL; not needed either > + } > + } > + > return 0; > } > > static void tegra241_cmdqv_write(void *opaque, hwaddr offset, uint64_t value, > unsigned size) > { > + Tegra241CMDQV *cmdqv = (Tegra241CMDQV *)opaque; > + Error *local_err = NULL; same here > + > + if (!cmdqv->vintf_page0) { > + if (!tegra241_cmdqv_mmap_vintf_page0(cmdqv, &local_err)) { > + error_report_err(local_err); > + local_err = NULL; > + } > + } > } > > static void tegra241_cmdqv_free_veventq(SMMUv3State *s) > diff --git a/hw/arm/tegra241-cmdqv.h b/hw/arm/tegra241-cmdqv.h > index 6ea0087f61..94bef8c978 100644 > --- a/hw/arm/tegra241-cmdqv.h > +++ b/hw/arm/tegra241-cmdqv.h > @@ -29,8 +29,11 @@ typedef struct Tegra241CMDQV { > SMMUv3AccelState *s_accel; > MemoryRegion mmio_cmdqv; > qemu_irq irq; > + void *vintf_page0; > } Tegra241CMDQV; > > +#define VINTF_REG_PAGE_SIZE 0x10000 can't we rename that into VINTF_SIZE. REG does not mean much. Before you talked about Control/Status vs Configuration. Besides, I am not sure having a define brings much in that case. Thanks Eric > + > #ifdef CONFIG_TEGRA241_CMDQV > const SMMUv3AccelCmdqvOps *tegra241_cmdqv_ops(void); > #else
