| Message ID | d352caf1-90c5-558c-1d25-cfd91d4e24fc@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 18.05.2017 11:00, Christian Borntraeger wrote: > On 05/18/2017 10:48 AM, David Hildenbrand wrote: >> On 18.05.2017 03:55, Thomas Huth wrote: >>> On 17.05.2017 18:49, David Hildenbrand wrote: >>>> On 17.05.2017 17:35, Thomas Huth wrote: >>>>> Currently we only present the plain z900 feature bits to the guest, >>>>> but QEMU already emulates some additional features (but not all of >>>>> the next CPU generation, so we can not use the next CPU level as >>>>> default yet). Since newer Linux kernels are checking the feature bits >>>>> and refuse to work if a required feature is missing, we should present >>>>> as much of the supported features as possible when we are running >>>>> with the default "qemu" CPU. >>>>> This patch now adds the "stfle", "extended immediate" and "stckf" >>>>> facility bits to the "qemu" CPU, which are already supported facilities. >>>>> It is unfortunately still not enough to run e.g. recent Fedora kernels, >>>>> but at least it's a first step into the right direction. >>>>> >>>> >>>> Three things: >>>> >>>> 1. Should we care about backwards compatibility? I think so. This should >>>> be fixed up using compat machine properties. (qemu model is a >>>> migration-safe model and could e.g. be used in KVM setups, too). >>> >>> Theoretically, I agree, but do we really care about backwards >>> compatibility at this point in time? All major distro kernels (except >>> Debian, I think) currently do not work in QEMU, so there is currently >>> not that much that can be migrated... >>> And currently, the "qemu" CPU is the very same as the "z900" CPU, so you >>> might also get along with simply using "-cpu z900" on the destination >>> instead, I guess. >> >> If possible, I would like to avoid changing migration safe CPU model. >> And I guess it shouldn't be too hard for now (unless we really change >> the base model to e.g. a z9, then some more work might have to be done) >> >> I think for now, setting "stfle=off" on "s390-cpu-qemu" for compat >> machines should do the trick. >> >>> >>>> 2. I would recommend to not enable STFLE for now. Why? >>>> >>>> It is/was an indication that the system is running on a z9 (and >>>> implicitly has the basic features). This was not only done because >>>> people were lazy, but because this bit was implicitly connected to other >>>> machine properties. >>> >>> Uh, that's ugly! >>> >>>> One popular example is the "DAT-enhancement facility 2". It introduced >>>> the "LOAD PAGE TABLE ENTRY ADDRESS" instruction, but no facility bit was >>>> introduced. SO there is no way to check if the instruction is available >>>> and actually working. >>> >>> Does the Linux kernel use this instruction at all? I just grep'ed >>> through the kernel sources and did not find it. If the Linux kernel does >>> not use it, I think we should ignore this interdependency and just >>> provide the STFLE feature bit to the guest - since recent Linux kernels >>> depend on it. >> >> Yes, current linux doesn't use it, I don't remember if previous versions >> did. Most likely not. The question is if they relied on the stfle==z9 >> assumption. The STFLE facility really is special in that sense. >> >>> >>>> Please note that we added a feature representation for this facility, >>>> because this would allow us later on to at least model removal of such a >>>> facility (if HW actually would drop it) on a CPU model level. >>> >>> What about STFLE bit 78, according to my version of the POP, it says: >>> >>> "The enhanced-DAT facility 2 is installed in the >>> z/Architecture architectural mode." >>> >>> ? >> >> As Aurelien already mentioned, there seemed to be different ways to >> enhance DAT :) enhanced-DAT facility 2 is 2GB page support. >> >>> >>>> 3. This introduces some inconsistency. s390x/cpu_models.c:set_feature() >>>> explicitly tests for such inconsistencies. >>>> >>>> So your QEMU CPU model would have a feature, but you would not be able >>>> to run that model using QEMU when manually specifying it on the command >>>> line. Especially, expanding the "qemu" model and feeding it back to QEMU >>>> will fail. >>> >>> I've checked that I can also successfully disable the features again at >>> the command line, using "-cpu qemu,eimm=false" for example, so not sure >>> what exactly you're talking about here. Could you please elaborate? >> >> Assume libvirt/the user expands the CPU model name "qemu" via >> "qmp-expand-cpu-model "qemu", you will get something like >> >> "z900-base,.....,stfle=on" >> >> If you feed that to QEMU using "-cpu z900-base,...,stfle=on", QEMU will >> detect the inconsistency when setting the property and abort. However, >> "-cpu qemu" will succeed. Please note that these checks actually make >> sense for KVM: >> > > Jason (now on cc) has a patch prepared for other reasons that disabled features > for given machines. I kept the ESOP example in that patch. > That would allow us to disable STFLE for old machines but enable it for 2.10 [...] > Maybe we should split that out and merge such a patch sooner than the > (yet in development) other changes? Yes, that sounds like a good idea, I think we could use the same mechanism here, too, so please split it out and submit it earlier! Thanks a lot, Thomas
On 18.05.2017 11:05, Thomas Huth wrote: > On 18.05.2017 11:00, Christian Borntraeger wrote: >> On 05/18/2017 10:48 AM, David Hildenbrand wrote: >>> On 18.05.2017 03:55, Thomas Huth wrote: >>>> On 17.05.2017 18:49, David Hildenbrand wrote: >>>>> On 17.05.2017 17:35, Thomas Huth wrote: >>>>>> Currently we only present the plain z900 feature bits to the guest, >>>>>> but QEMU already emulates some additional features (but not all of >>>>>> the next CPU generation, so we can not use the next CPU level as >>>>>> default yet). Since newer Linux kernels are checking the feature bits >>>>>> and refuse to work if a required feature is missing, we should present >>>>>> as much of the supported features as possible when we are running >>>>>> with the default "qemu" CPU. >>>>>> This patch now adds the "stfle", "extended immediate" and "stckf" >>>>>> facility bits to the "qemu" CPU, which are already supported facilities. >>>>>> It is unfortunately still not enough to run e.g. recent Fedora kernels, >>>>>> but at least it's a first step into the right direction. >>>>>> >>>>> >>>>> Three things: >>>>> >>>>> 1. Should we care about backwards compatibility? I think so. This should >>>>> be fixed up using compat machine properties. (qemu model is a >>>>> migration-safe model and could e.g. be used in KVM setups, too). >>>> >>>> Theoretically, I agree, but do we really care about backwards >>>> compatibility at this point in time? All major distro kernels (except >>>> Debian, I think) currently do not work in QEMU, so there is currently >>>> not that much that can be migrated... >>>> And currently, the "qemu" CPU is the very same as the "z900" CPU, so you >>>> might also get along with simply using "-cpu z900" on the destination >>>> instead, I guess. >>> >>> If possible, I would like to avoid changing migration safe CPU model. >>> And I guess it shouldn't be too hard for now (unless we really change >>> the base model to e.g. a z9, then some more work might have to be done) >>> >>> I think for now, setting "stfle=off" on "s390-cpu-qemu" for compat >>> machines should do the trick. >>> >>>> >>>>> 2. I would recommend to not enable STFLE for now. Why? >>>>> >>>>> It is/was an indication that the system is running on a z9 (and >>>>> implicitly has the basic features). This was not only done because >>>>> people were lazy, but because this bit was implicitly connected to other >>>>> machine properties. >>>> >>>> Uh, that's ugly! >>>> >>>>> One popular example is the "DAT-enhancement facility 2". It introduced >>>>> the "LOAD PAGE TABLE ENTRY ADDRESS" instruction, but no facility bit was >>>>> introduced. SO there is no way to check if the instruction is available >>>>> and actually working. >>>> >>>> Does the Linux kernel use this instruction at all? I just grep'ed >>>> through the kernel sources and did not find it. If the Linux kernel does >>>> not use it, I think we should ignore this interdependency and just >>>> provide the STFLE feature bit to the guest - since recent Linux kernels >>>> depend on it. >>> >>> Yes, current linux doesn't use it, I don't remember if previous versions >>> did. Most likely not. The question is if they relied on the stfle==z9 >>> assumption. The STFLE facility really is special in that sense. >>> >>>> >>>>> Please note that we added a feature representation for this facility, >>>>> because this would allow us later on to at least model removal of such a >>>>> facility (if HW actually would drop it) on a CPU model level. >>>> >>>> What about STFLE bit 78, according to my version of the POP, it says: >>>> >>>> "The enhanced-DAT facility 2 is installed in the >>>> z/Architecture architectural mode." >>>> >>>> ? >>> >>> As Aurelien already mentioned, there seemed to be different ways to >>> enhance DAT :) enhanced-DAT facility 2 is 2GB page support. >>> >>>> >>>>> 3. This introduces some inconsistency. s390x/cpu_models.c:set_feature() >>>>> explicitly tests for such inconsistencies. >>>>> >>>>> So your QEMU CPU model would have a feature, but you would not be able >>>>> to run that model using QEMU when manually specifying it on the command >>>>> line. Especially, expanding the "qemu" model and feeding it back to QEMU >>>>> will fail. >>>> >>>> I've checked that I can also successfully disable the features again at >>>> the command line, using "-cpu qemu,eimm=false" for example, so not sure >>>> what exactly you're talking about here. Could you please elaborate? >>> >>> Assume libvirt/the user expands the CPU model name "qemu" via >>> "qmp-expand-cpu-model "qemu", you will get something like >>> >>> "z900-base,.....,stfle=on" >>> >>> If you feed that to QEMU using "-cpu z900-base,...,stfle=on", QEMU will >>> detect the inconsistency when setting the property and abort. However, >>> "-cpu qemu" will succeed. Please note that these checks actually make >>> sense for KVM: >>> >> >> Jason (now on cc) has a patch prepared for other reasons that disabled features >> for given machines. I kept the ESOP example in that patch. >> That would allow us to disable STFLE for old machines but enable it for 2.10 > [...] >> Maybe we should split that out and merge such a patch sooner than the >> (yet in development) other changes? > > Yes, that sounds like a good idea, I think we could use the same > mechanism here, too, so please split it out and submit it earlier! > > Thanks a lot, > Thomas > I think this is useful but a different use case: What Christian/Jason have here is a way to fixup default models (e.g. z900, ZEC12...). This is necessary when introducing new features / movinf features from FULL into DEFAULT. We don't want to fixup default models but the s390x-cpu-qemu.
On 18.05.2017 11:14, David Hildenbrand wrote: > On 18.05.2017 11:05, Thomas Huth wrote: >> On 18.05.2017 11:00, Christian Borntraeger wrote: >>> On 05/18/2017 10:48 AM, David Hildenbrand wrote: >>>> On 18.05.2017 03:55, Thomas Huth wrote: >>>>> On 17.05.2017 18:49, David Hildenbrand wrote: >>>>>> On 17.05.2017 17:35, Thomas Huth wrote: >>>>>>> Currently we only present the plain z900 feature bits to the guest, >>>>>>> but QEMU already emulates some additional features (but not all of >>>>>>> the next CPU generation, so we can not use the next CPU level as >>>>>>> default yet). Since newer Linux kernels are checking the feature bits >>>>>>> and refuse to work if a required feature is missing, we should present >>>>>>> as much of the supported features as possible when we are running >>>>>>> with the default "qemu" CPU. >>>>>>> This patch now adds the "stfle", "extended immediate" and "stckf" >>>>>>> facility bits to the "qemu" CPU, which are already supported facilities. >>>>>>> It is unfortunately still not enough to run e.g. recent Fedora kernels, >>>>>>> but at least it's a first step into the right direction. >>>>>>> >>>>>> >>>>>> Three things: >>>>>> >>>>>> 1. Should we care about backwards compatibility? I think so. This should >>>>>> be fixed up using compat machine properties. (qemu model is a >>>>>> migration-safe model and could e.g. be used in KVM setups, too). >>>>> >>>>> Theoretically, I agree, but do we really care about backwards >>>>> compatibility at this point in time? All major distro kernels (except >>>>> Debian, I think) currently do not work in QEMU, so there is currently >>>>> not that much that can be migrated... >>>>> And currently, the "qemu" CPU is the very same as the "z900" CPU, so you >>>>> might also get along with simply using "-cpu z900" on the destination >>>>> instead, I guess. >>>> >>>> If possible, I would like to avoid changing migration safe CPU model. >>>> And I guess it shouldn't be too hard for now (unless we really change >>>> the base model to e.g. a z9, then some more work might have to be done) >>>> >>>> I think for now, setting "stfle=off" on "s390-cpu-qemu" for compat >>>> machines should do the trick. >>>> >>>>> >>>>>> 2. I would recommend to not enable STFLE for now. Why? >>>>>> >>>>>> It is/was an indication that the system is running on a z9 (and >>>>>> implicitly has the basic features). This was not only done because >>>>>> people were lazy, but because this bit was implicitly connected to other >>>>>> machine properties. >>>>> >>>>> Uh, that's ugly! >>>>> >>>>>> One popular example is the "DAT-enhancement facility 2". It introduced >>>>>> the "LOAD PAGE TABLE ENTRY ADDRESS" instruction, but no facility bit was >>>>>> introduced. SO there is no way to check if the instruction is available >>>>>> and actually working. >>>>> >>>>> Does the Linux kernel use this instruction at all? I just grep'ed >>>>> through the kernel sources and did not find it. If the Linux kernel does >>>>> not use it, I think we should ignore this interdependency and just >>>>> provide the STFLE feature bit to the guest - since recent Linux kernels >>>>> depend on it. >>>> >>>> Yes, current linux doesn't use it, I don't remember if previous versions >>>> did. Most likely not. The question is if they relied on the stfle==z9 >>>> assumption. The STFLE facility really is special in that sense. >>>> >>>>> >>>>>> Please note that we added a feature representation for this facility, >>>>>> because this would allow us later on to at least model removal of such a >>>>>> facility (if HW actually would drop it) on a CPU model level. >>>>> >>>>> What about STFLE bit 78, according to my version of the POP, it says: >>>>> >>>>> "The enhanced-DAT facility 2 is installed in the >>>>> z/Architecture architectural mode." >>>>> >>>>> ? >>>> >>>> As Aurelien already mentioned, there seemed to be different ways to >>>> enhance DAT :) enhanced-DAT facility 2 is 2GB page support. >>>> >>>>> >>>>>> 3. This introduces some inconsistency. s390x/cpu_models.c:set_feature() >>>>>> explicitly tests for such inconsistencies. >>>>>> >>>>>> So your QEMU CPU model would have a feature, but you would not be able >>>>>> to run that model using QEMU when manually specifying it on the command >>>>>> line. Especially, expanding the "qemu" model and feeding it back to QEMU >>>>>> will fail. >>>>> >>>>> I've checked that I can also successfully disable the features again at >>>>> the command line, using "-cpu qemu,eimm=false" for example, so not sure >>>>> what exactly you're talking about here. Could you please elaborate? >>>> >>>> Assume libvirt/the user expands the CPU model name "qemu" via >>>> "qmp-expand-cpu-model "qemu", you will get something like >>>> >>>> "z900-base,.....,stfle=on" >>>> >>>> If you feed that to QEMU using "-cpu z900-base,...,stfle=on", QEMU will >>>> detect the inconsistency when setting the property and abort. However, >>>> "-cpu qemu" will succeed. Please note that these checks actually make >>>> sense for KVM: >>>> >>> >>> Jason (now on cc) has a patch prepared for other reasons that disabled features >>> for given machines. I kept the ESOP example in that patch. >>> That would allow us to disable STFLE for old machines but enable it for 2.10 >> [...] >>> Maybe we should split that out and merge such a patch sooner than the >>> (yet in development) other changes? >> >> Yes, that sounds like a good idea, I think we could use the same >> mechanism here, too, so please split it out and submit it earlier! >> > I think this is useful but a different use case: > > What Christian/Jason have here is a way to fixup default models (e.g. > z900, ZEC12...). This is necessary when introducing new features / > movinf features from FULL into DEFAULT. > > We don't want to fixup default models but the s390x-cpu-qemu. Looking at the functions again, I think you're right, David... it's similar at a first glance, but I'd need slightly different functions here. Ok, so never mind, I guess I have to do it in a different way here... Thomas
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 3c12735..26b0ac9 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -30,6 +30,7 @@ #include "ipl.h" #include "hw/s390x/s390-virtio-ccw.h" #include "hw/s390x/css-bridge.h" +#include "cpu_models.h" static const char *const reset_dev_types[] = { TYPE_VIRTUAL_CSS_BRIDGE, @@ -481,6 +482,7 @@ DEFINE_CCW_MACHINE(2_10, "2.10", true); static void ccw_machine_2_9_instance_options(MachineState *machine) { ccw_machine_2_10_instance_options(machine); + s390_cpudef_featoff_greater(12, 1, S390_FEAT_ESOP); } static void ccw_machine_2_9_class_options(MachineClass *mc) diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c index 71ddb6c..5f295a5 100644 --- a/target/s390x/cpu_models.c +++ b/target/s390x/cpu_models.c @@ -78,6 +78,32 @@ static S390CPUDef s390_cpu_defs[] = { CPUDEF_INIT(0x3906, 14, 1, 47, 0x08000000U, "z14", "IBM z14 GA1"), }; +void s390_cpudef_featoff(uint8_t gen, uint8_t ec_ga, S390Feat feat) +{ + const S390CPUDef *def; + + def = s390_find_cpu_def(0, gen, ec_ga, NULL); + clear_bit(feat, (unsigned long *)&def->default_feat); +} + +void s390_cpudef_featoff_greater(uint8_t gen, uint8_t ec_ga, S390Feat feat) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(s390_cpu_defs); i++) { + const S390CPUDef *def = &s390_cpu_defs[i]; + + if (def->gen < gen) { + continue; + } + if (def->gen == gen && def->ec_ga < ec_ga) { + continue; + } + + clear_bit(feat, (unsigned long *)&def->default_feat); + } +} + uint32_t s390_get_hmfai(void) { static S390CPU *cpu; diff --git a/target/s390x/cpu_models.h b/target/s390x/cpu_models.h index 136a602..881eb65 100644 --- a/target/s390x/cpu_models.h +++ b/target/s390x/cpu_models.h @@ -69,6 +69,8 @@ typedef struct S390CPUModel { #define ibc_gen(x) (x == 0 ? 0 : ((x >> 4) + S390_GEN_Z10)) #define ibc_ec_ga(x) (x & 0xf) +void s390_cpudef_featoff(uint8_t gen, uint8_t ec_ga, S390Feat feat); +void s390_cpudef_featoff_greater(uint8_t gen, uint8_t ec_ga, S390Feat feat); uint32_t s390_get_hmfai(void); uint8_t s390_get_mha_pow(void); uint32_t s390_get_ibc_val(void); diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c index 3efd5dd..105e5f5 100644 --- a/target/s390x/gen-features.c +++ b/target/s390x/gen-features.c @@ -515,6 +515,7 @@ static uint16_t default_GEN12_GA1[] = { S390_FEAT_ADAPTER_EVENT_NOTIFICATION, S390_FEAT_ADAPTER_INT_SUPPRESSION, S390_FEAT_EDAT_2, + S390_FEAT_ESOP, };