Re: [PATCH v2 03/25] s390x: Guest support for Certificate Store Facility (CS)

[Thomas Huth]

On 09/05/2025 00.50, Zhuoying Cai wrote:
DIAG 320 is supported when the certificate-store (CS) facility
is installed.
Availability of CS facility is determined by byte 134 bit 5 of the
SCLP Read Info block. Byte 134's facilities cannot be represented
without the availability of the extended-length-SCCB, so add it as
a check for consistency.

Note: secure IPL is not available for Secure Execution (SE) guests,
as their images are already integrity protected, and an additional
protection of the kernel by secure IPL is not necessary.

Signed-off-by: Zhuoying Cai <zy...@linux.ibm.com>
---
  target/s390x/cpu_features.c         | 1 +
  target/s390x/cpu_features_def.h.inc | 1 +
  target/s390x/cpu_models.c           | 2 ++
  target/s390x/gen-features.c         | 1 +
  target/s390x/kvm/kvm.c              | 2 ++
  5 files changed, 7 insertions(+)

diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index 4b5be6798e..99089ab3f5 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -147,6 +147,7 @@ void s390_fill_feat_block(const S390FeatBitmap features, 
S390FeatType type,
          break;
      case S390_FEAT_TYPE_SCLP_FAC134:
          clear_be_bit(s390_feat_def(S390_FEAT_DIAG_318)->bit, data);
+        clear_be_bit(s390_feat_def(S390_FEAT_DIAG_320)->bit, data);
          break;
      default:
          return;
diff --git a/target/s390x/cpu_features_def.h.inc 
b/target/s390x/cpu_features_def.h.inc
index e23e603a79..65d38f546d 100644
--- a/target/s390x/cpu_features_def.h.inc
+++ b/target/s390x/cpu_features_def.h.inc
@@ -138,6 +138,7 @@ DEF_FEAT(SIE_IBS, "ibs", SCLP_CONF_CHAR_EXT, 10, "SIE: 
Interlock-and-broadcast-s
  
  /* Features exposed via SCLP SCCB Facilities byte 134 (bit numbers relative to byte-134) */
  DEF_FEAT(DIAG_318, "diag318", SCLP_FAC134, 0, "Control program name and version 
codes")
+DEF_FEAT(DIAG_320, "diag320", SCLP_FAC134, 5, "Provide Certificate Store 
functions")

Not sure, but "diag320" does not sound like a very good name for something 
related to security that is used in the interface for the users. Maybe 
"cert" or "cstore" would be a better name?

...
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 41840677ce..3d9fbe62ea 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -720,6 +720,7 @@ static uint16_t full_GEN16_GA1[] = {
      S390_FEAT_PAIE,
      S390_FEAT_UV_FEAT_AP,
      S390_FEAT_UV_FEAT_AP_INTR,
+    S390_FEAT_DIAG_320,
  };
  
  static uint16_t full_GEN17_GA1[] = {
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index b9f1422197..6bad1713d2 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -2487,6 +2487,8 @@ bool kvm_s390_get_host_cpu_model(S390CPUModel *model, 
Error **errp)
          set_bit(S390_FEAT_DIAG_318, model->features);
      }
  
+    set_bit(S390_FEAT_DIAG_320, model->features);
+
      /* Test for Ultravisor features that influence secure guest behavior */
      query_uv_feat_guest(model->features);

Question: Could this feature be enabled of the TCG max CPU, too? If so, it 
might be a good idea to do so, so it could be used in CI tests more easily.

By the way, any chance that you could also add a functional test to this 
patch series, so that we can be sure that there are no regressions in the 
future?

 Thomas