[Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's

[Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's

[Wei Yang]

pc_memory_pre_plug() is called during hotplug for both pc-dimm and
nvdimm. This is more proper to check apci hotplug capability before
check nvdimm specific capability.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/i386/pc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f2c15bf1f2..d48b6f9582 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2091,17 +2091,17 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         return;
     }
 
-    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
-        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
-        return;
-    }
-
     hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
 
+    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
+        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
+        return;
+    }
+
     pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
                      pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
 }
-- 
2.19.1

[Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's

[Wei Yang]

pc_memory_pre_plug() is called during hotplug for both pc-dimm and
nvdimm. This is more proper to check apci hotplug capability before
check nvdimm specific capability.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/i386/pc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f2c15bf1f2..d48b6f9582 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2091,17 +2091,17 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         return;
     }
 
-    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
-        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
-        return;
-    }
-
     hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
 
+    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
+        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
+        return;
+    }
+
     pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
                      pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
 }
-- 
2.19.1

Re: [Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's

[Thomas Huth]

On 11/04/2019 09.17, Wei Yang wrote:
> pc_memory_pre_plug() is called during hotplug for both pc-dimm and
> nvdimm. This is more proper to check apci hotplug capability before
> check nvdimm specific capability.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  hw/i386/pc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f2c15bf1f2..d48b6f9582 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2091,17 +2091,17 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
> -        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> -        return;
> -    }
> -
>      hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> +    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
> +        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> +        return;
> +    }
> +
>      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
>                       pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
>  }

Works fine for me, too (i.e. no crash with older machine types).

Tested-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's

[Thomas Huth]

On 11/04/2019 09.17, Wei Yang wrote:
> pc_memory_pre_plug() is called during hotplug for both pc-dimm and
> nvdimm. This is more proper to check apci hotplug capability before
> check nvdimm specific capability.
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  hw/i386/pc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f2c15bf1f2..d48b6f9582 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2091,17 +2091,17 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
> -        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> -        return;
> -    }
> -
>      hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> +    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
> +        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> +        return;
> +    }
> +
>      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
>                       pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
>  }

Works fine for me, too (i.e. no crash with older machine types).

Tested-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's

[Wei Yang]

On Thu, Apr 11, 2019 at 10:32:39AM +0200, Thomas Huth wrote:
>On 11/04/2019 09.17, Wei Yang wrote:
>> pc_memory_pre_plug() is called during hotplug for both pc-dimm and
>> nvdimm. This is more proper to check apci hotplug capability before
>> check nvdimm specific capability.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  hw/i386/pc.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index f2c15bf1f2..d48b6f9582 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -2091,17 +2091,17 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>          return;
>>      }
>>  
>> -    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
>> -        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>> -        return;
>> -    }
>> -
>>      hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err);
>>      if (local_err) {
>>          error_propagate(errp, local_err);
>>          return;
>>      }
>>  
>> +    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
>> +        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>> +        return;
>> +    }
>> +
>>      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
>>                       pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
>>  }
>
>Works fine for me, too (i.e. no crash with older machine types).
>
>Tested-by: Thomas Huth <thuth@redhat.com>
>

Thanks :-)

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's

[Wei Yang]

On Thu, Apr 11, 2019 at 10:32:39AM +0200, Thomas Huth wrote:
>On 11/04/2019 09.17, Wei Yang wrote:
>> pc_memory_pre_plug() is called during hotplug for both pc-dimm and
>> nvdimm. This is more proper to check apci hotplug capability before
>> check nvdimm specific capability.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  hw/i386/pc.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index f2c15bf1f2..d48b6f9582 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -2091,17 +2091,17 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>          return;
>>      }
>>  
>> -    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
>> -        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>> -        return;
>> -    }
>> -
>>      hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err);
>>      if (local_err) {
>>          error_propagate(errp, local_err);
>>          return;
>>      }
>>  
>> +    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
>> +        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>> +        return;
>> +    }
>> +
>>      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
>>                       pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
>>  }
>
>Works fine for me, too (i.e. no crash with older machine types).
>
>Tested-by: Thomas Huth <thuth@redhat.com>
>

Thanks :-)

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's

[Wei Yang]

On Thu, Apr 11, 2019 at 10:32:39AM +0200, Thomas Huth wrote:
>On 11/04/2019 09.17, Wei Yang wrote:
>> pc_memory_pre_plug() is called during hotplug for both pc-dimm and
>> nvdimm. This is more proper to check apci hotplug capability before
>> check nvdimm specific capability.
>> 
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>>  hw/i386/pc.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index f2c15bf1f2..d48b6f9582 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -2091,17 +2091,17 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>          return;
>>      }
>>  
>> -    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
>> -        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>> -        return;
>> -    }
>> -
>>      hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err);
>>      if (local_err) {
>>          error_propagate(errp, local_err);
>>          return;
>>      }
>>  
>> +    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
>> +        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>> +        return;
>> +    }
>> +
>>      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
>>                       pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
>>  }
>
>Works fine for me, too (i.e. no crash with older machine types).
>
>Tested-by: Thomas Huth <thuth@redhat.com>
>

Would someone pick up this one :-)

Thanks

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's

[Igor Mammedov]

On Thu, 11 Apr 2019 15:17:39 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> pc_memory_pre_plug() is called during hotplug for both pc-dimm and
> nvdimm. This is more proper to check apci hotplug capability before
> check nvdimm specific capability.
not sure what this about.
Currently we are checking if ACPI is enabled
  if (!pcms->acpi_dev || !acpi_enabled) { ...
before nvdimm check and it looks better to me that we cancel
nvdimm hotplug earlier than passing it to
    hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err)
with this patch ACPI device handler will be called before
nvdimm check happens, so it's +1 unnecessary call chain in
the case of nvdimm, which I'd rather not have.

Are there any issues with current call flow?
(commit message doesn't really explaining why we need this patch)

> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  hw/i386/pc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f2c15bf1f2..d48b6f9582 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2091,17 +2091,17 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
> -        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> -        return;
> -    }
> -
>      hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> +    if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
> +        error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
> +        return;
> +    }
> +
>      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
>                       pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
>  }

Re: [Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's

[Wei Yang]

On Mon, May 27, 2019 at 02:21:14PM +0200, Igor Mammedov wrote:
>On Thu, 11 Apr 2019 15:17:39 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> pc_memory_pre_plug() is called during hotplug for both pc-dimm and
>> nvdimm. This is more proper to check apci hotplug capability before
>> check nvdimm specific capability.
>not sure what this about.
>Currently we are checking if ACPI is enabled
>  if (!pcms->acpi_dev || !acpi_enabled) { ...
>before nvdimm check and it looks better to me that we cancel
>nvdimm hotplug earlier than passing it to
>    hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err)
>with this patch ACPI device handler will be called before
>nvdimm check happens, so it's +1 unnecessary call chain in
>the case of nvdimm, which I'd rather not have.
>
>Are there any issues with current call flow?
>(commit message doesn't really explaining why we need this patch)
>

My idea is to check more generic requirement and then specific one.

For example, the call flow looks like this:

pc_memory_pre_plug

    piix4_device_pre_plug_cb | ich9_pm_device_pre_plug_cb
        if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
            !lpc->pm.acpi_memory_hotplug.is_enabled)

    if (is_nvdimm && !ms->nvdimms_state->is_enabled)
    

In hotplug_handler_pre_plug(), it checks the acpi hotplug capability. And then
if it has memory hotplug capability and is nvdimm, we check whether nvdimm is
enabled.

This is why I suggest to change the order here. No functional issue for
current code.

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's

[Igor Mammedov]

On Tue, 28 May 2019 09:35:48 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Mon, May 27, 2019 at 02:21:14PM +0200, Igor Mammedov wrote:
> >On Thu, 11 Apr 2019 15:17:39 +0800
> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >  
> >> pc_memory_pre_plug() is called during hotplug for both pc-dimm and
> >> nvdimm. This is more proper to check apci hotplug capability before
> >> check nvdimm specific capability.  
> >not sure what this about.
> >Currently we are checking if ACPI is enabled
> >  if (!pcms->acpi_dev || !acpi_enabled) { ...
> >before nvdimm check and it looks better to me that we cancel
> >nvdimm hotplug earlier than passing it to
> >    hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err)
> >with this patch ACPI device handler will be called before
> >nvdimm check happens, so it's +1 unnecessary call chain in
> >the case of nvdimm, which I'd rather not have.
> >
> >Are there any issues with current call flow?
> >(commit message doesn't really explaining why we need this patch)
> >  
> 
> My idea is to check more generic requirement and then specific one.
> 
> For example, the call flow looks like this:
> 
> pc_memory_pre_plug
> 
>     piix4_device_pre_plug_cb | ich9_pm_device_pre_plug_cb
>         if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
>             !lpc->pm.acpi_memory_hotplug.is_enabled)
> 
>     if (is_nvdimm && !ms->nvdimms_state->is_enabled)
>     
> 
> In hotplug_handler_pre_plug(), it checks the acpi hotplug capability. And then
> if it has memory hotplug capability and is nvdimm, we check whether nvdimm is
> enabled.

I don't think pc_memory_pre_plug() should rely on what hotplug_handler_pre_plug()
checks or does. Similarly the later is taking care of whatever piix4 needs to care
and shouldn't care about what machine code does.

Moreover when hotplug_handler_pre_plug() starts to reserve resources, then
if you move check as suggested you'd need to rollback all that
hotplug_handler_pre_plug() done to gracefully abort hotplug.

So I'd leave the code as it is now, since it doesn't depend on concrete
hotplug_handler_pre_plug() implementation and won't break if
hotplug_handler_pre_plug() will start consuming resources (which could
happen and you won't even notice it since changed code is in piix4/q35
files when reviewing patches).

> This is why I suggest to change the order here. No functional issue for
> current code.
> 

Re: [Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's

[Wei Yang]

On Tue, May 28, 2019 at 02:26:27PM +0200, Igor Mammedov wrote:
>On Tue, 28 May 2019 09:35:48 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> On Mon, May 27, 2019 at 02:21:14PM +0200, Igor Mammedov wrote:
>> >On Thu, 11 Apr 2019 15:17:39 +0800
>> >Wei Yang <richardw.yang@linux.intel.com> wrote:
>> >  
>> >> pc_memory_pre_plug() is called during hotplug for both pc-dimm and
>> >> nvdimm. This is more proper to check apci hotplug capability before
>> >> check nvdimm specific capability.  
>> >not sure what this about.
>> >Currently we are checking if ACPI is enabled
>> >  if (!pcms->acpi_dev || !acpi_enabled) { ...
>> >before nvdimm check and it looks better to me that we cancel
>> >nvdimm hotplug earlier than passing it to
>> >    hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err)
>> >with this patch ACPI device handler will be called before
>> >nvdimm check happens, so it's +1 unnecessary call chain in
>> >the case of nvdimm, which I'd rather not have.
>> >
>> >Are there any issues with current call flow?
>> >(commit message doesn't really explaining why we need this patch)
>> >  
>> 
>> My idea is to check more generic requirement and then specific one.
>> 
>> For example, the call flow looks like this:
>> 
>> pc_memory_pre_plug
>> 
>>     piix4_device_pre_plug_cb | ich9_pm_device_pre_plug_cb
>>         if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
>>             !lpc->pm.acpi_memory_hotplug.is_enabled)
>> 
>>     if (is_nvdimm && !ms->nvdimms_state->is_enabled)
>>     
>> 
>> In hotplug_handler_pre_plug(), it checks the acpi hotplug capability. And then
>> if it has memory hotplug capability and is nvdimm, we check whether nvdimm is
>> enabled.
>
>I don't think pc_memory_pre_plug() should rely on what hotplug_handler_pre_plug()
>checks or does. Similarly the later is taking care of whatever piix4 needs to care
>and shouldn't care about what machine code does.
>

Agree. It is not proper to let hotplug_handler_pre_plug() take care about
machine code.

>Moreover when hotplug_handler_pre_plug() starts to reserve resources, then
>if you move check as suggested you'd need to rollback all that
>hotplug_handler_pre_plug() done to gracefully abort hotplug.
>

Confused.

hotplug_handler_pre_plug() doesn't reserve resources.
pc_dimm_pre_plug() does.

I didn't plan to move the code after pc_dimm_pre_plug().

>So I'd leave the code as it is now, since it doesn't depend on concrete
>hotplug_handler_pre_plug() implementation and won't break if
>hotplug_handler_pre_plug() will start consuming resources (which could
>happen and you won't even notice it since changed code is in piix4/q35
>files when reviewing patches).
>> This is why I suggest to change the order here. No functional issue for
>> current code.
>> 

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's

[Igor Mammedov]

On Wed, 29 May 2019 08:32:14 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Tue, May 28, 2019 at 02:26:27PM +0200, Igor Mammedov wrote:
> >On Tue, 28 May 2019 09:35:48 +0800
> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >  
> >> On Mon, May 27, 2019 at 02:21:14PM +0200, Igor Mammedov wrote:  
> >> >On Thu, 11 Apr 2019 15:17:39 +0800
> >> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >> >    
> >> >> pc_memory_pre_plug() is called during hotplug for both pc-dimm and
> >> >> nvdimm. This is more proper to check apci hotplug capability before
> >> >> check nvdimm specific capability.    
> >> >not sure what this about.
> >> >Currently we are checking if ACPI is enabled
> >> >  if (!pcms->acpi_dev || !acpi_enabled) { ...
> >> >before nvdimm check and it looks better to me that we cancel
> >> >nvdimm hotplug earlier than passing it to
> >> >    hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err)
> >> >with this patch ACPI device handler will be called before
> >> >nvdimm check happens, so it's +1 unnecessary call chain in
> >> >the case of nvdimm, which I'd rather not have.
> >> >
> >> >Are there any issues with current call flow?
> >> >(commit message doesn't really explaining why we need this patch)
> >> >    
> >> 
> >> My idea is to check more generic requirement and then specific one.
> >> 
> >> For example, the call flow looks like this:
> >> 
> >> pc_memory_pre_plug
> >> 
> >>     piix4_device_pre_plug_cb | ich9_pm_device_pre_plug_cb
> >>         if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
> >>             !lpc->pm.acpi_memory_hotplug.is_enabled)
> >> 
> >>     if (is_nvdimm && !ms->nvdimms_state->is_enabled)
> >>     
> >> 
> >> In hotplug_handler_pre_plug(), it checks the acpi hotplug capability. And then
> >> if it has memory hotplug capability and is nvdimm, we check whether nvdimm is
> >> enabled.  
> >
> >I don't think pc_memory_pre_plug() should rely on what hotplug_handler_pre_plug()
> >checks or does. Similarly the later is taking care of whatever piix4 needs to care
> >and shouldn't care about what machine code does.
> >  
> 
> Agree. It is not proper to let hotplug_handler_pre_plug() take care about
> machine code.
> 
> >Moreover when hotplug_handler_pre_plug() starts to reserve resources, then
> >if you move check as suggested you'd need to rollback all that
> >hotplug_handler_pre_plug() done to gracefully abort hotplug.
> >  
> 
> Confused.
> 
> hotplug_handler_pre_plug() doesn't reserve resources.


it's not currently, but if it would it would not work with your patch properly
or break unexpectedly since whoever would change hotplug_handler_pre_plug()
might not notice that machine code need to be taken care of.

Try to consider devices and machine as separate libraries. Which should
in reasonable limits be independent and work through documented interfaces.
In that case likehood of breaking something would be less than relying on
current code impl./call order with implicit inter-dependencies. 

> pc_dimm_pre_plug() does.
> 
> I didn't plan to move the code after pc_dimm_pre_plug().
> 
> >So I'd leave the code as it is now, since it doesn't depend on concrete
> >hotplug_handler_pre_plug() implementation and won't break if
> >hotplug_handler_pre_plug() will start consuming resources (which could
> >happen and you won't even notice it since changed code is in piix4/q35
> >files when reviewing patches).  
> >> This is why I suggest to change the order here. No functional issue for
> >> current code.
> >>   
> 

Re: [Qemu-devel] [PATCH] hw/i386/pc: check apci hotplug capability before nvdimm's

[Wei Yang]

On Wed, May 29, 2019 at 10:57:50AM +0200, Igor Mammedov wrote:
>On Wed, 29 May 2019 08:32:14 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> On Tue, May 28, 2019 at 02:26:27PM +0200, Igor Mammedov wrote:
>> >On Tue, 28 May 2019 09:35:48 +0800
>> >Wei Yang <richardw.yang@linux.intel.com> wrote:
>> >  
>> >> On Mon, May 27, 2019 at 02:21:14PM +0200, Igor Mammedov wrote:  
>> >> >On Thu, 11 Apr 2019 15:17:39 +0800
>> >> >Wei Yang <richardw.yang@linux.intel.com> wrote:
>> >> >    
>> >> >> pc_memory_pre_plug() is called during hotplug for both pc-dimm and
>> >> >> nvdimm. This is more proper to check apci hotplug capability before
>> >> >> check nvdimm specific capability.    
>> >> >not sure what this about.
>> >> >Currently we are checking if ACPI is enabled
>> >> >  if (!pcms->acpi_dev || !acpi_enabled) { ...
>> >> >before nvdimm check and it looks better to me that we cancel
>> >> >nvdimm hotplug earlier than passing it to
>> >> >    hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err)
>> >> >with this patch ACPI device handler will be called before
>> >> >nvdimm check happens, so it's +1 unnecessary call chain in
>> >> >the case of nvdimm, which I'd rather not have.
>> >> >
>> >> >Are there any issues with current call flow?
>> >> >(commit message doesn't really explaining why we need this patch)
>> >> >    
>> >> 
>> >> My idea is to check more generic requirement and then specific one.
>> >> 
>> >> For example, the call flow looks like this:
>> >> 
>> >> pc_memory_pre_plug
>> >> 
>> >>     piix4_device_pre_plug_cb | ich9_pm_device_pre_plug_cb
>> >>         if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
>> >>             !lpc->pm.acpi_memory_hotplug.is_enabled)
>> >> 
>> >>     if (is_nvdimm && !ms->nvdimms_state->is_enabled)
>> >>     
>> >> 
>> >> In hotplug_handler_pre_plug(), it checks the acpi hotplug capability. And then
>> >> if it has memory hotplug capability and is nvdimm, we check whether nvdimm is
>> >> enabled.  
>> >
>> >I don't think pc_memory_pre_plug() should rely on what hotplug_handler_pre_plug()
>> >checks or does. Similarly the later is taking care of whatever piix4 needs to care
>> >and shouldn't care about what machine code does.
>> >  
>> 
>> Agree. It is not proper to let hotplug_handler_pre_plug() take care about
>> machine code.
>> 
>> >Moreover when hotplug_handler_pre_plug() starts to reserve resources, then
>> >if you move check as suggested you'd need to rollback all that
>> >hotplug_handler_pre_plug() done to gracefully abort hotplug.
>> >  
>> 
>> Confused.
>> 
>> hotplug_handler_pre_plug() doesn't reserve resources.
>
>
>it's not currently, but if it would it would not work with your patch properly
>or break unexpectedly since whoever would change hotplug_handler_pre_plug()
>might not notice that machine code need to be taken care of.
>
>Try to consider devices and machine as separate libraries. Which should
>in reasonable limits be independent and work through documented interfaces.
>In that case likehood of breaking something would be less than relying on
>current code impl./call order with implicit inter-dependencies. 
>

So the logic here is check machine then device, right? I think this is
reasonable. To be honest, this rule is not that obvious.

Anyway, I think what you mentioned make sense. Thanks

-- 
Wei Yang
Help you, Help me