| Message ID | 20180705115943.29402-1-david@redhat.com |
|---|---|
| Headers | show |
| Series | memory-device: complete refactoring | expand |
On Thu, 5 Jul 2018 13:59:32 +0200 David Hildenbrand <david@redhat.com> wrote: > This is another part of the original series > [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers > And is based on > [PATCH v3 0/4] pc-dimm: pre_plug "slot" and "addr" assignment > > This series completes refactoring of pre_plug, plug and unplug logic of > memory devices. With this as a basis, one can easily have e.g. virtio > based memory devices (virtio-mem, virtio-pmem, virtio-fs?) with minor > modifications on e.g. x86 and s390x. > > Unfortunately, the "addr" property is already used for virtio devices, so > we will have to deal with device specific properties. So > set_addr() for memory devices is introduced to handle that (we already > have get_addr()). > > The only way I see to avoid that would be for virtio based devices to > introduce an indirection: > > E.g. right now for my virtio-mem prototype: > ... -object memory-backend-ram,id=mem0,size=8G \ > -device virtio-mem-pci,id=vm0,memdev=mem0,node=0,phys-addr=0x12345 Though it seems inconvenient to have different property name, it is not dimm device (even though it shares GPA resource) so it is fine for it to have it's own set of properties. (might make error reporting a bit ugly) Also what about usecase of mmio based virtio (ARM)? > To something like: > ... -object memory-backend-ram,id=mem0,size=8G \ > -object virtio-mem-backend,id=vmb0,memdev=mem0,addr=0x12345 \ > -device virtio-mem-pci,id=vm0,vmem=vmb0 \ it's broken conceptually, backends should deal only with host resource allocation while 'addr' is device model property and belongs to a frontend. > Or something like (that might be interesting for virtio-pmem): > ... -device virtio-mem-pci \ /* a virtio-mem bus */ > -object memory-backend-ram,id=mem0,size=8G \ > -device virtio-mem-backend,memdev=mem0,addr=0x12345 \ did you mean ^^^^ virtio-pmem device? Is it a separate controller and memory resource design connected together somehow? > But both alternatives have their pros and cons. > > Opinions? > > David Hildenbrand (11): > memory-device: fix error message when hinted address is too small > memory-device: introduce separate config option > memory-device: get_region_size()/get_plugged_size() might fail > memory-device: convert get_region_size() to get_memory_region() > memory-device: document MemoryDeviceClass > memory-device: add device class function set_addr() > pc-dimm: implement memory device class function set_addr() > memory-device: complete factoring out pre_plug handling > memory-device: complete factoring out plug handling > memory-device: complete factoring out unplug handling > memory-device: trace when pre_assigning/assigning/unassigning > addresses > > default-configs/i386-softmmu.mak | 3 +- > default-configs/ppc64-softmmu.mak | 3 +- > default-configs/x86_64-softmmu.mak | 3 +- > hw/Makefile.objs | 2 +- > hw/mem/Makefile.objs | 4 +- > hw/mem/memory-device.c | 60 +++++++++++++++++++----- > hw/mem/pc-dimm.c | 75 +++++++++++++++++------------- > hw/mem/trace-events | 5 +- > include/hw/mem/memory-device.h | 30 ++++++++---- > qapi/misc.json | 2 +- > 10 files changed, 126 insertions(+), 61 deletions(-) >
On 23.07.2018 16:23, Igor Mammedov wrote: > On Thu, 5 Jul 2018 13:59:32 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> This is another part of the original series >> [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers >> And is based on >> [PATCH v3 0/4] pc-dimm: pre_plug "slot" and "addr" assignment >> >> This series completes refactoring of pre_plug, plug and unplug logic of >> memory devices. With this as a basis, one can easily have e.g. virtio >> based memory devices (virtio-mem, virtio-pmem, virtio-fs?) with minor >> modifications on e.g. x86 and s390x. >> >> Unfortunately, the "addr" property is already used for virtio devices, so >> we will have to deal with device specific properties. So >> set_addr() for memory devices is introduced to handle that (we already >> have get_addr()). >> >> The only way I see to avoid that would be for virtio based devices to >> introduce an indirection: >> >> E.g. right now for my virtio-mem prototype: >> ... -object memory-backend-ram,id=mem0,size=8G \ >> -device virtio-mem-pci,id=vm0,memdev=mem0,node=0,phys-addr=0x12345 > Though it seems inconvenient to have different property name, > it is not dimm device (even though it shares GPA resource) > so it is fine for it to have it's own set of properties. > (might make error reporting a bit ugly) Same opinion here, that's why I prefer this approach. > > Also what about usecase of mmio based virtio (ARM)? Can you elaborate? > > >> To something like: >> ... -object memory-backend-ram,id=mem0,size=8G \ >> -object virtio-mem-backend,id=vmb0,memdev=mem0,addr=0x12345 \ >> -device virtio-mem-pci,id=vm0,vmem=vmb0 \ > it's broken conceptually, > backends should deal only with host resource allocation while > 'addr' is device model property and belongs to a frontend. Agreed. > >> Or something like (that might be interesting for virtio-pmem): >> ... -device virtio-mem-pci \ /* a virtio-mem bus */ >> -object memory-backend-ram,id=mem0,size=8G \ >> -device virtio-mem-backend,memdev=mem0,addr=0x12345 \ > did you mean ^^^^ virtio-pmem device? > > Is it a separate controller and memory resource design > connected together somehow? Yes, for this example s/mem/pmem/ of course (copy paste). virtio-mem and virtio-pmem are two separate device types with their own set of controllers (if any). I favor the original approach (above).
This is another part of the original series [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers And is based on [PATCH v3 0/4] pc-dimm: pre_plug "slot" and "addr" assignment This series completes refactoring of pre_plug, plug and unplug logic of memory devices. With this as a basis, one can easily have e.g. virtio based memory devices (virtio-mem, virtio-pmem, virtio-fs?) with minor modifications on e.g. x86 and s390x. Unfortunately, the "addr" property is already used for virtio devices, so we will have to deal with device specific properties. So set_addr() for memory devices is introduced to handle that (we already have get_addr()). The only way I see to avoid that would be for virtio based devices to introduce an indirection: E.g. right now for my virtio-mem prototype: ... -object memory-backend-ram,id=mem0,size=8G \ -device virtio-mem-pci,id=vm0,memdev=mem0,node=0,phys-addr=0x12345 To something like: ... -object memory-backend-ram,id=mem0,size=8G \ -object virtio-mem-backend,id=vmb0,memdev=mem0,addr=0x12345 \ -device virtio-mem-pci,id=vm0,vmem=vmb0 \ Or something like (that might be interesting for virtio-pmem): ... -device virtio-mem-pci \ /* a virtio-mem bus */ -object memory-backend-ram,id=mem0,size=8G \ -device virtio-mem-backend,memdev=mem0,addr=0x12345 \ But both alternatives have their pros and cons. Opinions? David Hildenbrand (11): memory-device: fix error message when hinted address is too small memory-device: introduce separate config option memory-device: get_region_size()/get_plugged_size() might fail memory-device: convert get_region_size() to get_memory_region() memory-device: document MemoryDeviceClass memory-device: add device class function set_addr() pc-dimm: implement memory device class function set_addr() memory-device: complete factoring out pre_plug handling memory-device: complete factoring out plug handling memory-device: complete factoring out unplug handling memory-device: trace when pre_assigning/assigning/unassigning addresses default-configs/i386-softmmu.mak | 3 +- default-configs/ppc64-softmmu.mak | 3 +- default-configs/x86_64-softmmu.mak | 3 +- hw/Makefile.objs | 2 +- hw/mem/Makefile.objs | 4 +- hw/mem/memory-device.c | 60 +++++++++++++++++++----- hw/mem/pc-dimm.c | 75 +++++++++++++++++------------- hw/mem/trace-events | 5 +- include/hw/mem/memory-device.h | 30 ++++++++---- qapi/misc.json | 2 +- 10 files changed, 126 insertions(+), 61 deletions(-)