Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo
On Tue, 4 Apr 2023 at 14:25, Markus Armbruster <armbru(a)redhat.com> wrote:
Peter Maydell <peter.maydell(a)linaro.org> writes:
> On Tue, 4 Apr 2023 at 09:25, Markus Armbruster <armbru(a)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)
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