[Qemu-devel] [PATCH 0/2] PCDIMM cleanup

[Qemu-devel] [PATCH 0/2] PCDIMM cleanup

[Wei Yang]

Two trivial cleanup for pc-dimm.

Patch [1] remove the check on class->hotpluggable since pc-dimm is always
hotpluggable.
Patch [2] remove realized callback since the task is done in pre_plug stage.

Wei Yang (2):
  pc-dimm: remove check on pc-dimm hotpluggable
  pc-dimm: remove realize callback

 hw/acpi/memory_hotplug.c |  5 -----
 hw/mem/nvdimm.c          | 11 -----------
 hw/mem/pc-dimm.c         |  5 -----
 include/hw/mem/pc-dimm.h |  3 ---
 4 files changed, 24 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH 1/2] pc-dimm: remove check on pc-dimm hotpluggable

[Wei Yang]

Function acpi_memory_plug_cb() is only invoked when dev is a PCDIMM,
which is hotpluggable. This means it is not necessary to check this
property again.

This patch removes this check.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/acpi/memory_hotplug.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 8c7c1013f3..b19673d337 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -266,10 +266,6 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
     MemStatus *mdev;
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
 
-    if (!dc->hotpluggable) {
-        return;
-    }
-
     mdev = acpi_memory_slot_status(mem_st, dev, errp);
     if (!mdev) {
         return;
-- 
2.19.1

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: remove check on pc-dimm hotpluggable

[Philippe Mathieu-Daudé]

On 2/19/19 7:07 AM, Wei Yang wrote:
> Function acpi_memory_plug_cb() is only invoked when dev is a PCDIMM,
> which is hotpluggable. This means it is not necessary to check this
> property again.
> 
> This patch removes this check.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  hw/acpi/memory_hotplug.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 8c7c1013f3..b19673d337 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -266,10 +266,6 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,

This is also obvious because it takes a HotplugHandler argument.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>      MemStatus *mdev;
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>  
> -    if (!dc->hotpluggable) {
> -        return;
> -    }
> -
>      mdev = acpi_memory_slot_status(mem_st, dev, errp);
>      if (!mdev) {
>          return;
> 

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: remove check on pc-dimm hotpluggable

[Igor Mammedov]

On Tue, 19 Feb 2019 13:26:47 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 2/19/19 7:07 AM, Wei Yang wrote:
> > Function acpi_memory_plug_cb() is only invoked when dev is a PCDIMM,
> > which is hotpluggable. This means it is not necessary to check this
> > property again.
> > 
> > This patch removes this check.
> > 
> > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> > ---
> >  hw/acpi/memory_hotplug.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> > index 8c7c1013f3..b19673d337 100644
> > --- a/hw/acpi/memory_hotplug.c
> > +++ b/hw/acpi/memory_hotplug.c
> > @@ -266,10 +266,6 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,  
> 
> This is also obvious because it takes a HotplugHandler argument.
                                          ^^^^
that's a sort of atavism, handler also takes care of coldplug wiring nowdays.
(it's just nobody came up with new name and fixed it up)


> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> >      MemStatus *mdev;
> >      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> >  
> > -    if (!dc->hotpluggable) {
> > -        return;
> > -    }
> > -
> >      mdev = acpi_memory_slot_status(mem_st, dev, errp);
> >      if (!mdev) {
> >          return;
> >   
> 

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: remove check on pc-dimm hotpluggable

[Philippe Mathieu-Daudé]

On 2/19/19 7:07 AM, Wei Yang wrote:
> Function acpi_memory_plug_cb() is only invoked when dev is a PCDIMM,
> which is hotpluggable. This means it is not necessary to check this
> property again.
> 
> This patch removes this check.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  hw/acpi/memory_hotplug.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 8c7c1013f3..b19673d337 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -266,10 +266,6 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
>      MemStatus *mdev;
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);

You have to remove the 'dc' variable now:

  CC      hw/acpi/memory_hotplug.o
hw/acpi/memory_hotplug.c: In function ‘acpi_memory_plug_cb’:
hw/acpi/memory_hotplug.c:267:18: error: unused variable ‘dc’
[-Werror=unused-variable]
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
                  ^~

Once removed:
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>  
> -    if (!dc->hotpluggable) {
> -        return;
> -    }
> -
>      mdev = acpi_memory_slot_status(mem_st, dev, errp);
>      if (!mdev) {
>          return;
> 

Re: [Qemu-devel] [PATCH 1/2] pc-dimm: remove check on pc-dimm hotpluggable

[Wei Yang]

On Tue, Feb 19, 2019 at 01:29:38PM +0100, Philippe Mathieu-Daudé wrote:
>On 2/19/19 7:07 AM, Wei Yang wrote:
>> Function acpi_memory_plug_cb() is only invoked when dev is a PCDIMM,
>> which is hotpluggable. This means it is not necessary to check this
>> property again.
>> 
>> This patch removes this check.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  hw/acpi/memory_hotplug.c | 4 ----
>>  1 file changed, 4 deletions(-)
>> 
>> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
>> index 8c7c1013f3..b19673d337 100644
>> --- a/hw/acpi/memory_hotplug.c
>> +++ b/hw/acpi/memory_hotplug.c
>> @@ -266,10 +266,6 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
>>      MemStatus *mdev;
>>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
>
>You have to remove the 'dc' variable now:
>
>  CC      hw/acpi/memory_hotplug.o
>hw/acpi/memory_hotplug.c: In function ‘acpi_memory_plug_cb’:
>hw/acpi/memory_hotplug.c:267:18: error: unused variable ‘dc’
>[-Werror=unused-variable]
>     DeviceClass *dc = DEVICE_GET_CLASS(dev);
>                  ^~

Thanks, I didn't notice this warning.

>
>Once removed:
>Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
>>  
>> -    if (!dc->hotpluggable) {
>> -        return;
>> -    }
>> -
>>      mdev = acpi_memory_slot_status(mem_st, dev, errp);
>>      if (!mdev) {
>>          return;
>> 

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

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);
 
     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;
-- 
2.19.1

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

[Philippe Mathieu-Daudé]

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;
> 

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

[Wei Yang]

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;
>> 

-- 
Wei Yang
Help you, Help me

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é <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;
> >   

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

[Wei Yang]

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?

-- 
Wei Yang
Help you, Help me