Hi Markus, On 05/07/19 20:01, Markus Armbruster wrote: > The subject is slightly misleading. Holes read as zero. So do > non-holes full of zeroes. The patch avoids reading the former, but > still reads the latter. > > Xiang Zheng <zhengxia...@huawei.com> writes: > >> Currently we fill the memory space with two 64MB NOR images when >> using persistent UEFI variables on virt board. Actually we only use >> a very small(non-zero) part of the memory while the rest significant >> large(zero) part of memory is wasted. > > Neglects to mention that the "virt board" is ARM. > >> So this patch checks the block status and only writes the non-zero part >> into memory. This requires pflash devices to use sparse files for >> backends. > > I started to draft an improved commit message, but then I realized this > patch can't work. > > The pflash_cfi01 device allocates its device memory like this: > > memory_region_init_rom_device( > &pfl->mem, OBJECT(dev), > &pflash_cfi01_ops, > pfl, > pfl->name, total_len, &local_err); > > pflash_cfi02 is similar. > > memory_region_init_rom_device() calls > memory_region_init_rom_device_nomigrate() calls qemu_ram_alloc() calls > qemu_ram_alloc_internal() calls g_malloc0(). Thus, all the device > memory gets written to even with this patch.
As far as I can see, qemu_ram_alloc_internal() calls g_malloc0() only to allocate the the new RAMBlock object called "new_block". The actual guest RAM allocation occurs inside ram_block_add(), which is also called by qemu_ram_alloc_internal(). One frame outwards the stack, qemu_ram_alloc() passes NULL to qemu_ram_alloc_internal(), for the 4th ("host") parameter. Therefore, in qemu_ram_alloc_internal(), we set "new_block->host" to NULL as well. Then in ram_block_add(), we take the (!new_block->host) branch, and call phys_mem_alloc(). Unfortunately, "phys_mem_alloc" is a function pointer, set with phys_mem_set_alloc(). The phys_mem_set_alloc() function is called from "target/s390x/kvm.c" (setting the function pointer to legacy_s390_alloc()), so it doesn't apply in this case. Therefore we end up calling the default qemu_anon_ram_alloc() function, through the funcptr. (I think anyway.) And qemu_anon_ram_alloc() boils down to mmap() + MAP_ANONYMOUS, in qemu_ram_mmap(). (Even on PPC64 hosts, because qemu_anon_ram_alloc() passes (-1) for "fd".) I may have missed something, of course -- I obviously didn't test it, just speculated from the source. Thanks Laszlo > > I'm afraid you neglected to test. > > I still believe this approach can be made to work. Need a replacement > for memory_region_init_rom_device() that uses mmap() with MAP_ANONYMOUS. >