* Re: [Qemu-devel] [PATCH for-4.0] hw/i386/pc: Fix crash when hot-plugging nvdimm on older machine types
2019-04-08 15:06 ` Thomas Huth
@ 2019-04-08 15:06 ` Thomas Huth
2019-04-08 21:29 ` Wei Yang
2019-04-08 22:26 ` Eduardo Habkost
2 siblings, 0 replies; 18+ messages in thread
From: Thomas Huth @ 2019-04-08 15:06 UTC (permalink / raw)
To: Wei Yang
Cc: qemu-devel, Paolo Bonzini, Xiao Guangrong, Eduardo Habkost,
Michael S. Tsirkin
On 08/04/2019 15.45, Wei Yang wrote:
> On Sun, Apr 07, 2019 at 11:23:14AM +0200, Thomas Huth wrote:
>> QEMU currently crashes when you try to hot-plug an "nvdimm" device
>> on older machine types:
>>
>> $ qemu-system-x86_64 -monitor stdio -M pc-1.1
>> QEMU 3.1.92 monitor - type 'help' for more information
>> (qemu) device_add nvdimm,id=nvdimmn1
>> qemu-system-x86_64: /home/thuth/devel/qemu/util/error.c:57: error_setv:
>> Assertion `*errp == ((void *)0)' failed.
>> Aborted (core dumped)
>>
>> The call to hotplug_handler_pre_plug() in pc_memory_pre_plug() has been
>> added recently before the check whether nvdimm is enabled. It should
>> be done after the check. And while we're at it, also check the errp
>> after the hotplug_handler_pre_plug(), otherwise errors are silently
>> ignored here.
>
> Thomas,
>
> Thanks for pointing this out, while I have some different idea on how to fix
> this.
>
> The reason of the core dump is errp already been set in
> hotplug_handler_pre_plug(), and this function check acpi hotplug capability.
> The order of this check is correct, while we should return when errp is set
> in hotplug_handler_pre_plug().
>
> I got a fix like this, which I have tested and looks good to me.
>
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 6077d27361..b11f3b15c1 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -2091,6 +2091,9 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> }
>
> hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
> + if (*errp) {
> + return;
> + }
Not sure, but I think you can not rely on the fact that the caller set
*errp = NULL already... that's why it is more common to use a local_err
variable and error_propagate() for such cases (which is what I did in my
patch).
Also, why don't you want the "nvdimm is not enabled: missing 'nvdimm' in
'-M'" check to be done first?
Thomas
> if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
> error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>
>>
>> Fixes: 9040e6dfa8c3fed87695a3de555d2c775727bb51
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> hw/i386/pc.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 6077d27361..f2c15bf1f2 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -2078,6 +2078,7 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> const MachineState *ms = MACHINE(hotplug_dev);
>> const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>> const uint64_t legacy_align = TARGET_PAGE_SIZE;
>> + Error *local_err = NULL;
>>
>> /*
>> * When -no-acpi is used with Q35 machine type, no ACPI is built,
>> @@ -2090,13 +2091,17 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> return;
>> }
>>
>> - hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
>> -
>> 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;
>> + }
>> +
>> pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
>> pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
>> }
>> --
>> 2.21.0
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0] hw/i386/pc: Fix crash when hot-plugging nvdimm on older machine types
2019-04-08 15:06 ` Thomas Huth
2019-04-08 15:06 ` Thomas Huth
@ 2019-04-08 21:29 ` Wei Yang
2019-04-08 21:29 ` Wei Yang
2019-04-11 1:56 ` Wei Yang
2019-04-08 22:26 ` Eduardo Habkost
2 siblings, 2 replies; 18+ messages in thread
From: Wei Yang @ 2019-04-08 21:29 UTC (permalink / raw)
To: Thomas Huth
Cc: Wei Yang, qemu-devel, Paolo Bonzini, Xiao Guangrong,
Eduardo Habkost, Michael S. Tsirkin
On Mon, Apr 08, 2019 at 05:06:49PM +0200, Thomas Huth wrote:
>On 08/04/2019 15.45, Wei Yang wrote:
>> On Sun, Apr 07, 2019 at 11:23:14AM +0200, Thomas Huth wrote:
>>> QEMU currently crashes when you try to hot-plug an "nvdimm" device
>>> on older machine types:
>>>
>>> $ qemu-system-x86_64 -monitor stdio -M pc-1.1
>>> QEMU 3.1.92 monitor - type 'help' for more information
>>> (qemu) device_add nvdimm,id=nvdimmn1
>>> qemu-system-x86_64: /home/thuth/devel/qemu/util/error.c:57: error_setv:
>>> Assertion `*errp == ((void *)0)' failed.
>>> Aborted (core dumped)
>>>
>>> The call to hotplug_handler_pre_plug() in pc_memory_pre_plug() has been
>>> added recently before the check whether nvdimm is enabled. It should
>>> be done after the check. And while we're at it, also check the errp
>>> after the hotplug_handler_pre_plug(), otherwise errors are silently
>>> ignored here.
>>
>> Thomas,
>>
>> Thanks for pointing this out, while I have some different idea on how to fix
>> this.
>>
>> The reason of the core dump is errp already been set in
>> hotplug_handler_pre_plug(), and this function check acpi hotplug capability.
>> The order of this check is correct, while we should return when errp is set
>> in hotplug_handler_pre_plug().
>>
>> I got a fix like this, which I have tested and looks good to me.
>>
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 6077d27361..b11f3b15c1 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -2091,6 +2091,9 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> }
>>
>> hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
>> + if (*errp) {
>> + return;
>> + }
>
>Not sure, but I think you can not rely on the fact that the caller set
>*errp = NULL already... that's why it is more common to use a local_err
>variable and error_propagate() for such cases (which is what I did in my
>patch).
>
Ok, that's fine for me.
>Also, why don't you want the "nvdimm is not enabled: missing 'nvdimm' in
>'-M'" check to be done first?
>
Because this function pc_memory_pre_plug() will be called not only when
nvdimm is hot-plugged but also dimm is hot-plugged. And
hotplug_handler_pre_plug() here is to check the acpi(if it has) hot-plug
capability.
So the check in pc_memory_pre_plug() is from generic to specific:
1. Do we have capability to hot-plug?
2. If the device is nvdimm, do we enabled nvdimm?
> Thomas
>
>
>
>> if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
>> error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>>
>>>
>>> Fixes: 9040e6dfa8c3fed87695a3de555d2c775727bb51
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>> hw/i386/pc.c | 9 +++++++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index 6077d27361..f2c15bf1f2 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -2078,6 +2078,7 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>> const MachineState *ms = MACHINE(hotplug_dev);
>>> const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>>> const uint64_t legacy_align = TARGET_PAGE_SIZE;
>>> + Error *local_err = NULL;
>>>
>>> /*
>>> * When -no-acpi is used with Q35 machine type, no ACPI is built,
>>> @@ -2090,13 +2091,17 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>> return;
>>> }
>>>
>>> - hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
>>> -
>>> 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;
>>> + }
>>> +
>>> pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
>>> pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
>>> }
>>> --
>>> 2.21.0
>>
>
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0] hw/i386/pc: Fix crash when hot-plugging nvdimm on older machine types
2019-04-08 21:29 ` Wei Yang
@ 2019-04-08 21:29 ` Wei Yang
2019-04-11 1:56 ` Wei Yang
1 sibling, 0 replies; 18+ messages in thread
From: Wei Yang @ 2019-04-08 21:29 UTC (permalink / raw)
To: Thomas Huth
Cc: Xiao Guangrong, Michael S. Tsirkin, qemu-devel, Wei Yang,
Paolo Bonzini, Eduardo Habkost
On Mon, Apr 08, 2019 at 05:06:49PM +0200, Thomas Huth wrote:
>On 08/04/2019 15.45, Wei Yang wrote:
>> On Sun, Apr 07, 2019 at 11:23:14AM +0200, Thomas Huth wrote:
>>> QEMU currently crashes when you try to hot-plug an "nvdimm" device
>>> on older machine types:
>>>
>>> $ qemu-system-x86_64 -monitor stdio -M pc-1.1
>>> QEMU 3.1.92 monitor - type 'help' for more information
>>> (qemu) device_add nvdimm,id=nvdimmn1
>>> qemu-system-x86_64: /home/thuth/devel/qemu/util/error.c:57: error_setv:
>>> Assertion `*errp == ((void *)0)' failed.
>>> Aborted (core dumped)
>>>
>>> The call to hotplug_handler_pre_plug() in pc_memory_pre_plug() has been
>>> added recently before the check whether nvdimm is enabled. It should
>>> be done after the check. And while we're at it, also check the errp
>>> after the hotplug_handler_pre_plug(), otherwise errors are silently
>>> ignored here.
>>
>> Thomas,
>>
>> Thanks for pointing this out, while I have some different idea on how to fix
>> this.
>>
>> The reason of the core dump is errp already been set in
>> hotplug_handler_pre_plug(), and this function check acpi hotplug capability.
>> The order of this check is correct, while we should return when errp is set
>> in hotplug_handler_pre_plug().
>>
>> I got a fix like this, which I have tested and looks good to me.
>>
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 6077d27361..b11f3b15c1 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -2091,6 +2091,9 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> }
>>
>> hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
>> + if (*errp) {
>> + return;
>> + }
>
>Not sure, but I think you can not rely on the fact that the caller set
>*errp = NULL already... that's why it is more common to use a local_err
>variable and error_propagate() for such cases (which is what I did in my
>patch).
>
Ok, that's fine for me.
>Also, why don't you want the "nvdimm is not enabled: missing 'nvdimm' in
>'-M'" check to be done first?
>
Because this function pc_memory_pre_plug() will be called not only when
nvdimm is hot-plugged but also dimm is hot-plugged. And
hotplug_handler_pre_plug() here is to check the acpi(if it has) hot-plug
capability.
So the check in pc_memory_pre_plug() is from generic to specific:
1. Do we have capability to hot-plug?
2. If the device is nvdimm, do we enabled nvdimm?
> Thomas
>
>
>
>> if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
>> error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
>>
>>>
>>> Fixes: 9040e6dfa8c3fed87695a3de555d2c775727bb51
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>> hw/i386/pc.c | 9 +++++++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index 6077d27361..f2c15bf1f2 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -2078,6 +2078,7 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>> const MachineState *ms = MACHINE(hotplug_dev);
>>> const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>>> const uint64_t legacy_align = TARGET_PAGE_SIZE;
>>> + Error *local_err = NULL;
>>>
>>> /*
>>> * When -no-acpi is used with Q35 machine type, no ACPI is built,
>>> @@ -2090,13 +2091,17 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>> return;
>>> }
>>>
>>> - hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
>>> -
>>> 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;
>>> + }
>>> +
>>> pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
>>> pcmc->enforce_aligned_dimm ? NULL : &legacy_align, errp);
>>> }
>>> --
>>> 2.21.0
>>
>
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0] hw/i386/pc: Fix crash when hot-plugging nvdimm on older machine types
2019-04-08 21:29 ` Wei Yang
2019-04-08 21:29 ` Wei Yang
@ 2019-04-11 1:56 ` Wei Yang
2019-04-11 1:56 ` Wei Yang
2019-04-11 4:50 ` Thomas Huth
1 sibling, 2 replies; 18+ messages in thread
From: Wei Yang @ 2019-04-11 1:56 UTC (permalink / raw)
To: Wei Yang
Cc: Thomas Huth, Wei Yang, qemu-devel, Paolo Bonzini, Xiao Guangrong,
Eduardo Habkost, Michael S. Tsirkin
On Mon, Apr 08, 2019 at 09:29:11PM +0000, Wei Yang wrote:
>>>
>>> Thomas,
>>>
>>> Thanks for pointing this out, while I have some different idea on how to fix
>>> this.
>>>
>>> The reason of the core dump is errp already been set in
>>> hotplug_handler_pre_plug(), and this function check acpi hotplug capability.
>>> The order of this check is correct, while we should return when errp is set
>>> in hotplug_handler_pre_plug().
>>>
>>> I got a fix like this, which I have tested and looks good to me.
>>>
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index 6077d27361..b11f3b15c1 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -2091,6 +2091,9 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>> }
>>>
>>> hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
>>> + if (*errp) {
>>> + return;
>>> + }
>>
>>Not sure, but I think you can not rely on the fact that the caller set
>>*errp = NULL already... that's why it is more common to use a local_err
>>variable and error_propagate() for such cases (which is what I did in my
>>patch).
>>
>
>Ok, that's fine for me.
>
>>Also, why don't you want the "nvdimm is not enabled: missing 'nvdimm' in
>>'-M'" check to be done first?
>>
>
>Because this function pc_memory_pre_plug() will be called not only when
>nvdimm is hot-plugged but also dimm is hot-plugged. And
>hotplug_handler_pre_plug() here is to check the acpi(if it has) hot-plug
>capability.
>
>So the check in pc_memory_pre_plug() is from generic to specific:
> 1. Do we have capability to hot-plug?
> 2. If the device is nvdimm, do we enabled nvdimm?
>
Thomas
Do you think this is a reasonable explanation?
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0] hw/i386/pc: Fix crash when hot-plugging nvdimm on older machine types
2019-04-11 1:56 ` Wei Yang
@ 2019-04-11 1:56 ` Wei Yang
2019-04-11 4:50 ` Thomas Huth
1 sibling, 0 replies; 18+ messages in thread
From: Wei Yang @ 2019-04-11 1:56 UTC (permalink / raw)
To: Wei Yang
Cc: Thomas Huth, Xiao Guangrong, Michael S. Tsirkin, qemu-devel,
Wei Yang, Paolo Bonzini, Eduardo Habkost
On Mon, Apr 08, 2019 at 09:29:11PM +0000, Wei Yang wrote:
>>>
>>> Thomas,
>>>
>>> Thanks for pointing this out, while I have some different idea on how to fix
>>> this.
>>>
>>> The reason of the core dump is errp already been set in
>>> hotplug_handler_pre_plug(), and this function check acpi hotplug capability.
>>> The order of this check is correct, while we should return when errp is set
>>> in hotplug_handler_pre_plug().
>>>
>>> I got a fix like this, which I have tested and looks good to me.
>>>
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index 6077d27361..b11f3b15c1 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -2091,6 +2091,9 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>> }
>>>
>>> hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
>>> + if (*errp) {
>>> + return;
>>> + }
>>
>>Not sure, but I think you can not rely on the fact that the caller set
>>*errp = NULL already... that's why it is more common to use a local_err
>>variable and error_propagate() for such cases (which is what I did in my
>>patch).
>>
>
>Ok, that's fine for me.
>
>>Also, why don't you want the "nvdimm is not enabled: missing 'nvdimm' in
>>'-M'" check to be done first?
>>
>
>Because this function pc_memory_pre_plug() will be called not only when
>nvdimm is hot-plugged but also dimm is hot-plugged. And
>hotplug_handler_pre_plug() here is to check the acpi(if it has) hot-plug
>capability.
>
>So the check in pc_memory_pre_plug() is from generic to specific:
> 1. Do we have capability to hot-plug?
> 2. If the device is nvdimm, do we enabled nvdimm?
>
Thomas
Do you think this is a reasonable explanation?
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0] hw/i386/pc: Fix crash when hot-plugging nvdimm on older machine types
2019-04-11 1:56 ` Wei Yang
2019-04-11 1:56 ` Wei Yang
@ 2019-04-11 4:50 ` Thomas Huth
2019-04-11 4:50 ` Thomas Huth
1 sibling, 1 reply; 18+ messages in thread
From: Thomas Huth @ 2019-04-11 4:50 UTC (permalink / raw)
To: Wei Yang, Wei Yang
Cc: qemu-devel, Paolo Bonzini, Xiao Guangrong, Eduardo Habkost,
Michael S. Tsirkin
Hi,
On 11/04/2019 03.56, Wei Yang wrote:
> On Mon, Apr 08, 2019 at 09:29:11PM +0000, Wei Yang wrote:
>>>>
>>>> Thomas,
>>>>
>>>> Thanks for pointing this out, while I have some different idea on how to fix
>>>> this.
>>>>
>>>> The reason of the core dump is errp already been set in
>>>> hotplug_handler_pre_plug(), and this function check acpi hotplug capability.
>>>> The order of this check is correct, while we should return when errp is set
>>>> in hotplug_handler_pre_plug().
>>>>
>>>> I got a fix like this, which I have tested and looks good to me.
>>>>
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index 6077d27361..b11f3b15c1 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -2091,6 +2091,9 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>> }
>>>>
>>>> hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
>>>> + if (*errp) {
>>>> + return;
>>>> + }
>>>
>>> Not sure, but I think you can not rely on the fact that the caller set
>>> *errp = NULL already... that's why it is more common to use a local_err
>>> variable and error_propagate() for such cases (which is what I did in my
>>> patch).
>>>
>>
>> Ok, that's fine for me.
>>
>>> Also, why don't you want the "nvdimm is not enabled: missing 'nvdimm' in
>>> '-M'" check to be done first?
>>>
>>
>> Because this function pc_memory_pre_plug() will be called not only when
>> nvdimm is hot-plugged but also dimm is hot-plugged. And
>> hotplug_handler_pre_plug() here is to check the acpi(if it has) hot-plug
>> capability.
>>
>> So the check in pc_memory_pre_plug() is from generic to specific:
>> 1. Do we have capability to hot-plug?
>> 2. If the device is nvdimm, do we enabled nvdimm?
>>
>
> Thomas
>
> Do you think this is a reasonable explanation?
Fine for me, I don't mind either way as long as the crash is fixed. Feel
free to send a patch that restores the desired order.
Thomas
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0] hw/i386/pc: Fix crash when hot-plugging nvdimm on older machine types
2019-04-11 4:50 ` Thomas Huth
@ 2019-04-11 4:50 ` Thomas Huth
0 siblings, 0 replies; 18+ messages in thread
From: Thomas Huth @ 2019-04-11 4:50 UTC (permalink / raw)
To: Wei Yang, Wei Yang
Cc: Paolo Bonzini, Michael S. Tsirkin, qemu-devel, Eduardo Habkost,
Xiao Guangrong
Hi,
On 11/04/2019 03.56, Wei Yang wrote:
> On Mon, Apr 08, 2019 at 09:29:11PM +0000, Wei Yang wrote:
>>>>
>>>> Thomas,
>>>>
>>>> Thanks for pointing this out, while I have some different idea on how to fix
>>>> this.
>>>>
>>>> The reason of the core dump is errp already been set in
>>>> hotplug_handler_pre_plug(), and this function check acpi hotplug capability.
>>>> The order of this check is correct, while we should return when errp is set
>>>> in hotplug_handler_pre_plug().
>>>>
>>>> I got a fix like this, which I have tested and looks good to me.
>>>>
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index 6077d27361..b11f3b15c1 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -2091,6 +2091,9 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>> }
>>>>
>>>> hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
>>>> + if (*errp) {
>>>> + return;
>>>> + }
>>>
>>> Not sure, but I think you can not rely on the fact that the caller set
>>> *errp = NULL already... that's why it is more common to use a local_err
>>> variable and error_propagate() for such cases (which is what I did in my
>>> patch).
>>>
>>
>> Ok, that's fine for me.
>>
>>> Also, why don't you want the "nvdimm is not enabled: missing 'nvdimm' in
>>> '-M'" check to be done first?
>>>
>>
>> Because this function pc_memory_pre_plug() will be called not only when
>> nvdimm is hot-plugged but also dimm is hot-plugged. And
>> hotplug_handler_pre_plug() here is to check the acpi(if it has) hot-plug
>> capability.
>>
>> So the check in pc_memory_pre_plug() is from generic to specific:
>> 1. Do we have capability to hot-plug?
>> 2. If the device is nvdimm, do we enabled nvdimm?
>>
>
> Thomas
>
> Do you think this is a reasonable explanation?
Fine for me, I don't mind either way as long as the crash is fixed. Feel
free to send a patch that restores the desired order.
Thomas
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.0] hw/i386/pc: Fix crash when hot-plugging nvdimm on older machine types
2019-04-08 15:06 ` Thomas Huth
2019-04-08 15:06 ` Thomas Huth
2019-04-08 21:29 ` Wei Yang
@ 2019-04-08 22:26 ` Eduardo Habkost
2019-04-08 22:26 ` Eduardo Habkost
2019-04-09 5:57 ` Markus Armbruster
2 siblings, 2 replies; 18+ messages in thread
From: Eduardo Habkost @ 2019-04-08 22:26 UTC (permalink / raw)
To: Thomas Huth
Cc: Wei Yang, Xiao Guangrong, Michael S. Tsirkin, qemu-devel,
Paolo Bonzini
On Mon, Apr 08, 2019 at 05:06:49PM +0200, Thomas Huth wrote:
> On 08/04/2019 15.45, Wei Yang wrote:
[...]
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 6077d27361..b11f3b15c1 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -2091,6 +2091,9 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > }
> >
> > hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
> > + if (*errp) {
> > + return;
> > + }
>
> Not sure, but I think you can not rely on the fact that the caller set
> *errp = NULL already... that's why it is more common to use a local_err
> variable and error_propagate() for such cases (which is what I did in my
> patch).
*errp can't be non-NULL (otherwise functions calling error_setg()
would crash). errp can be NULL, though, and that's why you need
a local_err variable.
I'd love to eliminate NULL errp from our codebase, but I couldn't
find a way to do it that is safe and simple (i.e. not letting us
pass NULL errp by mistake and not requiring a macro wrapping
every `&local_err` expression).
--
Eduardo
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0] hw/i386/pc: Fix crash when hot-plugging nvdimm on older machine types
2019-04-08 22:26 ` Eduardo Habkost
@ 2019-04-08 22:26 ` Eduardo Habkost
2019-04-09 5:57 ` Markus Armbruster
1 sibling, 0 replies; 18+ messages in thread
From: Eduardo Habkost @ 2019-04-08 22:26 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-devel, Paolo Bonzini, Michael S. Tsirkin, Wei Yang,
Xiao Guangrong
On Mon, Apr 08, 2019 at 05:06:49PM +0200, Thomas Huth wrote:
> On 08/04/2019 15.45, Wei Yang wrote:
[...]
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 6077d27361..b11f3b15c1 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -2091,6 +2091,9 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > }
> >
> > hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
> > + if (*errp) {
> > + return;
> > + }
>
> Not sure, but I think you can not rely on the fact that the caller set
> *errp = NULL already... that's why it is more common to use a local_err
> variable and error_propagate() for such cases (which is what I did in my
> patch).
*errp can't be non-NULL (otherwise functions calling error_setg()
would crash). errp can be NULL, though, and that's why you need
a local_err variable.
I'd love to eliminate NULL errp from our codebase, but I couldn't
find a way to do it that is safe and simple (i.e. not letting us
pass NULL errp by mistake and not requiring a macro wrapping
every `&local_err` expression).
--
Eduardo
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0] hw/i386/pc: Fix crash when hot-plugging nvdimm on older machine types
2019-04-08 22:26 ` Eduardo Habkost
2019-04-08 22:26 ` Eduardo Habkost
@ 2019-04-09 5:57 ` Markus Armbruster
2019-04-09 5:57 ` Markus Armbruster
1 sibling, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2019-04-09 5:57 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Thomas Huth, qemu-devel, Paolo Bonzini, Michael S. Tsirkin,
Wei Yang, Xiao Guangrong
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Mon, Apr 08, 2019 at 05:06:49PM +0200, Thomas Huth wrote:
>> On 08/04/2019 15.45, Wei Yang wrote:
> [...]
>> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> > index 6077d27361..b11f3b15c1 100644
>> > --- a/hw/i386/pc.c
>> > +++ b/hw/i386/pc.c
>> > @@ -2091,6 +2091,9 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> > }
>> >
>> > hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
>> > + if (*errp) {
>> > + return;
>> > + }
>>
>> Not sure, but I think you can not rely on the fact that the caller set
>> *errp = NULL already... that's why it is more common to use a local_err
>> variable and error_propagate() for such cases (which is what I did in my
>> patch).
>
> *errp can't be non-NULL (otherwise functions calling error_setg()
> would crash). errp can be NULL, though, and that's why you need
> a local_err variable.
Correct. The big comment in error.h advises:
* Receive an error and pass it on to the caller:
* Error *err = NULL;
* foo(arg, &err);
* if (err) {
* handle the error...
* error_propagate(errp, err);
* }
* where Error **errp is a parameter, by convention the last one.
*
* Do *not* "optimize" this to
* foo(arg, errp);
* if (*errp) { // WRONG!
* handle the error...
* }
* because errp may be NULL!
*
* But when all you do with the error is pass it on, please use
* foo(arg, errp);
* for readability.
> I'd love to eliminate NULL errp from our codebase, but I couldn't
> find a way to do it that is safe and simple (i.e. not letting us
> pass NULL errp by mistake and not requiring a macro wrapping
> every `&local_err` expression).
Also, I'd prefer not not deviate even more from GError.
Apropos NULL, we often pass NULL where we really ought to pass
&error_abort.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH for-4.0] hw/i386/pc: Fix crash when hot-plugging nvdimm on older machine types
2019-04-09 5:57 ` Markus Armbruster
@ 2019-04-09 5:57 ` Markus Armbruster
0 siblings, 0 replies; 18+ messages in thread
From: Markus Armbruster @ 2019-04-09 5:57 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Thomas Huth, Xiao Guangrong, Michael S. Tsirkin, qemu-devel,
Wei Yang, Paolo Bonzini
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Mon, Apr 08, 2019 at 05:06:49PM +0200, Thomas Huth wrote:
>> On 08/04/2019 15.45, Wei Yang wrote:
> [...]
>> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> > index 6077d27361..b11f3b15c1 100644
>> > --- a/hw/i386/pc.c
>> > +++ b/hw/i386/pc.c
>> > @@ -2091,6 +2091,9 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> > }
>> >
>> > hotplug_handler_pre_plug(pcms->acpi_dev, dev, errp);
>> > + if (*errp) {
>> > + return;
>> > + }
>>
>> Not sure, but I think you can not rely on the fact that the caller set
>> *errp = NULL already... that's why it is more common to use a local_err
>> variable and error_propagate() for such cases (which is what I did in my
>> patch).
>
> *errp can't be non-NULL (otherwise functions calling error_setg()
> would crash). errp can be NULL, though, and that's why you need
> a local_err variable.
Correct. The big comment in error.h advises:
* Receive an error and pass it on to the caller:
* Error *err = NULL;
* foo(arg, &err);
* if (err) {
* handle the error...
* error_propagate(errp, err);
* }
* where Error **errp is a parameter, by convention the last one.
*
* Do *not* "optimize" this to
* foo(arg, errp);
* if (*errp) { // WRONG!
* handle the error...
* }
* because errp may be NULL!
*
* But when all you do with the error is pass it on, please use
* foo(arg, errp);
* for readability.
> I'd love to eliminate NULL errp from our codebase, but I couldn't
> find a way to do it that is safe and simple (i.e. not letting us
> pass NULL errp by mistake and not requiring a macro wrapping
> every `&local_err` expression).
Also, I'd prefer not not deviate even more from GError.
Apropos NULL, we often pass NULL where we really ought to pass
&error_abort.
^ permalink raw reply [flat|nested] 18+ messages in thread