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

[Eduardo Habkost <ehabkost@redhat.com>]

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