target/s390x/cpu-qom.h | 1 + target/s390x/cpu_models.c | 211 +++++++++++++++++++++++++++----------- 2 files changed, 153 insertions(+), 59 deletions(-)
There was recently a discussion regarding CPU model versions. That concept does not fit s390x where we have a lot of feature variability. I proposed an alternative approach in [1], which might work for x86 as well (but I am not sure if x86 still can or wants to switch to that), and requires no real changes in upper layers. [1] and patch #2 contains more information on the motivation for this. E.g., specifying/expanding "z14-best" will result in the "best feature set possible on this accelerator, hw and, firmware". While a "z13" does not work under TCG and some z/VM versions, "z13-best" will work. As I had a couple of spare minutes this week, I hacked a quick prototype. I did not heavily test this (quick sanity tests under TCG only), but it should be decent enough to get the idea and play with it. I'll not be working next week, which is why I sent this out now for discussion. [1] https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg07222.html Cc: Daniel P. Berrangé <berrange@redhat.com> Cc: Markus Armbruster <armbru@redhat.com> Cc: Jiri Denemark <jdenemar@redhat.com> Cc: Eduardo Habkost <ehabkost@redhat.com> Cc: Michael Mueller <mimu@linux.ibm.com> David Hildenbrand (2): s390x/cpumodels: Factor out CPU feature dependencies s390x/cpumodel: Introduce "best" model variants target/s390x/cpu-qom.h | 1 + target/s390x/cpu_models.c | 211 +++++++++++++++++++++++++++----------- 2 files changed, 153 insertions(+), 59 deletions(-) -- 2.21.0
On Fri, 8 Nov 2019 at 11:08, David Hildenbrand <david@redhat.com> wrote: > > There was recently a discussion regarding CPU model versions. That concept > does not fit s390x where we have a lot of feature variability. I > proposed an alternative approach in [1], which might work for x86 as well > (but I am not sure if x86 still can or wants to switch to that), and > requires no real changes in upper layers. > > [1] and patch #2 contains more information on the motivation for this. > > E.g., specifying/expanding "z14-best" will result in the "best feature > set possible on this accelerator, hw and, firmware". While a "z13" does > not work under TCG and some z/VM versions, "z13-best" will work. I think other architectures call this concept "max", not "best". If we can manage some cross-architecture consistency that would be helpful, but is s390x using 'max' already for something else? thanks -- PMM
On 08.11.19 12:10, Peter Maydell wrote: > On Fri, 8 Nov 2019 at 11:08, David Hildenbrand <david@redhat.com> wrote: >> >> There was recently a discussion regarding CPU model versions. That concept >> does not fit s390x where we have a lot of feature variability. I >> proposed an alternative approach in [1], which might work for x86 as well >> (but I am not sure if x86 still can or wants to switch to that), and >> requires no real changes in upper layers. >> >> [1] and patch #2 contains more information on the motivation for this. >> >> E.g., specifying/expanding "z14-best" will result in the "best feature >> set possible on this accelerator, hw and, firmware". While a "z13" does >> not work under TCG and some z/VM versions, "z13-best" will work. > > I think other architectures call this concept "max", not "best". > If we can manage some cross-architecture consistency that would > be helpful, but is s390x using 'max' already for something else? We have the "max" model just like other architectures s390 max Enables all features supported by the accelerator in the current host It is basically the "host" model under KVM, and the "qemu" model under TCG (with minor differences for the latter). This series introduces e.g., s390 z900-best IBM zSeries 900 GA1 with best features supported by the accelerator in the current host s390 z14-best IBM z14 GA1 with best features supported by the accelerator in the current host s390 z14ZR1-best IBM z14 Model ZR1 GA1 with best features supported by the accelerator in the current host s390 gen15a-best IBM z15 GA1 with best features supported by the accelerator in the current host s390 gen15b-best IBM 8562 GA1 with best features supported by the accelerator in the current host There is a small but important difference between "max"/"host" and "best". Max really means "all features", including deprecated ones. "best", however, can disable experimental or deprecated features. Or any other features we don't want to be enabled when somebody selects a model manually. On s390x, the feature "csske" is deprecated. New HW still has it, but we want new guests to run without this facility. Dropping it from "max" would affect existing setups. We already changed the default model (e.g., -cpu z13) to disable it with never QEMU machines. E.g., nested virtualization features on some architectures could be a feature set you want to disable, although contained in the "max" model. (e.g., no migration support yet). I am not completely against calling these "max" models instead of "best" models, but I think this makes it clearer that there is indeed a difference. Maybe, we even want a "-cpu best" that would not map to "-cpu host"/"-cpu max", but to a cleaned up "-cpu host"/"-cpu max" (e.g., disable deprecated features). Long term, we might even want to change the default when no "-cpu" is specified to "-cpu best" - which should now be possible with the latest QEMU changes to query the default model for a specific QEMU machine. -- Thanks, David / dhildenb
On Fri, 8 Nov 2019 at 12:46, David Hildenbrand <david@redhat.com> wrote: > There is a small but important difference between "max"/"host" and > "best". Max really means "all features", including deprecated ones. > "best", however, can disable experimental or deprecated features. Or any > other features we don't want to be enabled when somebody selects a model > manually. > > On s390x, the feature "csske" is deprecated. New HW still has it, but we > want new guests to run without this facility. Dropping it from "max" > would affect existing setups. We already changed the default model > (e.g., -cpu z13) to disable it with never QEMU machines. > > E.g., nested virtualization features on some architectures could be a > feature set you want to disable, although contained in the "max" model. > (e.g., no migration support yet). > > > I am not completely against calling these "max" models instead of "best" > models, but I think this makes it clearer that there is indeed a difference. Hmm. I see the distinction, but is it one that's sufficiently worth making that we want to expose it to our users, possibly try to add it to the other architectures, etc ? How bad is it if the CPU provides some legacy deprecated feature that the guest just doesn't use ? 'max' already shouldn't include experimental features, at least for Arm -- those should be off by default, because they're experimental and you only want users to get them if they explicitly opt in via '-cpu something,+x-my-feature'. thanks -- PMM
On Fri, Nov 08, 2019 at 01:02:28PM +0000, Peter Maydell wrote: > On Fri, 8 Nov 2019 at 12:46, David Hildenbrand <david@redhat.com> wrote: > > There is a small but important difference between "max"/"host" and > > "best". Max really means "all features", including deprecated ones. > > "best", however, can disable experimental or deprecated features. Or any > > other features we don't want to be enabled when somebody selects a model > > manually. On x86, this is implemented by "host". "max" gives you the full set of features that can be enabled by the user. "host" gives you a reasonable set of features you will want to see enabled by default when the user says "use the host CPU". > > > > On s390x, the feature "csske" is deprecated. New HW still has it, but we > > want new guests to run without this facility. Dropping it from "max" > > would affect existing setups. We already changed the default model > > (e.g., -cpu z13) to disable it with never QEMU machines. > > > > E.g., nested virtualization features on some architectures could be a > > feature set you want to disable, although contained in the "max" model. > > (e.g., no migration support yet). > > > > > > I am not completely against calling these "max" models instead of "best" > > models, but I think this makes it clearer that there is indeed a difference. > > Hmm. I see the distinction, but is it one that's sufficiently > worth making that we want to expose it to our users, possibly > try to add it to the other architectures, etc ? How bad is it > if the CPU provides some legacy deprecated feature that the > guest just doesn't use ? > "max" isn't something we want to expose to end users. It is something we need to expose to other software components. > 'max' already shouldn't include experimental features, at least > for Arm -- those should be off by default, because they're > experimental and you only want users to get them if they > explicitly opt in via '-cpu something,+x-my-feature'. The whole point of "max" is to tell management software which features are valid to be enabled in a host. If "+x-my-feature" works, "x-my-feature" must be included in "max". -- Eduardo
On Fri, 8 Nov 2019 at 19:11, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Fri, Nov 08, 2019 at 01:02:28PM +0000, Peter Maydell wrote: > > On Fri, 8 Nov 2019 at 12:46, David Hildenbrand <david@redhat.com> wrote: > > > There is a small but important difference between "max"/"host" and > > > "best". Max really means "all features", including deprecated ones. > > > "best", however, can disable experimental or deprecated features. Or any > > > other features we don't want to be enabled when somebody selects a model > > > manually. > > On x86, this is implemented by "host". "max" gives you the full > set of features that can be enabled by the user. "host" gives > you a reasonable set of features you will want to see enabled by > default when the user says "use the host CPU". How does "host" work for TCG on x86? > > Hmm. I see the distinction, but is it one that's sufficiently > > worth making that we want to expose it to our users, possibly > > try to add it to the other architectures, etc ? How bad is it > > if the CPU provides some legacy deprecated feature that the > > guest just doesn't use ? > > > > "max" isn't something we want to expose to end users. It is > something we need to expose to other software components. We seem to have a disagreement here about what 'max' is intended for and what its semantics for. That seems unfortunate... For Arm, "max" is absolutely something we want to expose to end users. It's the easiest way for a user to say "give me something that's going to work". "host" doesn't work on TCG, only with KVM. > > 'max' already shouldn't include experimental features, at least > > for Arm -- those should be off by default, because they're > > experimental and you only want users to get them if they > > explicitly opt in via '-cpu something,+x-my-feature'. > > The whole point of "max" is to tell management software which > features are valid to be enabled in a host. If "+x-my-feature" > works, "x-my-feature" must be included in "max". No, definitely not. Experimental features should never be enabled by default -- the user must explicitly opt into them so they are aware that they're using something that might change behaviour or go away in a future QEMU version. Also, from my point of view "max" is not for the benefit of management software in particular. It's a convenience for users primarily (and also for management software if it doesn't want to care whether it's running KVM or TCG). If management software wants a way to ask "what features might be valid" we should provide them with one which doesn't require those features to be enabled-by-default in the 'max' CPU. thanks -- PMM
On 09.11.19 17:07, Peter Maydell wrote: > On Fri, 8 Nov 2019 at 19:11, Eduardo Habkost <ehabkost@redhat.com> wrote: >> >> On Fri, Nov 08, 2019 at 01:02:28PM +0000, Peter Maydell wrote: >>> On Fri, 8 Nov 2019 at 12:46, David Hildenbrand <david@redhat.com> wrote: >>>> There is a small but important difference between "max"/"host" and >>>> "best". Max really means "all features", including deprecated ones. >>>> "best", however, can disable experimental or deprecated features. Or any >>>> other features we don't want to be enabled when somebody selects a model >>>> manually. >> >> On x86, this is implemented by "host". "max" gives you the full >> set of features that can be enabled by the user. "host" gives >> you a reasonable set of features you will want to see enabled by >> default when the user says "use the host CPU". > > How does "host" work for TCG on x86? I think just like on s390x, host is limited to KVM only. > >>> Hmm. I see the distinction, but is it one that's sufficiently >>> worth making that we want to expose it to our users, possibly >>> try to add it to the other architectures, etc ? How bad is it >>> if the CPU provides some legacy deprecated feature that the >>> guest just doesn't use ? >>> >> >> "max" isn't something we want to expose to end users. It is >> something we need to expose to other software components. > > We seem to have a disagreement here about what 'max' is intended > for and what its semantics for. That seems unfortunate... > > For Arm, "max" is absolutely something we want to expose to > end users. It's the easiest way for a user to say "give me > something that's going to work". "host" doesn't work on TCG, > only with KVM. t460s: ~/git/qemu master $ s390x-softmmu/qemu-system-s390x -cpu help | grep max s390 max Enables all features supported by the accelerator in the current host t460s: ~/git/qemu master $ x86_64-softmmu/qemu-system-x86_64 -cpu help | grep max x86 max Enables all features supported by the accelerator in the current host x86 and s390x interpret the "all features supported" as "possible in this configuration", not "supported" like in a support statement. When not passing a "-cpu", you will automatically get the default model assigned (e.g., host vs. qemu model on s390x). "max" does no mimic that! > >>> 'max' already shouldn't include experimental features, at least >>> for Arm -- those should be off by default, because they're >>> experimental and you only want users to get them if they >>> explicitly opt in via '-cpu something,+x-my-feature'. >> >> The whole point of "max" is to tell management software which >> features are valid to be enabled in a host. If "+x-my-feature" >> works, "x-my-feature" must be included in "max". > > No, definitely not. Experimental features should never be > enabled by default -- the user must explicitly opt into them > so they are aware that they're using something that might > change behaviour or go away in a future QEMU version. > > Also, from my point of view "max" is not for the benefit > of management software in particular. It's a convenience > for users primarily (and also for management software if > it doesn't want to care whether it's running KVM or TCG). > > If management software wants a way to ask "what features > might be valid" we should provide them with one which > doesn't require those features to be enabled-by-default > in the 'max' CPU. > > thanks > -- PMM > My personal opinion: "max" really means "all features". If we want an automatic way to specify something you requested ("give me something that's going to work") we either have to change the definition of the max model for alla rchitectures or introduce something that really matches the "no -cpu specified" - e.g., "best". -- Thanks, David / dhildenb
On Mon, 18 Nov 2019 at 10:47, David Hildenbrand <david@redhat.com> wrote: > My personal opinion: "max" really means "all features". If we want an > automatic way to specify something you requested ("give me something > that's going to work") we either have to change the definition of the > max model for alla rchitectures or introduce something that really > matches the "no -cpu specified" - e.g., "best". I don't strongly object to 'max' including deprecated features, but I do definitely object to 'max' including by default any experimental (x- prefix) features. Those should never be enabled (whatever the '-cpu foo' name) unless the user has specifically opted into them: that's the point of the x- prefix. We need to be able to tell from the command line whether it's got any non-standard weirdness enabled. thanks -- PMM
On 18.11.19 11:53, Peter Maydell wrote: > On Mon, 18 Nov 2019 at 10:47, David Hildenbrand <david@redhat.com> wrote: >> My personal opinion: "max" really means "all features". If we want an >> automatic way to specify something you requested ("give me something >> that's going to work") we either have to change the definition of the >> max model for alla rchitectures or introduce something that really >> matches the "no -cpu specified" - e.g., "best". > > I don't strongly object to 'max' including deprecated features, > but I do definitely object to 'max' including by default any > experimental (x- prefix) features. Those should never be > enabled (whatever the '-cpu foo' name) unless the user has > specifically opted into them: that's the point of the x- prefix. > We need to be able to tell from the command line whether it's > got any non-standard weirdness enabled. I'll let Eduardo respond to that, as we don't really have experimental features on s390x, especially under KVM ("host" corresponds to "max"). > > thanks > -- PMM > -- Thanks, David / dhildenb
On Mon, 18 Nov 2019 at 10:56, David Hildenbrand <david@redhat.com> wrote: > > On 18.11.19 11:53, Peter Maydell wrote: > > On Mon, 18 Nov 2019 at 10:47, David Hildenbrand <david@redhat.com> wrote: > >> My personal opinion: "max" really means "all features". If we want an > >> automatic way to specify something you requested ("give me something > >> that's going to work") we either have to change the definition of the > >> max model for alla rchitectures or introduce something that really > >> matches the "no -cpu specified" - e.g., "best". > > > > I don't strongly object to 'max' including deprecated features, > > but I do definitely object to 'max' including by default any > > experimental (x- prefix) features. Those should never be > > enabled (whatever the '-cpu foo' name) unless the user has > > specifically opted into them: that's the point of the x- prefix. > > We need to be able to tell from the command line whether it's > > got any non-standard weirdness enabled. > > I'll let Eduardo respond to that, as we don't really have experimental > features on s390x, especially under KVM ("host" corresponds to "max"). Yeah, I would expect that if the kernel has fixed the KVM interface to a feature then it wouldn't be experimental. Experimental mostly will apply to TCG, where we might have implementations based on a draft version of an architecture specification (like the riscv hypervisor spec) that could incompatibly change in future. thanks -- PMM
On Mon, Nov 18, 2019 at 11:56:43AM +0100, David Hildenbrand wrote: > On 18.11.19 11:53, Peter Maydell wrote: > > On Mon, 18 Nov 2019 at 10:47, David Hildenbrand <david@redhat.com> wrote: > > > My personal opinion: "max" really means "all features". If we want an > > > automatic way to specify something you requested ("give me something > > > that's going to work") we either have to change the definition of the > > > max model for alla rchitectures or introduce something that really > > > matches the "no -cpu specified" - e.g., "best". > > > > I don't strongly object to 'max' including deprecated features, > > but I do definitely object to 'max' including by default any > > experimental (x- prefix) features. Those should never be > > enabled (whatever the '-cpu foo' name) unless the user has > > specifically opted into them: that's the point of the x- prefix. > > We need to be able to tell from the command line whether it's > > got any non-standard weirdness enabled. > > I'll let Eduardo respond to that, as we don't really have experimental > features on s390x, especially under KVM ("host" corresponds to "max"). Be them experimental or deprecated, we need all features included on "max" if we want to make them usable through libvirt. The fact Peter cares about defaults in "max" when used by humans indicates we have incompatible definitions of "max", and I don't think we can conciliate them. We could rename the CPU model that is intended for humans on arm, or we can document clearly that the semantics of "max" in x86/s390x are completely different from arm. Peter, what do you prefer? -- Eduardo
On Mon, 18 Nov 2019 at 18:49, Eduardo Habkost <ehabkost@redhat.com> wrote: > Be them experimental or deprecated, we need all features included > on "max" if we want to make them usable through libvirt. The > fact Peter cares about defaults in "max" when used by humans > indicates we have incompatible definitions of "max", and I don't > think we can conciliate them. > > We could rename the CPU model that is intended for humans on arm, > or we can document clearly that the semantics of "max" in > x86/s390x are completely different from arm. Peter, what do you > prefer? I don't want us to have different definitions of max on different architectures if we can avoid it. More importantly, I don't think that x- features should ever be enabled by default for *any* CPU type on *any* architecture (unless the CPU type itself was experimental, I suppose), whatever that architecture's semantics for 'max', 'best', etc are. I think the solution to this is to not use 'max' as some odd way of letting libvirt do feature detection. I'm not sure how trying to use 'max' as a proxy for "all features on" would work for features which can't exist on 'max' but do exist on other CPU types (for instance for Arm some features make no sense on the A-profile 'max' CPU and aren't provided there, but do exist for M-profile CPUs like cortex-m4), so I don't see how libvirt can usefully use it anyway. Why should it matter whether a feature is enabled or disabled by default in the CPU type? It ought to be probeable by QMP either way (ie there is a difference between "this CPU has this feature switch and it is on by default", "this CPU has this feature switch and it is off by default" and "this CPU does not have this feature switch at all", and presumably libvirt needs to distinguish them). thanks -- PMM
On Mon, Nov 18, 2019 at 09:19:55PM +0000, Peter Maydell wrote: > On Mon, 18 Nov 2019 at 18:49, Eduardo Habkost <ehabkost@redhat.com> wrote: > > Be them experimental or deprecated, we need all features included > > on "max" if we want to make them usable through libvirt. The > > fact Peter cares about defaults in "max" when used by humans > > indicates we have incompatible definitions of "max", and I don't > > think we can conciliate them. > > > > We could rename the CPU model that is intended for humans on arm, > > or we can document clearly that the semantics of "max" in > > x86/s390x are completely different from arm. Peter, what do you > > prefer? > > I don't want us to have different definitions of max on different > architectures if we can avoid it. More importantly, I don't think that > x- features should ever be enabled by default for *any* CPU type > on *any* architecture (unless the CPU type itself was experimental, > I suppose), whatever that architecture's semantics for 'max', > 'best', etc are. > > I think the solution to this is to not use 'max' as some odd way of > letting libvirt do feature detection. I'm not sure how trying to > use 'max' as a proxy for "all features on" would work for features > which can't exist on 'max' but do exist on other CPU types > (for instance for Arm some features make no sense on the > A-profile 'max' CPU and aren't provided there, but do exist for > M-profile CPUs like cortex-m4), so I don't see how libvirt > can usefully use it anyway. libvirt uses it through query-cpu-model-expansion. > > Why should it matter whether a feature is enabled > or disabled by default in the CPU type? It ought to be probeable > by QMP either way (ie there is a difference between > "this CPU has this feature switch and it is on by default", > "this CPU has this feature switch and it is off by default" > and "this CPU does not have this feature switch at all", > and presumably libvirt needs to distinguish them). Its use case is neither "this CPU has this feature switch" or for "it is on|off by default". We use it to probe for "this feature can be enabled in this host hardware+kernel+QEMU combination". In other words, in x86 and s390x "max" is just a reserved name for the query-cpu-model-expansion command arguments in s390x and x86. The fact that it is visible to users can be considered a bug, and we can fix that. If you still don't like how query-cpu-model-expansion works, we can also discuss that. But I'm not sure it would be a good use of our (and libvirt developers') time. -- Eduardo
On Mon, 18 Nov 2019 at 22:04, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Mon, Nov 18, 2019 at 09:19:55PM +0000, Peter Maydell wrote: > > Why should it matter whether a feature is enabled > > or disabled by default in the CPU type? It ought to be probeable > > by QMP either way (ie there is a difference between > > "this CPU has this feature switch and it is on by default", > > "this CPU has this feature switch and it is off by default" > > and "this CPU does not have this feature switch at all", > > and presumably libvirt needs to distinguish them). > > Its use case is neither "this CPU has this feature switch" or for > "it is on|off by default". We use it to probe for "this feature > can be enabled in this host hardware+kernel+QEMU combination". OK. Well, the answer to that depends on the name of the CPU, in general. So you can't use a fake CPU name to try to answer the question. > In other words, in x86 and s390x "max" is just a reserved name > for the query-cpu-model-expansion command arguments in s390x and > x86. The fact that it is visible to users can be considered a > bug, and we can fix that. I think 'max' is useful to users, and we've provided it to users, so removing it again would be a compatibility break. I'm not entirely sure where we go from here... > If you still don't like how query-cpu-model-expansion works, we > can also discuss that. But I'm not sure it would be a good use > of our (and libvirt developers') time. I don't hugely care about query-cpu-model-expansion. I just don't want it to have bad effects on the semantics of user-facing stuff like x- properties. thanks -- PMM
On 19.11.19 10:22, Peter Maydell wrote: > On Mon, 18 Nov 2019 at 22:04, Eduardo Habkost <ehabkost@redhat.com> wrote: >> >> On Mon, Nov 18, 2019 at 09:19:55PM +0000, Peter Maydell wrote: >>> Why should it matter whether a feature is enabled >>> or disabled by default in the CPU type? It ought to be probeable >>> by QMP either way (ie there is a difference between >>> "this CPU has this feature switch and it is on by default", >>> "this CPU has this feature switch and it is off by default" >>> and "this CPU does not have this feature switch at all", >>> and presumably libvirt needs to distinguish them). >> >> Its use case is neither "this CPU has this feature switch" or for >> "it is on|off by default". We use it to probe for "this feature >> can be enabled in this host hardware+kernel+QEMU combination". > > OK. Well, the answer to that depends on the name of the CPU, > in general. So you can't use a fake CPU name to try to answer > the question. > >> In other words, in x86 and s390x "max" is just a reserved name >> for the query-cpu-model-expansion command arguments in s390x and >> x86. The fact that it is visible to users can be considered a >> bug, and we can fix that. > > I think 'max' is useful to users, and we've provided it to users, > so removing it again would be a compatibility break. I'm not > entirely sure where we go from here... I agree, right now on s390x "-cpu max" behaves almost like "-cpu qemu"/"-cpu host", but you don't have to care about the accelerator. Removing it might we evil. Removing it is not really an option. > >> If you still don't like how query-cpu-model-expansion works, we >> can also discuss that. But I'm not sure it would be a good use >> of our (and libvirt developers') time. > > I don't hugely care about query-cpu-model-expansion. I > just don't want it to have bad effects on the semantics > of user-facing stuff like x- properties. IMHO, max should really include all features (yes, also the bad x-features on arm :) ) and we should have a way to give users the opportunity to specify "just give me the best model independent of the accelerator" - something like a "best" model, but I don't care about the name. E.g., we could introduce a new model for that purpose and start warning if "-cpu max" is used (and recommend them to use the other model) to run a VM and not simply to query model properties via the qmp interfaces. The users are aware of the model restrictions and can switch. -- Thanks, David / dhildenb
On Tue, 19 Nov 2019 at 09:59, David Hildenbrand <david@redhat.com> wrote: > > On 19.11.19 10:22, Peter Maydell wrote: > > I don't hugely care about query-cpu-model-expansion. I > > just don't want it to have bad effects on the semantics > > of user-facing stuff like x- properties. > > IMHO, max should really include all features (yes, also the bad > x-features on arm :) ) and we should have a way to give users the > opportunity to specify "just give me the best model independent of the > accelerator" - something like a "best" model, but I don't care about the > name. How would "max includes all features" work if we have two x- features (or even two normal features!) which are incompatible with each other? How does it work for features which are valid for some other CPU type but not for 'max'? The design seems to assume a rather simplified system where every feature is independent and can always be applied to every CPU, which I don't think is guaranteed to be the case. thanks -- PMM
On 19.11.19 11:36, Peter Maydell wrote: > On Tue, 19 Nov 2019 at 09:59, David Hildenbrand <david@redhat.com> wrote: >> >> On 19.11.19 10:22, Peter Maydell wrote: >>> I don't hugely care about query-cpu-model-expansion. I >>> just don't want it to have bad effects on the semantics >>> of user-facing stuff like x- properties. >> >> IMHO, max should really include all features (yes, also the bad >> x-features on arm :) ) and we should have a way to give users the >> opportunity to specify "just give me the best model independent of the >> accelerator" - something like a "best" model, but I don't care about the >> name. > > How would "max includes all features" work if we have two > x- features (or even two normal features!) which are incompatible > with each other? How does it work for features which are I assume the "max" model should at least be consistent, see below. > valid for some other CPU type but not for 'max'? The design > seems to assume a rather simplified system where every > feature is independent and can always be applied to every > CPU, which I don't think is guaranteed to be the case. I do agree that the use case of "max" for detecting which features can be enabled for any CPU model is quite simplified and would also not work like this on s390x. It really means "give me the maximum/latest/greatest you can". Some examples on s390x: On s390x, we don't allow to enable new features for older CPU generation. "vx" was introduced with a z13. If "max" is a z13, it can include "vx", if available. However, if you select a z12 (zEC12), you cannot enable "vx": "Feature '%s' is not available for CPU model '%s', it was introduced with later models." Also, we have dependency checks (target/s390x/cpu_models.c:check_consistency()), that at least warn on inconsistencies between features (feature X requires feature Y). We would need more fine-grained "max" models. E.g., z13-max vs. z13-best vs. z13-base. - z13-max: Maximum features that can be enabled on the current accel/host for a z13. - z13-best: Best features that can be enabled on the current accel/host for a z13. - z13-base: base feature set, independent of current accel/host for a z13. (we do have this already on s390x) > > thanks > -- PMM > -- Thanks, David / dhildenb
On Tue, Nov 19, 2019 at 12:00:14PM +0100, David Hildenbrand wrote: > On 19.11.19 11:36, Peter Maydell wrote: > > On Tue, 19 Nov 2019 at 09:59, David Hildenbrand <david@redhat.com> wrote: > > > > > > On 19.11.19 10:22, Peter Maydell wrote: > > > > I don't hugely care about query-cpu-model-expansion. I > > > > just don't want it to have bad effects on the semantics > > > > of user-facing stuff like x- properties. > > > > > > IMHO, max should really include all features (yes, also the bad > > > x-features on arm :) ) and we should have a way to give users the > > > opportunity to specify "just give me the best model independent of the > > > accelerator" - something like a "best" model, but I don't care about the > > > name. I'm in agreement with Peter, here: if "max" is user-visible, it's better to make it provide more usable defaults. > > > > How would "max includes all features" work if we have two > > x- features (or even two normal features!) which are incompatible > > with each other? How does it work for features which are > > I assume the "max" model should at least be consistent, see below. > > > valid for some other CPU type but not for 'max'? The design > > seems to assume a rather simplified system where every > > feature is independent and can always be applied to every > > CPU, which I don't think is guaranteed to be the case. > > I do agree that the use case of "max" for detecting which features can be > enabled for any CPU model is quite simplified and would also not work like > this on s390x. It really means "give me the maximum/latest/greatest you > can". Some examples on s390x: > > On s390x, we don't allow to enable new features for older CPU generation. > "vx" was introduced with a z13. If "max" is a z13, it can include "vx", if > available. However, if you select a z12 (zEC12), you cannot enable "vx": > > "Feature '%s' is not available for CPU model '%s', it was introduced with > later models." > > Also, we have dependency checks > (target/s390x/cpu_models.c:check_consistency()), that at least warn on > inconsistencies between features (feature X requires feature Y). > > We would need more fine-grained "max" models. E.g., z13-max vs. z13-best vs. > z13-base. > > - z13-max: Maximum features that can be enabled on the current > accel/host for a z13. > - z13-best: Best features that can be enabled on the current > accel/host for a z13. > - z13-base: base feature set, independent of current > accel/host for a z13. (we do have this already on s390x) We don't need to create new CPU types for that, do we? These different modes could be configured by a CPU option (e.g. "z13,features=max"[1], "z13,features=best"). If we do add new CPU options to tune feature defaults, libvirt can start using these options on query-cpu-model-expansion, and everybody will be happy. No need to change defaults in the "max" CPU model in a way that makes it less usable just to make query-cpu-model-expansion work. [1] Probably we need to call it something else instead of "features=max", just to avoid confusion with the "max" CPU model. Maybe "experimental-features=on", "recommended-features=on"? -- Eduardo
On 19.11.19 20:42, Eduardo Habkost wrote: > On Tue, Nov 19, 2019 at 12:00:14PM +0100, David Hildenbrand wrote: >> On 19.11.19 11:36, Peter Maydell wrote: >>> On Tue, 19 Nov 2019 at 09:59, David Hildenbrand <david@redhat.com> wrote: >>>> >>>> On 19.11.19 10:22, Peter Maydell wrote: >>>>> I don't hugely care about query-cpu-model-expansion. I >>>>> just don't want it to have bad effects on the semantics >>>>> of user-facing stuff like x- properties. >>>> >>>> IMHO, max should really include all features (yes, also the bad >>>> x-features on arm :) ) and we should have a way to give users the >>>> opportunity to specify "just give me the best model independent of the >>>> accelerator" - something like a "best" model, but I don't care about the >>>> name. > > I'm in agreement with Peter, here: if "max" is user-visible, it's > better to make it provide more usable defaults. Agreed, unless we warn the user about the model. >>> >>> How would "max includes all features" work if we have two >>> x- features (or even two normal features!) which are incompatible >>> with each other? How does it work for features which are >> >> I assume the "max" model should at least be consistent, see below. >> >>> valid for some other CPU type but not for 'max'? The design >>> seems to assume a rather simplified system where every >>> feature is independent and can always be applied to every >>> CPU, which I don't think is guaranteed to be the case. >> >> I do agree that the use case of "max" for detecting which features can be >> enabled for any CPU model is quite simplified and would also not work like >> this on s390x. It really means "give me the maximum/latest/greatest you >> can". Some examples on s390x: >> >> On s390x, we don't allow to enable new features for older CPU generation. >> "vx" was introduced with a z13. If "max" is a z13, it can include "vx", if >> available. However, if you select a z12 (zEC12), you cannot enable "vx": >> >> "Feature '%s' is not available for CPU model '%s', it was introduced with >> later models." >> >> Also, we have dependency checks >> (target/s390x/cpu_models.c:check_consistency()), that at least warn on >> inconsistencies between features (feature X requires feature Y). >> >> We would need more fine-grained "max" models. E.g., z13-max vs. z13-best vs. >> z13-base. >> >> - z13-max: Maximum features that can be enabled on the current >> accel/host for a z13. >> - z13-best: Best features that can be enabled on the current >> accel/host for a z13. >> - z13-base: base feature set, independent of current >> accel/host for a z13. (we do have this already on s390x) > > We don't need to create new CPU types for that, do we? These > different modes could be configured by a CPU option (e.g. > "z13,features=max"[1], "z13,features=best"). I somewhat don't like such options mixed into ordinary feature configuration if we can avoid it. It allows for things like z13,features=max,features=best The z13 model is migration-safe. So would be "z13,vx=off". "z13,features=best" would no longer be migration safe. IOW, reusing an existing model along with that feels wrong, read below. Especially, I dislike that one config option (features=best) disables and enables features at the same time. Read below. > > If we do add new CPU options to tune feature defaults, libvirt > can start using these options on query-cpu-model-expansion, and > everybody will be happy. No need to change defaults in the "max" > CPU model in a way that makes it less usable just to make > query-cpu-model-expansion work. > > [1] Probably we need to call it something else instead of > "features=max", just to avoid confusion with the "max" CPU > model. Maybe "experimental-features=on", > "recommended-features=on"? We already do have feature groups on s390x. So these could behave like "host/accelerator dependent" feature groups. I would prefer that over the suggestion above. The only downside is that z13,recommended-features=on could actually "disable" features that are already in z13 (e.g., "vx" is part of z13, but if it's not available in the host we would have to disable it). I don't like these semantic. Maybe introducing new types of models that don't have any features as default could come into play. E.g., z13-best => z13-raw,recommended-features=on z13-max => z13-raw,recommended-features=on,experimental-features=on Maybe we can find a better name for "recommended-features" and "experimental-features" to highlight that these are only "features available via the accelerator in the current host" We could also expose: z13-full => z13-raw,all-features=on Which would include all features theoretically valid for a model (but even if not available). -- Thanks, David / dhildenb
On Wed, Nov 20, 2019 at 11:28:29AM +0100, David Hildenbrand wrote: > On 19.11.19 20:42, Eduardo Habkost wrote: > > On Tue, Nov 19, 2019 at 12:00:14PM +0100, David Hildenbrand wrote: > > > On 19.11.19 11:36, Peter Maydell wrote: > > > > On Tue, 19 Nov 2019 at 09:59, David Hildenbrand <david@redhat.com> wrote: > > > > > > > > > > On 19.11.19 10:22, Peter Maydell wrote: > > > > > > I don't hugely care about query-cpu-model-expansion. I > > > > > > just don't want it to have bad effects on the semantics > > > > > > of user-facing stuff like x- properties. > > > > > > > > > > IMHO, max should really include all features (yes, also the bad > > > > > x-features on arm :) ) and we should have a way to give users the > > > > > opportunity to specify "just give me the best model independent of the > > > > > accelerator" - something like a "best" model, but I don't care about the > > > > > name. > > > > I'm in agreement with Peter, here: if "max" is user-visible, it's > > better to make it provide more usable defaults. > > Agreed, unless we warn the user about the model. > > > > > > > > > How would "max includes all features" work if we have two > > > > x- features (or even two normal features!) which are incompatible > > > > with each other? How does it work for features which are > > > > > > I assume the "max" model should at least be consistent, see below. > > > > > > > valid for some other CPU type but not for 'max'? The design > > > > seems to assume a rather simplified system where every > > > > feature is independent and can always be applied to every > > > > CPU, which I don't think is guaranteed to be the case. > > > > > > I do agree that the use case of "max" for detecting which features can be > > > enabled for any CPU model is quite simplified and would also not work like > > > this on s390x. It really means "give me the maximum/latest/greatest you > > > can". Some examples on s390x: > > > > > > On s390x, we don't allow to enable new features for older CPU generation. > > > "vx" was introduced with a z13. If "max" is a z13, it can include "vx", if > > > available. However, if you select a z12 (zEC12), you cannot enable "vx": > > > > > > "Feature '%s' is not available for CPU model '%s', it was introduced with > > > later models." > > > > > > Also, we have dependency checks > > > (target/s390x/cpu_models.c:check_consistency()), that at least warn on > > > inconsistencies between features (feature X requires feature Y). > > > > > > We would need more fine-grained "max" models. E.g., z13-max vs. z13-best vs. > > > z13-base. > > > > > > - z13-max: Maximum features that can be enabled on the current > > > accel/host for a z13. > > > - z13-best: Best features that can be enabled on the current > > > accel/host for a z13. > > > - z13-base: base feature set, independent of current > > > accel/host for a z13. (we do have this already on s390x) > > > > We don't need to create new CPU types for that, do we? These > > different modes could be configured by a CPU option (e.g. > > "z13,features=max"[1], "z13,features=best"). > > I somewhat don't like such options mixed into ordinary feature configuration > if we can avoid it. It allows for things like > > z13,features=max,features=best > > The z13 model is migration-safe. So would be "z13,vx=off". > "z13,features=best" would no longer be migration safe. > > IOW, reusing an existing model along with that feels wrong, read below. > Especially, I dislike that one config option (features=best) disables and > enables features at the same time. Read below. > > > > > If we do add new CPU options to tune feature defaults, libvirt > > can start using these options on query-cpu-model-expansion, and > > everybody will be happy. No need to change defaults in the "max" > > CPU model in a way that makes it less usable just to make > > query-cpu-model-expansion work. > > > > [1] Probably we need to call it something else instead of > > "features=max", just to avoid confusion with the "max" CPU > > model. Maybe "experimental-features=on", > > "recommended-features=on"? > We already do have feature groups on s390x. So these could behave like > "host/accelerator dependent" feature groups. I would prefer that over the > suggestion above. > > The only downside is that z13,recommended-features=on could actually > "disable" features that are already in z13 (e.g., "vx" is part of z13, but > if it's not available in the host we would have to disable it). I don't like > these semantic. Maybe introducing new types of models that don't have any > features as default could come into play. > > E.g., > > z13-best => z13-raw,recommended-features=on > z13-max => z13-raw,recommended-features=on,experimental-features=on > > Maybe we can find a better name for "recommended-features" and > "experimental-features" to highlight that these are only "features available > via the accelerator in the current host" > > We could also expose: > > z13-full => z13-raw,all-features=on > > Which would include all features theoretically valid for a model (but even > if not available). I don't have a strong opinion on the specifics. If you believe that's the best solution, I trust your judgement. My only point was that we don't always need to add new CPU types for every conceivable use case, and some (but not all) problems can be solved by simple new properties. (To be honest, I'm now having trouble mapping these ideas to real world problems or use cases. What are the specific problems we're trying to solve right now?) -- Eduardo
On 20.11.19 15:04, Eduardo Habkost wrote: > On Wed, Nov 20, 2019 at 11:28:29AM +0100, David Hildenbrand wrote: >> On 19.11.19 20:42, Eduardo Habkost wrote: >>> On Tue, Nov 19, 2019 at 12:00:14PM +0100, David Hildenbrand wrote: >>>> On 19.11.19 11:36, Peter Maydell wrote: >>>>> On Tue, 19 Nov 2019 at 09:59, David Hildenbrand <david@redhat.com> wrote: >>>>>> >>>>>> On 19.11.19 10:22, Peter Maydell wrote: >>>>>>> I don't hugely care about query-cpu-model-expansion. I >>>>>>> just don't want it to have bad effects on the semantics >>>>>>> of user-facing stuff like x- properties. >>>>>> >>>>>> IMHO, max should really include all features (yes, also the bad >>>>>> x-features on arm :) ) and we should have a way to give users the >>>>>> opportunity to specify "just give me the best model independent of the >>>>>> accelerator" - something like a "best" model, but I don't care about the >>>>>> name. >>> >>> I'm in agreement with Peter, here: if "max" is user-visible, it's >>> better to make it provide more usable defaults. >> >> Agreed, unless we warn the user about the model. >> >>>>> >>>>> How would "max includes all features" work if we have two >>>>> x- features (or even two normal features!) which are incompatible >>>>> with each other? How does it work for features which are >>>> >>>> I assume the "max" model should at least be consistent, see below. >>>> >>>>> valid for some other CPU type but not for 'max'? The design >>>>> seems to assume a rather simplified system where every >>>>> feature is independent and can always be applied to every >>>>> CPU, which I don't think is guaranteed to be the case. >>>> >>>> I do agree that the use case of "max" for detecting which features can be >>>> enabled for any CPU model is quite simplified and would also not work like >>>> this on s390x. It really means "give me the maximum/latest/greatest you >>>> can". Some examples on s390x: >>>> >>>> On s390x, we don't allow to enable new features for older CPU generation. >>>> "vx" was introduced with a z13. If "max" is a z13, it can include "vx", if >>>> available. However, if you select a z12 (zEC12), you cannot enable "vx": >>>> >>>> "Feature '%s' is not available for CPU model '%s', it was introduced with >>>> later models." >>>> >>>> Also, we have dependency checks >>>> (target/s390x/cpu_models.c:check_consistency()), that at least warn on >>>> inconsistencies between features (feature X requires feature Y). >>>> >>>> We would need more fine-grained "max" models. E.g., z13-max vs. z13-best vs. >>>> z13-base. >>>> >>>> - z13-max: Maximum features that can be enabled on the current >>>> accel/host for a z13. >>>> - z13-best: Best features that can be enabled on the current >>>> accel/host for a z13. >>>> - z13-base: base feature set, independent of current >>>> accel/host for a z13. (we do have this already on s390x) >>> >>> We don't need to create new CPU types for that, do we? These >>> different modes could be configured by a CPU option (e.g. >>> "z13,features=max"[1], "z13,features=best"). >> >> I somewhat don't like such options mixed into ordinary feature configuration >> if we can avoid it. It allows for things like >> >> z13,features=max,features=best >> >> The z13 model is migration-safe. So would be "z13,vx=off". >> "z13,features=best" would no longer be migration safe. >> >> IOW, reusing an existing model along with that feels wrong, read below. >> Especially, I dislike that one config option (features=best) disables and >> enables features at the same time. Read below. >> >>> >>> If we do add new CPU options to tune feature defaults, libvirt >>> can start using these options on query-cpu-model-expansion, and >>> everybody will be happy. No need to change defaults in the "max" >>> CPU model in a way that makes it less usable just to make >>> query-cpu-model-expansion work. >>> >>> [1] Probably we need to call it something else instead of >>> "features=max", just to avoid confusion with the "max" CPU >>> model. Maybe "experimental-features=on", >>> "recommended-features=on"? >> We already do have feature groups on s390x. So these could behave like >> "host/accelerator dependent" feature groups. I would prefer that over the >> suggestion above. >> >> The only downside is that z13,recommended-features=on could actually >> "disable" features that are already in z13 (e.g., "vx" is part of z13, but >> if it's not available in the host we would have to disable it). I don't like >> these semantic. Maybe introducing new types of models that don't have any >> features as default could come into play. >> >> E.g., >> >> z13-best => z13-raw,recommended-features=on >> z13-max => z13-raw,recommended-features=on,experimental-features=on >> >> Maybe we can find a better name for "recommended-features" and >> "experimental-features" to highlight that these are only "features available >> via the accelerator in the current host" >> >> We could also expose: >> >> z13-full => z13-raw,all-features=on >> >> Which would include all features theoretically valid for a model (but even >> if not available). > > I don't have a strong opinion on the specifics. If you believe > that's the best solution, I trust your judgement. > > My only point was that we don't always need to add new CPU types > for every conceivable use case, and some (but not all) problems > can be solved by simple new properties. Makes sense. > > (To be honest, I'm now having trouble mapping these ideas to real > world problems or use cases. What are the specific problems > we're trying to solve right now?) I mostly only care about z13-best or "z13-raw,recommended-features=on" for now. What's described in patch #2. Allow upper layers (or user on the command line) get a CPU model that has the best features supported by the accelerator on the current host. Esp., include new features to mitigate security issues. -- Thanks, David / dhildenb
On 08.11.19 20:10, Eduardo Habkost wrote: > On Fri, Nov 08, 2019 at 01:02:28PM +0000, Peter Maydell wrote: >> On Fri, 8 Nov 2019 at 12:46, David Hildenbrand <david@redhat.com> wrote: >>> There is a small but important difference between "max"/"host" and >>> "best". Max really means "all features", including deprecated ones. >>> "best", however, can disable experimental or deprecated features. Or any >>> other features we don't want to be enabled when somebody selects a model >>> manually. > > On x86, this is implemented by "host". "max" gives you the full > set of features that can be enabled by the user. "host" gives > you a reasonable set of features you will want to see enabled by > default when the user says "use the host CPU". Interesting. Maybe for s390x we might want to split host/max at one time, too. E.g., exclude deprecated features from "host". Thanks for that info. -- Thanks, David / dhildenb
Patchew URL: https://patchew.org/QEMU/20191108110714.7475-1-david@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: [PATCH v1 0/2] s390x/cpumodel: Introduce "best" model variants Type: series Message-id: 20191108110714.7475-1-david@redhat.com === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu - [tag update] patchew/20191108124219.31348-1-edgar.iglesias@gmail.com -> patchew/20191108124219.31348-1-edgar.iglesias@gmail.com - [tag update] patchew/20191108150123.12213-1-marcandre.lureau@redhat.com -> patchew/20191108150123.12213-1-marcandre.lureau@redhat.com Switched to a new branch 'test' 414a63f s390x/cpumodel: Introduce "best" model variants 40c0719 s390x/cpumodels: Factor out CPU feature dependencies === OUTPUT BEGIN === 1/2 Checking commit 40c07194c720 (s390x/cpumodels: Factor out CPU feature dependencies) 2/2 Checking commit 414a63fe401d (s390x/cpumodel: Introduce "best" model variants) ERROR: line over 90 characters #167: FILE: target/s390x/cpu_models.c:1311: + xcc->desc = g_strdup_printf("%s with best features supported by the accelerator in the current host", total: 1 errors, 0 warnings, 152 lines checked Patch 2/2 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20191108110714.7475-1-david@redhat.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
© 2016 - 2024 Red Hat, Inc.