Re: [Qemu-ppc] [PATCH 4/4] Disable "info irq" and "info pic" for target-ppc

[Alexander Graf]

On 06/29/15 12:43, Andreas Färber wrote:
> Am 29.06.2015 um 12:36 schrieb Andreas Färber:
> > Am 29.06.2015 um 12:22 schrieb Laurent Vivier:
> > > On 29/06/2015 12:06, Andreas Färber wrote:
> > > > Am 29.06.2015 um 11:55 schrieb Laurent Vivier:
> > > > > On 29/06/2015 11:51, Laurent Vivier wrote:
> > > > > > On 29/06/2015 11:30, Thomas Huth wrote:
> > > > > > > On Mon, 29 Jun 2015 09:52:56 +0200
> > > > > > > Laurent Vivier <address@hidden> wrote:
> > > > > > > > On 29/06/2015 07:36, David Gibson wrote:
> > > > > > > > > diff --git a/monitor.c b/monitor.c
> > > > > > > > > index aeea2b5..8c56bfa 100644
> > > > > > > > > --- a/monitor.c
> > > > > > > > > +++ b/monitor.c
> > > > > > > > > @@ -2573,7 +2573,7 @@ static mon_cmd_t info_cmds[] = {
> > > > > > > > >           .help       = "show the command line history",
> > > > > > > > >           .mhandler.cmd = hmp_info_history,
> > > > > > > > >       },
> > > > > > > > > -#if defined(TARGET_I386) || defined(TARGET_PPC) || defined(TARGET_MIPS) || \
> > > > > > > > > +#if defined(TARGET_I386) || defined(TARGET_MIPS) || \
> > > > > > > > >       defined(TARGET_LM32) || (defined(TARGET_SPARC) && 
> > > > > > > > > !defined(TARGET_SPARC64))
> > > > > > > > >       {
> > > > > > > > >           .name       = "irq",
> > > > > > > > Perhaps we can a use a "#if defined(CONFIG_I8259) ||
> > > > > > > > defined(CONFIG_LM32) || (defined(TARGE_SPARC) &&
> > > > > > > > !defined(TARGET_SPARC64))" instead, so we keep the command for PReP ?
> > > > > > > AFAIK this currently won't work since CONFIG_I8259 is only defined for
> > > > > > > the Makefiles, but not for the C pre-processor :-(
> > > > > > Yes, I see that afterward, but ...
> > > > > >
> > > > > > > So unless somebody fixes that first, I think David's approach is the
> > > > > > > only practicable solution right now.
> > > > > > if you add "config-devices.h" in GENERATED_HEADERS in Makefile.target,
> > > > > > and include "config-devices.h" in monitor.c, it works (all PREP
> > > > > > dependencies in default-configs/ppc64-softmmu.mak must be removed too)
> > > > > >
> > > > > > But does this change acceptable for a tiny improvement ?
> > > > > In fine, I think we can also do like for sparc:
> > > > >
> > > > > defined(TARGET_PPC) && !defined(TARGET_PPC64)
> > > > Alex specifically requested PReP to be made available in ppc64, too.
> > > Thank you Andreas.
> > >
> > > But why ? (I didn't find the answer with google, a link can be helpful).
> http://git.qemu-project.org/?p=qemu.git;a=commit;h=acbb090b2400f627a801074c4e3e006c7501bb26
>
> (found by looking at the ppc64-softmmu.mak Git history)
>
> Judging by Markus as the reporter, I assume it was a tree-wide analysis
> that came up with this inconsistency, which I was then asked to fix this
> way.

Yes, it's consistency. All 64bit targets allow to run their 32bit 
machine types as well. Whether this is a sensible thing to do or not is 
a different discussion that (if we want to) we need to do outside of the 
scope of this mail thread.

As far as this patch goes, you get a clear nack from me, as it's a 
regression for the prep target. Please just QOM'ify the interrupt 
controller, add an interfact that allows you to query the irq stats and 
then loop through all devices searching for that interface in the object 
tree. That way we should be able to get rid of all #ifdefs in that 
particular code and enable new irq controllers to expose their stats easily.


Alex