| Message ID | 1365691918-30594-8-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 >
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
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
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
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). > [...]
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 >
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
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.
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);
... 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(+)