[PATCH v9 09/30] s390x/diag: Implement DIAG 320 subcode 2

Zhuoying Cai posted 30 patches 3 weeks, 5 days ago
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>, Pierrick Bouvier <pierrick.bouvier@linaro.org>, Thomas Huth <thuth@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Ilya Leoshkevich <iii@linux.ibm.com>, David Hildenbrand <david@kernel.org>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Jared Rossi <jrossi@linux.ibm.com>, Zhuoying Cai <zycai@linux.ibm.com>, Jason Herne <jjherne@linux.ibm.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Hendrik Brueckner <brueckner@linux.ibm.com>
[PATCH v9 09/30] s390x/diag: Implement DIAG 320 subcode 2
Posted by Zhuoying Cai 3 weeks, 5 days ago
DIAG 320 subcode 2 provides verification-certificates (VCs) that are in the
certificate store. Only X509 certificates in DER format and SHA-256 hash
type are recognized.

The subcode value is denoted by setting the second-left-most bit
of an 8-byte field.

The Verification Certificate Block (VCB) contains the output data
when the operation completes successfully. It includes a common
header followed by zero or more Verification Certificate Entries (VCEs),
depending on the VCB input length and the VC range (from the first VC
index to the last VC index) in the certificate store.

Each VCE contains information about a certificate retrieved from
the S390IPLCertificateStore, such as the certificate name, key type,
key ID length, hash length, and the raw certificate data.
The key ID and hash are extracted from the raw certificate by the crypto API.

Note: SHA2-256 VC hash type is required for retrieving the hash
(fingerprint) of the certificate.

Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
---
 docs/specs/s390x-secure-ipl.rst |  22 ++
 hw/s390x/cert-store.h           |   3 +-
 include/hw/s390x/ipl/diag320.h  |  55 +++++
 target/s390x/diag.c             | 343 +++++++++++++++++++++++++++++++-
 4 files changed, 420 insertions(+), 3 deletions(-)

diff --git a/docs/specs/s390x-secure-ipl.rst b/docs/specs/s390x-secure-ipl.rst
index 52661fab00..708253ac91 100644
--- a/docs/specs/s390x-secure-ipl.rst
+++ b/docs/specs/s390x-secure-ipl.rst
@@ -38,3 +38,25 @@ Subcode 1 - query verification certificate storage information
     The output is returned in the verification-certificate-storage-size block
     (VCSSB). A VCSSB length of 4 indicates that no certificates are available
     in the CS.
+
+Subcode 2 - store verification certificates
+    Provides VCs that are in the certificate store.
+
+    The output is provided in a VCB, which includes a common header followed by
+    zero or more verification-certificate entries (VCEs).
+
+    The instruction expects the cert store to
+    maintain an origin of 1 for the index (i.e. a retrieval of the first
+    certificate in the store should be denoted by setting first-VC to 1).
+
+    The first-VC index and last-VC index fields of VCB specify the range of VCs
+    to be stored by subcode 2. Stored count and remained count fields specify
+    the number of VCs stored and could not be stored in the VCB due to
+    insufficient storage specified in the VCB input length field.
+
+    Each VCE contains a header followed by information extracted from a
+    certificate within the certificate store. The information includes:
+    key-id, hash, and certificate data. This information is stored
+    contiguously in a VCE (with zero-padding). Following the header, the
+    key-id is immediately stored. The hash and certificate data follow and
+    may be accessed via the respective offset fields stored in the VCE.
diff --git a/hw/s390x/cert-store.h b/hw/s390x/cert-store.h
index 7fc9503cb9..6f5ee63177 100644
--- a/hw/s390x/cert-store.h
+++ b/hw/s390x/cert-store.h
@@ -11,10 +11,9 @@
 #define HW_S390_CERT_STORE_H
 
 #include "hw/s390x/ipl/qipl.h"
+#include "hw/s390x/ipl/diag320.h"
 #include "crypto/x509-utils.h"
 
-#define CERT_NAME_MAX_LEN  64
-
 #define CERT_KEY_ID_LEN    QCRYPTO_HASH_DIGEST_LEN_SHA256
 #define CERT_HASH_LEN      QCRYPTO_HASH_DIGEST_LEN_SHA256
 
diff --git a/include/hw/s390x/ipl/diag320.h b/include/hw/s390x/ipl/diag320.h
index 6e4779c699..bfd6385b40 100644
--- a/include/hw/s390x/ipl/diag320.h
+++ b/include/hw/s390x/ipl/diag320.h
@@ -12,19 +12,37 @@
 
 #define DIAG_320_SUBC_QUERY_ISM     0
 #define DIAG_320_SUBC_QUERY_VCSI    1
+#define DIAG_320_SUBC_STORE_VC      2
 
 #define DIAG_320_RC_OK              0x0001
 #define DIAG_320_RC_NOT_SUPPORTED   0x0102
 #define DIAG_320_RC_INVAL_VCSSB_LEN 0x0202
+#define DIAG_320_RC_INVAL_VCB_LEN   0x0204
+#define DIAG_320_RC_BAD_RANGE       0x0302
 
 #define DIAG_320_ISM_QUERY_SUBCODES 0x80000000
 #define DIAG_320_ISM_QUERY_VCSI     0x40000000
+#define DIAG_320_ISM_STORE_VC       0x20000000
 
 #define VCSSB_NO_VC     4
 #define VCSSB_MIN_LEN   128
 #define VCE_HEADER_LEN  128
+/*
+ * If the VCE flags indicate an invalid certificate,
+ * the VCE length is set to 72, containing only the
+ * first five fields of VCEntry.
+ */
+#define VCE_INVALID_LEN 72
 #define VCB_HEADER_LEN  64
 
+#define CERT_NAME_MAX_LEN  64
+
+#define DIAG_320_VCE_FLAGS_VALID                0x80
+#define DIAG_320_VCE_KEYTYPE_SELF_DESCRIBING    0
+#define DIAG_320_VCE_KEYTYPE_ECDSA_P521         1
+#define DIAG_320_VCE_FORMAT_X509_DER            1
+#define DIAG_320_VCE_HASHTYPE_SHA2_256          1
+
 struct VCStorageSizeBlock {
     uint32_t length;
     uint8_t reserved0[3];
@@ -39,4 +57,41 @@ struct VCStorageSizeBlock {
 };
 typedef struct VCStorageSizeBlock VCStorageSizeBlock;
 
+struct VCBlock {
+    uint32_t in_len;
+    uint32_t reserved0;
+    uint16_t first_vc_index;
+    uint16_t last_vc_index;
+    uint32_t reserved1[5];
+    uint32_t out_len;
+    uint8_t reserved2[4];
+    uint16_t stored_ct;
+    uint16_t remain_ct;
+    uint32_t reserved3[5];
+    uint8_t vce_buf[];
+};
+typedef struct VCBlock VCBlock;
+
+struct VCEntry {
+    uint32_t len;
+    uint8_t flags;
+    uint8_t key_type;
+    uint16_t cert_idx;
+    uint8_t name[CERT_NAME_MAX_LEN];
+    uint8_t format;
+    uint8_t reserved0;
+    uint16_t keyid_len;
+    uint8_t reserved1;
+    uint8_t hash_type;
+    uint16_t hash_len;
+    uint32_t reserved2;
+    uint32_t cert_len;
+    uint32_t reserved3[2];
+    uint16_t hash_offset;
+    uint16_t cert_offset;
+    uint32_t reserved4[7];
+    uint8_t cert_buf[];
+};
+typedef struct VCEntry VCEntry;
+
 #endif
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index c44624e1e6..5326522fda 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -17,13 +17,16 @@
 #include "s390x-internal.h"
 #include "hw/watchdog/wdt_diag288.h"
 #include "system/cpus.h"
+#include "hw/s390x/cert-store.h"
 #include "hw/s390x/ipl.h"
 #include "hw/s390x/ipl/diag320.h"
 #include "hw/s390x/s390-virtio-ccw.h"
 #include "system/kvm.h"
 #include "kvm/kvm_s390x.h"
 #include "target/s390x/kvm/pv.h"
+#include "qapi/error.h"
 #include "qemu/error-report.h"
+#include "crypto/x509-utils.h"
 
 
 static inline bool diag_parm_addr_valid(uint64_t addr, size_t size, bool write)
@@ -236,8 +239,333 @@ static int handle_diag320_query_vcsi(S390CPU *cpu, uint64_t addr, uint64_t r1,
     return DIAG_320_RC_OK;
 }
 
+static bool is_cert_valid(const S390IPLCertificate *cert)
+{
+    int rc;
+    Error *err = NULL;
+
+    rc = qcrypto_x509_check_cert_times(cert->raw, cert->size, &err);
+    if (rc != 0) {
+        error_report_err(err);
+        return false;
+    }
+
+    return true;
+}
+
+static int handle_key_id(VCEntry *vce, const S390IPLCertificate *cert)
+{
+    int rc;
+    g_autofree unsigned char *key_id_data = NULL;
+    size_t key_id_len;
+    Error *err = NULL;
+
+    rc = qcrypto_x509_get_cert_key_id(cert->raw, cert->size,
+                                      QCRYPTO_HASH_ALGO_SHA256,
+                                      &key_id_data, &key_id_len, &err);
+    if (rc < 0) {
+        error_report_err(err);
+        return -1;
+    }
+
+    if (VCE_HEADER_LEN + key_id_len > be32_to_cpu(vce->len)) {
+        error_report("Unable to write key ID: exceeds buffer bounds");
+        return -1;
+    }
+
+    vce->keyid_len = cpu_to_be16(key_id_len);
+
+    memcpy(vce->cert_buf, key_id_data, key_id_len);
+
+    return 0;
+}
+
+static int handle_hash(VCEntry *vce, const S390IPLCertificate *cert,
+                       uint16_t keyid_field_len)
+{
+    int rc;
+    uint16_t hash_offset;
+    g_autofree void *hash_data = NULL;
+    size_t hash_len;
+    Error *err = NULL;
+
+    hash_len = CERT_HASH_LEN;
+    hash_data = g_malloc0(hash_len);
+    rc = qcrypto_get_x509_cert_fingerprint(cert->raw, cert->size,
+                                           QCRYPTO_HASH_ALGO_SHA256,
+                                           hash_data, &hash_len, &err);
+    if (rc < 0) {
+        error_report_err(err);
+        return -1;
+    }
+
+    hash_offset = VCE_HEADER_LEN + keyid_field_len;
+    if (hash_offset + hash_len > be32_to_cpu(vce->len)) {
+        error_report("Unable to write hash: exceeds buffer bounds");
+        return -1;
+    }
+
+    vce->hash_len = cpu_to_be16(hash_len);
+    vce->hash_type = DIAG_320_VCE_HASHTYPE_SHA2_256;
+    vce->hash_offset = cpu_to_be16(hash_offset);
+
+    memcpy((uint8_t *)vce + hash_offset, hash_data, hash_len);
+
+    return 0;
+}
+
+static int handle_cert(VCEntry *vce, const S390IPLCertificate *cert,
+                       uint16_t hash_field_len)
+{
+    int rc;
+    uint16_t cert_offset;
+    g_autofree uint8_t *cert_der = NULL;
+    size_t der_size;
+    Error *err = NULL;
+
+    rc = qcrypto_x509_convert_cert_der(cert->raw, cert->size,
+                                       &cert_der, &der_size, &err);
+    if (rc < 0) {
+        error_report_err(err);
+        return -1;
+    }
+
+    cert_offset = be16_to_cpu(vce->hash_offset) + hash_field_len;
+    if (cert_offset + der_size > be32_to_cpu(vce->len)) {
+        error_report("Unable to write certificate: exceeds buffer bounds");
+        return -1;
+    }
+
+    vce->format = DIAG_320_VCE_FORMAT_X509_DER;
+    vce->cert_len = cpu_to_be32(der_size);
+    vce->cert_offset = cpu_to_be16(cert_offset);
+
+    memcpy((uint8_t *)vce + cert_offset, cert_der, der_size);
+
+    return 0;
+}
+
+static int get_key_type(const S390IPLCertificate *cert)
+{
+    int rc;
+    Error *err = NULL;
+
+    rc = qcrypto_x509_check_ecc_curve_p521(cert->raw, cert->size, &err);
+    if (rc == -1) {
+        error_report_err(err);
+        return -1;
+    }
+
+    return (rc == 1) ? DIAG_320_VCE_KEYTYPE_ECDSA_P521 :
+                        DIAG_320_VCE_KEYTYPE_SELF_DESCRIBING;
+}
+
+static int build_vce_header(VCEntry *vce, const S390IPLCertificate *cert, int idx)
+{
+    int key_type;
+
+    vce->len = cpu_to_be32(VCE_HEADER_LEN);
+    vce->cert_idx = cpu_to_be16(idx + 1);
+    memcpy(vce->name, cert->name, CERT_NAME_MAX_LEN);
+
+    key_type = get_key_type(cert);
+    if (key_type == -1) {
+        return -1;
+    }
+    vce->key_type = key_type;
+
+    return 0;
+}
+
+static int build_vce_data(VCEntry *vce, const S390IPLCertificate *cert)
+{
+    uint16_t keyid_field_len;
+    uint16_t hash_field_len;
+    uint32_t cert_field_len;
+    uint32_t vce_len;
+    int rc;
+
+    rc = handle_key_id(vce, cert);
+    if (rc) {
+        return -1;
+    }
+    keyid_field_len = ROUND_UP(be16_to_cpu(vce->keyid_len), 4);
+
+    rc = handle_hash(vce, cert, keyid_field_len);
+    if (rc) {
+        return -1;
+    }
+    hash_field_len = ROUND_UP(be16_to_cpu(vce->hash_len), 4);
+
+    rc = handle_cert(vce, cert, hash_field_len);
+    if (rc || !is_cert_valid(cert)) {
+        return -1;
+    }
+    cert_field_len = ROUND_UP(be32_to_cpu(vce->cert_len), 4);
+
+    vce_len = VCE_HEADER_LEN + keyid_field_len + hash_field_len + cert_field_len;
+    if (vce_len > be32_to_cpu(vce->len)) {
+        return -1;
+    }
+
+    vce->flags |= DIAG_320_VCE_FLAGS_VALID;
+
+    /* Update vce length to reflect the actual size used by vce */
+    vce->len = cpu_to_be32(vce_len);
+
+    return 0;
+}
+
+static VCEntry *diag_320_build_vce(const S390IPLCertificate *cert, int idx)
+{
+    g_autofree VCEntry *vce = NULL;
+    uint32_t vce_max_size;
+    int rc;
+
+    /*
+     * Each field of the VCE is word-aligned.
+     * Allocate enough space for the largest possible size for this VCE.
+     * As the certificate fields (key-id, hash, data) are parsed, the
+     * VCE's length field will be updated accordingly.
+     */
+    vce_max_size = VCE_HEADER_LEN +
+                   ROUND_UP(CERT_KEY_ID_LEN, 4) +
+                   ROUND_UP(CERT_HASH_LEN, 4) +
+                   ROUND_UP(cert->der_size, 4);
+
+    vce = g_malloc0(vce_max_size);
+    rc = build_vce_header(vce, cert, idx);
+    if (rc) {
+        /*
+         * Error occurs - VCE does not contain a valid certificate.
+         * Bit 0 of the VCE flags is 0 and the VCE length is set.
+         */
+        vce->len = cpu_to_be32(VCE_INVALID_LEN);
+        goto out;
+    }
+
+    vce->len = cpu_to_be32(vce_max_size);
+    rc = build_vce_data(vce, cert);
+    if (rc) {
+        vce->len = cpu_to_be32(VCE_INVALID_LEN);
+    }
+
+out:
+    return g_steal_pointer(&vce);
+}
+
+static int handle_diag320_store_vc(S390CPU *cpu, uint64_t addr, uint64_t r1, uintptr_t ra,
+                                   S390IPLCertificateStore *cs)
+{
+    g_autofree VCBlock *vcb = NULL;
+    size_t entry_offset;
+    size_t remaining_space;
+    uint32_t vce_len;
+    uint16_t first_vc_index;
+    uint16_t last_vc_index;
+    int cs_start_index;
+    int cs_end_index;
+    uint32_t in_len;
+
+    vcb = g_new0(VCBlock, 1);
+    if (s390_cpu_virt_mem_read(cpu, addr, r1, vcb, sizeof(*vcb))) {
+        s390_cpu_virt_mem_handle_exc(cpu, ra);
+        return -1;
+    }
+
+    in_len = be32_to_cpu(vcb->in_len);
+    first_vc_index = be16_to_cpu(vcb->first_vc_index);
+    last_vc_index = be16_to_cpu(vcb->last_vc_index);
+
+    if (in_len % TARGET_PAGE_SIZE != 0) {
+        return DIAG_320_RC_INVAL_VCB_LEN;
+    }
+
+    if (first_vc_index > last_vc_index) {
+        return DIAG_320_RC_BAD_RANGE;
+    }
+
+    vcb->out_len = VCB_HEADER_LEN;
+
+    /*
+     * DIAG 320 subcode 2 expects to query a certificate store that
+     * maintains an index origin of 1. However, the S390IPLCertificateStore
+     * maintains an index origin of 0. Thus, the indices must be adjusted
+     * for correct access into the cert store. A couple of special cases
+     * must also be accounted for.
+     */
+
+    /* Both indices are 0; return header with no certs */
+    if (first_vc_index == 0 && last_vc_index == 0) {
+        goto out;
+    }
+
+    /* Normalize indices */
+    cs_start_index = (first_vc_index == 0) ? 0 : first_vc_index - 1;
+    cs_end_index = last_vc_index - 1;
+
+    /* Requested range is outside the cert store; return header with no certs */
+    if (cs_start_index >= cs->count || cs_end_index >= cs->count) {
+        goto out;
+    }
+
+    entry_offset = VCB_HEADER_LEN;
+    remaining_space = in_len - VCB_HEADER_LEN;
+
+    for (int i = cs_start_index; i <= cs_end_index; i++) {
+        VCEntry *vce;
+        const S390IPLCertificate *cert = &cs->certs[i];
+
+        /*
+         * Bit 0 of the VCE flags indicates whether the certificate is valid.
+         * The caller of DIAG320 subcode 2 is responsible for verifying that
+         * the VCE contains a valid certificate.
+         */
+        vce = diag_320_build_vce(cert, i);
+        vce_len = be32_to_cpu(vce->len);
+
+        /*
+         * If there is no more space to store the cert,
+         * set the remaining verification cert count and
+         * break early.
+         */
+        if (remaining_space < vce_len) {
+            vcb->remain_ct = cpu_to_be16(last_vc_index - i);
+            g_free(vce);
+            break;
+        }
+
+        /* Write VCE */
+        if (s390_cpu_virt_mem_write(cpu, addr + entry_offset, r1, vce, vce_len)) {
+            s390_cpu_virt_mem_handle_exc(cpu, ra);
+            g_free(vce);
+            return -1;
+        }
+
+        entry_offset += vce_len;
+        vcb->out_len += vce_len;
+        remaining_space -= vce_len;
+        vcb->stored_ct++;
+
+        g_free(vce);
+    }
+    vcb->stored_ct = cpu_to_be16(vcb->stored_ct);
+
+out:
+    vcb->out_len = cpu_to_be32(vcb->out_len);
+
+    if (s390_cpu_virt_mem_write(cpu, addr, r1, vcb, VCB_HEADER_LEN)) {
+        s390_cpu_virt_mem_handle_exc(cpu, ra);
+        return -1;
+    }
+
+    return DIAG_320_RC_OK;
+}
+
 QEMU_BUILD_BUG_MSG(sizeof(VCStorageSizeBlock) != VCSSB_MIN_LEN,
                    "size of VCStorageSizeBlock is wrong");
+QEMU_BUILD_BUG_MSG(sizeof(VCBlock) != VCB_HEADER_LEN, "size of VCBlock is wrong");
+QEMU_BUILD_BUG_MSG(sizeof(VCEntry) != VCE_HEADER_LEN, "size of VCEntry is wrong");
 
 void handle_diag_320(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
 {
@@ -268,7 +596,8 @@ void handle_diag_320(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
          * for now.
          */
         uint32_t ism_word0 = cpu_to_be32(DIAG_320_ISM_QUERY_SUBCODES |
-                                         DIAG_320_ISM_QUERY_VCSI);
+                                         DIAG_320_ISM_QUERY_VCSI |
+                                         DIAG_320_ISM_STORE_VC);
 
         if (s390_cpu_virt_mem_write(cpu, addr, r1, &ism_word0, sizeof(ism_word0))) {
             s390_cpu_virt_mem_handle_exc(cpu, ra);
@@ -294,6 +623,18 @@ void handle_diag_320(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
         }
         env->regs[r1 + 1] = rc;
         break;
+    case DIAG_320_SUBC_STORE_VC:
+        if (addr & ~TARGET_PAGE_MASK) {
+            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+            return;
+        }
+
+        rc = handle_diag320_store_vc(cpu, addr, r1, ra, cs);
+        if (rc == -1) {
+            return;
+        }
+        env->regs[r1 + 1] = rc;
+        break;
     default:
         env->regs[r1 + 1] = DIAG_320_RC_NOT_SUPPORTED;
         break;
-- 
2.53.0
Re: [PATCH v9 09/30] s390x/diag: Implement DIAG 320 subcode 2
Posted by Collin Walling 2 weeks, 5 days ago
On 3/5/26 17:41, Zhuoying Cai wrote:
> DIAG 320 subcode 2 provides verification-certificates (VCs) that are in the
> certificate store. Only X509 certificates in DER format and SHA-256 hash
> type are recognized.
> 
> The subcode value is denoted by setting the second-left-most bit
> of an 8-byte field.
> 
> The Verification Certificate Block (VCB) contains the output data
> when the operation completes successfully. It includes a common
> header followed by zero or more Verification Certificate Entries (VCEs),
> depending on the VCB input length and the VC range (from the first VC
> index to the last VC index) in the certificate store.
> 
> Each VCE contains information about a certificate retrieved from
> the S390IPLCertificateStore, such as the certificate name, key type,
> key ID length, hash length, and the raw certificate data.
> The key ID and hash are extracted from the raw certificate by the crypto API.
> 
> Note: SHA2-256 VC hash type is required for retrieving the hash
> (fingerprint) of the certificate.
> 
> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>

Functionally, this looks fine.  I've noted a few nits below, but I don't
think they're worth dragging things out.

The important thing to address:
 - documentation rewording
 - CERT_NAME_MAX_LEN change

The rest are inconsequential.

> ---
>  docs/specs/s390x-secure-ipl.rst |  22 ++
>  hw/s390x/cert-store.h           |   3 +-
>  include/hw/s390x/ipl/diag320.h  |  55 +++++
>  target/s390x/diag.c             | 343 +++++++++++++++++++++++++++++++-
>  4 files changed, 420 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/specs/s390x-secure-ipl.rst b/docs/specs/s390x-secure-ipl.rst
> index 52661fab00..708253ac91 100644
> --- a/docs/specs/s390x-secure-ipl.rst
> +++ b/docs/specs/s390x-secure-ipl.rst
> @@ -38,3 +38,25 @@ Subcode 1 - query verification certificate storage information
>      The output is returned in the verification-certificate-storage-size block
>      (VCSSB). A VCSSB length of 4 indicates that no certificates are available
>      in the CS.
> +
> +Subcode 2 - store verification certificates
> +    Provides VCs that are in the certificate store.
> +
> +    The output is provided in a VCB, which includes a common header followed by
> +    zero or more verification-certificate entries (VCEs).
> +
> +    The instruction expects the cert store to
> +    maintain an origin of 1 for the index (i.e. a retrieval of the first
> +    certificate in the store should be denoted by setting first-VC to 1).

Nit: early newline after "...cert store to".  There is space for a few
more characters on that line.

> +
> +    The first-VC index and last-VC index fields of VCB specify the range of VCs
> +    to be stored by subcode 2. Stored count and remained count fields specify
> +    the number of VCs stored and could not be stored in the VCB due to
> +    insufficient storage specified in the VCB input length field.
> +

This might read better.  It makes it more clear how they're used.

"""
The first-VC and last-VC fields of the VCB specify the index range of
VCs to be stored in the VCB. Certs are stored sequentially, starting
with first-VC index. As each cert is stored, a "stored count" is
incremented. If there is not enough space to store all certs requested
by the index range, a "remaining count" will be recorded and no more
certificates will be stored.
"""

> +    Each VCE contains a header followed by information extracted from a
> +    certificate within the certificate store. The information includes:
> +    key-id, hash, and certificate data. This information is stored
> +    contiguously in a VCE (with zero-padding). Following the header, the
> +    key-id is immediately stored. The hash and certificate data follow and
> +    may be accessed via the respective offset fields stored in the VCE.
> diff --git a/hw/s390x/cert-store.h b/hw/s390x/cert-store.h
> index 7fc9503cb9..6f5ee63177 100644
> --- a/hw/s390x/cert-store.h
> +++ b/hw/s390x/cert-store.h
> @@ -11,10 +11,9 @@
>  #define HW_S390_CERT_STORE_H
>  
>  #include "hw/s390x/ipl/qipl.h"
> +#include "hw/s390x/ipl/diag320.h"
>  #include "crypto/x509-utils.h"
>  
> -#define CERT_NAME_MAX_LEN  64
> -

Is there a reason why this was moved to diag320.h?

(comments below this point are inconsequential, only consider if a v10
is required).

>  #define CERT_KEY_ID_LEN    QCRYPTO_HASH_DIGEST_LEN_SHA256
>  #define CERT_HASH_LEN      QCRYPTO_HASH_DIGEST_LEN_SHA256
>  
> diff --git a/include/hw/s390x/ipl/diag320.h b/include/hw/s390x/ipl/diag320.h
> index 6e4779c699..bfd6385b40 100644
> --- a/include/hw/s390x/ipl/diag320.h
> +++ b/include/hw/s390x/ipl/diag320.h
> @@ -12,19 +12,37 @@
>  
>  #define DIAG_320_SUBC_QUERY_ISM     0
>  #define DIAG_320_SUBC_QUERY_VCSI    1
> +#define DIAG_320_SUBC_STORE_VC      2
>  
>  #define DIAG_320_RC_OK              0x0001
>  #define DIAG_320_RC_NOT_SUPPORTED   0x0102
>  #define DIAG_320_RC_INVAL_VCSSB_LEN 0x0202
> +#define DIAG_320_RC_INVAL_VCB_LEN   0x0204
> +#define DIAG_320_RC_BAD_RANGE       0x0302
>  
>  #define DIAG_320_ISM_QUERY_SUBCODES 0x80000000
>  #define DIAG_320_ISM_QUERY_VCSI     0x40000000
> +#define DIAG_320_ISM_STORE_VC       0x20000000
>  
>  #define VCSSB_NO_VC     4
>  #define VCSSB_MIN_LEN   128
>  #define VCE_HEADER_LEN  128
> +/*
> + * If the VCE flags indicate an invalid certificate,
> + * the VCE length is set to 72, containing only the
> + * first five fields of VCEntry.
> + */	
> +#define VCE_INVALID_LEN 72
>  #define VCB_HEADER_LEN  64
>  
> +#define CERT_NAME_MAX_LEN  64
> +
> +#define DIAG_320_VCE_FLAGS_VALID                0x80
> +#define DIAG_320_VCE_KEYTYPE_SELF_DESCRIBING    0
> +#define DIAG_320_VCE_KEYTYPE_ECDSA_P521         1
> +#define DIAG_320_VCE_FORMAT_X509_DER            1
> +#define DIAG_320_VCE_HASHTYPE_SHA2_256          1
> +
>  struct VCStorageSizeBlock {
>      uint32_t length;
>      uint8_t reserved0[3];
> @@ -39,4 +57,41 @@ struct VCStorageSizeBlock {
>  };
>  typedef struct VCStorageSizeBlock VCStorageSizeBlock;
>  
> +struct VCBlock {
> +    uint32_t in_len;
> +    uint32_t reserved0;
> +    uint16_t first_vc_index;
> +    uint16_t last_vc_index;
> +    uint32_t reserved1[5];
> +    uint32_t out_len;
> +    uint8_t reserved2[4];
> +    uint16_t stored_ct;
> +    uint16_t remain_ct;
> +    uint32_t reserved3[5];
> +    uint8_t vce_buf[];
> +};
> +typedef struct VCBlock VCBlock;
> +
> +struct VCEntry {
> +    uint32_t len;
> +    uint8_t flags;
> +    uint8_t key_type;
> +    uint16_t cert_idx;
> +    uint8_t name[CERT_NAME_MAX_LEN];
> +    uint8_t format;
> +    uint8_t reserved0;
> +    uint16_t keyid_len;
> +    uint8_t reserved1;
> +    uint8_t hash_type;
> +    uint16_t hash_len;
> +    uint32_t reserved2;
> +    uint32_t cert_len;
> +    uint32_t reserved3[2];
> +    uint16_t hash_offset;
> +    uint16_t cert_offset;
> +    uint32_t reserved4[7];
> +    uint8_t cert_buf[];
> +};
> +typedef struct VCEntry VCEntry;
> +
>  #endif
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index c44624e1e6..5326522fda 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -17,13 +17,16 @@
>  #include "s390x-internal.h"
>  #include "hw/watchdog/wdt_diag288.h"
>  #include "system/cpus.h"
> +#include "hw/s390x/cert-store.h"
>  #include "hw/s390x/ipl.h"
>  #include "hw/s390x/ipl/diag320.h"
>  #include "hw/s390x/s390-virtio-ccw.h"
>  #include "system/kvm.h"
>  #include "kvm/kvm_s390x.h"
>  #include "target/s390x/kvm/pv.h"
> +#include "qapi/error.h"
>  #include "qemu/error-report.h"
> +#include "crypto/x509-utils.h"
>  
>  
>  static inline bool diag_parm_addr_valid(uint64_t addr, size_t size, bool write)
> @@ -236,8 +239,333 @@ static int handle_diag320_query_vcsi(S390CPU *cpu, uint64_t addr, uint64_t r1,
>      return DIAG_320_RC_OK;
>  }
>  
> +static bool is_cert_valid(const S390IPLCertificate *cert)
> +{
> +    int rc;
> +    Error *err = NULL;
> +
> +    rc = qcrypto_x509_check_cert_times(cert->raw, cert->size, &err);
> +    if (rc != 0) {
> +        error_report_err(err);
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +static int handle_key_id(VCEntry *vce, const S390IPLCertificate *cert)
> +{
> +    int rc;
> +    g_autofree unsigned char *key_id_data = NULL;
> +    size_t key_id_len;
> +    Error *err = NULL;
> +
> +    rc = qcrypto_x509_get_cert_key_id(cert->raw, cert->size,
> +                                      QCRYPTO_HASH_ALGO_SHA256,
> +                                      &key_id_data, &key_id_len, &err);
> +    if (rc < 0) {
> +        error_report_err(err);
> +        return -1;
> +    }
> +
> +    if (VCE_HEADER_LEN + key_id_len > be32_to_cpu(vce->len)) {
> +        error_report("Unable to write key ID: exceeds buffer bounds");
> +        return -1;
> +    }
> +
> +    vce->keyid_len = cpu_to_be16(key_id_len);
> +
> +    memcpy(vce->cert_buf, key_id_data, key_id_len);
> +
> +    return 0;
> +}
> +
> +static int handle_hash(VCEntry *vce, const S390IPLCertificate *cert,
> +                       uint16_t keyid_field_len)
> +{
> +    int rc;
> +    uint16_t hash_offset;
> +    g_autofree void *hash_data = NULL;
> +    size_t hash_len;
> +    Error *err = NULL;
> +
> +    hash_len = CERT_HASH_LEN;
> +    hash_data = g_malloc0(hash_len);
> +    rc = qcrypto_get_x509_cert_fingerprint(cert->raw, cert->size,
> +                                           QCRYPTO_HASH_ALGO_SHA256,
> +                                           hash_data, &hash_len, &err);
> +    if (rc < 0) {
> +        error_report_err(err);
> +        return -1;
> +    }
> +
> +    hash_offset = VCE_HEADER_LEN + keyid_field_len;
> +    if (hash_offset + hash_len > be32_to_cpu(vce->len)) {
> +        error_report("Unable to write hash: exceeds buffer bounds");
> +        return -1;
> +    }
> +
> +    vce->hash_len = cpu_to_be16(hash_len);
> +    vce->hash_type = DIAG_320_VCE_HASHTYPE_SHA2_256;
> +    vce->hash_offset = cpu_to_be16(hash_offset);
> +
> +    memcpy((uint8_t *)vce + hash_offset, hash_data, hash_len);
> +
> +    return 0;
> +}
> +
> +static int handle_cert(VCEntry *vce, const S390IPLCertificate *cert,
> +                       uint16_t hash_field_len)
> +{
> +    int rc;
> +    uint16_t cert_offset;
> +    g_autofree uint8_t *cert_der = NULL;
> +    size_t der_size;
> +    Error *err = NULL;
> +
> +    rc = qcrypto_x509_convert_cert_der(cert->raw, cert->size,
> +                                       &cert_der, &der_size, &err);
> +    if (rc < 0) {
> +        error_report_err(err);
> +        return -1;
> +    }
> +
> +    cert_offset = be16_to_cpu(vce->hash_offset) + hash_field_len;
> +    if (cert_offset + der_size > be32_to_cpu(vce->len)) {
> +        error_report("Unable to write certificate: exceeds buffer bounds");
> +        return -1;
> +    }
> +
> +    vce->format = DIAG_320_VCE_FORMAT_X509_DER;
> +    vce->cert_len = cpu_to_be32(der_size);
> +    vce->cert_offset = cpu_to_be16(cert_offset);
> +
> +    memcpy((uint8_t *)vce + cert_offset, cert_der, der_size);
> +
> +    return 0;
> +}
> +
> +static int get_key_type(const S390IPLCertificate *cert)
> +{
> +    int rc;
> +    Error *err = NULL;
> +
> +    rc = qcrypto_x509_check_ecc_curve_p521(cert->raw, cert->size, &err);
> +    if (rc == -1) {
> +        error_report_err(err);
> +        return -1;
> +    }
> +
> +    return (rc == 1) ? DIAG_320_VCE_KEYTYPE_ECDSA_P521 :
> +                        DIAG_320_VCE_KEYTYPE_SELF_DESCRIBING;
> +}
> +
> +static int build_vce_header(VCEntry *vce, const S390IPLCertificate *cert, int idx)
> +{
> +    int key_type;
> +
> +    vce->len = cpu_to_be32(VCE_HEADER_LEN);
> +    vce->cert_idx = cpu_to_be16(idx + 1);
> +    memcpy(vce->name, cert->name, CERT_NAME_MAX_LEN);
> +
> +    key_type = get_key_type(cert);
> +    if (key_type == -1) {
> +        return -1;
> +    }
> +    vce->key_type = key_type;
> +
> +    return 0;
> +}
> +
> +static int build_vce_data(VCEntry *vce, const S390IPLCertificate *cert)
> +{
> +    uint16_t keyid_field_len;
> +    uint16_t hash_field_len;
> +    uint32_t cert_field_len;
> +    uint32_t vce_len;
> +    int rc;
> +
> +    rc = handle_key_id(vce, cert);
> +    if (rc) {
> +        return -1;
> +    }
> +    keyid_field_len = ROUND_UP(be16_to_cpu(vce->keyid_len), 4);
> +
> +    rc = handle_hash(vce, cert, keyid_field_len);
> +    if (rc) {
> +        return -1;
> +    }
> +    hash_field_len = ROUND_UP(be16_to_cpu(vce->hash_len), 4);
> +
> +    rc = handle_cert(vce, cert, hash_field_len);
> +    if (rc || !is_cert_valid(cert)) {
> +        return -1;
> +    }
> +    cert_field_len = ROUND_UP(be32_to_cpu(vce->cert_len), 4);
> +
> +    vce_len = VCE_HEADER_LEN + keyid_field_len + hash_field_len + cert_field_len;
> +    if (vce_len > be32_to_cpu(vce->len)) {
> +        return -1;
> +    }
> +
> +    vce->flags |= DIAG_320_VCE_FLAGS_VALID;
> +
> +    /* Update vce length to reflect the actual size used by vce */
> +    vce->len = cpu_to_be32(vce_len);
> +
> +    return 0;
> +}
> +
> +static VCEntry *diag_320_build_vce(const S390IPLCertificate *cert, int idx)
> +{
> +    g_autofree VCEntry *vce = NULL;
> +    uint32_t vce_max_size;
> +    int rc;
> +
> +    /*
> +     * Each field of the VCE is word-aligned.
> +     * Allocate enough space for the largest possible size for this VCE.
> +     * As the certificate fields (key-id, hash, data) are parsed, the
> +     * VCE's length field will be updated accordingly.
> +     */
> +    vce_max_size = VCE_HEADER_LEN +
> +                   ROUND_UP(CERT_KEY_ID_LEN, 4) +
> +                   ROUND_UP(CERT_HASH_LEN, 4) +
> +                   ROUND_UP(cert->der_size, 4);
> +
> +    vce = g_malloc0(vce_max_size);
> +    rc = build_vce_header(vce, cert, idx);
> +    if (rc) {
> +        /*
> +         * Error occurs - VCE does not contain a valid certificate.
> +         * Bit 0 of the VCE flags is 0 and the VCE length is set.
> +         */
> +        vce->len = cpu_to_be32(VCE_INVALID_LEN);
> +        goto out;
> +    }
> +
> +    vce->len = cpu_to_be32(vce_max_size);
> +    rc = build_vce_data(vce, cert);
> +    if (rc) {
> +        vce->len = cpu_to_be32(VCE_INVALID_LEN);
> +    }
> +
> +out:
> +    return g_steal_pointer(&vce);
> +}
> +
> +static int handle_diag320_store_vc(S390CPU *cpu, uint64_t addr, uint64_t r1, uintptr_t ra,
> +                                   S390IPLCertificateStore *cs)
> +{
> +    g_autofree VCBlock *vcb = NULL;
> +    size_t entry_offset;
> +    size_t remaining_space;
> +    uint32_t vce_len;
> +    uint16_t first_vc_index;
> +    uint16_t last_vc_index;
> +    int cs_start_index;
> +    int cs_end_index;
> +    uint32_t in_len;
> +
> +    vcb = g_new0(VCBlock, 1);
> +    if (s390_cpu_virt_mem_read(cpu, addr, r1, vcb, sizeof(*vcb))) {
> +        s390_cpu_virt_mem_handle_exc(cpu, ra);
> +        return -1;
> +    }
> +
> +    in_len = be32_to_cpu(vcb->in_len);
> +    first_vc_index = be16_to_cpu(vcb->first_vc_index);
> +    last_vc_index = be16_to_cpu(vcb->last_vc_index);
> +
> +    if (in_len % TARGET_PAGE_SIZE != 0) {
> +        return DIAG_320_RC_INVAL_VCB_LEN;
> +    }
> +
> +    if (first_vc_index > last_vc_index) {
> +        return DIAG_320_RC_BAD_RANGE;
> +    }
> +
> +    vcb->out_len = VCB_HEADER_LEN;
> +
> +    /*
> +     * DIAG 320 subcode 2 expects to query a certificate store that
> +     * maintains an index origin of 1. However, the S390IPLCertificateStore
> +     * maintains an index origin of 0. Thus, the indices must be adjusted
> +     * for correct access into the cert store. A couple of special cases
> +     * must also be accounted for.
> +     */
> +
> +    /* Both indices are 0; return header with no certs */
> +    if (first_vc_index == 0 && last_vc_index == 0) {
> +        goto out;
> +    }
> +
> +    /* Normalize indices */
> +    cs_start_index = (first_vc_index == 0) ? 0 : first_vc_index - 1;
> +    cs_end_index = last_vc_index - 1;
> +
> +    /* Requested range is outside the cert store; return header with no certs */
> +    if (cs_start_index >= cs->count || cs_end_index >= cs->count) {
> +        goto out;
> +    }
> +
> +    entry_offset = VCB_HEADER_LEN;
> +    remaining_space = in_len - VCB_HEADER_LEN;
> +
> +    for (int i = cs_start_index; i <= cs_end_index; i++) {
> +        VCEntry *vce;
> +        const S390IPLCertificate *cert = &cs->certs[i];
> +
> +        /*
> +         * Bit 0 of the VCE flags indicates whether the certificate is valid.
> +         * The caller of DIAG320 subcode 2 is responsible for verifying that
> +         * the VCE contains a valid certificate.
> +         */
> +        vce = diag_320_build_vce(cert, i);
> +        vce_len = be32_to_cpu(vce->len);

Why not just use vce->len?  It would seem that diag_320_build_vce will
set the len to one of:
	- max len
	- actual len
	- invalid len (72)

In all cases, endianness is accounted for.

> +
> +        /*
> +         * If there is no more space to store the cert,
> +         * set the remaining verification cert count and
> +         * break early.
> +         */
> +        if (remaining_space < vce_len) {
> +            vcb->remain_ct = cpu_to_be16(last_vc_index - i);
> +            g_free(vce);
> +            break;
> +        }
> +
> +        /* Write VCE */
> +        if (s390_cpu_virt_mem_write(cpu, addr + entry_offset, r1, vce, vce_len)) {
> +            s390_cpu_virt_mem_handle_exc(cpu, ra);
> +            g_free(vce);
> +            return -1;
> +        }
> +
> +        entry_offset += vce_len;

entry_offset seems to be serving the same purpose as vcb->out_len.  I
think it makes sense to drop the entry_offset variable and use
vcb->out_len in its place. Then there is one less thing to keep track of.

> +        vcb->out_len += vce_len;
> +        remaining_space -= vce_len;
> +        vcb->stored_ct++;
> +
> +        g_free(vce);
> +    }
> +    vcb->stored_ct = cpu_to_be16(vcb->stored_ct);
> +
> +out:
> +    vcb->out_len = cpu_to_be32(vcb->out_len);
> +
> +    if (s390_cpu_virt_mem_write(cpu, addr, r1, vcb, VCB_HEADER_LEN)) {
> +        s390_cpu_virt_mem_handle_exc(cpu, ra);
> +        return -1;
> +    }
> +
> +    return DIAG_320_RC_OK;
> +}
> +
>  QEMU_BUILD_BUG_MSG(sizeof(VCStorageSizeBlock) != VCSSB_MIN_LEN,
>                     "size of VCStorageSizeBlock is wrong");
> +QEMU_BUILD_BUG_MSG(sizeof(VCBlock) != VCB_HEADER_LEN, "size of VCBlock is wrong");
> +QEMU_BUILD_BUG_MSG(sizeof(VCEntry) != VCE_HEADER_LEN, "size of VCEntry is wrong");
>  
>  void handle_diag_320(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>  {
> @@ -268,7 +596,8 @@ void handle_diag_320(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>           * for now.
>           */
>          uint32_t ism_word0 = cpu_to_be32(DIAG_320_ISM_QUERY_SUBCODES |
> -                                         DIAG_320_ISM_QUERY_VCSI);
> +                                         DIAG_320_ISM_QUERY_VCSI |
> +                                         DIAG_320_ISM_STORE_VC);
>  
>          if (s390_cpu_virt_mem_write(cpu, addr, r1, &ism_word0, sizeof(ism_word0))) {
>              s390_cpu_virt_mem_handle_exc(cpu, ra);
> @@ -294,6 +623,18 @@ void handle_diag_320(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
>          }
>          env->regs[r1 + 1] = rc;
>          break;
> +    case DIAG_320_SUBC_STORE_VC:
> +        if (addr & ~TARGET_PAGE_MASK) {
> +            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> +            return;
> +        }
> +
> +        rc = handle_diag320_store_vc(cpu, addr, r1, ra, cs);
> +        if (rc == -1) {
> +            return;
> +        }
> +        env->regs[r1 + 1] = rc;
> +        break;
>      default:
>          env->regs[r1 + 1] = DIAG_320_RC_NOT_SUPPORTED;
>          break;


-- 
Regards,
  Collin
Re: [PATCH v9 09/30] s390x/diag: Implement DIAG 320 subcode 2
Posted by Zhuoying Cai 2 weeks, 2 days ago
Thanks for the review!

On 3/13/26 3:58 PM, Collin Walling wrote:
> On 3/5/26 17:41, Zhuoying Cai wrote:
>> DIAG 320 subcode 2 provides verification-certificates (VCs) that are in the
>> certificate store. Only X509 certificates in DER format and SHA-256 hash
>> type are recognized.
>>
>> The subcode value is denoted by setting the second-left-most bit
>> of an 8-byte field.
>>
>> The Verification Certificate Block (VCB) contains the output data
>> when the operation completes successfully. It includes a common
>> header followed by zero or more Verification Certificate Entries (VCEs),
>> depending on the VCB input length and the VC range (from the first VC
>> index to the last VC index) in the certificate store.
>>
>> Each VCE contains information about a certificate retrieved from
>> the S390IPLCertificateStore, such as the certificate name, key type,
>> key ID length, hash length, and the raw certificate data.
>> The key ID and hash are extracted from the raw certificate by the crypto API.
>>
>> Note: SHA2-256 VC hash type is required for retrieving the hash
>> (fingerprint) of the certificate.
>>
>> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
> 
> Functionally, this looks fine.  I've noted a few nits below, but I don't
> think they're worth dragging things out.
> 
> The important thing to address:
>  - documentation rewording
>  - CERT_NAME_MAX_LEN change
> 
> The rest are inconsequential.
> 
>> ---
>>  docs/specs/s390x-secure-ipl.rst |  22 ++
>>  hw/s390x/cert-store.h           |   3 +-
>>  include/hw/s390x/ipl/diag320.h  |  55 +++++
>>  target/s390x/diag.c             | 343 +++++++++++++++++++++++++++++++-
>>  4 files changed, 420 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/specs/s390x-secure-ipl.rst b/docs/specs/s390x-secure-ipl.rst
>> index 52661fab00..708253ac91 100644
>> --- a/docs/specs/s390x-secure-ipl.rst
>> +++ b/docs/specs/s390x-secure-ipl.rst
>> @@ -38,3 +38,25 @@ Subcode 1 - query verification certificate storage information
>>      The output is returned in the verification-certificate-storage-size block
>>      (VCSSB). A VCSSB length of 4 indicates that no certificates are available
>>      in the CS.
>> +
>> +Subcode 2 - store verification certificates
>> +    Provides VCs that are in the certificate store.
>> +
>> +    The output is provided in a VCB, which includes a common header followed by
>> +    zero or more verification-certificate entries (VCEs).
>> +
>> +    The instruction expects the cert store to
>> +    maintain an origin of 1 for the index (i.e. a retrieval of the first
>> +    certificate in the store should be denoted by setting first-VC to 1).
> 
> Nit: early newline after "...cert store to".  There is space for a few
> more characters on that line.
> 
>> +
>> +    The first-VC index and last-VC index fields of VCB specify the range of VCs
>> +    to be stored by subcode 2. Stored count and remained count fields specify
>> +    the number of VCs stored and could not be stored in the VCB due to
>> +    insufficient storage specified in the VCB input length field.
>> +
> 
> This might read better.  It makes it more clear how they're used.
> 
> """
> The first-VC and last-VC fields of the VCB specify the index range of
> VCs to be stored in the VCB. Certs are stored sequentially, starting
> with first-VC index. As each cert is stored, a "stored count" is
> incremented. If there is not enough space to store all certs requested
> by the index range, a "remaining count" will be recorded and no more
> certificates will be stored.
> """
> 
>> +    Each VCE contains a header followed by information extracted from a
>> +    certificate within the certificate store. The information includes:
>> +    key-id, hash, and certificate data. This information is stored
>> +    contiguously in a VCE (with zero-padding). Following the header, the
>> +    key-id is immediately stored. The hash and certificate data follow and
>> +    may be accessed via the respective offset fields stored in the VCE.
>> diff --git a/hw/s390x/cert-store.h b/hw/s390x/cert-store.h
>> index 7fc9503cb9..6f5ee63177 100644
>> --- a/hw/s390x/cert-store.h
>> +++ b/hw/s390x/cert-store.h
>> @@ -11,10 +11,9 @@
>>  #define HW_S390_CERT_STORE_H
>>  
>>  #include "hw/s390x/ipl/qipl.h"
>> +#include "hw/s390x/ipl/diag320.h"
>>  #include "crypto/x509-utils.h"
>>  
>> -#define CERT_NAME_MAX_LEN  64
>> -
> 
> Is there a reason why this was moved to diag320.h?
> 

It was moved to diag320.h because the same value is used by both
S390IPLCertificate and VCEntry from DIAG320. Since diag320.h is shared
by both QEMU and the BIOS, defining it there keeps the definition in a
single header and avoids duplication across multiple headers.

[...]