From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58140) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UUIWf-0007lT-Ty for qemu-devel@nongnu.org; Mon, 22 Apr 2013 11:19:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UUIWb-0006au-3s for qemu-devel@nongnu.org; Mon, 22 Apr 2013 11:19:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50039) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UUIWa-0006aq-Rx for qemu-devel@nongnu.org; Mon, 22 Apr 2013 11:19:25 -0400 Date: Mon, 22 Apr 2013 17:18:37 +0200 From: Igor Mammedov Message-ID: <20130422171837.34689ebe@nial.usersys.redhat.com> In-Reply-To: <51754FBB.5010305@suse.de> References: <1366063976-4909-1-git-send-email-imammedo@redhat.com> <1366063976-4909-7-git-send-email-imammedo@redhat.com> <51754FBB.5010305@suse.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 06/16] target-i386: pc: update rtc_cmos on CPU hot-plug List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?ISO-8859-1?B?RuRyYmVy?= Cc: aliguori@us.ibm.com, ehabkost@redhat.com, mst@redhat.com, jan.kiszka@siemens.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, aderumier@odiso.com, lcapitulino@redhat.com, jfrei@linux.vnet.ibm.com, yang.z.zhang@intel.com, pbonzini@redhat.com, lig.fnst@cn.fujitsu.com, rth@twiddle.net On Mon, 22 Apr 2013 16:56:59 +0200 Andreas F=E4rber wrote: > Am 16.04.2013 00:12, schrieb Igor Mammedov: > > ... so that on reboot BIOS could read current available CPU count > >=20 > > Signed-off-by: Igor Mammedov > > --- > > hw/i386/pc.c | 20 ++++++++++++++++++++ > > hw/timer/mc146818rtc.c | 7 +++++++ > > include/hw/timer/mc146818rtc.h | 1 + > > 3 files changed, 28 insertions(+) > >=20 > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index 8d75b34..dc1a78b 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -337,6 +337,21 @@ static void pc_cmos_init_late(void *opaque) > > qemu_unregister_reset(pc_cmos_init_late, opaque); > > } > > =20 > > +typedef struct rtc_cpu_hotplug_arg { > > + ISADevice *rtc_state; > > + Notifier cpu_added_notifier; > > +} rtc_cpu_hotplug_arg; >=20 > Are the arguments (rtc_state) intentionally placed before the common part? no, but it's not QOM type so it doesn't matter. I'll reorder it on next respin. >=20 > > + > > +static void rtc_notify_cpu_added(Notifier *notifier, void *data) > > +{ > > + rtc_cpu_hotplug_arg *arg =3D container_of(notifier, > > rtc_cpu_hotplug_arg, > > + cpu_added_notifier); > > + ISADevice *s =3D arg->rtc_state; > > + > > + /* increment the number of CPUs */ > > + rtc_set_memory(s, 0x5f, rtc_get_memory(s, 0x5f) + 1); > > +} >=20 > I think this proves Gleb's point of reordering the notifier and CPU > resumption since the first instruction executed might theoretically be > reading the RTC memory. I've already changed notifier clall place according to Gleb's suggestions. >=20 > Otherwise looks good with one minor nit... >=20 > > + > > void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, > > const char *boot_device, > > ISADevice *floppy, BusState *idebus0, BusState > > *idebus1, @@ -345,6 +360,7 @@ void pc_cmos_init(ram_addr_t ram_size, > > ram_addr_t above_4g_mem_size, int val, nb, i; > > FDriveType fd_type[2] =3D { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE }; > > static pc_cmos_init_late_arg arg; > > + static rtc_cpu_hotplug_arg cpu_hotplug_cb; > > =20 > > /* various important CMOS locations needed by PC/Bochs bios */ > > =20 > > @@ -383,6 +399,10 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t > > above_4g_mem_size,=20 > > /* set the number of CPU */ > > rtc_set_memory(s, 0x5f, smp_cpus - 1); > > + /* init CPU hotplug notifier */ > > + cpu_hotplug_cb.rtc_state =3D s; > > + cpu_hotplug_cb.cpu_added_notifier.notify =3D rtc_notify_cpu_added; > > + qemu_register_cpu_added_notifier(&cpu_hotplug_cb.cpu_added_notifie= r); > > =20 > > /* set boot devices, and disable floppy signature check if request= ed > > */ if (set_boot_dev(s, boot_device, fd_bootchk)) { > > diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c > > index 69e6844..e639942 100644 > > --- a/hw/timer/mc146818rtc.c > > +++ b/hw/timer/mc146818rtc.c > > @@ -677,6 +677,13 @@ void rtc_set_memory(ISADevice *dev, int addr, int > > val) s->cmos_data[addr] =3D val; > > } > > =20 > > +int rtc_get_memory(ISADevice *dev, int addr) > > +{ > > + RTCState *s =3D DO_UPCAST(RTCState, dev, dev); >=20 > If we apply my mc146818rtc.c QOM'ification patch to qom-cpu first, this > can become MC146818_RTC(dev). Anthony just ack'ed that, but qemu.git is > currently broken for testing, so I'm still holding off changes. if you push it into your qom-cpu tree now, I'll rebase on top of it. >=20 > Andreas >=20 > > + assert(addr >=3D 0 && addr <=3D 127); > > + return s->cmos_data[addr]; > > +} > > + > > static void rtc_set_date_from_host(ISADevice *dev) > > { > > RTCState *s =3D DO_UPCAST(RTCState, dev, dev); > > diff --git a/include/hw/timer/mc146818rtc.h > > b/include/hw/timer/mc146818rtc.h index 854ea3f..09f37b7 100644 > > --- a/include/hw/timer/mc146818rtc.h > > +++ b/include/hw/timer/mc146818rtc.h > > @@ -6,6 +6,7 @@ > > =20 > > ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq= ); > > void rtc_set_memory(ISADevice *dev, int addr, int val); > > +int rtc_get_memory(ISADevice *dev, int addr); > > void rtc_set_date(ISADevice *dev, const struct tm *tm); > > =20 > > #endif /* !MC146818RTC_H */ >=20