Hi Eric, > -----Original Message----- > From: Eric Auger <[email protected]> > Sent: 06 May 2026 13:45 > 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]; Krishnakant Jaju <[email protected]>; > [email protected] > Subject: Re: [PATCH v4 22/31] hw/arm/tegra241-cmdqv: Map VINTF page0 > into guest MMIO space > > External email: Use caution opening links or attachments > > > Hi Shameer, > > On 4/15/26 12:55 PM, Shameer Kolothum wrote: > > From: Nicolin Chen <[email protected]> > > > > Once a VCMDQ is allocated, map the mmap'd vintf_page0 region directly > > into the guest-visible MMIO space at offset 0x30000 as a RAM-backed > > MemoryRegion. This eliminates QEMU trapping for hot-path CONS/PROD > > index updates. > > > > After this patch, the two VCMDQ apertures use different access paths: > > the direct aperture (0x10000) remains QEMU-trapped and writes via > > vintf_ptr, while the VI aperture (0x30000) is a direct guest RAM > > mapping. Both paths write to the same underlying vintf_page0 memory, > > so no synchronisation between the apertures is needed. > > I fail to understand when the previous trapped path using ptr in > tegra241_cmdqv_read/write_vcmdq gets used versus that path. Is it > eventually used?
The spec does not prevent a guest from using the 0x10000 path for allocated VCMDQs, so the trap path remains valid and QEMU forwards those accesses to the mmap'd vintf_page0 via vintf_ptr. We cannot map 0x10000 directly to the guest as RAM because the kernel mmap only backs VCMDQs actually allocated via HW_QUEUE ioctl. If the guest allocates only 1 of 2 VCMDQs, exposing the full direct aperture page0 as RAM would give the guest access to unallocated VCMDQ slots. Hence, it remains trapped and QEMU only fwds to vintf page0 for allocated queues. Thanks, Shameer > > > > The mapping is installed lazily on first successful VCMDQ hardware > > queue allocation and removed when CMDQV or VINTF is disabled. > > > > 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 | 1 + > > hw/arm/tegra241-cmdqv.c | 37 > +++++++++++++++++++++++++++++++++++++ > > 2 files changed, 38 insertions(+) > > > > diff --git a/hw/arm/tegra241-cmdqv.h b/hw/arm/tegra241-cmdqv.h > > index 039d86374f..2befa6205e 100644 > > --- a/hw/arm/tegra241-cmdqv.h > > +++ b/hw/arm/tegra241-cmdqv.h > > @@ -46,6 +46,7 @@ typedef struct Tegra241CMDQV { > > IOMMUFDVeventq *veventq; > > IOMMUFDHWqueue *vcmdq[TEGRA241_CMDQV_MAX_CMDQ]; > > void *vintf_page0; > > + MemoryRegion *mr_vintf_page0; > > > > /* Register Cache */ > > uint32_t config; > > diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c > > index eb619e1134..bf989dd51f 100644 > > --- a/hw/arm/tegra241-cmdqv.c > > +++ b/hw/arm/tegra241-cmdqv.c > > @@ -15,6 +15,40 @@ > > #include "tegra241-cmdqv.h" > > #include "trace.h" > > > > +static void tegra241_cmdqv_guest_unmap_vintf_page0(Tegra241CMDQV > *cmdqv) > > +{ > > + if (!cmdqv->mr_vintf_page0) { > > + return; > > + } > > + > > + memory_region_del_subregion(&cmdqv->mmio_cmdqv, cmdqv- > >mr_vintf_page0); > > + object_unparent(OBJECT(cmdqv->mr_vintf_page0)); > > + g_free(cmdqv->mr_vintf_page0); > > + cmdqv->mr_vintf_page0 = NULL; > > +} > > + > > +static void tegra241_cmdqv_guest_map_vintf_page0(Tegra241CMDQV > *cmdqv) > > +{ > > + char *name; > > + > > + if (cmdqv->mr_vintf_page0) { > > + return; > > + } > > + > > + name = g_strdup_printf("%s vintf-page0", > > + memory_region_name(&cmdqv->mmio_cmdqv)); > > + cmdqv->mr_vintf_page0 = g_malloc0(sizeof(*cmdqv->mr_vintf_page0)); > > + memory_region_init_ram_device_ptr(cmdqv->mr_vintf_page0, > > + > > memory_region_owner(&cmdqv->mmio_cmdqv), > > + name, VINTF_PAGE_SIZE, > > + cmdqv->vintf_page0); > > + cmdqv->mr_vintf_page0->ram_device_skip_iommu_map = true; > I guess you need a setter, here to. > > + memory_region_add_subregion_overlap(&cmdqv->mmio_cmdqv, > > + CMDQV_VINTF_PAGE0_BASE, > > + cmdqv->mr_vintf_page0, 1); > > + g_free(name); > > +} > > + > > static void tegra241_cmdqv_free_vcmdq(Tegra241CMDQV *cmdqv, int > index) > > { > > IOMMUFDViommu *viommu = cmdqv->s_accel->viommu; > > @@ -72,6 +106,7 @@ static bool > tegra241_cmdqv_setup_vcmdq(Tegra241CMDQV *cmdqv, int index, > > hw_queue->viommu = viommu; > > cmdqv->vcmdq[index] = hw_queue; > > > > + tegra241_cmdqv_guest_map_vintf_page0(cmdqv); > > return true; > > } > > > > @@ -312,6 +347,7 @@ static void > tegra241_cmdqv_config_vintf_write(Tegra241CMDQV *cmdqv, > > cmdqv->vintf_status |= R_VINTF0_STATUS_ENABLE_OK_MASK; > > } > > } else { > > + tegra241_cmdqv_guest_unmap_vintf_page0(cmdqv); > > tegra241_cmdqv_free_all_vcmdq(cmdqv); > > tegra241_cmdqv_munmap_vintf_page0(cmdqv, errp); > > cmdqv->vintf_status &= ~R_VINTF0_STATUS_ENABLE_OK_MASK; > > @@ -438,6 +474,7 @@ static void tegra241_cmdqv_write_mmio(void > *opaque, hwaddr offset, > > if (value & R_CONFIG_CMDQV_EN_MASK) { > > cmdqv->status |= R_STATUS_CMDQV_ENABLED_MASK; > > } else { > > + tegra241_cmdqv_guest_unmap_vintf_page0(cmdqv); > > tegra241_cmdqv_free_all_vcmdq(cmdqv); > > cmdqv->status &= ~R_STATUS_CMDQV_ENABLED_MASK; > > } > Thanks > > Eric
