On 26.03.19 15:08, Igor Mammedov wrote:
> On Tue, 26 Mar 2019 14:50:58 +1100
> David Gibson <da...@gibson.dropbear.id.au> wrote:
>
>> qemu_getrampagesize() works out the minimum host page size backing any of
>> guest RAM. This is required in a few places, such as for POWER8 PAPR KVM
>> guests, because limitations of the hardware virtualization mean the guest
>> can't use pagesizes larger than the host pages backing its memory.
>>
>> However, it currently checks against *every* memory backend, whether or not
>> it is actually mapped into guest memory at the moment. This is incorrect.
>>
>> This can cause a problem attempting to add memory to a POWER8 pseries KVM
>> guest which is configured to allow hugepages in the guest (e.g.
>> -machine cap-hpt-max-page-size=16m). If you attempt to add non-hugepage,
>> you can (correctly) create a memory backend, however it (correctly) will
>> throw an error when you attempt to map that memory into the guest by
>> 'device_add'ing a pc-dimm.
>>
>> What's not correct is that if you then reset the guest a startup check
>> against qemu_getrampagesize() will cause a fatal error because of the new
>> memory object, even though it's not mapped into the guest.
> I'd say that backend should be remove by mgmt app since device_add failed
> instead of leaving it to hang around. (but fatal error either not a nice
> behavior on QEMU part)
Indeed, it should be removed. Depending on the options (huge pages with
prealloc?) memory might be consumed for unused memory. Undesired.
>
>>
>> This patch corrects the problem by adjusting find_max_supported_pagesize()
>> (called from qemu_getrampagesize() via object_child_foreach) to exclude
>> non-mapped memory backends.
> I'm not sure about if it's ok do so. It depends on where from
> qemu_getrampagesize() is called. For example s390 calls it rather early
> when initializing KVM, so there isn't anything mapped yet.
>
> And once I replace -mem-path with hostmem backend and drop
> qemu_mempath_getpagesize(mem_path) /which btw aren't guarantied to be mapped
> either/
> this patch might lead to incorrect results for initial memory as well.
>
>>
>> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
>> ---
>> exec.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> This is definitely a bug, but it's not a regression. I'm not sure if
>> this is 4.0 material at this stage of the freeze or not.
>>
>> diff --git a/exec.c b/exec.c
>> index 86a38d3b3b..6ab62f4eee 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -1692,9 +1692,10 @@ static int find_max_supported_pagesize(Object *obj,
>> void *opaque)
>> long *hpsize_min = opaque;
>>
>> if (object_dynamic_cast(obj, TYPE_MEMORY_BACKEND)) {
>> - long hpsize = host_memory_backend_pagesize(MEMORY_BACKEND(obj));
>> + HostMemoryBackend *backend = MEMORY_BACKEND(obj);
>> + long hpsize = host_memory_backend_pagesize(backend);
>>
>> - if (hpsize < *hpsize_min) {
>> + if (host_memory_backend_is_mapped(backend) && (hpsize <
>> *hpsize_min)) {
>> *hpsize_min = hpsize;
>> }
>> }
>
>
--
Thanks,
David / dhildenb
