On Fri, 27 May 2011 09:55:05 -0500
Anthony Liguori <anth...@codemonkey.ws> wrote:
> On 05/27/2011 09:04 AM, Luiz Capitulino wrote:
> > On Thu, 26 May 2011 22:23:10 +0300
> > Blue Swirl<blauwir...@gmail.com> wrote:
> >
> >> On Thu, May 26, 2011 at 8:25 PM, Markus Armbruster<arm...@redhat.com>
> >> wrote:
> >>> Luiz Capitulino<lcapitul...@redhat.com> writes:
> >>>
> >>>> On Fri, 6 May 2011 18:36:31 +0300
> >>>> Blue Swirl<blauwir...@gmail.com> wrote:
> >>>>
> >>>>> On Fri, May 6, 2011 at 12:08 PM, Markus Armbruster<arm...@redhat.com>
> >>>>> wrote:
> >>>>>> Blue Swirl<blauwir...@gmail.com> writes:
> >>>>>>
> >>>>>>> On Mon, May 2, 2011 at 6:57 PM, Luiz
> >>>>>>> Capitulino<lcapitul...@redhat.com> wrote:
> >>>>>>>> On Sat, 30 Apr 2011 09:33:15 +0300
> >>>>>>>> Blue Swirl<blauwir...@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>>> On Sat, Apr 30, 2011 at 1:40 AM, Luiz
> >>>>>>>>> Capitulino<lcapitul...@redhat.com> wrote:
> >>>>>>>>>> This series introduces the inject-nmi command for QMP, which sends
> >>>>>>>>>> an
> >>>>>>>>>> NMI to _all_ guest's CPUs.
> >>>>>>>>>>
> >>>>>>>>>> Also note that this series changes the human monitor nmi command
> >>>>>>>>>> to use
> >>>>>>>>>> the QMP implementation, which means that it now has a DIFFERENT
> >>>>>>>>>> behavior.
> >>>>>>>>>> Please, check patch 3/3 for details.
> >>>>>>>>>
> >>>>>>>>> As discussed earlier, please change the QMP version for future
> >>>>>>>>> expandability so that instead of single command 'inject-nmi',
> >>>>>>>>> 'inject'
> >>>>>>>>> takes parameter 'nmi'. HMP command 'nmi' can remain for now, but
> >>>>>>>>> 'inject' should be added.
> >>>>>>>>
> >>>>>>>> I'm not sure I agree with this, because we risky overloading
> >>>>>>>> 'inject' the
> >>>>>>>> same way we did with the 'change' command.
> >>>>>>>>
> >>>>>>>> What's 'inject' supposed to do in the future?
> >>>>>>>
> >>>>>>> Inject other IRQs, for example inject nmi could become an alias to
> >>>>>>> something like
> >>>>>>> inject /apic@fee00000:l1int
> >>>>>>> which would be a shorthand for
> >>>>>>> raise /apic@fee00000:l1int
> >>>>>>> lower /apic@fee00000:l1int
> >>>>>>>
> >>>>>>> I think we only need a registration framework for IRQs and other
> >>>>>>> signals.
> >>>>>>
> >>>>>> Yes, we could use nicer infrastructure for modeling IRQs. No, we
> >>>>>> shouldn't reject Lai's work because it doesn't get us there. Perfect
> >>>>>> is
> >>>>>> the enemy of good.
> >>>>>>
> >>>>>> Pick one:
> >>>>>>
> >>>>>> 1. We take inject-nmi now. Should we get a more general inject command
> >>>>>> like the one you envisage later, we can deprecate inject-nmi, and
> >>>>>> remove
> >>>>>> it after a suitable grace time. Big deal. We get the special problem
> >>>>>> solved now, without really compromising future solutions for the
> >>>>>> general
> >>>>>> problem.
> >>>>>>
> >>>>>> 2. We reject inject-nmi now. The itch Lai tries to scratch remains
> >>>>>> unscratched until we get a more general inject command.
> >>>>>>
> >>>>>> 2a. Rejection "motivates" Lai to solve the general problem[*]. Or
> >>>>>> maybe
> >>>>>> it motivates somebody else. We get the general problem solved sooner.
> >>>>>> And maybe I get a pony for my birthday, too.
> >>>>>>
> >>>>>> 2b. The general problem remains unsolved along with the special
> >>>>>> problem.
> >>>>>> We get nothing.
> >>>>>
> >>>>> 2c. Don't add full generic IRQ registration and aliases just now but
> >>>>> handle 'inject' with only 'nmi'. That way we introduce no legacy
> >>>>> baggage to the syntax.
> >>>>
> >>>> Can you give an example on how this is supposed to look like?
> >>>
> >>> No reply. When you demand a redesign to generalize a simple feature to
> >>> something only you envisage, please explain what exactly you want.
> >>> Documentation to stick into qmp-commands.hx would be a start. Here's
> >>> the baseline from Luiz, for your editing convenience.
> >>>
> >>>
> >>> inject-nmi
> >>> ----------
> >>>
> >>> Inject an NMI on guest's CPUs.
> >>>
> >>> Arguments: None.
> >>>
> >>> Example:
> >>>
> >>> -> { "execute": "inject-nmi" }
> >>> <- { "return": {} }
> >>>
> >>> Note: inject-nmi is only supported for x86 guest currently, it will
> >>> returns "Unsupported" error for non-x86 guest.
> >>
> >> I think I explained it many times, but let's try again.
> >>
> >> inject
> >> ----------
> >>
> >> Inject a signal on guest machine.
> >>
> >> Arguments: signal name.
> >>
> >> Example:
> >>
> >> -> { "execute": "inject",
> >> "arguments": { "signal": "nmi" } }
> >> <- { "return": {} }
> >>
> >> -> { "execute": "inject",
> >> "arguments": { "signal": "/apic@fee00000:l1int" } }
> >> <- { "return": {} }
> >
> > Shouldn't this be broken into device and signal (or pin) arguments?
>
>
> I dislike this approach strongly.
>
> Overloading verbs to have multiple meanings is a bad thing for QMP. It
> means less type safety. Think of a C interface:
>
> inject_nmi() <- good
> inject_nim() <- compile error
>
> inject("nmi") <- good
> inject("nim") <- runtime error
>
> Not to mention that "inject" doesn't mean "raise and then lower a pin".
> Inject means insert or put in.
>
> I'm not opposed to being able to have a way to raise/lower a qemu_irq,
> but (a) that's orthogonal to this operation (b) we should design that
> interface properly. b means that we should be able to enumerate pins,
> raise and lower pins, and pulse pins.
So, would you be in favor of merging the current series as it stands
currently and design this new interface as a new command?
>
> Regards,
>
> Anthony Liguori
>
> >> Note: the set of signals supported depends on the CPU architecture and
> >> board type, unknown or unsupported names will
> >> return "Unsupported" error.
> >
> > Unsuported error != bad usage error.
> >
>
