> On Sep 14, 2026, at 11:14, Shameer Kolothum Thodi <[email protected]> > wrote: >> -----Original Message----- >> From: Matt Ochs <[email protected]> >> Sent: 14 September 2026 16:56 >> To: Peter Maydell <[email protected]> >> Cc: [email protected]; [email protected]; Eric Auger >> <[email protected]>; Nicolin Chen <[email protected]>; Shameer >> Kolothum Thodi <[email protected]>; [email protected] >> Subject: Re: [PATCH v2] hw/arm/tegra241-cmdqv: Keep VINTF page0 region >> alive >> >> >>> On Sep 14, 2026, at 07:29, Peter Maydell <[email protected]> >> wrote: >>> On Fri, 11 Sept 2026 at 20:04, Matthew R. Ochs <[email protected]> >> wrote: >>>> >>>> >>>> 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_init_ram_device_ptr(&cmdqv->mr_vintf_page0, >>>> >>>> memory_region_owner(&cmdqv->mmio_cmdqv), >>>> name, VINTF_PAGE_SIZE, >>>> cmdqv->vintf_page0); >>> >>> ...but we don't actually initialize the MemoryRegion until here. >>> >>> I don't think you should assume that memory_region_* functions >>> will do anything sensible on a zeroed-out lump of memory. >> >> You're right. Although the current implementation of >> memory_region_is_mapped() only examines fields that are zero before >> initialization, using it here relies on the internal representation of >> an uninitialized MemoryRegion. >> >> memory_region_init_ram_device_ptr() has not yet called >> object_initialize() on the embedded region at that point. I'll restore >> the explicit initialization flag from v1 in v3 so that no MemoryRegion >> API is called before the region has been initialized. > > Or we could avoid the check altogether by tying the MemoryRegion lifetime > to the vintf_page0 mmap. > > - Move the initialization of mr_vintf_page0 to > tegra241_cmdqv_alloc_viommu(), just before the successful return. > Add it as a subregion, but disabled with memory_region_set_enabled(mr, > false). > > - tegra241_cmdqv_guest_map_vintf_page0() and > tegra241_cmdqv_guest_unmap_vintf_page0() then become just > memory_region_set_enabled(&cmdqv->mr_vintf_page0, true/false). > > - In tegra241_cmdqv_free_viommu(), remove the subregion and unparent > the region before the munmap. > > This way we tie the MemoryRegion lifetime to the vintf_page0 > mmap/munmap, and on VINTF0_CONFIG.ENABLE set/clear we just toggle > memory_region_set_enabled().
Thanks, this approach makes sense to me and addresses Peter's concern without requiring a separate initialization flag. I have reworked the patch as suggested. The VINTF page0 MemoryRegion is initialized and added disabled after successful CMDQV vIOMMU allocation. Guest enable, disable, and reset now only toggle the region's enabled state. The allocation-unwind path removes and unparents the still-disabled region before releasing its mmap. I will send this implementation as v3 shortly. -matt
