On 21.09.21 10:56, Michal Prívozník wrote:
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?
I think it might make sense to print a warning if we cannot support
prealloc and it was requested. We can then skip the warning once
virtio-mem supports the "prealloc" property that we can just enable.
Using hugepages without prealloc is in general unsafe, so I don't think
we need a virtio-mem specific warning. :)
Thanks!
--
Thanks,
David / dhildenb
