[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo |
Date: |
Wed, 5 Apr 2023 15:56:26 +0100 |
User-agent: |
Mutt/2.2.9 (2022-11-12) |
* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Tue, 4 Apr 2023 at 14:25, Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >
> > > On Tue, 4 Apr 2023 at 09:25, Markus Armbruster <armbru@redhat.com> wrote:
> > >> Hmm. We report it in query-status, which means it's relevant to QMP
> > >> clients. We provide the command to control it only in HMP, which means
> > >> it's not relevant to QMP clients.
> > >>
> > >> Why is reading it relevant to QMP clients, but not writing?
> > >
> > > I suspect that neither is very relevant to QMP clients, but I
> > > thought we had a requirement that HMP interfaces went
> > > via QMP ones ?
> >
> > Kind of. Here's my current boilerplate on the subject:
> >
> > HMP commands without a QMP equivalent are okay if their
> > functionality makes no sense in QMP, or is of use only for human
> > users.
> >
> > Example for "makes no sense in QMP": setting the current CPU,
> > because a QMP monitor doesn't have a current CPU.
> >
> > Examples for "is of use only for human users": HMP command "help",
> > the integrated pocket calculator.
> >
> > Debugging commands are kind of borderline. Debugging is commonly a
> > human activity, where HMP is just fine. However, humans create
> > tools to assist with their activities, and then QMP is useful.
> > While I wouldn't encourage HMP-only for the debugging use case, I
> > wouldn't veto it.
> >
> > When adding an HMP-only command, explain why it is HMP-only in the
> > commit message.
> >
> > > If not, we could just make the HMP query
> > > interface directly look at the TCG property, the way the
> > > write interface does.
> >
> > How useful is it HMP?
>
> Well, as usual, we have no idea if anybody really uses any feature.
> I've never used it myself but I have a vague recollection of reading
> list mail once from somebody who used it. You can construct theoretical
> scenarios where it might be nice (eg "boot guest OS quickly and then
> turn on the one-insn-per-tb mode once you get to the point of interest"),
> I guess. These theoretical scenarios are equally valid (or esoteric)
> whether you're trying to control QEMU via QMP or HMP.
>
> I think on balance I would go for:
> * remove (ie deprecate-and-drop) 'singlestep' from the QMP struct,
> rather than merely renaming it
> * if anybody comes along and says they want to do this via QMP,
> implement Paolo's idea of putting the accelerator object
> somewhere they can get at it and use qom-get/qom-set on it
> [My guess is this is very unlikely: nobody's complained so
> far that QMP doesn't permit setting 'singlestep'; and
> wanting read without write seems even more marginal.]
> * keep the HMP commands, but have both read and write directly
> talk to the accel object. I favour splitting the 'read'
> part out into its own 'info one-insn-per-tb', for consistency
> (then 'info status' matches the QMP query-status)
If it's pretty obscure, then the qom-set/get is fine; as long
as there is a way to do it, then just make sure in the commit
message you say what the replacement command is.
Dave
> In particular, the fact that messing with this obscure debug
> functionality requires updating the reference-output for a
> bunch of io tests that have no interest at all in it rather
> suggests that even if we did want to expose this to QMP that
> the query-status command is the wrong place to do it.
>
> thanks
> -- PMM
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
- Re: [PATCH v2 03/10] tcg: Use one-insn-per-tb accelerator property in curr_cflags(), (continued)
[PATCH v2 09/10] qapi/run-state.json: Fix missing newline at end of file, Peter Maydell, 2023/04/03
[PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo, Peter Maydell, 2023/04/03
- Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo, Markus Armbruster, 2023/04/04
- Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo, Peter Maydell, 2023/04/04
- Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo, Markus Armbruster, 2023/04/04
- Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo, Peter Maydell, 2023/04/04
- Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo, Paolo Bonzini, 2023/04/04
- Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo,
Dr. David Alan Gilbert <=
- Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo, Peter Maydell, 2023/04/05
- Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo, Dr. David Alan Gilbert, 2023/04/05
Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo, Paolo Bonzini, 2023/04/04
[PATCH v2 04/10] linux-user: Add '-one-insn-per-tb' option equivalent to '-singlestep', Peter Maydell, 2023/04/03
[PATCH v2 07/10] hmp: Add 'one-insn-per-tb' command equivalent to 'singlestep', Peter Maydell, 2023/04/03
Re: [PATCH v2 00/10] Deprecate/rename singlestep command line option, monitor interfaces, Richard Henderson, 2023/04/03