[Qemu-devel] [PATCH] pc: fix crash in rtc_set_memory() if initial cpu is marked as hotplugged

[Qemu-devel] [PATCH] pc: fix crash in rtc_set_memory() if initial cpu is marked as hotplugged

[Igor Mammedov]

'hotplugged' propperty is meant to be used on migration side when migrating
source with hotplugged devices.
However though it not exacly correct usage of 'hotplugged' property
it's possible to set generic hotplugged property for CPU using
 -cpu foo,hotplugged=on
or
 -global foo.hotplugged=on

in this case qemu crashes with following backtrace:

...

because pc_cpu_plug() assumes that hotplugged CPU could appear only after
rtc/fw_cfg are initialized.
Fix crash by replacing assumption with explicit checks of rtc/fw_cfg
and updating them only if they were initialized.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reported-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f3d7ad4..7b7e126 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1810,8 +1810,10 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
 
     /* increment the number of CPUs */
     pcms->boot_cpus++;
-    if (dev->hotplugged) {
+    if (pcms->rtc) {
         rtc_set_cpus_count(pcms->rtc, pcms->boot_cpus);
+    }
+    if (pcms->fw_cfg) {
         fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
     }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH] pc: fix crash in rtc_set_memory() if initial cpu is marked as hotplugged

[Eduardo Habkost]

On Fri, Dec 30, 2016 at 03:33:11PM +0100, Igor Mammedov wrote:
> 'hotplugged' propperty is meant to be used on migration side when migrating
> source with hotplugged devices.
> However though it not exacly correct usage of 'hotplugged' property
> it's possible to set generic hotplugged property for CPU using
>  -cpu foo,hotplugged=on
> or
>  -global foo.hotplugged=on
> 
> in this case qemu crashes with following backtrace:
> 
> ...
> 
> because pc_cpu_plug() assumes that hotplugged CPU could appear only after
> rtc/fw_cfg are initialized.
> Fix crash by replacing assumption with explicit checks of rtc/fw_cfg
> and updating them only if they were initialized.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>

I like how the new code doesn't depend on DeviceState::hotplugged
at all.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

But I would like to understand better how the "hotplugged"
property is really supposed to be used. I am not aware of any
existing case where the user needs to set "hotplugged" directly
and it works.

> ---
>  hw/i386/pc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f3d7ad4..7b7e126 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1810,8 +1810,10 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>  
>      /* increment the number of CPUs */
>      pcms->boot_cpus++;
> -    if (dev->hotplugged) {
> +    if (pcms->rtc) {
>          rtc_set_cpus_count(pcms->rtc, pcms->boot_cpus);
> +    }
> +    if (pcms->fw_cfg) {
>          fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
>      }
>  
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH] pc: fix crash in rtc_set_memory() if initial cpu is marked as hotplugged

[Paolo Bonzini]

On 30/12/2016 15:33, Igor Mammedov wrote:
> 'hotplugged' propperty is meant to be used on migration side when migrating
> source with hotplugged devices.
> However though it not exacly correct usage of 'hotplugged' property
> it's possible to set generic hotplugged property for CPU using
>  -cpu foo,hotplugged=on
> or
>  -global foo.hotplugged=on
> 
> in this case qemu crashes with following backtrace:
> 
> ...
> 
> because pc_cpu_plug() assumes that hotplugged CPU could appear only after
> rtc/fw_cfg are initialized.
> Fix crash by replacing assumption with explicit checks of rtc/fw_cfg
> and updating them only if they were initialized.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/i386/pc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f3d7ad4..7b7e126 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1810,8 +1810,10 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>  
>      /* increment the number of CPUs */
>      pcms->boot_cpus++;
> -    if (dev->hotplugged) {
> +    if (pcms->rtc) {
>          rtc_set_cpus_count(pcms->rtc, pcms->boot_cpus);
> +    }
> +    if (pcms->fw_cfg) {
>          fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
>      }
>  
> 

Looks good - adding qemu-stable too.

Paolo

Re: [Qemu-devel] [PATCH] pc: fix crash in rtc_set_memory() if initial cpu is marked as hotplugged

[Michael S. Tsirkin]

On Fri, Dec 30, 2016 at 03:33:11PM +0100, Igor Mammedov wrote:
> 'hotplugged' propperty is meant to be used on migration side when migrating
> source with hotplugged devices.
> However though it not exacly correct usage of 'hotplugged' property
> it's possible to set generic hotplugged property for CPU using
>  -cpu foo,hotplugged=on
> or
>  -global foo.hotplugged=on
> 
> in this case qemu crashes with following backtrace:
> 
> ...
> 
> because pc_cpu_plug() assumes that hotplugged CPU could appear only after
> rtc/fw_cfg are initialized.
> Fix crash by replacing assumption with explicit checks of rtc/fw_cfg
> and updating them only if they were initialized.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>

Looks like Paolo is merging this.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/i386/pc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f3d7ad4..7b7e126 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1810,8 +1810,10 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>  
>      /* increment the number of CPUs */
>      pcms->boot_cpus++;
> -    if (dev->hotplugged) {
> +    if (pcms->rtc) {
>          rtc_set_cpus_count(pcms->rtc, pcms->boot_cpus);
> +    }
> +    if (pcms->fw_cfg) {
>          fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
>      }
>  
> -- 
> 2.7.4