[Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Wei Yang]

Three trivial cleanup for pc-dimm.

Patch [1] remove the check on class->hotpluggable since pc-dimm is always
hotpluggable.
Patch [2] remove nvdimm_realize
Patch [2] remove pcdimm realize-callback

v2:
  * fix warning in Patch 1
  * split Patch 2 into two

Wei Yang (3):
  pc-dimm: remove check on pc-dimm hotpluggable
  mem/nvdimm: remove nvdimm_realize
  pc-dimm: revert "introduce 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 v2 1/3] 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

---
v2:
  * remove unused dc
---
 hw/acpi/memory_hotplug.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 8c7c1013f3..cb5284d36f 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -264,11 +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);
-
-    if (!dc->hotpluggable) {
-        return;
-    }
 
     mdev = acpi_memory_slot_status(mem_st, dev, errp);
     if (!mdev) {
-- 
2.19.1

[Qemu-devel] [PATCH v2 2/3] mem/nvdimm: remove nvdimm_realize

[Wei Yang]

nvdimm_realize is used to prepare its memory region, while this is done
in pre_plug stage.

This is time to remove it.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

---
v2: split nvdimm part here
---
 hw/mem/nvdimm.c | 11 -----------
 1 file changed, 11 deletions(-)

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

[Qemu-devel] [PATCH v2 3/3] pc-dimm: revert "introduce realize callback"

[Wei Yang]

realize callback in introduced to check if the backend memory is large
enough to contain label data and init its memory region, while this task
is handled in pre_plug stage.

Now it's time to remove it.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/mem/pc-dimm.c         | 5 -----
 include/hw/mem/pc-dimm.h | 3 ---
 2 files changed, 8 deletions(-)

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 v2 1/3] pc-dimm: remove check on pc-dimm hotpluggable

[Philippe Mathieu-Daudé]

On 2/20/19 1:51 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>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

> ---
> v2:
>   * remove unused dc
> ---
>  hw/acpi/memory_hotplug.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 8c7c1013f3..cb5284d36f 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -264,11 +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);
> -
> -    if (!dc->hotpluggable) {
> -        return;
> -    }
>  
>      mdev = acpi_memory_slot_status(mem_st, dev, errp);
>      if (!mdev) {
> 

Re: [Qemu-devel] [PATCH v2 2/3] mem/nvdimm: remove nvdimm_realize

[Philippe Mathieu-Daudé]

On 2/20/19 1:51 AM, Wei Yang wrote:
> nvdimm_realize is used to prepare its memory region, while this is done
> in pre_plug stage.

Via the device parent:

pc_dimm_pre_plug()
 -> memory_device_pre_plug()
     -> MemoryDeviceClass::get_memory_region()
        nvdimm_md_get_memory_region()

> 
> This is time to remove it.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

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

> 
> ---
> v2: split nvdimm part here
> ---
>  hw/mem/nvdimm.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> 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;
>  
> 

Re: [Qemu-devel] [PATCH v2 3/3] pc-dimm: revert "introduce realize callback"

[Philippe Mathieu-Daudé]

On 2/20/19 1:51 AM, Wei Yang wrote:
> realize callback in introduced to check if the backend memory is large
> enough to contain label data and init its memory region, while this task
> is handled in pre_plug stage.
> 
> Now it's time to remove it.

Good cleanup!

Michael, can you add:

"This reverts commit 9f318f8f7e689b9653b42bac73047f9719a1f34e."

Thanks,

Phil.

> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

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

> ---
>  hw/mem/pc-dimm.c         | 5 -----
>  include/hw/mem/pc-dimm.h | 3 ---
>  2 files changed, 8 deletions(-)
> 
> 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 v2 3/3] pc-dimm: revert "introduce realize callback"

[Wei Yang]

On Wed, Feb 20, 2019 at 02:26:16AM +0100, Philippe Mathieu-Daudé wrote:
>On 2/20/19 1:51 AM, Wei Yang wrote:
>> realize callback in introduced to check if the backend memory is large
>> enough to contain label data and init its memory region, while this task
>> is handled in pre_plug stage.
>> 
>> Now it's time to remove it.
>
>Good cleanup!
>

Glad you like it :-)

>Michael, can you add:
>
>"This reverts commit 9f318f8f7e689b9653b42bac73047f9719a1f34e."
>
>Thanks,
>
>Phil.
>
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>
>Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
>> ---
>>  hw/mem/pc-dimm.c         | 5 -----
>>  include/hw/mem/pc-dimm.h | 3 ---
>>  2 files changed, 8 deletions(-)
>> 
>> 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 v2 0/3] PCDIMM cleanup

[Xiao Guangrong]

On 2/20/19 8:51 AM, Wei Yang wrote:
> Three trivial cleanup for pc-dimm.
> 
> Patch [1] remove the check on class->hotpluggable since pc-dimm is always
> hotpluggable.
> Patch [2] remove nvdimm_realize
> Patch [2] remove pcdimm realize-callback
> 
> v2:
>    * fix warning in Patch 1
>    * split Patch 2 into two
> 
> Wei Yang (3):
>    pc-dimm: remove check on pc-dimm hotpluggable
>    mem/nvdimm: remove nvdimm_realize

>    pc-dimm: revert "introduce realize callback"

I think the word 'revert' is not so precise as that hints
the commit is bugly, instead, it was factored in the later
comments then becomes useless now.

Anyway, this pathset looks good to me.

Reviewed-by: Xiao Guangrong <xiaoguangrong@tencent.com>

Re: [Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Wei Yang]

On Thu, Feb 21, 2019 at 02:03:19PM +0800, Xiao Guangrong wrote:
>
>
>On 2/20/19 8:51 AM, Wei Yang wrote:
>> Three trivial cleanup for pc-dimm.
>> 
>> Patch [1] remove the check on class->hotpluggable since pc-dimm is always
>> hotpluggable.
>> Patch [2] remove nvdimm_realize
>> Patch [2] remove pcdimm realize-callback
>> 
>> v2:
>>    * fix warning in Patch 1
>>    * split Patch 2 into two
>> 
>> Wei Yang (3):
>>    pc-dimm: remove check on pc-dimm hotpluggable
>>    mem/nvdimm: remove nvdimm_realize
>
>>    pc-dimm: revert "introduce realize callback"
>
>I think the word 'revert' is not so precise as that hints
>the commit is bugly, instead, it was factored in the later
>comments then becomes useless now.
>

You are right. It is always difficult for me to pick up the proper word.

>Anyway, this pathset looks good to me.
>
>Reviewed-by: Xiao Guangrong <xiaoguangrong@tencent.com>

Thanks, Xiao.

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Igor Mammedov]

On Wed, 20 Feb 2019 08:51:21 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> Three trivial cleanup for pc-dimm.
> 
> Patch [1] remove the check on class->hotpluggable since pc-dimm is always
> hotpluggable.
> Patch [2] remove nvdimm_realize
> Patch [2] remove pcdimm realize-callback
even though this series doesn't break anything, I disagree with it
conceptually as it makes device less abstracted and make it more
dependent on how existing machine code uses it.
I'd drop whole series.

> 
> v2:
>   * fix warning in Patch 1
>   * split Patch 2 into two
> 
> Wei Yang (3):
>   pc-dimm: remove check on pc-dimm hotpluggable
>   mem/nvdimm: remove nvdimm_realize
>   pc-dimm: revert "introduce 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(-)
> 

Re: [Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Michael S. Tsirkin]

On Wed, Feb 20, 2019 at 08:51:21AM +0800, Wei Yang wrote:
> Three trivial cleanup for pc-dimm.
> 
> Patch [1] remove the check on class->hotpluggable since pc-dimm is always
> hotpluggable.
> Patch [2] remove nvdimm_realize
> Patch [2] remove pcdimm realize-callback

Please copy Igor on ACPI patches.

Thanks!


> v2:
>   * fix warning in Patch 1
>   * split Patch 2 into two
> 
> Wei Yang (3):
>   pc-dimm: remove check on pc-dimm hotpluggable
>   mem/nvdimm: remove nvdimm_realize
>   pc-dimm: revert "introduce 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

Re: [Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Wei Yang]

On Thu, Feb 21, 2019 at 03:50:04PM +0100, Igor Mammedov wrote:
>On Wed, 20 Feb 2019 08:51:21 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> Three trivial cleanup for pc-dimm.
>> 
>> Patch [1] remove the check on class->hotpluggable since pc-dimm is always
>> hotpluggable.
>> Patch [2] remove nvdimm_realize
>> Patch [2] remove pcdimm realize-callback
>even though this series doesn't break anything, I disagree with it
>conceptually as it makes device less abstracted and make it more
>dependent on how existing machine code uses it.
>I'd drop whole series.
>

Is Patch [1] also make device more dependent on existing implementation?

For example, when we look at the counterpart of acpi_memory_plug_cb():

    acpi_pcihp_device_plug_cb

which handle the pci device hotplug. We don't check the hotpluggable
property for pci devices.

To me, this is a general rule for PCDIMM, they are hotpluggable.

For Patch[2][3], I agree with you.

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Igor Mammedov]

On Sat, 23 Feb 2019 00:02:49 +0000
Wei Yang <richard.weiyang@gmail.com> wrote:

> On Thu, Feb 21, 2019 at 03:50:04PM +0100, Igor Mammedov wrote:
> >On Wed, 20 Feb 2019 08:51:21 +0800
> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >
> >> Three trivial cleanup for pc-dimm.
> >> 
> >> Patch [1] remove the check on class->hotpluggable since pc-dimm is always
> >> hotpluggable.
> >> Patch [2] remove nvdimm_realize
> >> Patch [2] remove pcdimm realize-callback
> >even though this series doesn't break anything, I disagree with it
> >conceptually as it makes device less abstracted and make it more
> >dependent on how existing machine code uses it.
> >I'd drop whole series.
> >
> 
> Is Patch [1] also make device more dependent on existing implementation?
yes, it's implementation detail that PCDIMM&Co are hotpluggable now.

> For example, when we look at the counterpart of acpi_memory_plug_cb():
> 
>     acpi_pcihp_device_plug_cb
> 
> which handle the pci device hotplug. We don't check the hotpluggable
> property for pci devices.
> 
> To me, this is a general rule for PCDIMM, they are hotpluggable.
yes, PCDIMMs are hotpluggable but apci device (piix4pm/ich9/...) handling hotplug
should ignore any non-hotpluggable one. If you remove check and then later
someone else would add non-hotpluggable memory device or make PC-DIMMs or its
variant of non-hotpluggable one, acpi device handling will break.
Hence I'd prefer to keep the check.  
 
> For Patch[2][3], I agree with you.
> 

Re: [Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Wei Yang]

On Mon, Feb 25, 2019 at 09:05:37AM +0100, Igor Mammedov wrote:
>On Sat, 23 Feb 2019 00:02:49 +0000
>Wei Yang <richard.weiyang@gmail.com> wrote:
>
>> On Thu, Feb 21, 2019 at 03:50:04PM +0100, Igor Mammedov wrote:
>> >On Wed, 20 Feb 2019 08:51:21 +0800
>> >Wei Yang <richardw.yang@linux.intel.com> wrote:
>> >
>> >> Three trivial cleanup for pc-dimm.
>> >> 
>> >> Patch [1] remove the check on class->hotpluggable since pc-dimm is always
>> >> hotpluggable.
>> >> Patch [2] remove nvdimm_realize
>> >> Patch [2] remove pcdimm realize-callback
>> >even though this series doesn't break anything, I disagree with it
>> >conceptually as it makes device less abstracted and make it more
>> >dependent on how existing machine code uses it.
>> >I'd drop whole series.
>> >
>> 
>> Is Patch [1] also make device more dependent on existing implementation?
>yes, it's implementation detail that PCDIMM&Co are hotpluggable now.
>
>> For example, when we look at the counterpart of acpi_memory_plug_cb():
>> 
>>     acpi_pcihp_device_plug_cb
>> 
>> which handle the pci device hotplug. We don't check the hotpluggable
>> property for pci devices.
>> 
>> To me, this is a general rule for PCDIMM, they are hotpluggable.
>yes, PCDIMMs are hotpluggable but apci device (piix4pm/ich9/...) handling hotplug
>should ignore any non-hotpluggable one. If you remove check and then later
>someone else would add non-hotpluggable memory device or make PC-DIMMs or its
>variant of non-hotpluggable one, acpi device handling will break.
>Hence I'd prefer to keep the check.  
> 

Ok, if we'd support un-hotpluggable mem device, we could retain this
check. But this maybe not a proper place.

Based on my understanding, at this point, every thing has been done
properly. If this check pass, we will send an acpi interrupt to notify
the guest. In case this is an un-hotpluggable device, every thing looks
good but no effect in guest. Because we skip the notification.

Maybe we need to move the check in pre-plug?

>> For Patch[2][3], I agree with you.
>> 

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Igor Mammedov]

On Mon, 25 Feb 2019 12:47:14 +0000
Wei Yang <richard.weiyang@gmail.com> wrote:

> On Mon, Feb 25, 2019 at 09:05:37AM +0100, Igor Mammedov wrote:
> >On Sat, 23 Feb 2019 00:02:49 +0000
> >Wei Yang <richard.weiyang@gmail.com> wrote:
> >  
> >> On Thu, Feb 21, 2019 at 03:50:04PM +0100, Igor Mammedov wrote:  
> >> >On Wed, 20 Feb 2019 08:51:21 +0800
> >> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >> >  
> >> >> Three trivial cleanup for pc-dimm.
> >> >> 
> >> >> Patch [1] remove the check on class->hotpluggable since pc-dimm is always
> >> >> hotpluggable.
> >> >> Patch [2] remove nvdimm_realize
> >> >> Patch [2] remove pcdimm realize-callback  
> >> >even though this series doesn't break anything, I disagree with it
> >> >conceptually as it makes device less abstracted and make it more
> >> >dependent on how existing machine code uses it.
> >> >I'd drop whole series.
> >> >  
> >> 
> >> Is Patch [1] also make device more dependent on existing implementation?  
> >yes, it's implementation detail that PCDIMM&Co are hotpluggable now.
> >  
> >> For example, when we look at the counterpart of acpi_memory_plug_cb():
> >> 
> >>     acpi_pcihp_device_plug_cb
> >> 
> >> which handle the pci device hotplug. We don't check the hotpluggable
> >> property for pci devices.
> >> 
> >> To me, this is a general rule for PCDIMM, they are hotpluggable.  
> >yes, PCDIMMs are hotpluggable but apci device (piix4pm/ich9/...) handling hotplug
> >should ignore any non-hotpluggable one. If you remove check and then later
> >someone else would add non-hotpluggable memory device or make PC-DIMMs or its
> >variant of non-hotpluggable one, acpi device handling will break.
> >Hence I'd prefer to keep the check.  
> >   
> 
> Ok, if we'd support un-hotpluggable mem device, we could retain this
> check. But this maybe not a proper place.
> 
> Based on my understanding, at this point, every thing has been done
> properly. If this check pass, we will send an acpi interrupt to notify
> the guest. In case this is an un-hotpluggable device, every thing looks
> good but no effect in guest. Because we skip the notification.
> 
> Maybe we need to move the check in pre-plug?
And what would happen then and afterwards?

Point is to make one of the handlers in chain to ignore plug event
(i.e. do not generate SCI event) while the rest of handlers complete
successfully mapping device in address space and whatever else.

> >> For Patch[2][3], I agree with you.
> >>   
> 

Re: [Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Wei Yang]

On Wed, Feb 27, 2019 at 02:12:42PM +0100, Igor Mammedov wrote:
>On Mon, 25 Feb 2019 12:47:14 +0000
>Wei Yang <richard.weiyang@gmail.com> wrote:
>
>> On Mon, Feb 25, 2019 at 09:05:37AM +0100, Igor Mammedov wrote:
>> >On Sat, 23 Feb 2019 00:02:49 +0000
>> >Wei Yang <richard.weiyang@gmail.com> wrote:
>> >  
>> >> On Thu, Feb 21, 2019 at 03:50:04PM +0100, Igor Mammedov wrote:  
>> >> >On Wed, 20 Feb 2019 08:51:21 +0800
>> >> >Wei Yang <richardw.yang@linux.intel.com> wrote:
>> >> >  
>> >> >> Three trivial cleanup for pc-dimm.
>> >> >> 
>> >> >> Patch [1] remove the check on class->hotpluggable since pc-dimm is always
>> >> >> hotpluggable.
>> >> >> Patch [2] remove nvdimm_realize
>> >> >> Patch [2] remove pcdimm realize-callback  
>> >> >even though this series doesn't break anything, I disagree with it
>> >> >conceptually as it makes device less abstracted and make it more
>> >> >dependent on how existing machine code uses it.
>> >> >I'd drop whole series.
>> >> >  
>> >> 
>> >> Is Patch [1] also make device more dependent on existing implementation?  
>> >yes, it's implementation detail that PCDIMM&Co are hotpluggable now.
>> >  
>> >> For example, when we look at the counterpart of acpi_memory_plug_cb():
>> >> 
>> >>     acpi_pcihp_device_plug_cb
>> >> 
>> >> which handle the pci device hotplug. We don't check the hotpluggable
>> >> property for pci devices.
>> >> 
>> >> To me, this is a general rule for PCDIMM, they are hotpluggable.  
>> >yes, PCDIMMs are hotpluggable but apci device (piix4pm/ich9/...) handling hotplug
>> >should ignore any non-hotpluggable one. If you remove check and then later
>> >someone else would add non-hotpluggable memory device or make PC-DIMMs or its
>> >variant of non-hotpluggable one, acpi device handling will break.
>> >Hence I'd prefer to keep the check.  
>> >   
>> 
>> Ok, if we'd support un-hotpluggable mem device, we could retain this
>> check. But this maybe not a proper place.
>> 
>> Based on my understanding, at this point, every thing has been done
>> properly. If this check pass, we will send an acpi interrupt to notify
>> the guest. In case this is an un-hotpluggable device, every thing looks
>> good but no effect in guest. Because we skip the notification.
>> 
>> Maybe we need to move the check in pre-plug?
>And what would happen then and afterwards?
>
>Point is to make one of the handlers in chain to ignore plug event
>(i.e. do not generate SCI event) while the rest of handlers complete
>successfully mapping device in address space and whatever else.
>

This will have a well prepared device in system but guest is not
notified, right?

My idea to move the check in pre-plug will result the qemu return when
it see no SCI event will be generated, so there is no device created.

I guess current behavior is a designed decision?

>> >> For Patch[2][3], I agree with you.
>> >>   
>> 

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Igor Mammedov]

On Wed, 27 Feb 2019 13:59:20 +0000
Wei Yang <richard.weiyang@gmail.com> wrote:

> On Wed, Feb 27, 2019 at 02:12:42PM +0100, Igor Mammedov wrote:
> >On Mon, 25 Feb 2019 12:47:14 +0000
> >Wei Yang <richard.weiyang@gmail.com> wrote:
> >  
> >> On Mon, Feb 25, 2019 at 09:05:37AM +0100, Igor Mammedov wrote:  
> >> >On Sat, 23 Feb 2019 00:02:49 +0000
> >> >Wei Yang <richard.weiyang@gmail.com> wrote:
> >> >    
> >> >> On Thu, Feb 21, 2019 at 03:50:04PM +0100, Igor Mammedov wrote:    
> >> >> >On Wed, 20 Feb 2019 08:51:21 +0800
> >> >> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >> >> >    
> >> >> >> Three trivial cleanup for pc-dimm.
> >> >> >> 
> >> >> >> Patch [1] remove the check on class->hotpluggable since pc-dimm is always
> >> >> >> hotpluggable.
> >> >> >> Patch [2] remove nvdimm_realize
> >> >> >> Patch [2] remove pcdimm realize-callback    
> >> >> >even though this series doesn't break anything, I disagree with it
> >> >> >conceptually as it makes device less abstracted and make it more
> >> >> >dependent on how existing machine code uses it.
> >> >> >I'd drop whole series.
> >> >> >    
> >> >> 
> >> >> Is Patch [1] also make device more dependent on existing implementation?    
> >> >yes, it's implementation detail that PCDIMM&Co are hotpluggable now.
> >> >    
> >> >> For example, when we look at the counterpart of acpi_memory_plug_cb():
> >> >> 
> >> >>     acpi_pcihp_device_plug_cb
> >> >> 
> >> >> which handle the pci device hotplug. We don't check the hotpluggable
> >> >> property for pci devices.
> >> >> 
> >> >> To me, this is a general rule for PCDIMM, they are hotpluggable.    
> >> >yes, PCDIMMs are hotpluggable but apci device (piix4pm/ich9/...) handling hotplug
> >> >should ignore any non-hotpluggable one. If you remove check and then later
> >> >someone else would add non-hotpluggable memory device or make PC-DIMMs or its
> >> >variant of non-hotpluggable one, acpi device handling will break.
> >> >Hence I'd prefer to keep the check.  
> >> >     
> >> 
> >> Ok, if we'd support un-hotpluggable mem device, we could retain this
> >> check. But this maybe not a proper place.
> >> 
> >> Based on my understanding, at this point, every thing has been done
> >> properly. If this check pass, we will send an acpi interrupt to notify
> >> the guest. In case this is an un-hotpluggable device, every thing looks
> >> good but no effect in guest. Because we skip the notification.
> >> 
> >> Maybe we need to move the check in pre-plug?  
> >And what would happen then and afterwards?
> >
> >Point is to make one of the handlers in chain to ignore plug event
> >(i.e. do not generate SCI event) while the rest of handlers complete
> >successfully mapping device in address space and whatever else.
> >  
> 
> This will have a well prepared device in system but guest is not
> notified, right?
yes, it's theoretically possible to move check up the call stack
to machine level and not call secondary hotplug handler on non hotplugged
device but that again depends on what secondary hotplug handler does.
I'd rather keep logic independent here unless there is good reason not
to do so.


> My idea to move the check in pre-plug will result the qemu return when
> it see no SCI event will be generated, so there is no device created.
> 
> I guess current behavior is a designed decision?
I'd say so.
PS:
QEMU's hotplug_hadler API is misnamed at this point as it handles both
cold (basic device wiring) and hotplug (processing hotplug).
Maybe we should rename it but I'm not irritated enough by it to do so.

> >> >> For Patch[2][3], I agree with you.
> >> >>     
> >>   
> 

Re: [Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Wei Yang]

On Wed, Feb 27, 2019 at 06:27:49PM +0100, Igor Mammedov wrote:
>On Wed, 27 Feb 2019 13:59:20 +0000
>Wei Yang <richard.weiyang@gmail.com> wrote:
>
>> On Wed, Feb 27, 2019 at 02:12:42PM +0100, Igor Mammedov wrote:
>> >On Mon, 25 Feb 2019 12:47:14 +0000
>> >Wei Yang <richard.weiyang@gmail.com> wrote:
>> >  
>> >> On Mon, Feb 25, 2019 at 09:05:37AM +0100, Igor Mammedov wrote:  
>> >> >On Sat, 23 Feb 2019 00:02:49 +0000
>> >> >Wei Yang <richard.weiyang@gmail.com> wrote:
>> >> >    
>> >> >> On Thu, Feb 21, 2019 at 03:50:04PM +0100, Igor Mammedov wrote:    
>> >> >> >On Wed, 20 Feb 2019 08:51:21 +0800
>> >> >> >Wei Yang <richardw.yang@linux.intel.com> wrote:
>> >> >> >    
>> >> >> >> Three trivial cleanup for pc-dimm.
>> >> >> >> 
>> >> >> >> Patch [1] remove the check on class->hotpluggable since pc-dimm is always
>> >> >> >> hotpluggable.
>> >> >> >> Patch [2] remove nvdimm_realize
>> >> >> >> Patch [2] remove pcdimm realize-callback    
>> >> >> >even though this series doesn't break anything, I disagree with it
>> >> >> >conceptually as it makes device less abstracted and make it more
>> >> >> >dependent on how existing machine code uses it.
>> >> >> >I'd drop whole series.
>> >> >> >    
>> >> >> 
>> >> >> Is Patch [1] also make device more dependent on existing implementation?    
>> >> >yes, it's implementation detail that PCDIMM&Co are hotpluggable now.
>> >> >    
>> >> >> For example, when we look at the counterpart of acpi_memory_plug_cb():
>> >> >> 
>> >> >>     acpi_pcihp_device_plug_cb
>> >> >> 
>> >> >> which handle the pci device hotplug. We don't check the hotpluggable
>> >> >> property for pci devices.
>> >> >> 
>> >> >> To me, this is a general rule for PCDIMM, they are hotpluggable.    
>> >> >yes, PCDIMMs are hotpluggable but apci device (piix4pm/ich9/...) handling hotplug
>> >> >should ignore any non-hotpluggable one. If you remove check and then later
>> >> >someone else would add non-hotpluggable memory device or make PC-DIMMs or its
>> >> >variant of non-hotpluggable one, acpi device handling will break.
>> >> >Hence I'd prefer to keep the check.  
>> >> >     
>> >> 
>> >> Ok, if we'd support un-hotpluggable mem device, we could retain this
>> >> check. But this maybe not a proper place.
>> >> 
>> >> Based on my understanding, at this point, every thing has been done
>> >> properly. If this check pass, we will send an acpi interrupt to notify
>> >> the guest. In case this is an un-hotpluggable device, every thing looks
>> >> good but no effect in guest. Because we skip the notification.
>> >> 
>> >> Maybe we need to move the check in pre-plug?  
>> >And what would happen then and afterwards?
>> >
>> >Point is to make one of the handlers in chain to ignore plug event
>> >(i.e. do not generate SCI event) while the rest of handlers complete
>> >successfully mapping device in address space and whatever else.
>> >  
>> 
>> This will have a well prepared device in system but guest is not
>> notified, right?
>yes, it's theoretically possible to move check up the call stack
>to machine level and not call secondary hotplug handler on non hotplugged
>device but that again depends on what secondary hotplug handler does.
>I'd rather keep logic independent here unless there is good reason not
>to do so.
>
>
>> My idea to move the check in pre-plug will result the qemu return when
>> it see no SCI event will be generated, so there is no device created.
>> 
>> I guess current behavior is a designed decision?
>I'd say so.
>PS:
>QEMU's hotplug_hadler API is misnamed at this point as it handles both
>cold (basic device wiring) and hotplug (processing hotplug).
>Maybe we should rename it but I'm not irritated enough by it to do so.
>

Got it, thanks.

>> >> >> For Patch[2][3], I agree with you.
>> >> >>     
>> >>   
>> 

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Wei Yang]

On Wed, Feb 27, 2019 at 06:27:49PM +0100, Igor Mammedov wrote:
>On Wed, 27 Feb 2019 13:59:20 +0000
>Wei Yang <richard.weiyang@gmail.com> wrote:
>
>> On Wed, Feb 27, 2019 at 02:12:42PM +0100, Igor Mammedov wrote:
>> >On Mon, 25 Feb 2019 12:47:14 +0000
>> >Wei Yang <richard.weiyang@gmail.com> wrote:
>> >  
>> >> >> To me, this is a general rule for PCDIMM, they are hotpluggable.    
>> >> >yes, PCDIMMs are hotpluggable but apci device (piix4pm/ich9/...) handling hotplug
>> >> >should ignore any non-hotpluggable one. If you remove check and then later
>> >> >someone else would add non-hotpluggable memory device or make PC-DIMMs or its
>> >> >variant of non-hotpluggable one, acpi device handling will break.
>> >> >Hence I'd prefer to keep the check.  
>> >> >     
>> >> 
>> >> Ok, if we'd support un-hotpluggable mem device, we could retain this
>> >> check. But this maybe not a proper place.
>> >> 
>> >> Based on my understanding, at this point, every thing has been done
>> >> properly. If this check pass, we will send an acpi interrupt to notify
>> >> the guest. In case this is an un-hotpluggable device, every thing looks
>> >> good but no effect in guest. Because we skip the notification.
>> >> 
>> >> Maybe we need to move the check in pre-plug?  
>> >And what would happen then and afterwards?
>> >
>> >Point is to make one of the handlers in chain to ignore plug event
>> >(i.e. do not generate SCI event) while the rest of handlers complete
>> >successfully mapping device in address space and whatever else.
>> >  
>> 
>> This will have a well prepared device in system but guest is not
>> notified, right?
>yes, it's theoretically possible to move check up the call stack
>to machine level and not call secondary hotplug handler on non hotplugged
>device but that again depends on what secondary hotplug handler does.
>I'd rather keep logic independent here unless there is good reason not
>to do so.
>
>
>> My idea to move the check in pre-plug will result the qemu return when
>> it see no SCI event will be generated, so there is no device created.
>> 
>> I guess current behavior is a designed decision?
>I'd say so.
>PS:
>QEMU's hotplug_hadler API is misnamed at this point as it handles both
>cold (basic device wiring) and hotplug (processing hotplug).
>Maybe we should rename it but I'm not irritated enough by it to do so.
>

After re-read the code, I found something more.

First let me describe my understanding a bit.

To hotplug a device, several part are related:

    * device itself
    * machine's hotplug_handler
    * bus's hotplug_handler
    * acpi configuration

Currently, some checks are mixed, which makes the logic not that clear.

Let's come back to the problem in this thread.

The check in concern is the device's hotpluggable property. And when we look
into device_set_realized, this property is already checked at the very
beginning. This means when we go all the way down to acpi_memory_plug_cb(), if
this device is un-hotpluggable, it is must not a hotplugged device. And the
acpi_send_event() will not be triggered.

Based on my understanding, mdev->dimm and mdev->is_enabld looks still
necessary to be set for a un-hotplugged device. For both hotpluggable and
un-hotpluggable device. Skip these two steps if the device is not hotpluggable
looks not consistent with others?

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Igor Mammedov]

On Thu, 28 Feb 2019 08:46:10 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Wed, Feb 27, 2019 at 06:27:49PM +0100, Igor Mammedov wrote:
> >On Wed, 27 Feb 2019 13:59:20 +0000
> >Wei Yang <richard.weiyang@gmail.com> wrote:
> >  
> >> On Wed, Feb 27, 2019 at 02:12:42PM +0100, Igor Mammedov wrote:  
> >> >On Mon, 25 Feb 2019 12:47:14 +0000
> >> >Wei Yang <richard.weiyang@gmail.com> wrote:
> >> >    
> >> >> >> To me, this is a general rule for PCDIMM, they are hotpluggable.      
> >> >> >yes, PCDIMMs are hotpluggable but apci device (piix4pm/ich9/...) handling hotplug
> >> >> >should ignore any non-hotpluggable one. If you remove check and then later
> >> >> >someone else would add non-hotpluggable memory device or make PC-DIMMs or its
> >> >> >variant of non-hotpluggable one, acpi device handling will break.
> >> >> >Hence I'd prefer to keep the check.  
> >> >> >       
> >> >> 
> >> >> Ok, if we'd support un-hotpluggable mem device, we could retain this
> >> >> check. But this maybe not a proper place.
> >> >> 
> >> >> Based on my understanding, at this point, every thing has been done
> >> >> properly. If this check pass, we will send an acpi interrupt to notify
> >> >> the guest. In case this is an un-hotpluggable device, every thing looks
> >> >> good but no effect in guest. Because we skip the notification.
> >> >> 
> >> >> Maybe we need to move the check in pre-plug?    
> >> >And what would happen then and afterwards?
> >> >
> >> >Point is to make one of the handlers in chain to ignore plug event
> >> >(i.e. do not generate SCI event) while the rest of handlers complete
> >> >successfully mapping device in address space and whatever else.
> >> >    
> >> 
> >> This will have a well prepared device in system but guest is not
> >> notified, right?  
> >yes, it's theoretically possible to move check up the call stack
> >to machine level and not call secondary hotplug handler on non hotplugged
> >device but that again depends on what secondary hotplug handler does.
> >I'd rather keep logic independent here unless there is good reason not
> >to do so.
> >
> >  
> >> My idea to move the check in pre-plug will result the qemu return when
> >> it see no SCI event will be generated, so there is no device created.
> >> 
> >> I guess current behavior is a designed decision?  
> >I'd say so.
> >PS:
> >QEMU's hotplug_hadler API is misnamed at this point as it handles both
> >cold (basic device wiring) and hotplug (processing hotplug).
> >Maybe we should rename it but I'm not irritated enough by it to do so.
> >  
> 
> After re-read the code, I found something more.
> 
> First let me describe my understanding a bit.
> 
> To hotplug a device, several part are related:
> 
>     * device itself
>     * machine's hotplug_handler
>     * bus's hotplug_handler
>     * acpi configuration
> 
> Currently, some checks are mixed, which makes the logic not that clear.
> 
> Let's come back to the problem in this thread.
> 
> The check in concern is the device's hotpluggable property. And when we look
> into device_set_realized, this property is already checked at the very
> beginning. This means when we go all the way down to acpi_memory_plug_cb(), if
> this device is un-hotpluggable, it is must not a hotplugged device. And the
> acpi_send_event() will not be triggered.
> 
> Based on my understanding, mdev->dimm and mdev->is_enabld looks still
> necessary to be set for a un-hotplugged device. For both hotpluggable and
> un-hotpluggable device. Skip these two steps if the device is not hotpluggable
> looks not consistent with others?
it might be inconsistent and broken since we don't actually have
a nonhotpluggable memory device yet. But I'd would fix it when such device
is added (it might depend on being added device whether it needs to be tracked
by acpi memory hotplug path or if it uses other means in which case check
is correct)

Re: [Qemu-devel] [PATCH v2 0/3] PCDIMM cleanup

[Wei Yang]

On Thu, Feb 28, 2019 at 02:57:07PM +0100, Igor Mammedov wrote:
>On Thu, 28 Feb 2019 08:46:10 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> On Wed, Feb 27, 2019 at 06:27:49PM +0100, Igor Mammedov wrote:
>> >On Wed, 27 Feb 2019 13:59:20 +0000
>> >Wei Yang <richard.weiyang@gmail.com> wrote:
>> >  
>> >> On Wed, Feb 27, 2019 at 02:12:42PM +0100, Igor Mammedov wrote:  
>> >> >On Mon, 25 Feb 2019 12:47:14 +0000
>> >> >Wei Yang <richard.weiyang@gmail.com> wrote:
>> >> >    
>> >> >> >> To me, this is a general rule for PCDIMM, they are hotpluggable.      
>> >> >> >yes, PCDIMMs are hotpluggable but apci device (piix4pm/ich9/...) handling hotplug
>> >> >> >should ignore any non-hotpluggable one. If you remove check and then later
>> >> >> >someone else would add non-hotpluggable memory device or make PC-DIMMs or its
>> >> >> >variant of non-hotpluggable one, acpi device handling will break.
>> >> >> >Hence I'd prefer to keep the check.  
>> >> >> >       
>> >> >> 
>> >> >> Ok, if we'd support un-hotpluggable mem device, we could retain this
>> >> >> check. But this maybe not a proper place.
>> >> >> 
>> >> >> Based on my understanding, at this point, every thing has been done
>> >> >> properly. If this check pass, we will send an acpi interrupt to notify
>> >> >> the guest. In case this is an un-hotpluggable device, every thing looks
>> >> >> good but no effect in guest. Because we skip the notification.
>> >> >> 
>> >> >> Maybe we need to move the check in pre-plug?    
>> >> >And what would happen then and afterwards?
>> >> >
>> >> >Point is to make one of the handlers in chain to ignore plug event
>> >> >(i.e. do not generate SCI event) while the rest of handlers complete
>> >> >successfully mapping device in address space and whatever else.
>> >> >    
>> >> 
>> >> This will have a well prepared device in system but guest is not
>> >> notified, right?  
>> >yes, it's theoretically possible to move check up the call stack
>> >to machine level and not call secondary hotplug handler on non hotplugged
>> >device but that again depends on what secondary hotplug handler does.
>> >I'd rather keep logic independent here unless there is good reason not
>> >to do so.
>> >
>> >  
>> >> My idea to move the check in pre-plug will result the qemu return when
>> >> it see no SCI event will be generated, so there is no device created.
>> >> 
>> >> I guess current behavior is a designed decision?  
>> >I'd say so.
>> >PS:
>> >QEMU's hotplug_hadler API is misnamed at this point as it handles both
>> >cold (basic device wiring) and hotplug (processing hotplug).
>> >Maybe we should rename it but I'm not irritated enough by it to do so.
>> >  
>> 
>> After re-read the code, I found something more.
>> 
>> First let me describe my understanding a bit.
>> 
>> To hotplug a device, several part are related:
>> 
>>     * device itself
>>     * machine's hotplug_handler
>>     * bus's hotplug_handler
>>     * acpi configuration
>> 
>> Currently, some checks are mixed, which makes the logic not that clear.
>> 
>> Let's come back to the problem in this thread.
>> 
>> The check in concern is the device's hotpluggable property. And when we look
>> into device_set_realized, this property is already checked at the very
>> beginning. This means when we go all the way down to acpi_memory_plug_cb(), if
>> this device is un-hotpluggable, it is must not a hotplugged device. And the
>> acpi_send_event() will not be triggered.
>> 
>> Based on my understanding, mdev->dimm and mdev->is_enabld looks still
>> necessary to be set for a un-hotplugged device. For both hotpluggable and
>> un-hotpluggable device. Skip these two steps if the device is not hotpluggable
>> looks not consistent with others?
>it might be inconsistent and broken since we don't actually have
>a nonhotpluggable memory device yet. But I'd would fix it when such device
>is added (it might depend on being added device whether it needs to be tracked
>by acpi memory hotplug path or if it uses other means in which case check
>is correct)
>

Ok, got your point.

-- 
Wei Yang
Help you, Help me