Patchwork [07/19] rtc: update rtc_cmos on CPU hot-plug

login
register
mail settings
Submitter Igor Mammedov
Date April 11, 2013, 2:51 p.m.
Message ID <1365691918-30594-8-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/235795/
State New
Headers show

Comments

Igor Mammedov - April 11, 2013, 2:51 p.m.
... 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(+)
Eduardo Habkost - April 11, 2013, 6:59 p.m.
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?

What happens if a CPU is hotplugged after the machine has started but
before the guest OS has booted? Are we supposed to make sure the BIOS do
the right thing if a CPU is hotplugged before the OS has booted, or this
simply won't be supported?

> 
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 69e6844..958ed6b 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -82,6 +82,7 @@ typedef struct RTCState {
>      Notifier clock_reset_notifier;
>      LostTickPolicy lost_tick_policy;
>      Notifier suspend_notifier;
> +    Notifier cpu_added_notifier;
>  } RTCState;
>  
>  static void rtc_set_time(RTCState *s);
> @@ -759,6 +760,14 @@ static void rtc_notify_suspend(Notifier *notifier, void *data)
>      rtc_set_memory(&s->dev, 0xF, 0xFE);
>  }
>  
> +static void rtc_notify_cpu_added(Notifier *notifier, void *data)
> +{
> +    RTCState *s = container_of(notifier, RTCState, cpu_added_notifier);
> +
> +    /* increment the number of CPUs */
> +    s->cmos_data[0x5f] += 1;
> +}
> +
>  static void rtc_reset(void *opaque)
>  {
>      RTCState *s = opaque;
> @@ -852,6 +861,9 @@ static int rtc_initfn(ISADevice *dev)
>      s->suspend_notifier.notify = rtc_notify_suspend;
>      qemu_register_suspend_notifier(&s->suspend_notifier);
>  
> +    s->cpu_added_notifier.notify = rtc_notify_cpu_added;
> +    qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
> +
>      memory_region_init_io(&s->io, &cmos_ops, s, "rtc", 2);
>      isa_register_ioport(dev, &s->io, base);
>  
> -- 
> 1.8.2
>
Igor Mammedov - April 12, 2013, 10:53 a.m.
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.

> 
> What happens if a CPU is hotplugged after the machine has started but
> before the guest OS has booted? Are we supposed to make sure the BIOS do
> the right thing if a CPU is hotplugged before the OS has booted, or this
> simply won't be supported?
BIOS uses this value to set in ACPI tables what CPUs are present.
 1. if hot-plug happens before BIOS reads it then OS will see all CPUs
and SCI it receives will be nop.
 2. if hot-plug happens after BIOS reads it, OS will handle SCI as usual
    and hotplug CPU instead of initializing it smp_boot() time.
BIOS itself has nothing to do with hot-plug, it's OSPM job.

> 
> > 
> > diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> > index 69e6844..958ed6b 100644
> > --- a/hw/timer/mc146818rtc.c
> > +++ b/hw/timer/mc146818rtc.c
> > @@ -82,6 +82,7 @@ typedef struct RTCState {
> >      Notifier clock_reset_notifier;
> >      LostTickPolicy lost_tick_policy;
> >      Notifier suspend_notifier;
> > +    Notifier cpu_added_notifier;
> >  } RTCState;
> >  
> >  static void rtc_set_time(RTCState *s);
> > @@ -759,6 +760,14 @@ static void rtc_notify_suspend(Notifier *notifier, void *data)
> >      rtc_set_memory(&s->dev, 0xF, 0xFE);
> >  }
> >  
> > +static void rtc_notify_cpu_added(Notifier *notifier, void *data)
> > +{
> > +    RTCState *s = container_of(notifier, RTCState, cpu_added_notifier);
> > +
> > +    /* increment the number of CPUs */
> > +    s->cmos_data[0x5f] += 1;
> > +}
> > +
> >  static void rtc_reset(void *opaque)
> >  {
> >      RTCState *s = opaque;
> > @@ -852,6 +861,9 @@ static int rtc_initfn(ISADevice *dev)
> >      s->suspend_notifier.notify = rtc_notify_suspend;
> >      qemu_register_suspend_notifier(&s->suspend_notifier);
> >  
> > +    s->cpu_added_notifier.notify = rtc_notify_cpu_added;
> > +    qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
> > +
> >      memory_region_init_io(&s->io, &cmos_ops, s, "rtc", 2);
> >      isa_register_ioport(dev, &s->io, base);
> >  
> > -- 
> > 1.8.2
> > 
> 
> -- 
> Eduardo
Eduardo Habkost - April 12, 2013, 1:35 p.m.
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
make the initialization ordering so subtle if we don't have to?

> 
> > 
> > What happens if a CPU is hotplugged after the machine has started but
> > before the guest OS has booted? Are we supposed to make sure the BIOS do
> > the right thing if a CPU is hotplugged before the OS has booted, or this
> > simply won't be supported?
> BIOS uses this value to set in ACPI tables what CPUs are present.
>  1. if hot-plug happens before BIOS reads it then OS will see all CPUs
> and SCI it receives will be nop.
>  2. if hot-plug happens after BIOS reads it, OS will handle SCI as usual
>     and hotplug CPU instead of initializing it smp_boot() time.
> BIOS itself has nothing to do with hot-plug, it's OSPM job.

Makes sense, thanks.

What happens if the CPU is hotplugged after the BIOS builds the ACPI
tables, but long before the OS starts handling ACPI events? Is the OS
guaranteed to run the CPU hotplug ACPI method (that's \_GPE._E02 in the
DSDT, right?), even if that happens?

> 
> > 
> > > 
> > > diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> > > index 69e6844..958ed6b 100644
> > > --- a/hw/timer/mc146818rtc.c
> > > +++ b/hw/timer/mc146818rtc.c
> > > @@ -82,6 +82,7 @@ typedef struct RTCState {
> > >      Notifier clock_reset_notifier;
> > >      LostTickPolicy lost_tick_policy;
> > >      Notifier suspend_notifier;
> > > +    Notifier cpu_added_notifier;
> > >  } RTCState;
> > >  
> > >  static void rtc_set_time(RTCState *s);
> > > @@ -759,6 +760,14 @@ static void rtc_notify_suspend(Notifier *notifier, void *data)
> > >      rtc_set_memory(&s->dev, 0xF, 0xFE);
> > >  }
> > >  
> > > +static void rtc_notify_cpu_added(Notifier *notifier, void *data)
> > > +{
> > > +    RTCState *s = container_of(notifier, RTCState, cpu_added_notifier);
> > > +
> > > +    /* increment the number of CPUs */
> > > +    s->cmos_data[0x5f] += 1;
> > > +}
> > > +
> > >  static void rtc_reset(void *opaque)
> > >  {
> > >      RTCState *s = opaque;
> > > @@ -852,6 +861,9 @@ static int rtc_initfn(ISADevice *dev)
> > >      s->suspend_notifier.notify = rtc_notify_suspend;
> > >      qemu_register_suspend_notifier(&s->suspend_notifier);
> > >  
> > > +    s->cpu_added_notifier.notify = rtc_notify_cpu_added;
> > > +    qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
> > > +
> > >      memory_region_init_io(&s->io, &cmos_ops, s, "rtc", 2);
> > >      isa_register_ioport(dev, &s->io, base);
> > >  
> > > -- 
> > > 1.8.2
> > > 
> > 
> > -- 
> > Eduardo
> 
> 
> -- 
> Regards,
>   Igor
Igor Mammedov - April 12, 2013, 3:16 p.m.
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.

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.

> > > 
> > > What happens if a CPU is hotplugged after the machine has started but
> > > before the guest OS has booted? Are we supposed to make sure the BIOS do
> > > the right thing if a CPU is hotplugged before the OS has booted, or this
> > > simply won't be supported?
> > BIOS uses this value to set in ACPI tables what CPUs are present.
> >  1. if hot-plug happens before BIOS reads it then OS will see all CPUs
> > and SCI it receives will be nop.
> >  2. if hot-plug happens after BIOS reads it, OS will handle SCI as usual
> >     and hotplug CPU instead of initializing it smp_boot() time.
> > BIOS itself has nothing to do with hot-plug, it's OSPM job.
> 
> Makes sense, thanks.
> 
> What happens if the CPU is hotplugged after the BIOS builds the ACPI
> tables, but long before the OS starts handling ACPI events? Is the OS
> guaranteed to run the CPU hotplug ACPI method (that's \_GPE._E02 in the
> DSDT, right?), even if that happens?
Theoretically interrupt should not disappear on it's own and OS should pick it
up. But to say it for sure I need to test this case. If SCI will be lost for
some reason, then OS won't notice new CPU until another CPU hot-plug event
happens.

> > 
> > > 
> > > > 
> > > > diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> > > > index 69e6844..958ed6b 100644
> > > > --- a/hw/timer/mc146818rtc.c
> > > > +++ b/hw/timer/mc146818rtc.c
> > > > @@ -82,6 +82,7 @@ typedef struct RTCState {
> > > >      Notifier clock_reset_notifier;
> > > >      LostTickPolicy lost_tick_policy;
> > > >      Notifier suspend_notifier;
> > > > +    Notifier cpu_added_notifier;
> > > >  } RTCState;
> > > >  
> > > >  static void rtc_set_time(RTCState *s);
> > > > @@ -759,6 +760,14 @@ static void rtc_notify_suspend(Notifier *notifier, void *data)
> > > >      rtc_set_memory(&s->dev, 0xF, 0xFE);
> > > >  }
> > > >  
> > > > +static void rtc_notify_cpu_added(Notifier *notifier, void *data)
> > > > +{
> > > > +    RTCState *s = container_of(notifier, RTCState, cpu_added_notifier);
> > > > +
> > > > +    /* increment the number of CPUs */
> > > > +    s->cmos_data[0x5f] += 1;
> > > > +}
> > > > +
> > > >  static void rtc_reset(void *opaque)
> > > >  {
> > > >      RTCState *s = opaque;
> > > > @@ -852,6 +861,9 @@ static int rtc_initfn(ISADevice *dev)
> > > >      s->suspend_notifier.notify = rtc_notify_suspend;
> > > >      qemu_register_suspend_notifier(&s->suspend_notifier);
> > > >  
> > > > +    s->cpu_added_notifier.notify = rtc_notify_cpu_added;
> > > > +    qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
> > > > +
> > > >      memory_region_init_io(&s->io, &cmos_ops, s, "rtc", 2);
> > > >      isa_register_ioport(dev, &s->io, base);
> > > >  
> > > > -- 
> > > > 1.8.2
> > > > 
> > > 
> > > -- 
> > > Eduardo
> > 
> > 
> > -- 
> > Regards,
> >   Igor
> 
> -- 
> Eduardo
Eduardo Habkost - April 12, 2013, 3:35 p.m.
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?

This would be one additional step towards making pc_cmos_init() be
replaced by QOM-based code (if that's what you want to do in the long
term).


> 
> > > > 
> > > > What happens if a CPU is hotplugged after the machine has started but
> > > > before the guest OS has booted? Are we supposed to make sure the BIOS do
> > > > the right thing if a CPU is hotplugged before the OS has booted, or this
> > > > simply won't be supported?
> > > BIOS uses this value to set in ACPI tables what CPUs are present.
> > >  1. if hot-plug happens before BIOS reads it then OS will see all CPUs
> > > and SCI it receives will be nop.
> > >  2. if hot-plug happens after BIOS reads it, OS will handle SCI as usual
> > >     and hotplug CPU instead of initializing it smp_boot() time.
> > > BIOS itself has nothing to do with hot-plug, it's OSPM job.
> > 
> > Makes sense, thanks.
> > 
> > What happens if the CPU is hotplugged after the BIOS builds the ACPI
> > tables, but long before the OS starts handling ACPI events? Is the OS
> > guaranteed to run the CPU hotplug ACPI method (that's \_GPE._E02 in the
> > DSDT, right?), even if that happens?
> Theoretically interrupt should not disappear on it's own and OS should pick it
> up. But to say it for sure I need to test this case. If SCI will be lost for
> some reason, then OS won't notice new CPU until another CPU hot-plug event
> happens.


Makes sense, thanks.

(Anyway, if an interrupt gets lost, it's probably a BIOS or OS bug).

> [...]
Igor Mammedov - April 12, 2013, 4:24 p.m.
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.

> 
> This would be one additional step towards making pc_cmos_init() be
> replaced by QOM-based code (if that's what you want to do in the long
> term).
> 
> 
> > 
> > > > > 
> > > > > What happens if a CPU is hotplugged after the machine has started but
> > > > > before the guest OS has booted? Are we supposed to make sure the BIOS do
> > > > > the right thing if a CPU is hotplugged before the OS has booted, or this
> > > > > simply won't be supported?
> > > > BIOS uses this value to set in ACPI tables what CPUs are present.
> > > >  1. if hot-plug happens before BIOS reads it then OS will see all CPUs
> > > > and SCI it receives will be nop.
> > > >  2. if hot-plug happens after BIOS reads it, OS will handle SCI as usual
> > > >     and hotplug CPU instead of initializing it smp_boot() time.
> > > > BIOS itself has nothing to do with hot-plug, it's OSPM job.
> > > 
> > > Makes sense, thanks.
> > > 
> > > What happens if the CPU is hotplugged after the BIOS builds the ACPI
> > > tables, but long before the OS starts handling ACPI events? Is the OS
> > > guaranteed to run the CPU hotplug ACPI method (that's \_GPE._E02 in the
> > > DSDT, right?), even if that happens?
> > Theoretically interrupt should not disappear on it's own and OS should pick it
> > up. But to say it for sure I need to test this case. If SCI will be lost for
> > some reason, then OS won't notice new CPU until another CPU hot-plug event
> > happens.
> 
> 
> Makes sense, thanks.
> 
> (Anyway, if an interrupt gets lost, it's probably a BIOS or OS bug).
> 
> > [...]
> 
> -- 
> Eduardo
>
Igor Mammedov - April 15, 2013, 9:38 a.m.
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
Eduardo Habkost - April 15, 2013, 5:53 p.m.
On Mon, Apr 15, 2013 at 11:38:45AM +0200, Igor Mammedov wrote:
[...]
> > > 
> > > 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

I don't see it as a feature of the mc146818rtc chip itself, but just
something that the PC board does with the chip when stuff happens. So I
wouldn't try to make it a method of mc146818rtc, but just something
handled externally from the chip, entirely in the PC code.

Patch

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 69e6844..958ed6b 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -82,6 +82,7 @@  typedef struct RTCState {
     Notifier clock_reset_notifier;
     LostTickPolicy lost_tick_policy;
     Notifier suspend_notifier;
+    Notifier cpu_added_notifier;
 } RTCState;
 
 static void rtc_set_time(RTCState *s);
@@ -759,6 +760,14 @@  static void rtc_notify_suspend(Notifier *notifier, void *data)
     rtc_set_memory(&s->dev, 0xF, 0xFE);
 }
 
+static void rtc_notify_cpu_added(Notifier *notifier, void *data)
+{
+    RTCState *s = container_of(notifier, RTCState, cpu_added_notifier);
+
+    /* increment the number of CPUs */
+    s->cmos_data[0x5f] += 1;
+}
+
 static void rtc_reset(void *opaque)
 {
     RTCState *s = opaque;
@@ -852,6 +861,9 @@  static int rtc_initfn(ISADevice *dev)
     s->suspend_notifier.notify = rtc_notify_suspend;
     qemu_register_suspend_notifier(&s->suspend_notifier);
 
+    s->cpu_added_notifier.notify = rtc_notify_cpu_added;
+    qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
+
     memory_region_init_io(&s->io, &cmos_ops, s, "rtc", 2);
     isa_register_ioport(dev, &s->io, base);