Re: [Qemu-devel] [PATCH V4 5/8] pc: Update rtc_cmos in pc_cpu_plug

[Igor Mammedov]

On Wed, 8 Oct 2014 09:12:11 +0800
Gu Zheng <address@hidden> wrote:

> Hi Igor,
> 
> On 10/07/2014 09:01 PM, Igor Mammedov wrote:
> 
> > On Mon, 29 Sep 2014 18:52:34 +0800
> > Gu Zheng <address@hidden> wrote:
> > 
> >> Update rtc_cmos in pc_cpu_plug directly instead of the notifier.
> >>
> >> v4:
> >>  -Make link<rtc> property in PCMachine rather than the global
> >>   variables.
> >>  -Split out the removal of unused notifier into separate patch.
> >>
> >> Signed-off-by: Gu Zheng <address@hidden>
> >> ---
> >>  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).

> And in the next patch, just removing the unused CPU hot-plug notifier.

> 
> Thanks,
> Gu
> 
> > 
> >>          cpu_resume(cpu);
> >>      }
> >>  }
> > 
> > .
> > 
> 
> 
>