Re: [Qemu-devel] [PATCH] s390x: remove direct reference to mem_path global form s90x code

[David Hildenbrand]

On 25.01.19 11:40, Igor Mammedov wrote:
> On Fri, 25 Jan 2019 09:03:49 +0100
> David Hildenbrand <address@hidden> wrote:
> 
>> On 24.01.19 17:57, Igor Mammedov wrote:
>>> I plan to deprecate -mem-path option and replace it with memory-backend,
>>> for that it's necessary to get rid of mem_path global variable.
>>> Do it for s390x case, replacing it with alternative way to enable
>>> 1Mb hugepages capability.
>>>
>>> Signed-off-by: Igor Mammedov <address@hidden>
>>> ---
>>> PS:
>>> Original code nor the new one probably is not entirely correct when
>>> huge pages are enabled in case where mixed initial RAM and memory
>>> backends are used, backend's page size might not match initial RAM's
>>> so I'm not sure if enabling 1MB cap is correct in this case on s390
>>> (should it be the same for all RAM???).
>>> With new approach 1Mb cap is not enabled if the smallest page size
>>> is not 1Mb.  
>>
>> There is no memory hotplug (DIMM/NVDIMM), so there really only is
>> initial memory.
> Ok, but what about coming up virtio-mem?

virtio-mem will at least initially only support !huge pages. But
longterm it is indeed possible.

But hotplugging will require runtime checks (if 1mb was not enabled, we
must not allow hotplugging 1mb via any mechanism). We can do such checks
in the machine hotplug handler than.

> 
> 
>>> ---
>>>  target/s390x/kvm.c | 37 ++++++++++++++++---------------------
>>>  1 file changed, 16 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>> index 2ebf26a..22e868a 100644
>>> --- a/target/s390x/kvm.c
>>> +++ b/target/s390x/kvm.c
>>> @@ -285,33 +285,28 @@ void kvm_s390_crypto_reset(void)
>>>      }
>>>  }
>>>  
>>> -static int kvm_s390_configure_mempath_backing(KVMState *s)
>>> +static int kvm_s390_configure_hugepage_backing(KVMState *s)
>>>  {
>>> -    size_t path_psize = qemu_mempath_getpagesize(mem_path);
>>> +    size_t psize = qemu_getrampagesize();
>>>  
>>> -    if (path_psize == 4 * KiB) {  
>>
>> if you keep this (modified) check you have to do minimal changes in the
>> code below. (e.g. not indent error messages)
> Do you mean to keep this function as is and only 
>  s/qemu_mempath_getpagesize(mem_path)/qemu_getrampagesize()/

Basically yes, but whatever you prefer.

Thanks!

-- 

Thanks,

David / dhildenb