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

David Hildenbrand posted 11 patches 5 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180611121655.19616-1-david@redhat.com
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test s390x passed
There is a newer version of this series
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(-)
[Qemu-devel] [PATCH v1 00/11] pc-dimm: next bunch of cleanups
Posted by David Hildenbrand 5 years, 9 months ago
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.

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.

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(-)

-- 
2.17.1


Re: [Qemu-devel] [PATCH v1 00/11] pc-dimm: next bunch of cleanups
Posted by Igor Mammedov 5 years, 9 months ago
On Mon, 11 Jun 2018 14:16:44 +0200
David Hildenbrand <david@redhat.com> 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.

> 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(-)
> 


Re: [Qemu-devel] [PATCH v1 00/11] pc-dimm: next bunch of cleanups
Posted by David Hildenbrand 5 years, 9 months ago
On 13.06.2018 15:34, Igor Mammedov wrote:
> On Mon, 11 Jun 2018 14:16:44 +0200
> David Hildenbrand <david@redhat.com> 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

Re: [Qemu-devel] [PATCH v1 00/11] pc-dimm: next bunch of cleanups
Posted by David Hildenbrand 5 years, 9 months ago
On 13.06.2018 16:11, David Hildenbrand wrote:
> On 13.06.2018 15:34, Igor Mammedov wrote:
>> On Mon, 11 Jun 2018 14:16:44 +0200
>> David Hildenbrand <david@redhat.com> 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?)

I'll drag these 4 along and perform some renaming in patch nr 3.


-- 

Thanks,

David / dhildenb