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

[David Hildenbrand]

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