Re: [Qemu-devel] [PATCH V3 5/7] pc: Update rtc_cmos in pc_cpu_plug

[Gu Zheng]

Hi Igor,
On 09/26/2014 09:23 PM, Igor Mammedov wrote:

> On Wed, 17 Sep 2014 09:24:01 +0800
> Gu Zheng <address@hidden> wrote:
> 
>> Update rtc_cmos in pc_cpu_plug directly instead of the notifier, with
>> this change, there will no user of CPU hot-plug notifier any more, so
>> remove it.
>>
>> Signed-off-by: Gu Zheng <address@hidden>
>> ---
>>  hw/i386/pc.c            |   25 ++++++-------------------
>>  include/sysemu/sysemu.h |    3 ---
>>  qom/cpu.c               |   10 ----------
>>  3 files changed, 6 insertions(+), 32 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index e25e71b..8b887c7 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -353,20 +353,7 @@ static void pc_cmos_init_late(void *opaque)
>>      qemu_unregister_reset(pc_cmos_init_late, opaque);
>>  }
>>  
>> -typedef struct RTCCPUHotplugArg {
>> -    Notifier cpu_added_notifier;
>> -    ISADevice *rtc_state;
>> -} RTCCPUHotplugArg;
>> -
>> -static void rtc_notify_cpu_added(Notifier *notifier, void *data)
>> -{
>> -    RTCCPUHotplugArg *arg = container_of(notifier, RTCCPUHotplugArg,
>> -                                         cpu_added_notifier);
>> -    ISADevice *s = arg->rtc_state;
>> -
>> -    /* increment the number of CPUs */
>> -    rtc_set_memory(s, 0x5f, rtc_get_memory(s, 0x5f) + 1);
>> -}
>> +static ISADevice *rtc_state;
> It's not recommended to use global variables.
> 
> Instead of make link<rtc> property in PCMachine and use it pretty much like 
> acpi_dev
> is used in 4/7.

Right, this way is more reasonable and neat.

> 
>>  
>>  void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>                    const char *boot_device,
>> @@ -376,7 +363,6 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t 
>> above_4g_mem_size,
>>      int val, nb, i;
>>      FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
>>      static pc_cmos_init_late_arg arg;
>> -    static RTCCPUHotplugArg cpu_hotplug_cb;
>>  
>>      /* various important CMOS locations needed by PC/Bochs bios */
>>  
>> @@ -415,10 +401,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t 
>> above_4g_mem_size,
>>  
>>      /* set the number of CPU */
>>      rtc_set_memory(s, 0x5f, smp_cpus - 1);
>> -    /* init CPU hotplug notifier */
>> -    cpu_hotplug_cb.rtc_state = s;
>> -    cpu_hotplug_cb.cpu_added_notifier.notify = rtc_notify_cpu_added;
>> -    qemu_register_cpu_added_notifier(&cpu_hotplug_cb.cpu_added_notifier);
>> +
>> +    rtc_state = s;
>>  
>>      if (set_boot_dev(s, boot_device)) {
>>          exit(1);
>> @@ -1630,6 +1614,9 @@ static void pc_cpu_plug(HotplugHandler *hotplug_dev,
>>  
>>      hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>>      hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>> +
>> +    /* increment the number of CPUs */
>> +    rtc_set_memory(rtc_state, 0x5f, rtc_get_memory(rtc_state, 0x5f) + 1);
> looks wrong, what if error happens in plug() handler???

Ah, yes, we need to confirm the plug cb result before update rtc_state.

Thanks,
Gu

> 
>>  out:
>>      error_propagate(errp, local_err);
>>  }
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index d8539fd..acfe494 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -183,9 +183,6 @@ void do_pci_device_hot_remove(Monitor *mon, const QDict 
>> *qdict);
>>  /* generic hotplug */
>>  void drive_hot_add(Monitor *mon, const QDict *qdict);
>>  
>> -/* CPU hotplug */
>> -void qemu_register_cpu_added_notifier(Notifier *notifier);
>> -
>>  /* pcie aer error injection */
>>  void pcie_aer_inject_error_print(Monitor *mon, const QObject *data);
>>  int do_pcie_aer_inject_error(Monitor *mon,
>> diff --git a/qom/cpu.c b/qom/cpu.c
>> index b32dd0a..19c5de5 100644
>> --- a/qom/cpu.c
>> +++ b/qom/cpu.c
>> @@ -107,15 +107,6 @@ static void cpu_common_get_memory_mapping(CPUState *cpu,
>>      error_setg(errp, "Obtaining memory mappings is unsupported on this 
>> CPU.");
>>  }
>>  
>> -/* CPU hot-plug notifiers */
>> -static NotifierList cpu_added_notifiers =
>> -    NOTIFIER_LIST_INITIALIZER(cpu_add_notifiers);
>> -
>> -void qemu_register_cpu_added_notifier(Notifier *notifier)
>> -{
>> -    notifier_list_add(&cpu_added_notifiers, notifier);
>> -}
>> -
>>  void cpu_reset_interrupt(CPUState *cpu, int mask)
>>  {
>>      cpu->interrupt_request &= ~mask;
>> @@ -303,7 +294,6 @@ static void cpu_common_realizefn(DeviceState *dev, Error 
>> **errp)
>>  
>>      if (dev->hotplugged) {
>>          cpu_synchronize_post_init(cpu);
>> -        notifier_list_notify(&cpu_added_notifiers, dev);
>>          cpu_resume(cpu);
>>      }
>>  }
> 
> .
>