| Message ID | 20180920103243.28474-14-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | memory-device: complete refactoring + virtio-pmem | expand |
On Thu, Sep 20, 2018 at 12:32:34PM +0200, David Hildenbrand wrote: > With the new memory device functions in place, we can factor out > unplugging of memory devices completely. > > Signed-off-by: David Hildenbrand <david@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/mem/memory-device.c | 8 ++++++-- > hw/mem/pc-dimm.c | 4 +--- > include/hw/mem/memory-device.h | 2 +- > 3 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c > index 34e92148ec..3e5a3a7951 100644 > --- a/hw/mem/memory-device.c > +++ b/hw/mem/memory-device.c > @@ -289,9 +289,13 @@ void memory_device_plug(MemoryDeviceState *md, MachineState *ms) > addr - ms->device_memory->base, mr); > } > > -void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr) > +void memory_device_unplug(MemoryDeviceState *md, MachineState *ms) > { > - /* we expect a previous call to memory_device_get_free_addr() */ > + const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md); > + MemoryRegion *mr; > + > + /* we expect a previous call to memory_device_pre_plug() */ > + mr = mdc->get_memory_region(md, &error_abort); > g_assert(ms->device_memory); > > memory_region_del_subregion(&ms->device_memory->mr, mr); > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > index db8f71c672..915908b104 100644 > --- a/hw/mem/pc-dimm.c > +++ b/hw/mem/pc-dimm.c > @@ -74,12 +74,10 @@ void pc_dimm_unplug(DeviceState *dev, MachineState *machine) > { > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > - MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(dev); > MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm, > &error_abort); > - MemoryRegion *mr = mdc->get_memory_region(MEMORY_DEVICE(dev), &error_abort); > > - memory_device_unplug_region(machine, mr); > + memory_device_unplug(MEMORY_DEVICE(dev), machine); > vmstate_unregister_ram(vmstate_mr, dev); > } > > diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h > index 5ba36f98fb..fd0b43c224 100644 > --- a/include/hw/mem/memory-device.h > +++ b/include/hw/mem/memory-device.h > @@ -61,7 +61,7 @@ uint64_t get_plugged_memory_size(void); > void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms, > const uint64_t *legacy_align, Error **errp); > void memory_device_plug(MemoryDeviceState *md, MachineState *ms); > -void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr); > +void memory_device_unplug(MemoryDeviceState *md, MachineState *ms); > uint64_t memory_device_get_region_size(const MemoryDeviceState *md, > Error **errp); >
On Thu, 20 Sep 2018 12:32:34 +0200 David Hildenbrand <david@redhat.com> wrote: > With the new memory device functions in place, we can factor out > unplugging of memory devices completely. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/mem/memory-device.c | 8 ++++++-- > hw/mem/pc-dimm.c | 4 +--- > include/hw/mem/memory-device.h | 2 +- > 3 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c > index 34e92148ec..3e5a3a7951 100644 > --- a/hw/mem/memory-device.c > +++ b/hw/mem/memory-device.c > @@ -289,9 +289,13 @@ void memory_device_plug(MemoryDeviceState *md, MachineState *ms) > addr - ms->device_memory->base, mr); > } > > -void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr) > +void memory_device_unplug(MemoryDeviceState *md, MachineState *ms) I'd keep machine argument 1st since it's machine helper after all, but that's a matter of taste. > { > - /* we expect a previous call to memory_device_get_free_addr() */ > + const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md); > + MemoryRegion *mr; > + > + /* we expect a previous call to memory_device_pre_plug() */ ... 'succeeded and can't fail at this point */ > + mr = mdc->get_memory_region(md, &error_abort); > g_assert(ms->device_memory); > > memory_region_del_subregion(&ms->device_memory->mr, mr); > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > index db8f71c672..915908b104 100644 > --- a/hw/mem/pc-dimm.c > +++ b/hw/mem/pc-dimm.c > @@ -74,12 +74,10 @@ void pc_dimm_unplug(DeviceState *dev, MachineState *machine) > { > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > - MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(dev); > MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm, > &error_abort); > - MemoryRegion *mr = mdc->get_memory_region(MEMORY_DEVICE(dev), &error_abort); > > - memory_device_unplug_region(machine, mr); > + memory_device_unplug(MEMORY_DEVICE(dev), machine); > vmstate_unregister_ram(vmstate_mr, dev); > } > > diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h > index 5ba36f98fb..fd0b43c224 100644 > --- a/include/hw/mem/memory-device.h > +++ b/include/hw/mem/memory-device.h > @@ -61,7 +61,7 @@ uint64_t get_plugged_memory_size(void); > void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms, > const uint64_t *legacy_align, Error **errp); > void memory_device_plug(MemoryDeviceState *md, MachineState *ms); > -void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr); > +void memory_device_unplug(MemoryDeviceState *md, MachineState *ms); > uint64_t memory_device_get_region_size(const MemoryDeviceState *md, > Error **errp); > Otherwise looks good Reviewed-by: Igor Mammedov <imammedo@redhat.com>
On 24/09/2018 16:18, Igor Mammedov wrote: > On Thu, 20 Sep 2018 12:32:34 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> With the new memory device functions in place, we can factor out >> unplugging of memory devices completely. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/mem/memory-device.c | 8 ++++++-- >> hw/mem/pc-dimm.c | 4 +--- >> include/hw/mem/memory-device.h | 2 +- >> 3 files changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c >> index 34e92148ec..3e5a3a7951 100644 >> --- a/hw/mem/memory-device.c >> +++ b/hw/mem/memory-device.c >> @@ -289,9 +289,13 @@ void memory_device_plug(MemoryDeviceState *md, MachineState *ms) >> addr - ms->device_memory->base, mr); >> } >> >> -void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr) >> +void memory_device_unplug(MemoryDeviceState *md, MachineState *ms) > I'd keep machine argument 1st since it's machine helper after all, > but that's a matter of taste. I'll stick to this for now (this is also how pc_dimm handles it, and probably not worth the trouble). > >> { >> - /* we expect a previous call to memory_device_get_free_addr() */ >> + const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md); >> + MemoryRegion *mr; >> + >> + /* we expect a previous call to memory_device_pre_plug() */ > ... 'succeeded and can't fail at this point */ > >> + mr = mdc->get_memory_region(md, &error_abort); >> g_assert(ms->device_memory); >> >> memory_region_del_subregion(&ms->device_memory->mr, mr); >> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c >> index db8f71c672..915908b104 100644 >> --- a/hw/mem/pc-dimm.c >> +++ b/hw/mem/pc-dimm.c >> @@ -74,12 +74,10 @@ void pc_dimm_unplug(DeviceState *dev, MachineState *machine) >> { >> PCDIMMDevice *dimm = PC_DIMM(dev); >> PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); >> - MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(dev); >> MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm, >> &error_abort); >> - MemoryRegion *mr = mdc->get_memory_region(MEMORY_DEVICE(dev), &error_abort); >> >> - memory_device_unplug_region(machine, mr); >> + memory_device_unplug(MEMORY_DEVICE(dev), machine); >> vmstate_unregister_ram(vmstate_mr, dev); >> } >> >> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h >> index 5ba36f98fb..fd0b43c224 100644 >> --- a/include/hw/mem/memory-device.h >> +++ b/include/hw/mem/memory-device.h >> @@ -61,7 +61,7 @@ uint64_t get_plugged_memory_size(void); >> void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms, >> const uint64_t *legacy_align, Error **errp); >> void memory_device_plug(MemoryDeviceState *md, MachineState *ms); >> -void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr); >> +void memory_device_unplug(MemoryDeviceState *md, MachineState *ms); >> uint64_t memory_device_get_region_size(const MemoryDeviceState *md, >> Error **errp); >> > Otherwise looks good > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > Thanks!
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c index 34e92148ec..3e5a3a7951 100644 --- a/hw/mem/memory-device.c +++ b/hw/mem/memory-device.c @@ -289,9 +289,13 @@ void memory_device_plug(MemoryDeviceState *md, MachineState *ms) addr - ms->device_memory->base, mr); } -void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr) +void memory_device_unplug(MemoryDeviceState *md, MachineState *ms) { - /* we expect a previous call to memory_device_get_free_addr() */ + const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md); + MemoryRegion *mr; + + /* we expect a previous call to memory_device_pre_plug() */ + mr = mdc->get_memory_region(md, &error_abort); g_assert(ms->device_memory); memory_region_del_subregion(&ms->device_memory->mr, mr); diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index db8f71c672..915908b104 100644 --- a/hw/mem/pc-dimm.c +++ b/hw/mem/pc-dimm.c @@ -74,12 +74,10 @@ void pc_dimm_unplug(DeviceState *dev, MachineState *machine) { PCDIMMDevice *dimm = PC_DIMM(dev); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); - MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(dev); MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm, &error_abort); - MemoryRegion *mr = mdc->get_memory_region(MEMORY_DEVICE(dev), &error_abort); - memory_device_unplug_region(machine, mr); + memory_device_unplug(MEMORY_DEVICE(dev), machine); vmstate_unregister_ram(vmstate_mr, dev); } diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h index 5ba36f98fb..fd0b43c224 100644 --- a/include/hw/mem/memory-device.h +++ b/include/hw/mem/memory-device.h @@ -61,7 +61,7 @@ uint64_t get_plugged_memory_size(void); void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms, const uint64_t *legacy_align, Error **errp); void memory_device_plug(MemoryDeviceState *md, MachineState *ms); -void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr); +void memory_device_unplug(MemoryDeviceState *md, MachineState *ms); uint64_t memory_device_get_region_size(const MemoryDeviceState *md, Error **errp);
With the new memory device functions in place, we can factor out unplugging of memory devices completely. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/mem/memory-device.c | 8 ++++++-- hw/mem/pc-dimm.c | 4 +--- include/hw/mem/memory-device.h | 2 +- 3 files changed, 8 insertions(+), 6 deletions(-)