| Message ID | 20240530074544.25444-1-philmd@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | hw/s390x: Alias @dump-skeys -> @dump-s390-skey and deprecate | expand |
On 30/05/24, Philippe Mathieu-Daudé wrote: > We are trying to unify all qemu-system-FOO to a single binary. > In order to do that we need to remove QAPI target specific code. > > @dump-skeys is only available on qemu-system-s390x. This series > rename it as @dump-s390-skey, making it available on other > binaries. We take care of backward compatibility via deprecation. > > Philippe Mathieu-Daudé (4): > hw/s390x: Introduce the @dump-s390-skeys QMP command > hw/s390x: Introduce the 'dump_s390_skeys' HMP command > hw/s390x: Deprecate the HMP 'dump_skeys' command > hw/s390x: Deprecate the QMP @dump-skeys command Series: Reviewed-by: Anton Johansson <anjo@rev.ng>
On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: > We are trying to unify all qemu-system-FOO to a single binary. > In order to do that we need to remove QAPI target specific code. > > @dump-skeys is only available on qemu-system-s390x. This series > rename it as @dump-s390-skey, making it available on other > binaries. We take care of backward compatibility via deprecation. > > Philippe Mathieu-Daudé (4): > hw/s390x: Introduce the @dump-s390-skeys QMP command > hw/s390x: Introduce the 'dump_s390_skeys' HMP command > hw/s390x: Deprecate the HMP 'dump_skeys' command > hw/s390x: Deprecate the QMP @dump-skeys command Why do we have to rename the command? Just for the sake of it? I think renaming HMP commands is maybe ok, but breaking the API in QMP is something you should consider twice. And even if we decide to rename ... maybe we should discuss whether it makes sense to come up with a generic command instead: As far as I know, ARM also has something similar, called MTE. Maybe we also want to dump MTE keys one day? So the new command should maybe be called "dump-memory-keys" instead? Or should it maybe rather be an option to the existing "dump-guest-memory" command instead? Thomas
* Thomas Huth (thuth@redhat.com) wrote: > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: > > We are trying to unify all qemu-system-FOO to a single binary. > > In order to do that we need to remove QAPI target specific code. > > > > @dump-skeys is only available on qemu-system-s390x. This series > > rename it as @dump-s390-skey, making it available on other > > binaries. We take care of backward compatibility via deprecation. > > > > Philippe Mathieu-Daudé (4): > > hw/s390x: Introduce the @dump-s390-skeys QMP command > > hw/s390x: Introduce the 'dump_s390_skeys' HMP command > > hw/s390x: Deprecate the HMP 'dump_skeys' command > > hw/s390x: Deprecate the QMP @dump-skeys command > > Why do we have to rename the command? Just for the sake of it? I think > renaming HMP commands is maybe ok, but breaking the API in QMP is something > you should consider twice. > > And even if we decide to rename ... maybe we should discuss whether it makes > sense to come up with a generic command instead: As far as I know, ARM also > has something similar, called MTE. Maybe we also want to dump MTE keys one > day? So the new command should maybe be called "dump-memory-keys" instead? I think there are at least two different concepts; but I agree it would be nice to keep a single command for matching concepts across different architectures; I can't say I know the details of any, but: a) Page table things - I think x86 PKRU/PKEY (???) is a page table thing where pages marked a special way are associated with keys. That sounds similar to what the skeys are??? b) Upper bit things - where you steal a few bits from the virtual address and then use that to associate some security; I think that's closer to what MTE is isn't it? I'm not sure the two fit in the same command. Dave > Or should it maybe rather be an option to the existing "dump-guest-memory" > command instead? > > Thomas >
On 31/05/2024 16.02, Dr. David Alan Gilbert wrote: > * Thomas Huth (thuth@redhat.com) wrote: >> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: >>> We are trying to unify all qemu-system-FOO to a single binary. >>> In order to do that we need to remove QAPI target specific code. >>> >>> @dump-skeys is only available on qemu-system-s390x. This series >>> rename it as @dump-s390-skey, making it available on other >>> binaries. We take care of backward compatibility via deprecation. >>> >>> Philippe Mathieu-Daudé (4): >>> hw/s390x: Introduce the @dump-s390-skeys QMP command >>> hw/s390x: Introduce the 'dump_s390_skeys' HMP command >>> hw/s390x: Deprecate the HMP 'dump_skeys' command >>> hw/s390x: Deprecate the QMP @dump-skeys command >> >> Why do we have to rename the command? Just for the sake of it? I think >> renaming HMP commands is maybe ok, but breaking the API in QMP is something >> you should consider twice. >> >> And even if we decide to rename ... maybe we should discuss whether it makes >> sense to come up with a generic command instead: As far as I know, ARM also >> has something similar, called MTE. Maybe we also want to dump MTE keys one >> day? So the new command should maybe be called "dump-memory-keys" instead? > > I think there are at least two different concepts; but I agree it would be > nice to keep a single command for matching concepts across different architectures; > I can't say I know the details of any, but: > > a) Page table things - I think x86 PKRU/PKEY (???) is a page table thing > where pages marked a special way are associated with keys. > That sounds similar to what the skeys are??? Sounds a little bit similar, but s390 storage keys are independent from page tables. It's rather that each page (4096 bytes) of RAM has an additional 7-bit value that contains the storage key and some additional bits. It's also usable when page tables are still disabled. > I'm not sure the two fit in the same command. Does it make sense to dump all the MTE or x86 keys all at once? If so, we could maybe come up with an unified command. Otherwise it might not make sense, indeed. Thomas
On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote: > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: > > We are trying to unify all qemu-system-FOO to a single binary. > > In order to do that we need to remove QAPI target specific code. > > > > @dump-skeys is only available on qemu-system-s390x. This series > > rename it as @dump-s390-skey, making it available on other > > binaries. We take care of backward compatibility via deprecation. > > > > Philippe Mathieu-Daudé (4): > > hw/s390x: Introduce the @dump-s390-skeys QMP command > > hw/s390x: Introduce the 'dump_s390_skeys' HMP command > > hw/s390x: Deprecate the HMP 'dump_skeys' command > > hw/s390x: Deprecate the QMP @dump-skeys command > > Why do we have to rename the command? Just for the sake of it? I think > renaming HMP commands is maybe ok, but breaking the API in QMP is something > you should consider twice. That was going to be my question too. Seems like its possible to simply stub out the existing command for other targets. The renaming is just window dressing. > > And even if we decide to rename ... maybe we should discuss whether it makes > sense to come up with a generic command instead: As far as I know, ARM also > has something similar, called MTE. Maybe we also want to dump MTE keys one > day? So the new command should maybe be called "dump-memory-keys" instead? > Or should it maybe rather be an option to the existing "dump-guest-memory" > command instead? With regards, Daniel
* Thomas Huth (thuth@redhat.com) wrote: > On 31/05/2024 16.02, Dr. David Alan Gilbert wrote: > > * Thomas Huth (thuth@redhat.com) wrote: > > > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: > > > > We are trying to unify all qemu-system-FOO to a single binary. > > > > In order to do that we need to remove QAPI target specific code. > > > > > > > > @dump-skeys is only available on qemu-system-s390x. This series > > > > rename it as @dump-s390-skey, making it available on other > > > > binaries. We take care of backward compatibility via deprecation. > > > > > > > > Philippe Mathieu-Daudé (4): > > > > hw/s390x: Introduce the @dump-s390-skeys QMP command > > > > hw/s390x: Introduce the 'dump_s390_skeys' HMP command > > > > hw/s390x: Deprecate the HMP 'dump_skeys' command > > > > hw/s390x: Deprecate the QMP @dump-skeys command > > > > > > Why do we have to rename the command? Just for the sake of it? I think > > > renaming HMP commands is maybe ok, but breaking the API in QMP is something > > > you should consider twice. > > > > > > And even if we decide to rename ... maybe we should discuss whether it makes > > > sense to come up with a generic command instead: As far as I know, ARM also > > > has something similar, called MTE. Maybe we also want to dump MTE keys one > > > day? So the new command should maybe be called "dump-memory-keys" instead? > > > > I think there are at least two different concepts; but I agree it would be > > nice to keep a single command for matching concepts across different architectures; > > I can't say I know the details of any, but: > > > > a) Page table things - I think x86 PKRU/PKEY (???) is a page table thing > > where pages marked a special way are associated with keys. > > That sounds similar to what the skeys are??? > > Sounds a little bit similar, but s390 storage keys are independent from page > tables. It's rather that each page (4096 bytes) of RAM has an additional > 7-bit value that contains the storage key and some additional bits. It's > also usable when page tables are still disabled. > > > I'm not sure the two fit in the same command. > > Does it make sense to dump all the MTE or x86 keys all at once? If so, we > could maybe come up with an unified command. Otherwise it might not make > sense, indeed. So they're all really different granularities: s390 - one key/physical page ARM MTE - one tag/16 bytes physical x86 - per virtual page; then a current register with 16 permission sets For x86 I guess it would be easy to dump the register, and then the values for a range of virtual addresses. But then if you dumped a range of physical addresses on ARM MTE you'd get hundreds times more output than for the equivalent s390. Dave > Thomas >
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote: > > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: > > > We are trying to unify all qemu-system-FOO to a single binary. > > > In order to do that we need to remove QAPI target specific code. > > > > > > @dump-skeys is only available on qemu-system-s390x. This series > > > rename it as @dump-s390-skey, making it available on other > > > binaries. We take care of backward compatibility via deprecation. > > > > > > Philippe Mathieu-Daudé (4): > > > hw/s390x: Introduce the @dump-s390-skeys QMP command > > > hw/s390x: Introduce the 'dump_s390_skeys' HMP command > > > hw/s390x: Deprecate the HMP 'dump_skeys' command > > > hw/s390x: Deprecate the QMP @dump-skeys command > > > > Why do we have to rename the command? Just for the sake of it? I think > > renaming HMP commands is maybe ok, but breaking the API in QMP is something > > you should consider twice. > > That was going to be my question too. Seems like its possible to simply > stub out the existing command for other targets. Are these commands really supposed to be stable, or are they just debug commands? If they are debug, then add the x- and don't worry too much. Dave > The renaming is just window dressing. > > > > > And even if we decide to rename ... maybe we should discuss whether it makes > > sense to come up with a generic command instead: As far as I know, ARM also > > has something similar, called MTE. Maybe we also want to dump MTE keys one > > day? So the new command should maybe be called "dump-memory-keys" instead? > > Or should it maybe rather be an option to the existing "dump-guest-memory" > > command instead? > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
"Dr. David Alan Gilbert" <dave@treblig.org> writes: > * Daniel P. Berrangé (berrange@redhat.com) wrote: >> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote: >> > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: >> > > We are trying to unify all qemu-system-FOO to a single binary. >> > > In order to do that we need to remove QAPI target specific code. >> > > >> > > @dump-skeys is only available on qemu-system-s390x. This series >> > > rename it as @dump-s390-skey, making it available on other >> > > binaries. We take care of backward compatibility via deprecation. >> > > >> > > Philippe Mathieu-Daudé (4): >> > > hw/s390x: Introduce the @dump-s390-skeys QMP command >> > > hw/s390x: Introduce the 'dump_s390_skeys' HMP command >> > > hw/s390x: Deprecate the HMP 'dump_skeys' command >> > > hw/s390x: Deprecate the QMP @dump-skeys command >> > >> > Why do we have to rename the command? Just for the sake of it? I think >> > renaming HMP commands is maybe ok, but breaking the API in QMP is something >> > you should consider twice. PRO rename: the command's tie to S390 is them immediately obvious, which may be useful when the command becomes available in qemu-systems capable of running other targets. CON rename: users need to adapt. What are the users? Not libvirt, as far as I can tell. >> That was going to be my question too. Seems like its possible to simply >> stub out the existing command for other targets. That's going to happen whether we rename the commands or not. > Are these commands really supposed to be stable, or are they just debug > commands? If they are debug, then add the x- and don't worry too much. docs/devel/qapi-code-gen.rst: Names beginning with ``x-`` used to signify "experimental". This convention has been replaced by special feature "unstable". Feature "unstable" is what makes something unstable, and is what machines should check. An "x-" prefix may still be useful for humans. Machines should *not* key on the prefix. It's unreliable anyway: InputBarrierProperties member @x-origin is stable despite it's name. Renames to gain or lose the prefix may or may not be worth the bother. Making an existing part of the interface unstable should be treated similar to deprecating it: we keep it stable for at least the deprecation period. Not written policy, because we didn't consider such changes when we documented our deprecation policy in docs/about/deprecated.rst. Alternative path to a renamed command (*if* we want that): 1. Make it unstable. 2. But keep it stable for two releases. 3. Rename. [...]
Hi Daniel, Dave, Markus & Thomas. On 4/6/24 06:58, Markus Armbruster wrote: > "Dr. David Alan Gilbert" <dave@treblig.org> writes: >> * Daniel P. Berrangé (berrange@redhat.com) wrote: >>> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote: >>>> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: >>>>> We are trying to unify all qemu-system-FOO to a single binary. >>>>> In order to do that we need to remove QAPI target specific code. >>>>> >>>>> @dump-skeys is only available on qemu-system-s390x. This series >>>>> rename it as @dump-s390-skey, making it available on other >>>>> binaries. We take care of backward compatibility via deprecation. >>>>> >>>>> Philippe Mathieu-Daudé (4): >>>>> hw/s390x: Introduce the @dump-s390-skeys QMP command >>>>> hw/s390x: Introduce the 'dump_s390_skeys' HMP command >>>>> hw/s390x: Deprecate the HMP 'dump_skeys' command >>>>> hw/s390x: Deprecate the QMP @dump-skeys command >>>> >>>> Why do we have to rename the command? Just for the sake of it? I think >>>> renaming HMP commands is maybe ok, but breaking the API in QMP is something >>>> you should consider twice. I'm looking at how to include this command in the new "single binary". Markus explained in an earlier series, just expanding this command as stub to targets that don't implement it is not backward compatible and breaks QMP introspection. Currently on s390x we get a result, on other targets the command doesn't exist. If we add a stubs, then other targets return something (even if it is an empty list), confusing management interface. So this approach use to deprecate process to include a new command which behaves differently on non-s390x targets. If we don't care for this particular case, better. However I'd still like to discuss this approach for other target-specific commands. > PRO rename: the command's tie to S390 is them immediately obvious, which > may be useful when the command becomes available in qemu-systems capable > of running other targets. > > CON rename: users need to adapt. > > What are the users? Not libvirt, as far as I can tell. Years ago we said, "all HMP must be based on QMP". Now we realize HMP became stable because QMP-exposed, although not consumed externally... Does the concept of "internal QMP commands" makes sense for HMP debug ones? (Looking at a way to not expose them). We could use the "x-" prefix to not care about stable / backward compat, but what is the point of exposing to QMP commands that will never be accessed there? >>> That was going to be my question too. Seems like its possible to simply >>> stub out the existing command for other targets. > > That's going to happen whether we rename the commands or not. > >> Are these commands really supposed to be stable, or are they just debug >> commands? If they are debug, then add the x- and don't worry too much. OK. > docs/devel/qapi-code-gen.rst: > > Names beginning with ``x-`` used to signify "experimental". This > convention has been replaced by special feature "unstable". > > Feature "unstable" is what makes something unstable, and is what > machines should check. What I mentioned earlier could be 'Feature "internal" or "debug"'. > An "x-" prefix may still be useful for humans. Machines should *not* > key on the prefix. It's unreliable anyway: InputBarrierProperties > member @x-origin is stable despite it's name. Renames to gain or lose > the prefix may or may not be worth the bother. Could follow the rules and be renamed as "origin-coordinate-x". > Making an existing part of the interface unstable should be treated > similar to deprecating it: we keep it stable for at least the > deprecation period. Not written policy, because we didn't consider such > changes when we documented our deprecation policy in > docs/about/deprecated.rst. > > Alternative path to a renamed command (*if* we want that): > > 1. Make it unstable. > > 2. But keep it stable for two releases. > > 3. Rename. > > [...] >
On Tue, Jun 04, 2024 at 11:45:11AM +0200, Philippe Mathieu-Daudé wrote: > Hi Daniel, Dave, Markus & Thomas. > > On 4/6/24 06:58, Markus Armbruster wrote: > > "Dr. David Alan Gilbert" <dave@treblig.org> writes: > > > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > > > On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote: > > > > > On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: > > > > > > We are trying to unify all qemu-system-FOO to a single binary. > > > > > > In order to do that we need to remove QAPI target specific code. > > > > > > > > > > > > @dump-skeys is only available on qemu-system-s390x. This series > > > > > > rename it as @dump-s390-skey, making it available on other > > > > > > binaries. We take care of backward compatibility via deprecation. > > > > > > > > > > > > Philippe Mathieu-Daudé (4): > > > > > > hw/s390x: Introduce the @dump-s390-skeys QMP command > > > > > > hw/s390x: Introduce the 'dump_s390_skeys' HMP command > > > > > > hw/s390x: Deprecate the HMP 'dump_skeys' command > > > > > > hw/s390x: Deprecate the QMP @dump-skeys command > > > > > > > > > > Why do we have to rename the command? Just for the sake of it? I think > > > > > renaming HMP commands is maybe ok, but breaking the API in QMP is something > > > > > you should consider twice. > > I'm looking at how to include this command in the new "single binary". > > Markus explained in an earlier series, just expanding this command as > stub to targets that don't implement it is not backward compatible and > breaks QMP introspection. Currently on s390x we get a result, on other > targets the command doesn't exist. If we add a stubs, then other targets > return something (even if it is an empty list), confusing management > interface. I'm not convinced about calling this "not backward compatible". We're always free to add new commands, and there's never any guarantee that you can execute a given command under every possible QEMU configuration. IOW, adding 'dump-skeys' and having it always return an error on non-s390x targets is valid IMHO, and I wouldn't call that a backwards compatibilit problem. Apps shouldn't assume that just because a command is reported in introspection, it can be used in any scenario. An app is expected to itself understand the behaviour of any given command sufficiently well, to know when they can execute it, or be prpared to accept errors. > > PRO rename: the command's tie to S390 is them immediately obvious, which > > may be useful when the command becomes available in qemu-systems capable > > of running other targets. > > > > CON rename: users need to adapt. > > > > What are the users? Not libvirt, as far as I can tell. > > Years ago we said, "all HMP must be based on QMP". Now we realize HMP > became stable because QMP-exposed, although not consumed externally... That's not the case though. We've added plenty of conversions of HMP to QMP, while declaring them unstable. We might have missed that in some cases, but that's just a bug, and we can fix that any time by adding the 'unstable' feature tag to the schema. > Does the concept of "internal QMP commands" makes sense for HMP debug > ones? (Looking at a way to not expose them). We could use the "x-" > prefix to not care about stable / backward compat, but what is the point > of exposing to QMP commands that will never be accessed there? Debug only, unstable commands are totally valid for QMP. There's nothing that requires them to be inherantly HMP only. QAPI modelling can benefit many debug only, unstable commands, but we can also still just return printf formatted strings as documented here: https://www.qemu.org/docs/master/devel/writing-monitor-commands.html#writing-a-debugging-aid-returning-unstructured-text I remain convinced that we should eventually delete the HMP impl as it exists today, and just provide a QMP client whose interactive interface matches what HMP provides today. There's no compelling need for HMP to run inside QEMU, if QMP exposes all the required functionality. > > docs/devel/qapi-code-gen.rst: > > > > Names beginning with ``x-`` used to signify "experimental". This > > convention has been replaced by special feature "unstable". > > > > Feature "unstable" is what makes something unstable, and is what > > machines should check. > > What I mentioned earlier could be 'Feature "internal" or "debug"'. IMHO we don't need to invent new terms for this. "unstable" is sufficient as it expresses that the command's inputs and outputs are liable to change their format, which is the case for most of the historical debug only commands. With regards, Daniel
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > Hi Daniel, Dave, Markus & Thomas. > > On 4/6/24 06:58, Markus Armbruster wrote: >> "Dr. David Alan Gilbert" <dave@treblig.org> writes: >>> * Daniel P. Berrangé (berrange@redhat.com) wrote: >>>> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote: >>>>> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: >>>>>> We are trying to unify all qemu-system-FOO to a single binary. >>>>>> In order to do that we need to remove QAPI target specific code. >>>>>> >>>>>> @dump-skeys is only available on qemu-system-s390x. This series >>>>>> rename it as @dump-s390-skey, making it available on other >>>>>> binaries. We take care of backward compatibility via deprecation. >>>>>> >>>>>> Philippe Mathieu-Daudé (4): >>>>>> hw/s390x: Introduce the @dump-s390-skeys QMP command >>>>>> hw/s390x: Introduce the 'dump_s390_skeys' HMP command >>>>>> hw/s390x: Deprecate the HMP 'dump_skeys' command >>>>>> hw/s390x: Deprecate the QMP @dump-skeys command >>>>> >>>>> Why do we have to rename the command? Just for the sake of it? I think >>>>> renaming HMP commands is maybe ok, but breaking the API in QMP is something >>>>> you should consider twice. > > I'm looking at how to include this command in the new "single binary". > > Markus explained in an earlier series, just expanding this command as > stub to targets that don't implement it is not backward compatible and > breaks QMP introspection. Currently on s390x we get a result, on other > targets the command doesn't exist. If we add a stubs, then other targets > return something (even if it is an empty list), confusing management > interface. Loss of introspection precision is a concern, not a hard "no". We weigh all the concerns, and pick a solution we hate the least :) > So this approach use to deprecate process to include a new command > which behaves differently on non-s390x targets. > > If we don't care for this particular case, better. However I'd still > like to discuss this approach for other target-specific commands. > >> PRO rename: the command's tie to S390 is them immediately obvious, which >> may be useful when the command becomes available in qemu-systems capable >> of running other targets. >> >> CON rename: users need to adapt. >> >> What are the users? Not libvirt, as far as I can tell. > > Years ago we said, "all HMP must be based on QMP". In practice, it's closer to "HMP must be base on QMP when the functionality does or should exist in QMP." > Now we realize HMP > became stable because QMP-exposed, although not consumed externally... I'm afraid I didn't get this part. > Does the concept of "internal QMP commands" makes sense for HMP debug > ones? (Looking at a way to not expose them). We could use the "x-" > prefix to not care about stable / backward compat, but what is the point > of exposing to QMP commands that will never be accessed there? > >>>> That was going to be my question too. Seems like its possible to simply >>>> stub out the existing command for other targets. >> >> That's going to happen whether we rename the commands or not. >> >>> Are these commands really supposed to be stable, or are they just debug >>> commands? If they are debug, then add the x- and don't worry too much. > > OK. > >> docs/devel/qapi-code-gen.rst: >> >> Names beginning with ``x-`` used to signify "experimental". This >> convention has been replaced by special feature "unstable". >> >> Feature "unstable" is what makes something unstable, and is what >> machines should check. > > What I mentioned earlier could be 'Feature "internal" or "debug"'. What's the difference to "unstable"? >> An "x-" prefix may still be useful for humans. Machines should *not* >> key on the prefix. It's unreliable anyway: InputBarrierProperties >> member @x-origin is stable despite it's name. Renames to gain or lose >> the prefix may or may not be worth the bother. > > Could follow the rules and be renamed as "origin-coordinate-x". I don't think it's worth the trouble. The "x-" prefix is now strictly for humans, and humans can figure out what the x- in @x-origin, @y-origin means. [...]
* Markus Armbruster (armbru@redhat.com) wrote: > Philippe Mathieu-Daudé <philmd@linaro.org> writes: > > > Hi Daniel, Dave, Markus & Thomas. > > > > On 4/6/24 06:58, Markus Armbruster wrote: > >> "Dr. David Alan Gilbert" <dave@treblig.org> writes: > >>> * Daniel P. Berrangé (berrange@redhat.com) wrote: > >>>> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote: > >>>>> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: > >>>>>> We are trying to unify all qemu-system-FOO to a single binary. > >>>>>> In order to do that we need to remove QAPI target specific code. > >>>>>> > >>>>>> @dump-skeys is only available on qemu-system-s390x. This series > >>>>>> rename it as @dump-s390-skey, making it available on other > >>>>>> binaries. We take care of backward compatibility via deprecation. > >>>>>> > >>>>>> Philippe Mathieu-Daudé (4): > >>>>>> hw/s390x: Introduce the @dump-s390-skeys QMP command > >>>>>> hw/s390x: Introduce the 'dump_s390_skeys' HMP command > >>>>>> hw/s390x: Deprecate the HMP 'dump_skeys' command > >>>>>> hw/s390x: Deprecate the QMP @dump-skeys command > >>>>> > >>>>> Why do we have to rename the command? Just for the sake of it? I think > >>>>> renaming HMP commands is maybe ok, but breaking the API in QMP is something > >>>>> you should consider twice. > > > > I'm looking at how to include this command in the new "single binary". > > > > Markus explained in an earlier series, just expanding this command as > > stub to targets that don't implement it is not backward compatible and > > breaks QMP introspection. Currently on s390x we get a result, on other > > targets the command doesn't exist. If we add a stubs, then other targets > > return something (even if it is an empty list), confusing management > > interface. > > Loss of introspection precision is a concern, not a hard "no". > > We weigh all the concerns, and pick a solution we hate the least :) > > > So this approach use to deprecate process to include a new command > > which behaves differently on non-s390x targets. > > > > If we don't care for this particular case, better. However I'd still > > like to discuss this approach for other target-specific commands. > > > >> PRO rename: the command's tie to S390 is them immediately obvious, which > >> may be useful when the command becomes available in qemu-systems capable > >> of running other targets. > >> > >> CON rename: users need to adapt. > >> > >> What are the users? Not libvirt, as far as I can tell. > > > > Years ago we said, "all HMP must be based on QMP". > > In practice, it's closer to "HMP must be base on QMP when the > functionality does or should exist in QMP." > > > Now we realize HMP > > became stable because QMP-exposed, although not consumed externally... > > I'm afraid I didn't get this part. > > > Does the concept of "internal QMP commands" makes sense for HMP debug > > ones? (Looking at a way to not expose them). We could use the "x-" > > prefix to not care about stable / backward compat, but what is the point > > of exposing to QMP commands that will never be accessed there? > > > >>>> That was going to be my question too. Seems like its possible to simply > >>>> stub out the existing command for other targets. > >> > >> That's going to happen whether we rename the commands or not. > >> > >>> Are these commands really supposed to be stable, or are they just debug > >>> commands? If they are debug, then add the x- and don't worry too much. > > > > OK. > > > >> docs/devel/qapi-code-gen.rst: > >> > >> Names beginning with ``x-`` used to signify "experimental". This > >> convention has been replaced by special feature "unstable". > >> > >> Feature "unstable" is what makes something unstable, and is what > >> machines should check. > > > > What I mentioned earlier could be 'Feature "internal" or "debug"'. > > What's the difference to "unstable"? It should be clear *why* something is marked x- - something that's marked 'x-' because the feature is still in development is expected to shake out at some point, and the interface designed so it can. (and at some point the developer should get a prod to be asked whethere the x- can be removed). That's different from it permenantly being x- because it's expected to change as the needs of the people debugging change. Dave > >> An "x-" prefix may still be useful for humans. Machines should *not* > >> key on the prefix. It's unreliable anyway: InputBarrierProperties > >> member @x-origin is stable despite it's name. Renames to gain or lose > >> the prefix may or may not be worth the bother. > > > > Could follow the rules and be renamed as "origin-coordinate-x". > > I don't think it's worth the trouble. The "x-" prefix is now strictly > for humans, and humans can figure out what the x- in @x-origin, > @y-origin means. > > [...] >
"Dr. David Alan Gilbert" <dave@treblig.org> writes: > * Markus Armbruster (armbru@redhat.com) wrote: >> Philippe Mathieu-Daudé <philmd@linaro.org> writes: >> >> > Hi Daniel, Dave, Markus & Thomas. >> > >> > On 4/6/24 06:58, Markus Armbruster wrote: >> >> "Dr. David Alan Gilbert" <dave@treblig.org> writes: >> >>> * Daniel P. Berrangé (berrange@redhat.com) wrote: >> >>>> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote: >> >>>>> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: >> >>>>>> We are trying to unify all qemu-system-FOO to a single binary. >> >>>>>> In order to do that we need to remove QAPI target specific code. >> >>>>>> >> >>>>>> @dump-skeys is only available on qemu-system-s390x. This series >> >>>>>> rename it as @dump-s390-skey, making it available on other >> >>>>>> binaries. We take care of backward compatibility via deprecation. >> >>>>>> >> >>>>>> Philippe Mathieu-Daudé (4): >> >>>>>> hw/s390x: Introduce the @dump-s390-skeys QMP command >> >>>>>> hw/s390x: Introduce the 'dump_s390_skeys' HMP command >> >>>>>> hw/s390x: Deprecate the HMP 'dump_skeys' command >> >>>>>> hw/s390x: Deprecate the QMP @dump-skeys command >> >>>>> >> >>>>> Why do we have to rename the command? Just for the sake of it? I think >> >>>>> renaming HMP commands is maybe ok, but breaking the API in QMP is something >> >>>>> you should consider twice. >> > >> > I'm looking at how to include this command in the new "single binary". >> > >> > Markus explained in an earlier series, just expanding this command as >> > stub to targets that don't implement it is not backward compatible and >> > breaks QMP introspection. Currently on s390x we get a result, on other >> > targets the command doesn't exist. If we add a stubs, then other targets >> > return something (even if it is an empty list), confusing management >> > interface. >> >> Loss of introspection precision is a concern, not a hard "no". >> >> We weigh all the concerns, and pick a solution we hate the least :) >> >> > So this approach use to deprecate process to include a new command >> > which behaves differently on non-s390x targets. >> > >> > If we don't care for this particular case, better. However I'd still >> > like to discuss this approach for other target-specific commands. >> > >> >> PRO rename: the command's tie to S390 is them immediately obvious, which >> >> may be useful when the command becomes available in qemu-systems capable >> >> of running other targets. >> >> >> >> CON rename: users need to adapt. >> >> >> >> What are the users? Not libvirt, as far as I can tell. >> > >> > Years ago we said, "all HMP must be based on QMP". >> >> In practice, it's closer to "HMP must be base on QMP when the >> functionality does or should exist in QMP." >> >> > Now we realize HMP >> > became stable because QMP-exposed, although not consumed externally... >> >> I'm afraid I didn't get this part. >> >> > Does the concept of "internal QMP commands" makes sense for HMP debug >> > ones? (Looking at a way to not expose them). We could use the "x-" >> > prefix to not care about stable / backward compat, but what is the point >> > of exposing to QMP commands that will never be accessed there? >> > >> >>>> That was going to be my question too. Seems like its possible to simply >> >>>> stub out the existing command for other targets. >> >> >> >> That's going to happen whether we rename the commands or not. >> >> >> >>> Are these commands really supposed to be stable, or are they just debug >> >>> commands? If they are debug, then add the x- and don't worry too much. >> > >> > OK. >> > >> >> docs/devel/qapi-code-gen.rst: >> >> >> >> Names beginning with ``x-`` used to signify "experimental". This >> >> convention has been replaced by special feature "unstable". >> >> >> >> Feature "unstable" is what makes something unstable, and is what >> >> machines should check. >> > >> > What I mentioned earlier could be 'Feature "internal" or "debug"'. >> >> What's the difference to "unstable"? > > It should be clear *why* something is marked x- - something that's > marked 'x-' because the feature is still in development is expected to shake > out at some point, and the interface designed so it can. > (and at some point the developer should get a prod to be asked whethere the > x- can be removed). > That's different from it permenantly being x- because it's expected to > change as the needs of the people debugging change. When you add special feature 'unstable', the tooling insists you cover it in the doc comment. Review should then ensure the doc comment explains why it is unstable. Examples: # @unstable: Member @x-perf is experimental. # @unstable: This command is meant for debugging. > Dave > >> >> An "x-" prefix may still be useful for humans. Machines should *not* >> >> key on the prefix. It's unreliable anyway: InputBarrierProperties >> >> member @x-origin is stable despite it's name. Renames to gain or lose >> >> the prefix may or may not be worth the bother. >> > >> > Could follow the rules and be renamed as "origin-coordinate-x". >> >> I don't think it's worth the trouble. The "x-" prefix is now strictly >> for humans, and humans can figure out what the x- in @x-origin, >> @y-origin means. >> >> [...] >>
* Markus Armbruster (armbru@redhat.com) wrote: > "Dr. David Alan Gilbert" <dave@treblig.org> writes: > > > * Markus Armbruster (armbru@redhat.com) wrote: > >> Philippe Mathieu-Daudé <philmd@linaro.org> writes: > >> > >> > Hi Daniel, Dave, Markus & Thomas. > >> > > >> > On 4/6/24 06:58, Markus Armbruster wrote: > >> >> "Dr. David Alan Gilbert" <dave@treblig.org> writes: > >> >>> * Daniel P. Berrangé (berrange@redhat.com) wrote: > >> >>>> On Fri, May 31, 2024 at 06:47:45AM +0200, Thomas Huth wrote: > >> >>>>> On 30/05/2024 09.45, Philippe Mathieu-Daudé wrote: > >> >>>>>> We are trying to unify all qemu-system-FOO to a single binary. > >> >>>>>> In order to do that we need to remove QAPI target specific code. > >> >>>>>> > >> >>>>>> @dump-skeys is only available on qemu-system-s390x. This series > >> >>>>>> rename it as @dump-s390-skey, making it available on other > >> >>>>>> binaries. We take care of backward compatibility via deprecation. > >> >>>>>> > >> >>>>>> Philippe Mathieu-Daudé (4): > >> >>>>>> hw/s390x: Introduce the @dump-s390-skeys QMP command > >> >>>>>> hw/s390x: Introduce the 'dump_s390_skeys' HMP command > >> >>>>>> hw/s390x: Deprecate the HMP 'dump_skeys' command > >> >>>>>> hw/s390x: Deprecate the QMP @dump-skeys command > >> >>>>> > >> >>>>> Why do we have to rename the command? Just for the sake of it? I think > >> >>>>> renaming HMP commands is maybe ok, but breaking the API in QMP is something > >> >>>>> you should consider twice. > >> > > >> > I'm looking at how to include this command in the new "single binary". > >> > > >> > Markus explained in an earlier series, just expanding this command as > >> > stub to targets that don't implement it is not backward compatible and > >> > breaks QMP introspection. Currently on s390x we get a result, on other > >> > targets the command doesn't exist. If we add a stubs, then other targets > >> > return something (even if it is an empty list), confusing management > >> > interface. > >> > >> Loss of introspection precision is a concern, not a hard "no". > >> > >> We weigh all the concerns, and pick a solution we hate the least :) > >> > >> > So this approach use to deprecate process to include a new command > >> > which behaves differently on non-s390x targets. > >> > > >> > If we don't care for this particular case, better. However I'd still > >> > like to discuss this approach for other target-specific commands. > >> > > >> >> PRO rename: the command's tie to S390 is them immediately obvious, which > >> >> may be useful when the command becomes available in qemu-systems capable > >> >> of running other targets. > >> >> > >> >> CON rename: users need to adapt. > >> >> > >> >> What are the users? Not libvirt, as far as I can tell. > >> > > >> > Years ago we said, "all HMP must be based on QMP". > >> > >> In practice, it's closer to "HMP must be base on QMP when the > >> functionality does or should exist in QMP." > >> > >> > Now we realize HMP > >> > became stable because QMP-exposed, although not consumed externally... > >> > >> I'm afraid I didn't get this part. > >> > >> > Does the concept of "internal QMP commands" makes sense for HMP debug > >> > ones? (Looking at a way to not expose them). We could use the "x-" > >> > prefix to not care about stable / backward compat, but what is the point > >> > of exposing to QMP commands that will never be accessed there? > >> > > >> >>>> That was going to be my question too. Seems like its possible to simply > >> >>>> stub out the existing command for other targets. > >> >> > >> >> That's going to happen whether we rename the commands or not. > >> >> > >> >>> Are these commands really supposed to be stable, or are they just debug > >> >>> commands? If they are debug, then add the x- and don't worry too much. > >> > > >> > OK. > >> > > >> >> docs/devel/qapi-code-gen.rst: > >> >> > >> >> Names beginning with ``x-`` used to signify "experimental". This > >> >> convention has been replaced by special feature "unstable". > >> >> > >> >> Feature "unstable" is what makes something unstable, and is what > >> >> machines should check. > >> > > >> > What I mentioned earlier could be 'Feature "internal" or "debug"'. > >> > >> What's the difference to "unstable"? > > > > It should be clear *why* something is marked x- - something that's > > marked 'x-' because the feature is still in development is expected to shake > > out at some point, and the interface designed so it can. > > (and at some point the developer should get a prod to be asked whethere the > > x- can be removed). > > That's different from it permenantly being x- because it's expected to > > change as the needs of the people debugging change. > > When you add special feature 'unstable', the tooling insists you cover > it in the doc comment. Review should then ensure the doc comment > explains why it is unstable. Examples: > > # @unstable: Member @x-perf is experimental. > > # @unstable: This command is meant for debugging. OK, that makes some sense. Dave > > Dave > > > >> >> An "x-" prefix may still be useful for humans. Machines should *not* > >> >> key on the prefix. It's unreliable anyway: InputBarrierProperties > >> >> member @x-origin is stable despite it's name. Renames to gain or lose > >> >> the prefix may or may not be worth the bother. > >> > > >> > Could follow the rules and be renamed as "origin-coordinate-x". > >> > >> I don't think it's worth the trouble. The "x-" prefix is now strictly > >> for humans, and humans can figure out what the x- in @x-origin, > >> @y-origin means. > >> > >> [...] > >> >