| Message ID | 20190411071739.22889-1-richardw.yang@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/i386/pc: check apci hotplug capability before nvdimm's | expand |
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>
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 :-)
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
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); > }
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.
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. >
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. >>
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. > >> >
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
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); }
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(-)