| Message ID | 1435556214-2916-5-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
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 <david@gibson.dropbear.id.au> > --- > 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 ? Laurent
On Mon, 29 Jun 2015 09:52:56 +0200 Laurent Vivier <lvivier@redhat.com> 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 <david@gibson.dropbear.id.au> > > --- > > 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 :-( So unless somebody fixes that first, I think David's approach is the only practicable solution right now. Thomas
On 29/06/2015 11:30, Thomas Huth wrote: > On Mon, 29 Jun 2015 09:52:56 +0200 > Laurent Vivier <lvivier@redhat.com> 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 <david@gibson.dropbear.id.au> >>> --- >>> 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
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 <lvivier@redhat.com> 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 <david@gibson.dropbear.id.au> >>>> --- >>>> 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 ? In fine, I think we can also do like for sparc: defined(TARGET_PPC) && !defined(TARGET_PPC64) Laurent
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 <lvivier@redhat.com> 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. Regards, Andreas
On Mon, 29 Jun 2015 11:55:08 +0200 Laurent Vivier <lvivier@redhat.com> wrote: > > > 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 <lvivier@redhat.com> 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 <david@gibson.dropbear.id.au> > >>>> --- > >>>> 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 ? > > In fine, I think we can also do like for sparc: > > defined(TARGET_PPC) && !defined(TARGET_PPC64) +1 I like that idea, sounds like a good compromise. Thomas
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 <lvivier@redhat.com> 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). Is there any 64bit PReP ? BTW using CONFIG_I8259 cannot enable it only for PReP and not for other PPC64. So the solution from David is the best. Laurent
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 <lvivier@redhat.com> 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). > > Is there any 64bit PReP ? > > BTW using CONFIG_I8259 cannot enable it only for PReP and not for other > PPC64. So the solution from David is the best. For all newer targets (excluding sparc64 etc.) the 64-bit target is expected to deliver all 32-bit targets as well, for use with KVM in that bitness or for convenience of installing just one executable. Alex is on CC and if you ping him hard enough he might reply himself. ;) So, generating a header file, like you proposed above, seems like the most elegant solution to me here. That won't help the qtest issue I raised on the other patch though, which would need a QMP lookup as discussed in the ivshmem context. I rebased that patch on Friday and can look into factoring that out into libqtest.[ch]. Regards, Andreas
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 <lvivier@redhat.com> 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. Andreas >> >> Is there any 64bit PReP ? >> >> BTW using CONFIG_I8259 cannot enable it only for PReP and not for other >> PPC64. So the solution from David is the best. > > For all newer targets (excluding sparc64 etc.) the 64-bit target is > expected to deliver all 32-bit targets as well, for use with KVM in that > bitness or for convenience of installing just one executable. > > Alex is on CC and if you ping him hard enough he might reply himself. ;) > > So, generating a header file, like you proposed above, seems like the > most elegant solution to me here. > That won't help the qtest issue I raised on the other patch though, > which would need a QMP lookup as discussed in the ivshmem context. I > rebased that patch on Friday and can look into factoring that out into > libqtest.[ch]. > > Regards, > Andreas >
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 <lvivier@redhat.com> 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
On Mon, Jun 29, 2015 at 01:02:50PM +0200, Alexander Graf wrote: > 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 <lvivier@redhat.com> 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 "just"!?! > 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.
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",
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 <david@gibson.dropbear.id.au> --- monitor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)