[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 07/19] rtc: update rtc_cmos on CPU hot-plug
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH 07/19] rtc: update rtc_cmos on CPU hot-plug |
Date: |
Fri, 12 Apr 2013 18:24:23 +0200 |
On Fri, 12 Apr 2013 12:35:14 -0300
Eduardo Habkost <address@hidden> 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 <address@hidden> 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 <address@hidden> 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 <address@hidden>
> > > > > > 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
>
--
Regards,
Igor
- Re: [Qemu-devel] [PATCH 03/19] cpu: make kvm-stub.o a part of CPU library, (continued)
- [Qemu-devel] [PATCH 06/19] introduce CPU hot-plug notifier, Igor Mammedov, 2013/04/11
- [Qemu-devel] [PATCH 07/19] rtc: update rtc_cmos on CPU hot-plug, Igor Mammedov, 2013/04/11
- Re: [Qemu-devel] [PATCH 07/19] rtc: update rtc_cmos on CPU hot-plug, Eduardo Habkost, 2013/04/11
- Re: [Qemu-devel] [PATCH 07/19] rtc: update rtc_cmos on CPU hot-plug, Igor Mammedov, 2013/04/12
- Re: [Qemu-devel] [PATCH 07/19] rtc: update rtc_cmos on CPU hot-plug, Eduardo Habkost, 2013/04/12
- Re: [Qemu-devel] [PATCH 07/19] rtc: update rtc_cmos on CPU hot-plug, Igor Mammedov, 2013/04/12
- Re: [Qemu-devel] [PATCH 07/19] rtc: update rtc_cmos on CPU hot-plug, Eduardo Habkost, 2013/04/12
- Re: [Qemu-devel] [PATCH 07/19] rtc: update rtc_cmos on CPU hot-plug,
Igor Mammedov <=
- Re: [Qemu-devel] [PATCH 07/19] rtc: update rtc_cmos on CPU hot-plug, Igor Mammedov, 2013/04/15
- Re: [Qemu-devel] [PATCH 07/19] rtc: update rtc_cmos on CPU hot-plug, Eduardo Habkost, 2013/04/15
[Qemu-devel] [PATCH 09/19] cpu: add helper cpu_exists(), to check if CPU with specified id exists, Igor Mammedov, 2013/04/11
[Qemu-devel] [PATCH 08/19] cpu: introduce get_arch_id() method and override it for target-i386, Igor Mammedov, 2013/04/11
Re: [Qemu-devel] [PATCH 08/19] cpu: introduce get_arch_id() method and override it for target-i386, Andreas Färber, 2013/04/15