[Qemu-devel] [PATCH v2 0/5] target/s390x: cpu model cleanups + improvements

[Qemu-devel] [PATCH v2 0/5] target/s390x: cpu model cleanups + improvements

[David Hildenbrand]

We want to reuse e.g. s390_get_feat_block() in tcg. Turns out, that
set_bit_inv() doesn't work as expected on other architectures (if we
want to expose the uint8_t[] based data to the VM later), so let's
fix that.

Some other minor cleanups, including better baselining support if one
of the MSA subfunctions part of a base model is missing.

v1 -> v2:
- try to make the compiler not spit out warnings

David Hildenbrand (5):
  target/s390x: drop BE_BIT()
  target/s390x: indicate query subfunction in s390_fill_feat_block
  target/s390x: introduce (test|set)_be_bit
  s390x/kvm: better comment regarding zPCI feature availability
  target/s390x: improve baselining if certain base features are missing

 target/s390x/cpu_features.c | 33 +++++++++++++++++++++++++++------
 target/s390x/cpu_features.h |  9 ++++++++-
 target/s390x/cpu_models.c   | 33 +++++++++++++++++++++++++++++++++
 target/s390x/kvm.c          | 29 +++--------------------------
 4 files changed, 71 insertions(+), 33 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH v2 1/5] target/s390x: drop BE_BIT()

[David Hildenbrand]

Unused and broken, let's just get rid of it.

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu_features.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
index 14bc311..770435e 100644
--- a/target/s390x/cpu_features.h
+++ b/target/s390x/cpu_features.h
@@ -92,6 +92,5 @@ typedef struct {
 const S390FeatGroupDef *s390_feat_group_def(S390FeatGroup group);
 
 #define BE_BIT_NR(BIT) (BIT ^ (BITS_PER_LONG - 1))
-#define BE_BIT(BIT) (1ULL < BE_BIT_NR(BIT))
 
 #endif /* TARGET_S390X_CPU_FEATURES_H */
-- 
2.9.4

[Qemu-devel] [PATCH v2 2/5] target/s390x: indicate query subfunction in s390_fill_feat_block

[David Hildenbrand]

We'll have to do the same for TCG, so let's just move it in there.

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu_features.c | 31 ++++++++++++++++++++++++++-----
 target/s390x/kvm.c          | 13 -------------
 2 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index fa887d9..8b14917 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -336,11 +336,32 @@ void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
     S390Feat feat;
     int bit_nr;
 
-    if (type == S390_FEAT_TYPE_STFL && test_bit(S390_FEAT_ZARCH, features)) {
-        /* Features that are always active */
-        data[0] |= 0x20;  /* z/Architecture */
-        data[17] |= 0x20; /* Configuration-z-architectural-mode */
-    }
+    switch (type) {
+    case S390_FEAT_TYPE_STFL:
+        if (test_bit(S390_FEAT_ZARCH, features)) {
+            /* Features that are always active */
+            data[0] |= 0x20;  /* z/Architecture */
+            data[17] |= 0x20; /* Configuration-z-architectural-mode */
+        }
+        break;
+    case S390_FEAT_TYPE_PTFF:
+    case S390_FEAT_TYPE_KMAC:
+    case S390_FEAT_TYPE_KMC:
+    case S390_FEAT_TYPE_KM:
+    case S390_FEAT_TYPE_KIMD:
+    case S390_FEAT_TYPE_KLMD:
+    case S390_FEAT_TYPE_PCKMO:
+    case S390_FEAT_TYPE_KMCTR:
+    case S390_FEAT_TYPE_KMF:
+    case S390_FEAT_TYPE_KMO:
+    case S390_FEAT_TYPE_PCC:
+    case S390_FEAT_TYPE_PPNO:
+    case S390_FEAT_TYPE_KMA:
+        data[0] |= 0x80; /* query is always available */
+        break;
+    default:
+        break;
+    };
 
     feat = find_first_bit(features, S390_FEAT_MAX);
     while (feat < S390_FEAT_MAX) {
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 831492f..999ea57 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2506,41 +2506,28 @@ static int configure_cpu_subfunc(const S390FeatBitmap features)
     s390_fill_feat_block(features, S390_FEAT_TYPE_PLO, prop.plo);
     if (test_bit(S390_FEAT_TOD_CLOCK_STEERING, features)) {
         s390_fill_feat_block(features, S390_FEAT_TYPE_PTFF, prop.ptff);
-        prop.ptff[0] |= 0x80; /* query is always available */
     }
     if (test_bit(S390_FEAT_MSA, features)) {
         s390_fill_feat_block(features, S390_FEAT_TYPE_KMAC, prop.kmac);
-        prop.kmac[0] |= 0x80; /* query is always available */
         s390_fill_feat_block(features, S390_FEAT_TYPE_KMC, prop.kmc);
-        prop.kmc[0] |= 0x80; /* query is always available */
         s390_fill_feat_block(features, S390_FEAT_TYPE_KM, prop.km);
-        prop.km[0] |= 0x80; /* query is always available */
         s390_fill_feat_block(features, S390_FEAT_TYPE_KIMD, prop.kimd);
-        prop.kimd[0] |= 0x80; /* query is always available */
         s390_fill_feat_block(features, S390_FEAT_TYPE_KLMD, prop.klmd);
-        prop.klmd[0] |= 0x80; /* query is always available */
     }
     if (test_bit(S390_FEAT_MSA_EXT_3, features)) {
         s390_fill_feat_block(features, S390_FEAT_TYPE_PCKMO, prop.pckmo);
-        prop.pckmo[0] |= 0x80; /* query is always available */
     }
     if (test_bit(S390_FEAT_MSA_EXT_4, features)) {
         s390_fill_feat_block(features, S390_FEAT_TYPE_KMCTR, prop.kmctr);
-        prop.kmctr[0] |= 0x80; /* query is always available */
         s390_fill_feat_block(features, S390_FEAT_TYPE_KMF, prop.kmf);
-        prop.kmf[0] |= 0x80; /* query is always available */
         s390_fill_feat_block(features, S390_FEAT_TYPE_KMO, prop.kmo);
-        prop.kmo[0] |= 0x80; /* query is always available */
         s390_fill_feat_block(features, S390_FEAT_TYPE_PCC, prop.pcc);
-        prop.pcc[0] |= 0x80; /* query is always available */
     }
     if (test_bit(S390_FEAT_MSA_EXT_5, features)) {
         s390_fill_feat_block(features, S390_FEAT_TYPE_PPNO, prop.ppno);
-        prop.ppno[0] |= 0x80; /* query is always available */
     }
     if (test_bit(S390_FEAT_MSA_EXT_8, features)) {
         s390_fill_feat_block(features, S390_FEAT_TYPE_KMA, prop.kma);
-        prop.kma[0] |= 0x80; /* query is always available */
     }
     return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
 }
-- 
2.9.4

[Qemu-devel] [PATCH v2 3/5] target/s390x: introduce (test|set)_be_bit

[David Hildenbrand]

Using ordinary bitmap operations to set/test bits does not work properly
on architectures !s390x. Let's drop (test|set)_bit_inv and introduce
(test|set)_be_bit instead. These functions work on uint8_t array, not on
unsigned longs arrays and are for now only used in the context of
CPU features.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu_features.c |  8 ++++----
 target/s390x/cpu_features.h |  8 ++++++++
 target/s390x/kvm.c          | 14 ++------------
 3 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index 8b14917..1d3a036 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -340,8 +340,8 @@ void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
     case S390_FEAT_TYPE_STFL:
         if (test_bit(S390_FEAT_ZARCH, features)) {
             /* Features that are always active */
-            data[0] |= 0x20;  /* z/Architecture */
-            data[17] |= 0x20; /* Configuration-z-architectural-mode */
+            set_be_bit(2, data);   /* z/Architecture */
+            set_be_bit(138, data); /* Configuration-z-architectural-mode */
         }
         break;
     case S390_FEAT_TYPE_PTFF:
@@ -357,7 +357,7 @@ void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
     case S390_FEAT_TYPE_PCC:
     case S390_FEAT_TYPE_PPNO:
     case S390_FEAT_TYPE_KMA:
-        data[0] |= 0x80; /* query is always available */
+        set_be_bit(0, data); /* query is always available */
         break;
     default:
         break;
@@ -368,7 +368,7 @@ void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
         if (s390_features[feat].type == type) {
             bit_nr = s390_features[feat].bit;
             /* big endian on uint8_t array */
-            data[bit_nr / 8] |= 0x80 >> (bit_nr % 8);
+            set_be_bit(bit_nr, data);
         }
         feat = find_next_bit(features, S390_FEAT_MAX, feat + 1);
     }
diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
index 770435e..e306aa7 100644
--- a/target/s390x/cpu_features.h
+++ b/target/s390x/cpu_features.h
@@ -93,4 +93,12 @@ const S390FeatGroupDef *s390_feat_group_def(S390FeatGroup group);
 
 #define BE_BIT_NR(BIT) (BIT ^ (BITS_PER_LONG - 1))
 
+static inline void set_be_bit(unsigned int bit_nr, uint8_t *array)
+{
+    array[bit_nr / 8] |= 0x80 >> (bit_nr % 8);
+}
+static inline bool test_be_bit(unsigned int bit_nr, const uint8_t *array)
+{
+    return array[bit_nr / 8] & (0x80 >> (bit_nr % 8));
+}
 #endif /* TARGET_S390X_CPU_FEATURES_H */
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 999ea57..9bec481 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2430,16 +2430,6 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
     abort();
 }
 
-static inline int test_bit_inv(long nr, const unsigned long *addr)
-{
-    return test_bit(BE_BIT_NR(nr), addr);
-}
-
-static inline void set_bit_inv(long nr, unsigned long *addr)
-{
-    set_bit(BE_BIT_NR(nr), addr);
-}
-
 static int query_cpu_subfunc(S390FeatBitmap features)
 {
     struct kvm_s390_vm_cpu_subfunc prop;
@@ -2566,7 +2556,7 @@ static int query_cpu_feat(S390FeatBitmap features)
     }
 
     for (i = 0; i < ARRAY_SIZE(kvm_to_feat); i++) {
-        if (test_bit_inv(kvm_to_feat[i][0], (unsigned long *)prop.feat)) {
+        if (test_be_bit(kvm_to_feat[i][0], (uint8_t *) prop.feat)) {
             set_bit(kvm_to_feat[i][1], features);
         }
     }
@@ -2585,7 +2575,7 @@ static int configure_cpu_feat(const S390FeatBitmap features)
 
     for (i = 0; i < ARRAY_SIZE(kvm_to_feat); i++) {
         if (test_bit(kvm_to_feat[i][1], features)) {
-            set_bit_inv(kvm_to_feat[i][0], (unsigned long *)prop.feat);
+            set_be_bit(kvm_to_feat[i][0], (uint8_t *) prop.feat);
         }
     }
     return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
-- 
2.9.4

[Qemu-devel] [PATCH v2 4/5] s390x/kvm: better comment regarding zPCI feature availability

[David Hildenbrand]

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 9bec481..84416c0 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -2661,7 +2661,7 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
         clear_bit(S390_FEAT_CMM_NT, model->features);
     }
 
-    /* set zpci and aen facilities */
+    /* We emulate a zPCI bus and AEN, therefore we don't need HW support */
     set_bit(S390_FEAT_ZPCI, model->features);
     set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
 
-- 
2.9.4

[Qemu-devel] [PATCH v2 5/5] target/s390x: improve baselining if certain base features are missing

[David Hildenbrand]

There are certain features that we put into base models, but that are
not relevant for the actual search. The most famous example are
MSA subfunctions that might be disabled on certain real hardware out
there.

While the kvm host model detection will usually detect the correct model
on such machines (as it will in the common case not pass features to check
for into s390_find_cpu_def()), baselining will fall back to a quite old
model just because some MSA subfunctions are missing.

Let's improve that by ignoring lack of these features while performing
the search for a base model.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu_models.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index f4e5bb6..92120f4 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -78,6 +78,9 @@ static S390CPUDef s390_cpu_defs[] = {
     CPUDEF_INIT(0x3906, 14, 1, 47, 0x08000000U, "z14", "IBM z14 GA1"),
 };
 
+/* features part of a base model but not relevant for finding a base model */
+S390FeatBitmap ignored_base_feat;
+
 void s390_cpudef_featoff(uint8_t gen, uint8_t ec_ga, S390Feat feat)
 {
     const S390CPUDef *def;
@@ -237,6 +240,11 @@ const S390CPUDef *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga,
         if (features) {
             /* see if the model satisfies the minimum features */
             bitmap_andnot(missing, def->base_feat, features, S390_FEAT_MAX);
+            /*
+             * Ignore certain features that are in the base model, but not
+             * relevant for the search (esp. MSA subfunctions).
+             */
+            bitmap_andnot(missing, missing, ignored_base_feat, S390_FEAT_MAX);
             if (!bitmap_empty(missing, S390_FEAT_MAX)) {
                 break;
             }
@@ -1209,10 +1217,35 @@ static const TypeInfo host_s390_cpu_type_info = {
 };
 #endif
 
+static void init_ignored_base_feat(void)
+{
+    static const int feats[] = {
+         /* MSA subfunctions that could not be available on certain machines */
+         S390_FEAT_KMAC_DEA,
+         S390_FEAT_KMAC_TDEA_128,
+         S390_FEAT_KMAC_TDEA_192,
+         S390_FEAT_KMC_DEA,
+         S390_FEAT_KMC_TDEA_128,
+         S390_FEAT_KMC_TDEA_192,
+         S390_FEAT_KM_DEA,
+         S390_FEAT_KM_TDEA_128,
+         S390_FEAT_KM_TDEA_192,
+         S390_FEAT_KIMD_SHA_1,
+         S390_FEAT_KLMD_SHA_1,
+    };
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(feats); i++) {
+        set_bit(feats[i], ignored_base_feat);
+    }
+}
+
 static void register_types(void)
 {
     int i;
 
+    init_ignored_base_feat();
+
     /* init all bitmaps from gnerated data initially */
     for (i = 0; i < ARRAY_SIZE(s390_cpu_defs); i++) {
         s390_init_feat_bitmap(s390_cpu_defs[i].base_init,
-- 
2.9.4

Re: [Qemu-devel] [PATCH v2 0/5] target/s390x: cpu model cleanups + improvements

[Cornelia Huck]

On Thu, 20 Jul 2017 14:37:16 +0200
David Hildenbrand <david@redhat.com> wrote:

> We want to reuse e.g. s390_get_feat_block() in tcg. Turns out, that
> set_bit_inv() doesn't work as expected on other architectures (if we
> want to expose the uint8_t[] based data to the VM later), so let's
> fix that.
> 
> Some other minor cleanups, including better baselining support if one
> of the MSA subfunctions part of a base model is missing.
> 
> v1 -> v2:
> - try to make the compiler not spit out warnings

It seems you were successful :)

> 
> David Hildenbrand (5):
>   target/s390x: drop BE_BIT()
>   target/s390x: indicate query subfunction in s390_fill_feat_block
>   target/s390x: introduce (test|set)_be_bit
>   s390x/kvm: better comment regarding zPCI feature availability
>   target/s390x: improve baselining if certain base features are missing
> 
>  target/s390x/cpu_features.c | 33 +++++++++++++++++++++++++++------
>  target/s390x/cpu_features.h |  9 ++++++++-
>  target/s390x/cpu_models.c   | 33 +++++++++++++++++++++++++++++++++
>  target/s390x/kvm.c          | 29 +++--------------------------
>  4 files changed, 71 insertions(+), 33 deletions(-)

I think this is still 2.10 material.

Thanks, queued to s390-next.