| Message ID | 20190219060719.8211-3-richardw.yang@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] pc-dimm: remove check on pc-dimm hotpluggable | expand |
On 2/19/19 7:07 AM, Wei Yang wrote: > PCDIMM's realize callback is introduced to do proper setup for NVDIMM. > > Currently the NVDIMM setup task is nvdimm_prepare_memory_region(), which > is done in pre_plug stage. This means related task has already been done > at realize point. > > This patch remove PCDIMM realize callback. > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > --- > hw/acpi/memory_hotplug.c | 1 - > hw/mem/nvdimm.c | 11 ----------- > hw/mem/pc-dimm.c | 5 ----- > include/hw/mem/pc-dimm.h | 3 --- > 4 files changed, 20 deletions(-) > > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c > index b19673d337..cb5284d36f 100644 > --- a/hw/acpi/memory_hotplug.c > +++ b/hw/acpi/memory_hotplug.c > @@ -264,7 +264,6 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st, > DeviceState *dev, Error **errp) > { > MemStatus *mdev; > - DeviceClass *dc = DEVICE_GET_CLASS(dev); OK now I see the leftover of patch #1. > > mdev = acpi_memory_slot_status(mem_st, dev, errp); > if (!mdev) { > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c > index bf2adf5e16..8f69576926 100644 > --- a/hw/mem/nvdimm.c > +++ b/hw/mem/nvdimm.c > @@ -136,15 +136,6 @@ static MemoryRegion *nvdimm_md_get_memory_region(MemoryDeviceState *md, > return nvdimm->nvdimm_mr; > } > > -static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp) > -{ > - NVDIMMDevice *nvdimm = NVDIMM(dimm); > - > - if (!nvdimm->nvdimm_mr) { > - nvdimm_prepare_memory_region(nvdimm, errp); > - } > -} > - > /* > * the caller should check the input parameters before calling > * label read/write functions. > @@ -192,12 +183,10 @@ static Property nvdimm_properties[] = { > > static void nvdimm_class_init(ObjectClass *oc, void *data) > { > - PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc); > MemoryDeviceClass *mdc = MEMORY_DEVICE_CLASS(oc); > NVDIMMClass *nvc = NVDIMM_CLASS(oc); > DeviceClass *dc = DEVICE_CLASS(oc); > > - ddc->realize = nvdimm_realize; > mdc->get_memory_region = nvdimm_md_get_memory_region; > dc->props = nvdimm_properties; > I'd split your patch in 2: - the previous part, cleaning NVDIMM, - the part below, doing "git revert 9f318f8f7e6" and improving the commit description. > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > index 152400b1fc..5832c0ba92 100644 > --- a/hw/mem/pc-dimm.c > +++ b/hw/mem/pc-dimm.c > @@ -159,7 +159,6 @@ static void pc_dimm_init(Object *obj) > static void pc_dimm_realize(DeviceState *dev, Error **errp) > { > PCDIMMDevice *dimm = PC_DIMM(dev); > - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > > if (!dimm->hostmem) { > error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set"); > @@ -178,10 +177,6 @@ static void pc_dimm_realize(DeviceState *dev, Error **errp) > return; > } > > - if (ddc->realize) { > - ddc->realize(dimm, errp); > - } > - > host_memory_backend_set_mapped(dimm->hostmem, true); > } > > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > index 01436b9f50..d18f8246b7 100644 > --- a/include/hw/mem/pc-dimm.h > +++ b/include/hw/mem/pc-dimm.h > @@ -59,8 +59,6 @@ typedef struct PCDIMMDevice { > > /** > * PCDIMMDeviceClass: > - * @realize: called after common dimm is realized so that the dimm based > - * devices get the chance to do specified operations. > * @get_vmstate_memory_region: returns #MemoryRegion which indicates the > * memory of @dimm should be kept during live migration. Will not fail > * after the device was realized. > @@ -70,7 +68,6 @@ typedef struct PCDIMMDeviceClass { > DeviceClass parent_class; > > /* public */ > - void (*realize)(PCDIMMDevice *dimm, Error **errp); > MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm, > Error **errp); > } PCDIMMDeviceClass; >
On Tue, Feb 19, 2019 at 01:36:57PM +0100, Philippe Mathieu-Daudé wrote: >On 2/19/19 7:07 AM, Wei Yang wrote: >> PCDIMM's realize callback is introduced to do proper setup for NVDIMM. >> >> Currently the NVDIMM setup task is nvdimm_prepare_memory_region(), which >> is done in pre_plug stage. This means related task has already been done >> at realize point. >> >> This patch remove PCDIMM realize callback. >> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> >> --- >> hw/acpi/memory_hotplug.c | 1 - >> hw/mem/nvdimm.c | 11 ----------- >> hw/mem/pc-dimm.c | 5 ----- >> include/hw/mem/pc-dimm.h | 3 --- >> 4 files changed, 20 deletions(-) >> >> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c >> index b19673d337..cb5284d36f 100644 >> --- a/hw/acpi/memory_hotplug.c >> +++ b/hw/acpi/memory_hotplug.c >> @@ -264,7 +264,6 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st, >> DeviceState *dev, Error **errp) >> { >> MemStatus *mdev; >> - DeviceClass *dc = DEVICE_GET_CLASS(dev); > >OK now I see the leftover of patch #1. > Ah, I should move this in patch #1. Thanks. >> >> mdev = acpi_memory_slot_status(mem_st, dev, errp); >> if (!mdev) { >> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c >> index bf2adf5e16..8f69576926 100644 >> --- a/hw/mem/nvdimm.c >> +++ b/hw/mem/nvdimm.c >> @@ -136,15 +136,6 @@ static MemoryRegion *nvdimm_md_get_memory_region(MemoryDeviceState *md, >> return nvdimm->nvdimm_mr; >> } >> >> -static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp) >> -{ >> - NVDIMMDevice *nvdimm = NVDIMM(dimm); >> - >> - if (!nvdimm->nvdimm_mr) { >> - nvdimm_prepare_memory_region(nvdimm, errp); >> - } >> -} >> - >> /* >> * the caller should check the input parameters before calling >> * label read/write functions. >> @@ -192,12 +183,10 @@ static Property nvdimm_properties[] = { >> >> static void nvdimm_class_init(ObjectClass *oc, void *data) >> { >> - PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc); >> MemoryDeviceClass *mdc = MEMORY_DEVICE_CLASS(oc); >> NVDIMMClass *nvc = NVDIMM_CLASS(oc); >> DeviceClass *dc = DEVICE_CLASS(oc); >> >> - ddc->realize = nvdimm_realize; >> mdc->get_memory_region = nvdimm_md_get_memory_region; >> dc->props = nvdimm_properties; >> > >I'd split your patch in 2: >- the previous part, cleaning NVDIMM, >- the part below, doing "git revert 9f318f8f7e6" and improving the >commit description. > Sounds reasonable. Let me split this into two. >> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c >> index 152400b1fc..5832c0ba92 100644 >> --- a/hw/mem/pc-dimm.c >> +++ b/hw/mem/pc-dimm.c >> @@ -159,7 +159,6 @@ static void pc_dimm_init(Object *obj) >> static void pc_dimm_realize(DeviceState *dev, Error **errp) >> { >> PCDIMMDevice *dimm = PC_DIMM(dev); >> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); >> >> if (!dimm->hostmem) { >> error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set"); >> @@ -178,10 +177,6 @@ static void pc_dimm_realize(DeviceState *dev, Error **errp) >> return; >> } >> >> - if (ddc->realize) { >> - ddc->realize(dimm, errp); >> - } >> - >> host_memory_backend_set_mapped(dimm->hostmem, true); >> } >> >> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h >> index 01436b9f50..d18f8246b7 100644 >> --- a/include/hw/mem/pc-dimm.h >> +++ b/include/hw/mem/pc-dimm.h >> @@ -59,8 +59,6 @@ typedef struct PCDIMMDevice { >> >> /** >> * PCDIMMDeviceClass: >> - * @realize: called after common dimm is realized so that the dimm based >> - * devices get the chance to do specified operations. >> * @get_vmstate_memory_region: returns #MemoryRegion which indicates the >> * memory of @dimm should be kept during live migration. Will not fail >> * after the device was realized. >> @@ -70,7 +68,6 @@ typedef struct PCDIMMDeviceClass { >> DeviceClass parent_class; >> >> /* public */ >> - void (*realize)(PCDIMMDevice *dimm, Error **errp); >> MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm, >> Error **errp); >> } PCDIMMDeviceClass; >>
On Tue, 19 Feb 2019 13:36:57 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 2/19/19 7:07 AM, Wei Yang wrote: > > PCDIMM's realize callback is introduced to do proper setup for NVDIMM. > > > > Currently the NVDIMM setup task is nvdimm_prepare_memory_region(), which > > is done in pre_plug stage. This means related task has already been done > > at realize point. > > > > This patch remove PCDIMM realize callback. pre_plug is machine helper and that it calls nvdimm_prepare_memory_region() indirectly is just of side effect of current nvdimm impl. Another machine might choose to implement it's own pre_plug to use with nvdimms, in which case it might not call the function before realize, hence this patch makes nvdimm device model incomplete and less robust. Pls drop this patch > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > > --- > > hw/acpi/memory_hotplug.c | 1 - > > hw/mem/nvdimm.c | 11 ----------- > > hw/mem/pc-dimm.c | 5 ----- > > include/hw/mem/pc-dimm.h | 3 --- > > 4 files changed, 20 deletions(-) > > > > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c > > index b19673d337..cb5284d36f 100644 > > --- a/hw/acpi/memory_hotplug.c > > +++ b/hw/acpi/memory_hotplug.c > > @@ -264,7 +264,6 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st, > > DeviceState *dev, Error **errp) > > { > > MemStatus *mdev; > > - DeviceClass *dc = DEVICE_GET_CLASS(dev); > > OK now I see the leftover of patch #1. > > > > > mdev = acpi_memory_slot_status(mem_st, dev, errp); > > if (!mdev) { > > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c > > index bf2adf5e16..8f69576926 100644 > > --- a/hw/mem/nvdimm.c > > +++ b/hw/mem/nvdimm.c > > @@ -136,15 +136,6 @@ static MemoryRegion *nvdimm_md_get_memory_region(MemoryDeviceState *md, > > return nvdimm->nvdimm_mr; > > } > > > > -static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp) > > -{ > > - NVDIMMDevice *nvdimm = NVDIMM(dimm); > > - > > - if (!nvdimm->nvdimm_mr) { > > - nvdimm_prepare_memory_region(nvdimm, errp); > > - } > > -} > > - > > /* > > * the caller should check the input parameters before calling > > * label read/write functions. > > @@ -192,12 +183,10 @@ static Property nvdimm_properties[] = { > > > > static void nvdimm_class_init(ObjectClass *oc, void *data) > > { > > - PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc); > > MemoryDeviceClass *mdc = MEMORY_DEVICE_CLASS(oc); > > NVDIMMClass *nvc = NVDIMM_CLASS(oc); > > DeviceClass *dc = DEVICE_CLASS(oc); > > > > - ddc->realize = nvdimm_realize; > > mdc->get_memory_region = nvdimm_md_get_memory_region; > > dc->props = nvdimm_properties; > > > > I'd split your patch in 2: > - the previous part, cleaning NVDIMM, > - the part below, doing "git revert 9f318f8f7e6" and improving the > commit description. > > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > > index 152400b1fc..5832c0ba92 100644 > > --- a/hw/mem/pc-dimm.c > > +++ b/hw/mem/pc-dimm.c > > @@ -159,7 +159,6 @@ static void pc_dimm_init(Object *obj) > > static void pc_dimm_realize(DeviceState *dev, Error **errp) > > { > > PCDIMMDevice *dimm = PC_DIMM(dev); > > - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > > > > if (!dimm->hostmem) { > > error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set"); > > @@ -178,10 +177,6 @@ static void pc_dimm_realize(DeviceState *dev, Error **errp) > > return; > > } > > > > - if (ddc->realize) { > > - ddc->realize(dimm, errp); > > - } > > - > > host_memory_backend_set_mapped(dimm->hostmem, true); > > } > > > > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > > index 01436b9f50..d18f8246b7 100644 > > --- a/include/hw/mem/pc-dimm.h > > +++ b/include/hw/mem/pc-dimm.h > > @@ -59,8 +59,6 @@ typedef struct PCDIMMDevice { > > > > /** > > * PCDIMMDeviceClass: > > - * @realize: called after common dimm is realized so that the dimm based > > - * devices get the chance to do specified operations. > > * @get_vmstate_memory_region: returns #MemoryRegion which indicates the > > * memory of @dimm should be kept during live migration. Will not fail > > * after the device was realized. > > @@ -70,7 +68,6 @@ typedef struct PCDIMMDeviceClass { > > DeviceClass parent_class; > > > > /* public */ > > - void (*realize)(PCDIMMDevice *dimm, Error **errp); > > MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm, > > Error **errp); > > } PCDIMMDeviceClass; > >
On Thu, Feb 21, 2019 at 03:37:53PM +0100, Igor Mammedov wrote: >On Tue, 19 Feb 2019 13:36:57 +0100 >Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> On 2/19/19 7:07 AM, Wei Yang wrote: >> > PCDIMM's realize callback is introduced to do proper setup for NVDIMM. >> > >> > Currently the NVDIMM setup task is nvdimm_prepare_memory_region(), which >> > is done in pre_plug stage. This means related task has already been done >> > at realize point. >> > >> > This patch remove PCDIMM realize callback. > >pre_plug is machine helper and that it calls nvdimm_prepare_memory_region() >indirectly is just of side effect of current nvdimm impl. Another machine >might choose to implement it's own pre_plug to use with nvdimms, in which >case it might not call the function before realize, hence this patch makes >nvdimm device model incomplete and less robust. Sounds reasonable. Call nvdimm_prepare_memory_region() two times is really ugly. Hmm... could we improve this?
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c index b19673d337..cb5284d36f 100644 --- a/hw/acpi/memory_hotplug.c +++ b/hw/acpi/memory_hotplug.c @@ -264,7 +264,6 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st, DeviceState *dev, Error **errp) { MemStatus *mdev; - DeviceClass *dc = DEVICE_GET_CLASS(dev); mdev = acpi_memory_slot_status(mem_st, dev, errp); if (!mdev) { diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c index bf2adf5e16..8f69576926 100644 --- a/hw/mem/nvdimm.c +++ b/hw/mem/nvdimm.c @@ -136,15 +136,6 @@ static MemoryRegion *nvdimm_md_get_memory_region(MemoryDeviceState *md, return nvdimm->nvdimm_mr; } -static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp) -{ - NVDIMMDevice *nvdimm = NVDIMM(dimm); - - if (!nvdimm->nvdimm_mr) { - nvdimm_prepare_memory_region(nvdimm, errp); - } -} - /* * the caller should check the input parameters before calling * label read/write functions. @@ -192,12 +183,10 @@ static Property nvdimm_properties[] = { static void nvdimm_class_init(ObjectClass *oc, void *data) { - PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc); MemoryDeviceClass *mdc = MEMORY_DEVICE_CLASS(oc); NVDIMMClass *nvc = NVDIMM_CLASS(oc); DeviceClass *dc = DEVICE_CLASS(oc); - ddc->realize = nvdimm_realize; mdc->get_memory_region = nvdimm_md_get_memory_region; dc->props = nvdimm_properties; diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index 152400b1fc..5832c0ba92 100644 --- a/hw/mem/pc-dimm.c +++ b/hw/mem/pc-dimm.c @@ -159,7 +159,6 @@ static void pc_dimm_init(Object *obj) static void pc_dimm_realize(DeviceState *dev, Error **errp) { PCDIMMDevice *dimm = PC_DIMM(dev); - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); if (!dimm->hostmem) { error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set"); @@ -178,10 +177,6 @@ static void pc_dimm_realize(DeviceState *dev, Error **errp) return; } - if (ddc->realize) { - ddc->realize(dimm, errp); - } - host_memory_backend_set_mapped(dimm->hostmem, true); } diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h index 01436b9f50..d18f8246b7 100644 --- a/include/hw/mem/pc-dimm.h +++ b/include/hw/mem/pc-dimm.h @@ -59,8 +59,6 @@ typedef struct PCDIMMDevice { /** * PCDIMMDeviceClass: - * @realize: called after common dimm is realized so that the dimm based - * devices get the chance to do specified operations. * @get_vmstate_memory_region: returns #MemoryRegion which indicates the * memory of @dimm should be kept during live migration. Will not fail * after the device was realized. @@ -70,7 +68,6 @@ typedef struct PCDIMMDeviceClass { DeviceClass parent_class; /* public */ - void (*realize)(PCDIMMDevice *dimm, Error **errp); MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm, Error **errp); } PCDIMMDeviceClass;
PCDIMM's realize callback is introduced to do proper setup for NVDIMM. Currently the NVDIMM setup task is nvdimm_prepare_memory_region(), which is done in pre_plug stage. This means related task has already been done at realize point. This patch remove PCDIMM realize callback. Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> --- hw/acpi/memory_hotplug.c | 1 - hw/mem/nvdimm.c | 11 ----------- hw/mem/pc-dimm.c | 5 ----- include/hw/mem/pc-dimm.h | 3 --- 4 files changed, 20 deletions(-)