s390x cpu model improvements for KVM

Let's also properly forward that bit. It should always be set. I verified it under z/VM, it seems to be always set there. For now, zKVM guests never get that bit set when the CPU model is active.

The PoP mentiones, that z800 + z900 (HW generation 7) always set this bit to 0, so let's take care of that.

Signed-off-by David Hildenbrand <[email protected]>

Currently, under z/VM on a 0x2827, QEMU will detect a 0x2828 if no IBC value is provided. QEMU will simply take the last model of that HW generation, which happens to be the BC version.

Let's improve our search for that case by selecting the latest CPU definition that matches the CPU type. This might still detect e.g. a GA2 version on a GA1 system, but as we don't have further information at hand, there isn't too much we can do about it.

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/cpu_models.c | 9 +++++++++
 1 file changed, 9 insertions(+)

target/s390x/cpu_models.c +9 -0

index 9e23535..b6220c8 100644
@@ -184,6 +184,7 @@ const S390CPUDef *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga,
                                     S390FeatBitmap features)
 {
     const S390CPUDef *last_compatible = NULL;
+    const S390CPUDef *matching_cpu_type = NULL;
     int i;
 
     if (!gen) {

@@ -218,8 +219,16 @@ const S390CPUDef *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga,
         if (def->type == type && def->ec_ga == ec_ga) {
             return def;
         }
+        /* remember if we've at least seen one with the same cpu type */
+        if (def->type == type) {
+            matching_cpu_type = def;
+        }
         last_compatible = def;
     }
+    /* prefer the model with the same cpu type, esp. don't take the BC for EC */
+    if (matching_cpu_type) {
+        return matching_cpu_type;
+    }
     return last_compatible;
 }
 

Quoted 9 lines · David Hildenbrand

Currently, under z/VM on a 0x2827, QEMU will detect a 0x2828 if no IBC value is provided. QEMU will simply take the last model of that HW generation, which happens to be the BC version.

Let's improve our search for that case by selecting the latest CPU definition that matches the CPU type. This might still detect e.g. a GA2 version on a GA1 system, but as we don't have further information at hand, there isn't too much we can do about it.

Isn't it better to choose the earliest machine possible in this case? Systems
are backwards compatible far more often than they are forwards compatible.

Said in a more verbose way, I'd rather pretend to be a GA1 on a GA2 (ignoring
some new features) than pretend to be a GA2 on a GA1 and attempt to use something that does not exist.

I'm sure you've thought of all of this though, so what am I missing?

Quoted 35 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/cpu_models.c | 9 +++++++++
 1 file changed, 9 insertions(+)

target/s390x/cpu_models.c +9 -0

index 9e23535..b6220c8 100644
@@ -184,6 +184,7 @@ const S390CPUDef *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga,
                                     S390FeatBitmap features)
 {
     const S390CPUDef *last_compatible = NULL;
+    const S390CPUDef *matching_cpu_type = NULL;
     int i;
 
     if (!gen) {

@@ -218,8 +219,16 @@ const S390CPUDef *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga,
         if (def->type == type && def->ec_ga == ec_ga) {
             return def;
         }
+        /* remember if we've at least seen one with the same cpu type */
+        if (def->type == type) {
+            matching_cpu_type = def;
+        }
         last_compatible = def;
     }
+    /* prefer the model with the same cpu type, esp. don't take the BC for EC */
+    if (matching_cpu_type) {
+        return matching_cpu_type;
+    }
     return last_compatible;
 }
 

Quoted 15 lines · Jason J. Herne

Quoted 9 lines · David Hildenbrand

Currently, under z/VM on a 0x2827, QEMU will detect a 0x2828 if no IBC value is provided. QEMU will simply take the last model of that HW generation, which happens to be the BC version.

Let's improve our search for that case by selecting the latest CPU definition that matches the CPU type. This might still detect e.g. a GA2 version on a GA1 system, but as we don't have further information at hand, there isn't too much we can do about it.

Isn't it better to choose the earliest machine possible in this case? Systems
are backwards compatible far more often than they are forwards compatible.

Quoted 7 lines

Said in a more verbose way, I'd rather pretend to be a GA1 on a GA2 (ignoring
some new features) than pretend to be a GA2 on a GA1 and attempt to use something that does not exist.

I'm sure you've thought of all of this though, so what am I missing?

Hi Jason,

excellent question. And of course I thought of it:

Fact 1: All GAX models share the same base feature set. A GAX++ might support "more features".

Fact 2: Without an IBC, the guest can't detect the GA version.

a) If we have no IBC (esp. unblocked_ibc == 0), the IBC we will present to the guest in read_SCP_info() will be 0. The guest will not know which GA version it has. The problem of missing IBC propagates.

b) If we don't have a feature of the GA++ version, also our guest won't have it. So in summary, the guest also has no idea of its GA version.

So this gives more "flexibility" and is backwards compatible.

Think about it like this: You're running on 0x2827 GA2.

Old QEMU version indicated "0x2828 GA1 == 0x2827 GA2". After you updated QEMU, you suddenly detect "0x2827 GA1". You're previous libvirt guest might suddenly refuse to run.

So it boils down to "it's just a QEMU representation that is more flexible, the guest can't see the difference".

Thanks!

Quoted 36 lines

Quoted 35 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 target/s390x/cpu_models.c | 9 +++++++++
 1 file changed, 9 insertions(+)

target/s390x/cpu_models.c +9 -0

index 9e23535..b6220c8 100644
@@ -184,6 +184,7 @@ const S390CPUDef *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga,
                                     S390FeatBitmap features)
 {
     const S390CPUDef *last_compatible = NULL;
+    const S390CPUDef *matching_cpu_type = NULL;
     int i;
 
     if (!gen) {

@@ -218,8 +219,16 @@ const S390CPUDef *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga,
         if (def->type == type && def->ec_ga == ec_ga) {
             return def;
         }
+        /* remember if we've at least seen one with the same cpu type */
+        if (def->type == type) {
+            matching_cpu_type = def;
+        }
         last_compatible = def;
     }
+    /* prefer the model with the same cpu type, esp. don't take the BC for EC */
+    if (matching_cpu_type) {
+        return matching_cpu_type;
+    }
     return last_compatible;
 }
 

Maybe integrating some of the discussion from above into the commit message would be helpful.

On 06/02/2017 07:28 PM, Halil Pasic wrote:
[...]

Quoted 2 lines

Maybe integrating some of the discussion from above into the commit message would be helpful.

applied with the following patch description

Currently, under z/VM on a 0x2827, QEMU will detect a 0x2828 if no IBC value is provided. QEMU will simply take the last model of that HW generation, which happens to be the BC version.

Let's improve our search for that case by selecting the latest CPU definition that matches the CPU type. This for example will avoid detecting an z13 as a z13s.

We might still detect a GA2 version on a GA1 system, but as we don't have further information at hand, there isn't too much we can do about it. The alternative of always presenting the oldest GA is not backward compatible, e.g:
You're running on 0x2827 GA2.
Old QEMU version indicated "0x2828 GA1 == 0x2827 GA2". After you updated QEMU, you suddenly detect "0x2827 GA1". You're previous libvirt guest might suddenly refuse to run.

In the end presenting a newer GA level does not matter because:

1: All GAX models share the same base feature set. A GAX++ might support "more features".
2: Without an IBC, the guest can't detect the GA version.

If we have no IBC (esp. unblocked_ibc == 0), the IBC we will present to the guest in read_SCP_info() will be 0. The guest will not know which GA version it has. The problem of missing IBC propagates.

If we don't have a feature of the GA++ version, also our guest won't have it. So in summary, the guest also has no idea of its GA version.

Signed-off-by David Hildenbrand <[email protected]>

Message-Id: <[email protected]>

Acked-by Jason J. Herne <[email protected]>

Reviewed-by Halil Pasic <[email protected]>

Signed-off-by Christian Borntraeger <[email protected]>

[improve patch description by reusing mailing list discussion]

Your change makes a lot of sense, but ain't easy to understand IMHO.

Maybe integrating some of the discussion from above into the commit message would be helpful. Especially this assumption that the client code is fine with getting a more recent GA version (and anything influenced by it) if the ec_ga parameter is (basically) invalid, because ec_ga invalid means no IBC, and that (somehow) ensures the guest will be unaware of the picked GA (and everything affected by it)

I do understand that your patch is clearly superior to what we have now, but I'm sill not sure why are we picking the last and not the firs one with the matching type (and not necessarily matching ec_ga). I guess the solution is to be sought along 'more "flexibility". That would mean the GA does not matter for the guest, but matters for something else (QEMU, libvirt, I do not know).

But if it matters, then for some to me unknown reasons newer GA must be safe there too. Fact is (by picking the last one) the algorithm ain't stable against appending a new GA to the list in a new qemu version (that's why I assume it must be safe).

From a customer point of view, it's better to detect a GA2 on a GA1 than

Let me add one meta comment too. IMHO what makes the function hard to understand, is that we have a bunch of optional parameters, and that although this seems to be some sort of approximation problem (find the best good enough), the metric ain't obvious for somebody not to familiar with the problem domain (like me). AFAIU you are fixing the following problem domain (me). Seems that the metric implemented was sub-optimal:
if parameters present {type}, absent {ec_ga} maybe present {gen, features}, then type is not honored appropriately. One can probably figure out what's intended by understanding all the usages (which makes reviewing it demanding). For instance, you also refer to a particular usage in your title/subject (that is "model->def = s390_find_cpu_def(cpu_type, ibc_gen(unblocked_ibc), ibc_ec_ga(unblocked_ibc), NULL);" in target/s390x/kvm.c).

Since I agree it's an improvement:

Reviewed-by Halil Pasic <[email protected]>

Quoted 5 lines

Let me add one meta comment too. IMHO what makes the function hard to understand, is that we have a bunch of optional parameters, and that although this seems to be some sort of approximation problem (find the best good enough), the metric ain't obvious for somebody not to familiar with the problem domain (like me). AFAIU you are fixing the following

Quoted 8 lines

problem domain (me). Seems that the metric implemented was sub-optimal:
if parameters present {type}, absent {ec_ga} maybe present {gen, features}, then type is not honored appropriately. One can probably figure out what's intended by understanding all the usages (which makes reviewing it demanding). For instance, you also refer to a particular usage in your title/subject (that is "model->def = s390_find_cpu_def(cpu_type, ibc_gen(unblocked_ibc), ibc_ec_ga(unblocked_ibc), NULL);" in target/s390x/kvm.c).

Actually, I thought that all relevant z/VM systems would properly forward the IBC. By relevant, I am speaking about zEC12+. So I never saw this while working on the CPU model.

We could split this function into multiple: find by (cpuid), (cpuid,gen,ec_ga), (features), (gen,ec_ga,features). All without optional parameters. But I doubt that this will result in better code, especially on the caller side.

If you have an idea how to improve that piece of code, feel free to send a cleanup.

Thanks for having a look!

We should properly forward the CPUID format bit. CPU model detection on systems without IBC might select a model with a wrong CPU type (esp. selecting the business class (BC) model on an enterprise class (EC) machine). So let's improve that, too.

I am also preparing a patch to properly indicate the correct cpu type to the guest via STORE CPU ID and STORE HYPERVISOR INFORMATION for the TCG case, but will send that separately after I tested it (want to write a kvm-unit-test for STORE CPU ID).

David Hildenbrand (2):
s390x/cpumodel: take care of the cpuid format bit for KVM s390x/cpumodel: improve defintion search without an IBC

 target/s390x/cpu_models.c | 10 ++++++++++
 target/s390x/cpu_models.h | 10 +++++++---
 target/s390x/kvm.c        |  1 +
 3 files changed, 18 insertions(+), 3 deletions(-)

Quoted 10 lines · David Hildenbrand

We should properly forward the CPUID format bit. CPU model detection on systems without IBC might select a model with a wrong CPU type (esp. selecting the business class (BC) model on an enterprise class (EC) machine). So let's improve that, too.

I am also preparing a patch to properly indicate the correct cpu type to the guest via STORE CPU ID and STORE HYPERVISOR INFORMATION for the TCG case, but will send that separately after I tested it (want to write a kvm-unit-test for STORE CPU ID).

Quoted 9 lines

David Hildenbrand (2):
s390x/cpumodel: take care of the cpuid format bit for KVM s390x/cpumodel: improve defintion search without an IBC

 target/s390x/cpu_models.c | 10 ++++++++++
 target/s390x/cpu_models.h | 10 +++++++---
 target/s390x/kvm.c        |  1 +
 3 files changed, 18 insertions(+), 3 deletions(-)

I like both patches in terms of "yes, its the right thing to do". Jason can you have a look? I might need some days to catch up with other things before I can look into the patches.