Message ID | 20180926094219.20322-15-david@redhat.com |
---|---|
State | New |
Headers | show |
Series | memory-device: complete refactoring + virtio-pmem | expand |
Hi David, On 9/26/18 11:42 AM, David Hildenbrand wrote: > With the new memory device functions in place, we can factor out > plugging of memory devices completely. > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/mem/memory-device.c | 13 ++++++++++--- > hw/mem/pc-dimm.c | 9 +-------- > include/hw/mem/memory-device.h | 3 +-- > 3 files changed, 12 insertions(+), 13 deletions(-) > > diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c > index 54e3f23b15..3914e2fe6f 100644 > --- a/hw/mem/memory-device.c > +++ b/hw/mem/memory-device.c > @@ -275,10 +275,17 @@ out: > error_propagate(errp, local_err); > } > > -void memory_device_plug_region(MachineState *ms, MemoryRegion *mr, > - uint64_t addr) > +void memory_device_plug(MemoryDeviceState *md, MachineState *ms) > { > - /* we expect a previous call to memory_device_get_free_addr() */ > + const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md); > + const uint64_t addr = mdc->get_addr(md); > + MemoryRegion *mr; > + > + /* > + * We expect a previous call to memory_device_pre_plug() succeeded so > + * it and can't fail at this point. comment to be reworded Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric > + */ > + mr = mdc->get_memory_region(md, &error_abort); > g_assert(ms->device_memory); > > memory_region_add_subregion(&ms->device_memory->mr, > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > index ea9968e379..07fa29a748 100644 > --- a/hw/mem/pc-dimm.c > +++ b/hw/mem/pc-dimm.c > @@ -63,17 +63,10 @@ out: > void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine, Error **errp) > { > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > - MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(dimm); > MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm, > &error_abort); > - MemoryRegion *mr = mdc->get_memory_region(MEMORY_DEVICE(dimm), > - &error_abort); > - uint64_t addr; > - > - addr = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP, > - &error_abort); > > - memory_device_plug_region(machine, mr, addr); > + memory_device_plug(MEMORY_DEVICE(dimm), machine); > vmstate_register_ram(vmstate_mr, DEVICE(dimm)); > } > > diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h > index df48b85285..8b58529843 100644 > --- a/include/hw/mem/memory-device.h > +++ b/include/hw/mem/memory-device.h > @@ -63,8 +63,7 @@ MemoryDeviceInfoList *qmp_memory_device_list(void); > 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_region(MachineState *ms, MemoryRegion *mr, > - uint64_t addr); > +void memory_device_plug(MemoryDeviceState *md, MachineState *ms); > void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr); > uint64_t memory_device_get_region_size(const MemoryDeviceState *md, > Error **errp); >
On 30/09/2018 17:55, Auger Eric wrote: > Hi David, > > On 9/26/18 11:42 AM, David Hildenbrand wrote: >> With the new memory device functions in place, we can factor out >> plugging of memory devices completely. >> >> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >> Reviewed-by: Igor Mammedov <imammedo@redhat.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/mem/memory-device.c | 13 ++++++++++--- >> hw/mem/pc-dimm.c | 9 +-------- >> include/hw/mem/memory-device.h | 3 +-- >> 3 files changed, 12 insertions(+), 13 deletions(-) >> >> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c >> index 54e3f23b15..3914e2fe6f 100644 >> --- a/hw/mem/memory-device.c >> +++ b/hw/mem/memory-device.c >> @@ -275,10 +275,17 @@ out: >> error_propagate(errp, local_err); >> } >> >> -void memory_device_plug_region(MachineState *ms, MemoryRegion *mr, >> - uint64_t addr) >> +void memory_device_plug(MemoryDeviceState *md, MachineState *ms) >> { >> - /* we expect a previous call to memory_device_get_free_addr() */ >> + const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md); >> + const uint64_t addr = mdc->get_addr(md); >> + MemoryRegion *mr; >> + >> + /* >> + * We expect a previous call to memory_device_pre_plug() succeeded so >> + * it and can't fail at this point. > comment to be reworded > Igor requested that rewording. I can turn that into "We expect that a previous call ... succeeded, so " .. > Reviewed-by: Eric Auger <eric.auger@redhat.com> > > Thanks > > Eric
On 01/10/2018 10:15, David Hildenbrand wrote: > On 30/09/2018 17:55, Auger Eric wrote: >> Hi David, >> >> On 9/26/18 11:42 AM, David Hildenbrand wrote: >>> With the new memory device functions in place, we can factor out >>> plugging of memory devices completely. >>> >>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >>> Reviewed-by: Igor Mammedov <imammedo@redhat.com> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >>> hw/mem/memory-device.c | 13 ++++++++++--- >>> hw/mem/pc-dimm.c | 9 +-------- >>> include/hw/mem/memory-device.h | 3 +-- >>> 3 files changed, 12 insertions(+), 13 deletions(-) >>> >>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c >>> index 54e3f23b15..3914e2fe6f 100644 >>> --- a/hw/mem/memory-device.c >>> +++ b/hw/mem/memory-device.c >>> @@ -275,10 +275,17 @@ out: >>> error_propagate(errp, local_err); >>> } >>> >>> -void memory_device_plug_region(MachineState *ms, MemoryRegion *mr, >>> - uint64_t addr) >>> +void memory_device_plug(MemoryDeviceState *md, MachineState *ms) >>> { >>> - /* we expect a previous call to memory_device_get_free_addr() */ >>> + const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md); >>> + const uint64_t addr = mdc->get_addr(md); >>> + MemoryRegion *mr; >>> + >>> + /* >>> + * We expect a previous call to memory_device_pre_plug() succeeded so >>> + * it and can't fail at this point. >> comment to be reworded >> > > Igor requested that rewording. I can turn that into "We expect that a > previous call ... succeeded, so " .. > "We expect that a previous call to memory_device_pre_plug() succeeded, so it can't fail at this point." to be precise :) >> Reviewed-by: Eric Auger <eric.auger@redhat.com> >> >> Thanks >> >> Eric > >
On Mon, 1 Oct 2018 10:18:20 +0200 David Hildenbrand <david@redhat.com> wrote: > On 01/10/2018 10:15, David Hildenbrand wrote: > > On 30/09/2018 17:55, Auger Eric wrote: > >> Hi David, > >> > >> On 9/26/18 11:42 AM, David Hildenbrand wrote: > >>> With the new memory device functions in place, we can factor out > >>> plugging of memory devices completely. > >>> > >>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > >>> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > >>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>> --- > >>> hw/mem/memory-device.c | 13 ++++++++++--- > >>> hw/mem/pc-dimm.c | 9 +-------- > >>> include/hw/mem/memory-device.h | 3 +-- > >>> 3 files changed, 12 insertions(+), 13 deletions(-) > >>> > >>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c > >>> index 54e3f23b15..3914e2fe6f 100644 > >>> --- a/hw/mem/memory-device.c > >>> +++ b/hw/mem/memory-device.c > >>> @@ -275,10 +275,17 @@ out: > >>> error_propagate(errp, local_err); > >>> } > >>> > >>> -void memory_device_plug_region(MachineState *ms, MemoryRegion *mr, > >>> - uint64_t addr) > >>> +void memory_device_plug(MemoryDeviceState *md, MachineState *ms) > >>> { > >>> - /* we expect a previous call to memory_device_get_free_addr() */ > >>> + const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md); > >>> + const uint64_t addr = mdc->get_addr(md); > >>> + MemoryRegion *mr; > >>> + > >>> + /* > >>> + * We expect a previous call to memory_device_pre_plug() succeeded so > >>> + * it and can't fail at this point. > >> comment to be reworded > >> > > > > Igor requested that rewording. I can turn that into "We expect that a > > previous call ... succeeded, so " .. > > > > "We expect that a previous call to memory_device_pre_plug() succeeded, > so it can't fail at this point." looks good to me > > to be precise :) > > >> Reviewed-by: Eric Auger <eric.auger@redhat.com> > >> > >> Thanks > >> > >> Eric > > > > > >
diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c index 54e3f23b15..3914e2fe6f 100644 --- a/hw/mem/memory-device.c +++ b/hw/mem/memory-device.c @@ -275,10 +275,17 @@ out: error_propagate(errp, local_err); } -void memory_device_plug_region(MachineState *ms, MemoryRegion *mr, - uint64_t addr) +void memory_device_plug(MemoryDeviceState *md, MachineState *ms) { - /* we expect a previous call to memory_device_get_free_addr() */ + const MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(md); + const uint64_t addr = mdc->get_addr(md); + MemoryRegion *mr; + + /* + * We expect a previous call to memory_device_pre_plug() succeeded so + * it and can't fail at this point. + */ + mr = mdc->get_memory_region(md, &error_abort); g_assert(ms->device_memory); memory_region_add_subregion(&ms->device_memory->mr, diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index ea9968e379..07fa29a748 100644 --- a/hw/mem/pc-dimm.c +++ b/hw/mem/pc-dimm.c @@ -63,17 +63,10 @@ out: void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine, Error **errp) { PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); - MemoryDeviceClass *mdc = MEMORY_DEVICE_GET_CLASS(dimm); MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm, &error_abort); - MemoryRegion *mr = mdc->get_memory_region(MEMORY_DEVICE(dimm), - &error_abort); - uint64_t addr; - - addr = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP, - &error_abort); - memory_device_plug_region(machine, mr, addr); + memory_device_plug(MEMORY_DEVICE(dimm), machine); vmstate_register_ram(vmstate_mr, DEVICE(dimm)); } diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h index df48b85285..8b58529843 100644 --- a/include/hw/mem/memory-device.h +++ b/include/hw/mem/memory-device.h @@ -63,8 +63,7 @@ MemoryDeviceInfoList *qmp_memory_device_list(void); 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_region(MachineState *ms, MemoryRegion *mr, - uint64_t addr); +void memory_device_plug(MemoryDeviceState *md, MachineState *ms); void memory_device_unplug_region(MachineState *ms, MemoryRegion *mr); uint64_t memory_device_get_region_size(const MemoryDeviceState *md, Error **errp);