Re: [Qemu-devel] [PATCH v1 00/11] pc-dimm: next bunch of cleanups

[David Hildenbrand]

On 13.06.2018 15:34, Igor Mammedov wrote:
> On Mon, 11 Jun 2018 14:16:44 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> This is another set of cleanups as the result from
>>     [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers
>> And is based on the two series
>>     [PATCH v1 0/2] memory: fix alignment checks/asserts
>>     [PATCH v2 0/6] spapr: machine hotplug handler cleanups
>>
>> These cleanup are the last step before factoring out the
>> pre_plug, plug and unplug logic of memory devices completely, to make it
>> more independent from pc-dimm.
> patches 1-4 are fine,
> the rest starting from 5/11 is based on wrong assumptions so should
> be reworked or dropped.

@Paolo can you pick up patch 1-4, so we have them out of the way (while
I potentially create new and more patches?)

> 
>> But these patches will already try to make an important point: Assigning
>> physical addresses of memory devices will not be done in pre_plug but
>> during plug. Other properties, like the "slot" property, however can be
>> handled during pre_plug and is done in this patch series, because they
>> don't realy on the device to be realized.
>>
>> Igor proposed to move all property checks + assigmnets to the pre_plug
>> stage. Hovewer for determining a physical address of a memory device, we
>> need to know the exact size and the alignment of the underlying memory
>> region.
>>
>> This region might not be available and initialized before the device has
>> been realized (e.g. for NVDIMM). So my point is: Accessing derived
>> "properties" ("memory region" set up based on "memdev" property and maybe
>> others e.g. for NVDIMM) via device class functions should not be done
>> before the device has been realized. These functions should not be
>> called during pre_plug.
> It's impl. issue/bug of NVDIMM where it does initialization late, which
> leads to access to uninitialized region in several places and we should fix.
> 
> There is nothing fundamental that prevents accessing size/memory of
> backend that was linked to dimm device at pre_plug() time,
> since all user specified properties are already set and it's time
> for machine to check if properties are correct from machine's pov
> and set its own values before proceeding to device.realize() and
> plug() handler. That includes setting default GPA property. 
> 
> Hence I'm not willing to agree going backwards or adding more
> code/refactoring based on broken behavior.
> 
>>
>> Enforcing this, already leads to sime nice cleanup numbers in pc-dimm
>> code.
>>
>> David Hildenbrand (11):
>>   pc-dimm: remove leftover "struct pc_dimms_capacity"
>>   nvdimm: no need to overwrite get_vmstate_memory_region()
>>   pc: factor out pc-dimm checks into pc_dimm_pre_plug()
>>   hostmem: drop error variable from host_memory_backend_get_memory()
>>   spapr: move memory hotplug size check into plug code
>>   pc-dimm: don't allow to access "size" before the device was realized
>>   pc-dimm: get_memory_region() can never fail
>>   pc-dimm: get_memory_region() will never return a NULL pointer
>>   pc-dimm: remove pc_dimm_get_vmstate_memory_region()
>>   pc-dimm: introduce and use pc_dimm_memory_pre_plug()
>>   pc-dimm: assign and verify the "slot" property during pre_plug
>>
>>  backends/hostmem.c       |  3 +-
>>  hw/i386/pc.c             | 53 ++++++++++++++------------
>>  hw/mem/nvdimm.c          | 12 ++----
>>  hw/mem/pc-dimm.c         | 82 +++++++++++++++-------------------------
>>  hw/misc/ivshmem.c        |  3 +-
>>  hw/ppc/spapr.c           | 36 +++++-------------
>>  include/hw/mem/pc-dimm.h |  6 ++-
>>  include/sysemu/hostmem.h |  3 +-
>>  numa.c                   |  3 +-
>>  9 files changed, 81 insertions(+), 120 deletions(-)
>>
> 


-- 

Thanks,

David / dhildenb