Re: [Qemu-devel] [PATCH 2/2] pc-dimm: remove realize callback

[Igor Mammedov]

On Tue, 19 Feb 2019 13:36:57 +0100
Philippe Mathieu-Daudé <address@hidden> 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 <address@hidden>
> > ---
> >  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;
> >