Re: [PATCH v5 05/16] qemu: Build command line for virtio-mem

On 9/17/21 2:54 PM, David Hildenbrand wrote: > On 15.09.21 13:07, Michal Prívozník wrote: >> On 9/14/21 3:05 PM, David Hildenbrand wrote: >>> On 13.09.21 16:52, Michal Privoznik wrote: >>>> Nothing special is happening here. All important changes were >>>> done when for 'virtio-pmem' (adjusting the code to put virtio >>>> memory on PCI bus, generating alias using >>>> qemuDomainDeviceAliasIndex(). The only bit that might look >>>> suspicious is no prealloc for virtio-mem. But if you think about >>>> it, the whole purpose of this device is to change amount of >>>> memory exposed to guest on the fly. There is no point in locking >>>> the whole backend in memory. >>> >>> Do I understand correctly that we'll always try setting "reserve=off", >>> and "prealloc=off"? That would be awesome :) >> >> prealloc=off would be set (almost) unconditionally for all >> memory-backend-* objects that are associated with virtio-mem device. And >> if QEMU is new enough to have .reserve attribute then reserve=off is set >> too. >> > > Ah, perfect. > >>> >>> I do wonder if we want to warn or bail out if "priv->memPrealloc" is set >>> but we are temporarily not able to support it -- as discussed, because >>> virtio-mem in QEMU yet has to be taught about it. >> >> priv->memPrealloc might not be what you think it is. The fact whether >> immediate allocation was requested is stored in def->mem.allocation; and >> priv->memPrealloc is just there to track whether -mem-prealloc what >> already put onto (paritally) generated cmd line and thus the rest of >> generators must refrain from setting prealloc=on. >> >> Codewise speaking, if you'd look at qemuBuildCommandLine() (this is >> where cmd line generation happens) then you'd see >> qemuBuildMemCommandLine() being called first and only after that >> qemuBuildMemoryDeviceCommandLine() being called second. >> >> The former is responsible for generating generic memory for guest (e.g. >> -m XX -mem-prealloc -mem-path ...  which is the old style, nowadays a >> combination of -machine memory-backend= + -object memory-backend-*  is >> generated). >> >> Long story short, if priv->memPrealloc isn't true at this point, then >> libvirt doesn't have to set prealloc=off explicitly, because off is the >> default. > > That makes sense. Yet I wonder if we should warn that preallocation is > currently not supported for virtio-mem and will be ignored. Your > decision :) We could do that, yes. Let me respin the series with that change - these patches don't apply cleanly anymore anyway. Should a similar warning be produced when hugepages are configured?