Re: [Qemu-devel] [PATCH 07/19] rtc: update rtc_cmos on CPU hot-plug

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 12 Apr 2013 18:24:23 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Fri, 12 Apr 2013 12:35:14 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Apr 12, 2013 at 05:16:20PM +0200, Igor Mammedov wrote:
> > > On Fri, 12 Apr 2013 10:35:53 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > On Fri, Apr 12, 2013 at 12:53:51PM +0200, Igor Mammedov wrote:
> > > > > On Thu, 11 Apr 2013 15:59:40 -0300
> > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > 
> > > > > > On Thu, Apr 11, 2013 at 04:51:46PM +0200, Igor Mammedov wrote:
> > > > > > > ... so that on reboot BIOS could read current available CPU
> > > > > > > count
> > > > > > > 
> > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > > v2:
> > > > > > >   *
> > > > > > > s/qemu_register_cpu_add_notifier()/qemu_register_cpu_added_notifier()/
> > > > > > > --- hw/timer/mc146818rtc.c | 12 ++++++++++++
> > > > > > >  1 file changed, 12 insertions(+)
> > > > > > 
> > > > > > Initialization of the cmos fields (including 0x5F) is done on
> > > > > > pc.c:pc_cmos_init(). What about making the field increment inside
> > > > > > pc.c as well?
> > > > > I looked at possibility but discarded it because to increment it
> > > > > there initial value should be -1 (field is zero based) which is not
> > > > > obvious, plug ugly casting to singed variable.
> > > > > Result looked ugly.
> > > > 
> > > > I was thinking about simply adding exactly the same code with exactly
> > > > the same logic, but inside pc.c instead of of mc146818rtc.c. Instead
> > > > of registering the notifier inside rtc_initfn(), register exactly the
> > > > same notifier with exactly the same code, but inside pc_cmos_init()
> > > > (that's where 0x5f is initialized).
> > > > 
> > > > It would even be safer and easier review and ensure correctness: with
> > > > this patch, the notifier is registered very early, even before
> > > > pc_cmos_init() initializes 0x5f to smp_cpus. CPU hotplug events are
> > > > unlikely to be emitted before pc_cmos_init() is called, but still: why
> > > it isn't be called, hot-add is available only after machine initialized.
> > > 
> > > > make the initialization ordering so subtle if we don't have to?
> > > Currently cmos init doesn't look like proper QOM object and has 3 stage
> > > initialization: realize(), then pc_cmos_init() the last
> > > pc_cmos_init_late(). The last 2 calls are made after realize(), setting
> > > various properties. Which looks wrong from QOM perspective, so I'm
> > > against of stuffing more internal stuff in arbitrary places. We should
> > > do opposite instead.
> > 
> > True, but as we already have this weird 3-stage initialization process
> > and we won't fix it really soon, I would really prefer to keep parts of
> > the code that are closely related and depend on each other in the same
> > part of the code.
> > 
> > > 
> > > If you look at mc146818rtc.c or hw/acpi/piix4.c, all notifiers are
> > > private to object and registered at realize() time. It looks like
> > > initialization order of mc146818rtc should be fixed, instead of
> > > adapting new code to it.
> > > 
> > > So since this patch doesn't break or violate anything in current code,
> > > I'd like to leave it as it is.
> > 
> > If you insist into making the mc146818rtc device take care of
> > maintaining the 0x5f value by itself, why not doing:
> > 
> >     s->cmos_data[0x5f] = smp_cpus - 1;
> > 
> > inside rtc_initfn() instead of pc_cmos_init() as well?
> Device is used not only by target-i386.
> Right way would be to redesign rtc_init() and rtc_initfn() and it would be
> quite an intrusive patch.
> 
> That said it looks like current patch is incorrect if other targets
> are considered, where s->cmos_data[0x5f] doesn't mean smp_cpus - 1. That
> looks like a good reason to place notifier into pc.c and make it board
> specific. I'll redo it for the next respin.
> 
On the other hand, it probably would be better to make it a method and
override it in pc.c