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

[Laurent Vivier]

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:
>>> The "info irq" and "info pic" HMP commands are available on some, but not
>>> all targets, and what they do isn't terribly consistent.  For SPARC and
>>> LM32 they do something platform specific, but for x86, powerpc, and MIPS
>>> they print some information from the i8259 (and only the i8259) interrupt
>>> controller.
>>>
>>> It's debatable whether these commands are any use at all, and we should
>>> probably make better, qdev aware ways of getting information from a
>>> machines PICs.  However, those don't exist yet, so on x86 it's at least
>>> potentially useful to have these HMP commands.  I can't speak for MIPS.
>>>
>>> For ppc, though, the i8259, if it exists at all, is usually just a
>>> secondary controller for legacy ISA.  The only case where i8259 is the
>>> main system PIC on ppc is for the ancient and little-used PReP platform.
>>>
>>> So, even without QOM-ish replacement, the info pic and info irq HMP
>>> commands have no value on ppc.
>>>
>>> This patch, therefore, disables these commands for ppc targets.  This will
>>> allow ppc builds which don't include PReP to not include ISA bus support
>>> either.
>>>
>>> Signed-off-by: David Gibson <address@hidden>
>>> ---
>>>  monitor.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> 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 ?

Laurent