From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48389) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yck16-0001CV-2u for qemu-devel@nongnu.org; Mon, 30 Mar 2015 20:26:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yck10-00030Q-Kd for qemu-devel@nongnu.org; Mon, 30 Mar 2015 20:26:52 -0400 Date: Tue, 31 Mar 2015 11:05:24 +1100 From: David Gibson Message-ID: <20150331000524.GN9908@voom.fritz.box> References: <1425615506-1829-1-git-send-email-david@gibson.dropbear.id.au> <1425615506-1829-3-git-send-email-david@gibson.dropbear.id.au> <87mw2vvsao.fsf@blackfin.pond.sub.org> <87384mvq1y.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="PdAWLd+WEPmMbsbx" Content-Disposition: inline In-Reply-To: <87384mvq1y.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH 2/6] Remove monitor.c dependency on CONFIG_I8259 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: agraf@suse.de, mst@redhat.com, qemu-devel@nongnu.org, michael@walle.cc, lcapitulino@redhat.com, blauwirbel@gmail.com, andreas.faerber@web.de, qemu-ppc@nongnu.org --PdAWLd+WEPmMbsbx Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Mar 30, 2015 at 10:37:45AM +0200, Markus Armbruster wrote: > Markus Armbruster writes: >=20 > > David Gibson writes: > > > >> The hmp commands "irq" and "pic" are a bit of a mess. They're impleme= nted > >> on a number of targets, but not all. On sparc32 and LM32 they do targ= et > >> specific things, but on the remainder (i386, ppc and mips) they call i= nto > >> the i8259 PIC code. > > > > Where "info pic" does absolutely nothing unless we're using "isa-i8259". > > In particular, it does nothing when we're using "kvm-i8259" instead. > > Fun! > > > >> But really, what these commands do shouldn't be dependent on the target > >> arch, but on the specific machine that's in use. On ppc, for example, > >> the "prep" machine usually does have an ISA bridge with an i8259, but > >> most of the other machine types have never had an i8259 at all. Simil= arly > >> the sparc specific target would stop working if we ever had a sparc32 > >> machine that wasn't sun4m. > >> > >> This patch cleans things up by implementing these hmp commands on all > >> targets via a MachineClass callback. If the callback is NULL, for now > >> we fallback to target specific defaults that match the existing behavi= our. > >> The hope is we can remove those later with target specific cleanups. > >> > >> Signed-off-by: David Gibson > > > >> --- > >> hw/intc/i8259.c | 4 ++-- > >> include/hw/boards.h | 2 ++ > >> include/hw/i386/pc.h | 4 ++-- > >> monitor.c | 57 ++++++++++++++++++++++++++++++++++++++-----= --------- > >> 4 files changed, 48 insertions(+), 19 deletions(-) > >> > >> diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c > >> index 0f5c025..43e90b9 100644 > >> --- a/hw/intc/i8259.c > >> +++ b/hw/intc/i8259.c > >> @@ -429,7 +429,7 @@ static void pic_realize(DeviceState *dev, Error **= errp) > >> pc->parent_realize(dev, errp); > >> } > >> =20 > >> -void hmp_info_pic(Monitor *mon, const QDict *qdict) > >> +void i8259_hmp_info_pic(Monitor *mon, const QDict *qdict) > >> { > >> int i; > >> PICCommonState *s; > >> @@ -447,7 +447,7 @@ void hmp_info_pic(Monitor *mon, const QDict *qdict) > >> } > >> } > >> =20 > >> -void hmp_info_irq(Monitor *mon, const QDict *qdict) > >> +void i8259_hmp_info_irq(Monitor *mon, const QDict *qdict) > >> { > >> #ifndef DEBUG_IRQ_COUNT > >> monitor_printf(mon, "irq statistic code not compiled.\n"); > >> diff --git a/include/hw/boards.h b/include/hw/boards.h > >> index 3ddc449..214a778 100644 > >> --- a/include/hw/boards.h > >> +++ b/include/hw/boards.h > >> @@ -111,6 +111,8 @@ struct MachineClass { > >> =20 > >> HotplugHandler *(*get_hotplug_handler)(MachineState *machine, > >> DeviceState *dev); > >> + void (*hmp_info_irq)(Monitor *mon, const QDict *qdict); > >> + void (*hmp_info_pic)(Monitor *mon, const QDict *qdict); > >> }; > >> =20 > >> /** > > > > Here you're defining the MachineClass callback. > > > > The callback is designed for HMP. This will make code implementing it > > depend on the monitor. You could do a QMP-style callback instead, and > > get a dependency on QAPI or QObject instead. > > > > But I'm fine with it as is. > > > >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > >> index 08ab67d..0f376c6 100644 > >> --- a/include/hw/i386/pc.h > >> +++ b/include/hw/i386/pc.h > >> @@ -121,8 +121,8 @@ qemu_irq *i8259_init(ISABus *bus, qemu_irq parent_= irq); > >> qemu_irq *kvm_i8259_init(ISABus *bus); > >> int pic_read_irq(DeviceState *d); > >> int pic_get_output(DeviceState *d); > >> -void hmp_info_pic(Monitor *mon, const QDict *qdict); > >> -void hmp_info_irq(Monitor *mon, const QDict *qdict); > >> +void i8259_hmp_info_pic(Monitor *mon, const QDict *qdict); > >> +void i8259_hmp_info_irq(Monitor *mon, const QDict *qdict); > >> =20 > >> /* Global System Interrupts */ > >> =20 > >> diff --git a/monitor.c b/monitor.c > >> index c86a89e..ca226a9 100644 > >> --- a/monitor.c > >> +++ b/monitor.c > >> @@ -1064,6 +1064,48 @@ static void hmp_info_history(Monitor *mon, cons= t QDict *qdict) > >> } > >> } > >> =20 > >> +static void hmp_info_pic(Monitor *mon, const QDict *qdict) > >> +{ > >> + MachineClass *mc =3D MACHINE_GET_CLASS(current_machine); > >> + > >> + if (mc->hmp_info_pic) { > >> + (mc->hmp_info_pic)(mon, qdict); > > > > Here you're using the MachineClass callback. > > > > However, you're not setting it anywhere, so the callback is dead code. > > > > Interfacing to the machine-specific parts with a MachineClass callback > > sounds sensible enough, but not without at least one user. Please > > either add one, or drop the dead code for now. >=20 > Scratch that, you're adding one in the next patch. >=20 > Suggest to point to it in the commit message. Remember, reviewers > effectively squint at your patches through a toilet roll, and can really > use help with the non-local stuff, especially connections between > patches. >=20 > >> + } else { > >> + /* FIXME: Backwards compat fallbacks. These can go away once > >> + * we've finished converting to natively using MachineClass, > >> + * rather thatn QEMUMachine */ > >> +#if defined(TARGET_SPARC) && !defined(TARGET_SPARC64) > >> + sun4m_hmp_info_pic(mon, qdict); > >> +#elif defined(TARGET_LM32) > >> + lm32_hmp_info_pic(mon, qdict); > >> +#elif defined(TARGET_i386) || defined(TARGET_PPC) || defined(TARGET_M= IPS) > >> + i8259_hmp_info_pic(mon, qdict); > >> +#endif > >> + } > >> +} > >> + > >> +static void hmp_info_irq(Monitor *mon, const QDict *qdict) > >> +{ > >> + /* FIXME: The ifdefs can go away once the sun4m and LM32 machines > >> + * are converted to use machine classes natively */ > >> + MachineClass *mc =3D MACHINE_GET_CLASS(current_machine); > >> + > >> + if (mc->hmp_info_irq) { > >> + (mc->hmp_info_irq)(mon, qdict); > >> + } else { > >> + /* FIXME: Backwards compat fallbacks. These can go away once > >> + * we've finished converting to natively using MachineClass, > >> + * rather thatn QEMUMachine */ > >> +#if defined(TARGET_SPARC) && !defined(TARGET_SPARC64) > >> + sun4m_hmp_info_irq(mon, qdict); > >> +#elif defined(TARGET_LM32) > >> + lm32_hmp_info_irq(mon, qdict); > >> +#elif defined(TARGET_i386) || defined(TARGET_PPC) || defined(TARGET_M= IPS) > >> + i8259_hmp_info_irq(mon, qdict); > >> +#endif > >> + } > >> +} > >> + > >> static void hmp_info_cpustats(Monitor *mon, const QDict *qdict) > >> { > >> CPUState *cpu; > >> @@ -2661,35 +2703,20 @@ static mon_cmd_t info_cmds[] =3D { > >> .help =3D "show the command line history", > >> .mhandler.cmd =3D hmp_info_history, > >> }, > >> -#if defined(TARGET_I386) || defined(TARGET_PPC) || defined(TARGET_MIP= S) || \ > >> - defined(TARGET_LM32) || (defined(TARGET_SPARC) && !defined(TARGET= _SPARC64)) >=20 > This adds "info irq" and "info pic" to the targets that didn't have them > before, implemented by i8259.c's hmp_info_pic() and hmp_info_irq(). > They do nothing unless the machine has an "isa-i8259" device. >=20 > Cases: >=20 > 1. If the machine has one, and it's the only interrupt controller, the > commands work as advertized. >=20 > 2. If the machine doesn't have one, the commands are empty promises. >=20 > 3. If the machine has one, but it's not the only interrupt controller, > the commands confidently claim the i8259 is all there is. > Misinformation. >=20 > Cases 2 and 3 are common, case 1 is rare. >=20 > We can: >=20 > A. Fix the commands to cover all interrupt controllers. >=20 > B. Fix them to warn the user about missing interrupt controllers. >=20 > We can approximate this by warning always, because it's almost never > the only interrupt controller anyway :) >=20 > C. Rip 'em both out and be done with it. >=20 > D. Do nothing. >=20 > E. Provide them as is on all targets. >=20 > Spread the badness fairly. >=20 > I vote for C or B. A seems not worthwhile. I'd love to do C, if we can get confirmation that no-one's really using the existing HMP commands. That would make a bunch of things simpler. > What does your series do? Remember, I'm squinting through a toilet > roll... The current draft aims to do 2 things: * Keep identical behaviour on machines that currently have a non-trivial implementation of "info pic" and "info irq" * For all other machines, implement the commands as no-op. I guess that's closest to your option (E). --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --PdAWLd+WEPmMbsbx Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVGeTEAAoJEGw4ysog2bOSfrsQAIjCI+gWl30D1IRcp0TfLQL3 ueJoIo6546BEO1OcGMwlWjmF9bhF8KK4p2XTlJbQV7PHKoDqBMLXD3SjnFaXR0mM jIcOORmhhM2MIN/aJD99p19+qVG3ThhJ1wk89P2UORrz3hl0S802ZWaZ7OsMr4wr BvefVls4l4yfBXxdyJ7HDQlDCw0ayPUVyLS5msZZ6Xdr5TQhKGSPQtiuxXWp2QzE r7vWT0hUc7gJGr4yL45mGZg68b37sVZCpOPh9XRhjuuXOHuzJhA3bLq5vk3U8COL nfrxdquH94sr0zuTeXkesTTu9H5oqnPGNeititJuPvmCdDwyT3HeDpl1tafZFWox ewYqMZwGCIGJew/VjvD8tFHIRPIfQ2Yfo7yBgANuvlnSr1M2jmWMf0zSf0DG+Tuw JFlk0jVDP6hevssNdDhzRhJ5TvxGPAtNxQwyujQKDAtGYs+siQ6SRFGoCSwQMe1H kVC0ziQhl/tmtjuH+p7gNfvpe8FriF81brJxDRadYJg5tYiRE8u6zIVo7eMuyInP SpuJzrFObTXj8TO94/3wJvm/zZee/hoUUIx0f06g8zdy4HGtkNDjtDBT0mZDy4LS aY+J7kLilY/Lc9tVONHV74JZuNY5yiXBXvOy2kyEqsdlmqyZCn4sgbLu+FpC0b5C QaYlPAufxKwu4Ez6ZuHJ =kQGv -----END PGP SIGNATURE----- --PdAWLd+WEPmMbsbx--