From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58677) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xc2so-0006jy-GW for qemu-devel@nongnu.org; Wed, 08 Oct 2014 21:51:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xc2sj-0006pz-Pe for qemu-devel@nongnu.org; Wed, 08 Oct 2014 21:51:10 -0400 Received: from [59.151.112.132] (port=46252 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xc2si-0006pb-SL for qemu-devel@nongnu.org; Wed, 08 Oct 2014 21:51:05 -0400 Message-ID: <5435E6AC.4090803@cn.fujitsu.com> Date: Thu, 9 Oct 2014 09:36:44 +0800 From: Gu Zheng MIME-Version: 1.0 References: <1411987957-31190-1-git-send-email-guz.fnst@cn.fujitsu.com> <1411987957-31190-6-git-send-email-guz.fnst@cn.fujitsu.com> <20141007150137.4077dd13@nial.usersys.redhat.com> <54348F6B.7090000@cn.fujitsu.com> <20141008142518.0ecbd6aa@nial.usersys.redhat.com> In-Reply-To: <20141008142518.0ecbd6aa@nial.usersys.redhat.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V4 5/8] pc: Update rtc_cmos in pc_cpu_plug List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: chen.fan.fnst@cn.fujitsu.com, isimatu.yasuaki@jp.fujitsu.com, qemu-devel@nongnu.org, afaerber@suse.de, tangchen@cn.fujitsu.com On 10/08/2014 08:25 PM, Igor Mammedov wrote: > On Wed, 8 Oct 2014 09:12:11 +0800 > Gu Zheng wrote: > >> Hi Igor, >> >> On 10/07/2014 09:01 PM, Igor Mammedov wrote: >> >>> On Mon, 29 Sep 2014 18:52:34 +0800 >>> Gu Zheng wrote: >>> >>>> Update rtc_cmos in pc_cpu_plug directly instead of the notifier. >>>> >>>> v4: >>>> -Make link property in PCMachine rather than the global >>>> variables. >>>> -Split out the removal of unused notifier into separate patch. >>>> >>>> Signed-off-by: Gu Zheng >>>> --- >>>> hw/i386/pc.c | 37 ++++++++++++++++--------------------- >>>> hw/i386/pc_piix.c | 2 +- >>>> hw/i386/pc_q35.c | 2 +- >>>> include/hw/i386/pc.h | 3 ++- >>>> qom/cpu.c | 1 - >>>> 5 files changed, 20 insertions(+), 25 deletions(-) >>>> >>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >>>> index dcb9332..301e704 100644 >>>> --- a/hw/i386/pc.c >>>> +++ b/hw/i386/pc.c >>>> @@ -355,30 +355,15 @@ 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); >>>> -} >>>> - >>>> void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, >>>> - const char *boot_device, >>>> + const char *boot_device, MachineState *machine, >>>> ISADevice *floppy, BusState *idebus0, BusState *idebus1, >>>> ISADevice *s) >>>> { >>>> 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; >>>> + PCMachineState *pc_machine = PC_MACHINE(machine); >>>> >>>> /* various important CMOS locations needed by PC/Bochs bios */ >>>> >>>> @@ -417,10 +402,14 @@ 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); >>>> + >>>> + object_property_add_link(OBJECT(machine), "rtc_state", >>>> + TYPE_ISA_DEVICE, >>>> + (Object **)&pc_machine->rtc, >>>> + object_property_allow_set_link, >>>> + OBJ_PROP_LINK_UNREF_ON_RELEASE, &error_abort); >>>> + object_property_set_link(OBJECT(machine), OBJECT(s), >>>> + "rtc_state", &error_abort); >>>> >>>> if (set_boot_dev(s, boot_device)) { >>>> exit(1); >>>> @@ -1633,6 +1622,12 @@ 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); >>>> + if (local_err) { >>>> + goto out; >>>> + } >>>> + >>>> + /* increment the number of CPUs */ >>>> + rtc_set_memory(pcms->rtc, 0x5f, rtc_get_memory(pcms->rtc, 0x5f) + 1); >>>> out: >>>> error_propagate(errp, local_err); >>>> } >>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>>> index 103d756..2c8d4dc 100644 >>>> --- a/hw/i386/pc_piix.c >>>> +++ b/hw/i386/pc_piix.c >>>> @@ -266,7 +266,7 @@ static void pc_init1(MachineState *machine, >>>> } >>>> >>>> pc_cmos_init(below_4g_mem_size, above_4g_mem_size, machine->boot_order, >>>> - floppy, idebus[0], idebus[1], rtc_state); >>>> + machine, floppy, idebus[0], idebus[1], rtc_state); >>>> >>>> if (pci_enabled && usb_enabled(false)) { >>>> pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci"); >>>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c >>>> index d4a907c..94ba98d 100644 >>>> --- a/hw/i386/pc_q35.c >>>> +++ b/hw/i386/pc_q35.c >>>> @@ -266,7 +266,7 @@ static void pc_q35_init(MachineState *machine) >>>> 8, NULL, 0); >>>> >>>> pc_cmos_init(below_4g_mem_size, above_4g_mem_size, machine->boot_order, >>>> - floppy, idebus[0], idebus[1], rtc_state); >>>> + machine, floppy, idebus[0], idebus[1], rtc_state); >>>> >>>> /* the rest devices to which pci devfn is automatically assigned */ >>>> pc_vga_init(isa_bus, host_bus); >>>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h >>>> index 77316d5..7a4bff4 100644 >>>> --- a/include/hw/i386/pc.h >>>> +++ b/include/hw/i386/pc.h >>>> @@ -33,6 +33,7 @@ struct PCMachineState { >>>> MemoryRegion hotplug_memory; >>>> >>>> HotplugHandler *acpi_dev; >>>> + ISADevice *rtc; >>>> >>>> uint64_t max_ram_below_4g; >>>> }; >>>> @@ -210,7 +211,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi, >>>> uint32 hpet_irqs); >>>> void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd); >>>> void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, >>>> - const char *boot_device, >>>> + const char *boot_device, MachineState *machine, >>>> ISADevice *floppy, BusState *ide0, BusState *ide1, >>>> ISADevice *s); >>>> void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus); >>>> diff --git a/qom/cpu.c b/qom/cpu.c >>>> index ba8b402..09c7e41 100644 >>>> --- a/qom/cpu.c >>>> +++ b/qom/cpu.c >>>> @@ -307,7 +307,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); >>> this hunk belongs to the next patch >> >> This notifier was only used to update rtc state, and after we dropped >> rtc state update into pc_cpu_plug, this line will be dead code, so I >> think deleting it here is more reasonable. > Well the same reasoning would apply to CPU hot-plug notifier as well. > But above hunk is not related to RTC but it's a part of notifier itself. > RTC related part is refactoring RTC callback but not mechanism that > called it (i.e. CPU hot-plug notifier). Got it, thanks. Best regards, Gu > >> And in the next patch, just removing the unused CPU hot-plug notifier. > >> >> Thanks, >> Gu >> >>> >>>> cpu_resume(cpu); >>>> } >>>> } >>> >>> . >>> >> >> >> > > . >