Re: [PATCH v9 22/30] pc-bios/s390-ccw: Add additional security checks for secure boot

[Collin Walling <walling@linux.ibm.com>]

On 3/5/26 17:41, Zhuoying Cai wrote:
> Add additional checks to ensure that components do not overlap with
> signed components when loaded into memory.
> 
> Add additional checks to ensure the load addresses of unsigned components
> are greater than or equal to 0x2000.
> 
> When the secure IPL code loading attributes facility (SCLAF) is installed,
> all signed components must contain a secure code loading attributes block
> (SCLAB).
> 
> The SCLAB provides further validation of information on where to load the
> signed binary code from the load device, and where to start the execution
> of the loaded OS code.
> 
> When SCLAF is installed, its content must be evaluated during secure IPL.
> However, a missing SCLAB will not be reported in audit mode. The SCALB
> checking will be skipped in this case.
> 
> Add IPL Information Error Indicators (IIEI) and Component Error
> Indicators (CEI) for IPL Information Report Block (IIRB).

This goes back to my comment in patch 19: who/what is going to make use
of the IIRB? If no one or nothing will inspect this data structure, then
perhaps it's sufficient enough to simply log the errors?

> 
> When SCLAF is installed, additional secure boot checks are performed
> during zipl and store results of verification into IIRB.
> 
> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
> ---
>  include/hw/s390x/ipl/qipl.h   |  29 +++-
>  pc-bios/s390-ccw/s390-ccw.h   |   1 +
>  pc-bios/s390-ccw/sclp.c       |   8 +
>  pc-bios/s390-ccw/sclp.h       |   1 +
>  pc-bios/s390-ccw/secure-ipl.c | 314 +++++++++++++++++++++++++++++++++-
>  pc-bios/s390-ccw/secure-ipl.h |  42 +++++
>  6 files changed, 391 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
> index 1b6cb3231d..9518fcb1dc 100644
> --- a/include/hw/s390x/ipl/qipl.h
> +++ b/include/hw/s390x/ipl/qipl.h
> @@ -136,10 +136,20 @@ struct IplInfoReportBlockHeader {
>  };
>  typedef struct IplInfoReportBlockHeader IplInfoReportBlockHeader;
>  
> +/* IPL Info Error Indicators */
> +#define S390_IIEI_NO_SIGNED_COMP      0x8000 /* bit 0 */
> +#define S390_IIEI_NO_SCLAB            0x4000 /* bit 1 */
> +#define S390_IIEI_NO_GLOBAL_SCLAB     0x2000 /* bit 2 */
> +#define S390_IIEI_MORE_GLOBAL_SCLAB   0x1000 /* bit 3 */
> +#define S390_IIEI_FOUND_UNSIGNED_COMP 0x800 /* bit 4 */
> +#define S390_IIEI_MORE_SIGNED_COMP    0x400 /* bit 5 */
> +
>  struct IplInfoBlockHeader {
>      uint32_t len;
>      uint8_t  type;
> -    uint8_t  reserved1[11];
> +    uint8_t  reserved1[3];
> +    uint16_t iiei;
> +    uint8_t  reserved2[6];
>  };
>  typedef struct IplInfoBlockHeader IplInfoBlockHeader;
>  
> @@ -163,13 +173,28 @@ typedef struct IplSignatureCertificateList IplSignatureCertificateList;
>  #define S390_IPL_DEV_COMP_FLAG_SC  0x80
>  #define S390_IPL_DEV_COMP_FLAG_CSV 0x40
>  
> +/* IPL Device Component Error Indicators */
> +#define S390_CEI_INVALID_SCLAB             0x80000000 /* bit 0 */
> +#define S390_CEI_INVALID_SCLAB_LEN         0x40000000 /* bit 1 */
> +#define S390_CEI_INVALID_SCLAB_FORMAT      0x20000000 /* bit 2 */
> +#define S390_CEI_UNMATCHED_SCLAB_LOAD_ADDR 0x10000000 /* bit 3 */
> +#define S390_CEI_UNMATCHED_SCLAB_LOAD_PSW  0x8000000  /* bit 4 */
> +#define S390_CEI_INVALID_LOAD_PSW          0x4000000  /* bit 5 */
> +#define S390_CEI_NUC_NOT_IN_GLOBAL_SCLA    0x2000000  /* bit 6 */
> +#define S390_CEI_SCLAB_OLA_NOT_ONE         0x1000000  /* bit 7 */
> +#define S390_CEI_SC_NOT_IN_GLOBAL_SCLAB    0x800000   /* bit 8 */
> +#define S390_CEI_SCLAB_LOAD_ADDR_NOT_ZERO  0x400000   /* bit 9 */
> +#define S390_CEI_SCLAB_LOAD_PSW_NOT_ZERO   0x200000   /* bit 10 */
> +#define S390_CEI_INVALID_UNSIGNED_ADDR     0x100000   /* bit 11 */
> +
>  struct IplDeviceComponentEntry {
>      uint64_t addr;
>      uint64_t len;
>      uint8_t  flags;
>      uint8_t  reserved1[5];
>      uint16_t cert_index;
> -    uint8_t  reserved2[8];
> +    uint32_t cei;
> +    uint8_t  reserved2[4];
>  };
>  typedef struct IplDeviceComponentEntry IplDeviceComponentEntry;
>  
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index a0d568696a..7d1a9d4acc 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -82,6 +82,7 @@ void sclp_setup(void);
>  void sclp_get_loadparm_ascii(char *loadparm);
>  bool sclp_is_diag320_on(void);
>  bool sclp_is_sipl_on(void);
> +bool sclp_is_sclaf_on(void);
>  int sclp_read(char *str, size_t count);
>  
>  /* virtio.c */
> diff --git a/pc-bios/s390-ccw/sclp.c b/pc-bios/s390-ccw/sclp.c
> index e3b6a1f07e..5dbddff9ae 100644
> --- a/pc-bios/s390-ccw/sclp.c
> +++ b/pc-bios/s390-ccw/sclp.c
> @@ -151,6 +151,14 @@ bool sclp_is_sipl_on(void)
>      return fac_ipl & SCCB_FAC_IPL_SIPL_BIT;
>  }
>  
> +bool sclp_is_sclaf_on(void)
> +{
> +    uint16_t fac_ipl = 0;
> +
> +    sclp_get_fac_ipl(&fac_ipl);
> +    return fac_ipl & SCCB_FAC_IPL_SCLAF_BIT;
> +}
> +
>  int sclp_read(char *str, size_t count)
>  {
>      ReadEventData *sccb = (void *)_sccb;
> diff --git a/pc-bios/s390-ccw/sclp.h b/pc-bios/s390-ccw/sclp.h
> index cf147f4634..3441020d6b 100644
> --- a/pc-bios/s390-ccw/sclp.h
> +++ b/pc-bios/s390-ccw/sclp.h
> @@ -52,6 +52,7 @@ typedef struct SCCBHeader {
>  #define SCCB_DATA_LEN (SCCB_SIZE - sizeof(SCCBHeader))
>  #define SCCB_FAC134_DIAG320_BIT 0x4
>  #define SCCB_FAC_IPL_SIPL_BIT 0x4000
> +#define SCCB_FAC_IPL_SCLAF_BIT 0x1000
>  
>  typedef struct ReadInfo {
>      SCCBHeader h;
> diff --git a/pc-bios/s390-ccw/secure-ipl.c b/pc-bios/s390-ccw/secure-ipl.c
> index 68596491c5..840b88a699 100644
> --- a/pc-bios/s390-ccw/secure-ipl.c
> +++ b/pc-bios/s390-ccw/secure-ipl.c
> @@ -198,6 +198,12 @@ static bool secure_ipl_supported(void)
>          return false;
>      }
>  
> +    if (!sclp_is_sclaf_on()) {
> +        puts("Secure IPL Code Loading Attributes Facility is not supported by"
> +             " the hypervisor!");
> +        return false;
> +    }
> +
>      return true;
>  }
>  
> @@ -258,6 +264,286 @@ static void addr_overlap_check(SecureIplCompAddrRange *comp_addr_range,
>      *addr_range_index += 1;
>  }
>  
> +static void check_unsigned_addr(uint64_t load_addr, IplDeviceComponentEntry *comp_entry)
> +{
> +    /* unsigned load address must be greater than or equal to 0x2000 */
> +    if (load_addr >= 0x2000) {
> +        return;
> +    }
> +
> +    set_comp_cei_with_log(comp_entry, S390_CEI_INVALID_UNSIGNED_ADDR,
> +                          "Load address is less than 0x2000");
> +}

I think I still would have preferred some sort of assert-styled function
that could be used in place of a lot of these simpler functions e.g.:

```
component_check(load_addr >= 0x2000, comp_entry,
                S390_CEI_INVALID_UNSIGNED_ADDR,
                "Load address is less than 0x2000");
```

(Might need a more fitting name, though.)

It's basically the `set_comp_cei_with_log()` function with a boolean
parameter up front.

> +
> +static bool check_sclab_presence(uint8_t *sclab_magic,
> +                                 IplDeviceComponentEntry *comp_entry)
> +{
> +    /* identifies the presence of SCLAB */
> +    if (magic_match(sclab_magic, ZIPL_MAGIC)) {
> +        return true;
> +    }
> +
> +    if (comp_entry) {
> +        comp_entry->cei |= S390_CEI_INVALID_SCLAB;
> +    }
> +
> +    /* a missing SCLAB will not be reported in audit mode */
> +    return false;
> +}
> +
> +static void check_sclab_length(uint16_t sclab_len, IplDeviceComponentEntry *comp_entry)
> +{
> +    if (sclab_len >= S390_SECURE_IPL_SCLAB_MIN_LEN) {
> +        return;
> +    }
> +
> +    set_comp_cei_with_log(comp_entry,
> +                          S390_CEI_INVALID_SCLAB_LEN | S390_CEI_INVALID_SCLAB,
> +                          "Invalid SCLAB length");
> +}
> +
> +static void check_sclab_format(uint8_t sclab_format, IplDeviceComponentEntry *comp_entry)
> +{
> +    /* SCLAB format must set to zero, indicating a format-0 SCLAB being used */
> +    if (sclab_format == 0) {
> +        return;
> +    }
> +
> +    set_comp_cei_with_log(comp_entry, S390_CEI_INVALID_SCLAB_FORMAT,
> +                          "Format-0 SCLAB is not being used");
> +}
> +
> +static void check_sclab_opsw(SecureCodeLoadingAttributesBlock *sclab,
> +                             SecureIplSclabInfo *sclab_info,
> +                             IplDeviceComponentEntry *comp_entry)
> +{
> +    const char *msg;
> +    uint32_t cei_flag = 0;
> +
> +    if (!(sclab->flags & S390_SECURE_IPL_SCLAB_FLAG_OPSW)) {
> +        /* OPSW = 0 - Load PSW field in SCLAB must contain zeros */
> +        if (sclab->load_psw != 0) {
> +            cei_flag |= S390_CEI_SCLAB_LOAD_PSW_NOT_ZERO;
> +            msg = "Load PSW is not zero when Override PSW bit is zero";
> +        }
> +    } else {
> +        /* OPSW = 1 indicating global SCLAB */
> +        sclab_info->global_count += 1;
> +        if (sclab_info->global_count == 1) {
> +            sclab_info->load_psw = sclab->load_psw;
> +            sclab_info->flags = sclab->flags;
> +        }
> +
> +        /* OLA must set to one */
> +        if (!(sclab->flags & S390_SECURE_IPL_SCLAB_FLAG_OLA)) {
> +            cei_flag |= S390_CEI_SCLAB_OLA_NOT_ONE;
> +            msg = "Override Load Address bit is not set to one in the global SCLAB";
> +        }
> +    }
> +
> +    if (cei_flag) {
> +        set_comp_cei_with_log(comp_entry, cei_flag, msg);
> +    }
> +}
> +
> +static void check_sclab_ola(SecureCodeLoadingAttributesBlock *sclab, uint64_t load_addr,
> +                            IplDeviceComponentEntry *comp_entry)
> +{
> +    const char *msg;
> +    uint32_t cei_flag = 0;
> +
> +    if (!(sclab->flags & S390_SECURE_IPL_SCLAB_FLAG_OLA)) {
> +        /* OLA = 0 - Load address field in SCLAB must contain zeros */
> +        if (sclab->load_addr != 0) {
> +            cei_flag |= S390_CEI_SCLAB_LOAD_ADDR_NOT_ZERO;
> +            msg = "Load Address is not zero when Override Load Address bit is zero";
> +        }
> +    } else {
> +        /* OLA = 1 - Load address field must match storage address of the component */
> +        if (sclab->load_addr != load_addr) {
> +            cei_flag |= S390_CEI_UNMATCHED_SCLAB_LOAD_ADDR;
> +            msg = "Load Address does not match with component load address";
> +        }
> +    }
> +
> +    if (cei_flag) {
> +        set_comp_cei_with_log(comp_entry, cei_flag, msg);
> +    }
> +}
> +
> +static void check_sclab_nuc(uint16_t sclab_flags, IplDeviceComponentEntry *comp_entry)
> +{
> +    const char *msg;
> +    bool is_nuc_set;
> +    bool is_global_sclab;
> +
> +    is_nuc_set = sclab_flags & S390_SECURE_IPL_SCLAB_FLAG_NUC;
> +    is_global_sclab = sclab_flags & S390_SECURE_IPL_SCLAB_FLAG_OPSW;
> +    if (is_nuc_set && !is_global_sclab) {
> +        msg = "No Unsigned Components bit is set, but not in the global SCLAB";
> +        set_comp_cei_with_log(comp_entry, S390_CEI_NUC_NOT_IN_GLOBAL_SCLA, msg);
> +    }
> +}
> +
> +static void check_sclab_sc(uint16_t sclab_flags, IplDeviceComponentEntry *comp_entry)
> +{
> +    const char *msg;
> +    bool is_sc_set;
> +    bool is_global_sclab;
> +
> +    is_sc_set = sclab_flags & S390_SECURE_IPL_SCLAB_FLAG_SC;
> +    is_global_sclab = sclab_flags & S390_SECURE_IPL_SCLAB_FLAG_OPSW;
> +    if (is_sc_set && !is_global_sclab) {
> +        msg = "Single Component bit is set, but not in the global SCLAB";
> +        set_comp_cei_with_log(comp_entry, S390_CEI_SC_NOT_IN_GLOBAL_SCLAB, msg);
> +    }
> +}
> +
> +static bool is_psw_valid(uint64_t psw, SecureIplCompAddrRange *comp_addr_range,
> +                         int range_index)
> +{
> +    uint32_t addr = psw & 0x7fffffff;
> +
> +    /* PSW points within a signed binary code component */
> +    for (int i = 0; i < range_index; i++) {
> +        if (comp_addr_range[i].is_signed &&
> +            addr >= comp_addr_range[i].start_addr &&
> +            addr <= comp_addr_range[i].end_addr - 2) {
> +            return true;
> +       }
> +    }
> +
> +    return false;
> +}
> +
> +static void check_load_psw(SecureIplCompAddrRange *comp_addr_range,
> +                           int addr_range_index, uint64_t sclab_load_psw,
> +                           uint64_t load_psw, IplDeviceComponentEntry *comp_entry)
> +{
> +    bool valid;
> +
> +    valid = is_psw_valid(sclab_load_psw, comp_addr_range, addr_range_index) &&
> +            is_psw_valid(load_psw, comp_addr_range, addr_range_index);
> +    if (!valid) {
> +        set_comp_cei_with_log(comp_entry, S390_CEI_INVALID_LOAD_PSW, "Invalid PSW");
> +    }
> +
> +    /* compare load PSW with the PSW specified in component */
> +    if (sclab_load_psw != load_psw) {
> +        set_comp_cei_with_log(comp_entry, S390_CEI_UNMATCHED_SCLAB_LOAD_PSW,
> +                              "Load PSW does not match with PSW in component");
> +    }
> +}
> +
> +static void check_nuc(uint16_t global_sclab_flags, int unsigned_count,
> +                      IplDeviceComponentList *comp_list)
> +{
> +    bool is_nuc_set;
> +
> +    is_nuc_set = global_sclab_flags & S390_SECURE_IPL_SCLAB_FLAG_NUC;
> +    if (is_nuc_set && unsigned_count > 0) {
> +        comp_list->ipl_info_header.iiei |= S390_IIEI_FOUND_UNSIGNED_COMP;
> +        zipl_secure_handle("Unsigned components are not allowed");
> +    }
> +}
> +
> +static void check_sc(uint16_t global_sclab_flags,
> +                     int signed_count, int unsigned_count,
> +                     IplDeviceComponentList *comp_list)
> +{
> +    bool is_sc_set;
> +
> +    is_sc_set = global_sclab_flags & S390_SECURE_IPL_SCLAB_FLAG_SC;
> +    if (is_sc_set && signed_count != 1 && unsigned_count >= 0) {
> +        comp_list->ipl_info_header.iiei |= S390_IIEI_MORE_SIGNED_COMP;
> +        zipl_secure_handle("Only one signed component is allowed");
> +    }
> +}
> +
> +void check_global_sclab(SecureIplSclabInfo sclab_info,
> +                        int unsigned_count, int signed_count,
> +                        IplDeviceComponentList *comp_list)
> +{
> +    if (sclab_info.count == 0) {
> +        return;
> +    }
> +
> +    if (sclab_info.global_count == 0) {
> +        comp_list->ipl_info_header.iiei |= S390_IIEI_NO_GLOBAL_SCLAB;
> +        zipl_secure_handle("Global SCLAB does not exists");
> +        return;
> +    }
> +
> +    if (sclab_info.global_count > 1) {
> +        comp_list->ipl_info_header.iiei |= S390_IIEI_MORE_GLOBAL_SCLAB;
> +        zipl_secure_handle("More than one global SCLAB");
> +        return;
> +    }
> +
> +    if (sclab_info.flags) {
> +        /* Unsigned components are not allowed if NUC flag is set in the global SCLAB */
> +        check_nuc(sclab_info.flags, unsigned_count, comp_list);
> +
> +        /* Only one signed component is allowed is SC flag is set in the global SCLAB */
> +        check_sc(sclab_info.flags, signed_count, unsigned_count, comp_list);

There's `check_sc()` and also `check_sclab_sc()` (and
`check_signed_comp()`, which is similar).  At least with the first two,
the functions are very easy to mix up (in fact, I was going to suggest
renaming check_sc to check_sclab_sc).

> +    }
> +}
> +
> +static void check_signed_comp(int signed_count, IplDeviceComponentList *comp_list)
> +{
> +    if (signed_count > 0) {
> +        return;
> +    }
> +
> +    comp_list->ipl_info_header.iiei |= S390_IIEI_NO_SIGNED_COMP;
> +    zipl_secure_handle("Secure boot is on, but components are not signed");
> +}
> +
> +static void check_sclab_count(int count, IplDeviceComponentList *comp_list)
> +{
> +    if (count > 0) {
> +        return;
> +    }
> +
> +    comp_list->ipl_info_header.iiei |= S390_IIEI_NO_SCLAB;
> +    zipl_secure_handle("No recognizable SCLAB");
> +}
> +
> +static void check_sclab(uint64_t comp_addr, uint64_t comp_len,
> +                        IplDeviceComponentEntry *comp_entry,
> +                        SecureIplSclabInfo *sclab_info)
> +{
> +    SclabOriginLocator *sclab_locator;
> +    SecureCodeLoadingAttributesBlock *sclab;
> +    bool exist;
> +
> +    /* sclab locator is located at the last 8 bytes of the signed comp */
> +    sclab_locator = (SclabOriginLocator *)(comp_addr + comp_len - 8);
> +
> +    /* return early if sclab does not exist */
> +    exist = check_sclab_presence(sclab_locator->magic, comp_entry);

Could just merge this into the check below:

```
if (!check_sclab_presence(...)) {
    return;
}
```

> +    if (!exist) {
> +        return;
> +    }
> +
> +    check_sclab_length(sclab_locator->len, comp_entry);
> +
> +    /* return early if sclab is invalid */
> +    if (comp_entry && (comp_entry->cei & S390_CEI_INVALID_SCLAB)) {
> +        return;
> +    }
> +
> +    sclab_info->count += 1;
> +    sclab = (SecureCodeLoadingAttributesBlock *)(comp_addr + comp_len -
> +                                                 sclab_locator->len);
> +
> +    check_sclab_format(sclab->format, comp_entry);
> +    check_sclab_opsw(sclab, sclab_info, comp_entry);
> +    check_sclab_ola(sclab, comp_addr, comp_entry);
> +    check_sclab_nuc(sclab->flags, comp_entry);
> +    check_sclab_sc(sclab->flags, comp_entry);
> +}
> +
>  static int zipl_load_signature(ComponentEntry *entry, uint64_t sig_sec)
>  {
>      if (zipl_load_segment(entry, sig_sec) < 0) {
> @@ -304,6 +590,9 @@ int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec)
>      SecureIplCompAddrRange comp_addr_range[MAX_CERTIFICATES];
>      int addr_range_index = 0;
>      int signed_count = 0;
> +    int unsigned_count = 0;
> +    SecureIplSclabInfo sclab_info = { 0 };
> +    IplDeviceComponentEntry *comp_entry;
>  
>      if (!secure_ipl_supported()) {
>          panic("Unable to boot in secure/audit mode");
> @@ -335,10 +624,21 @@ int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec)
>              addr_overlap_check(comp_addr_range, &addr_range_index,
>                                 comp_addr, comp_addr + comp_len, sig_len > 0);
>  
> +            comp_entry = (comp_entry_idx < MAX_CERTIFICATES) ?
> +                         &comp_list.device_entries[comp_entry_idx] : NULL;
> +

This took me a little bit of time to understand why this was happening.
It'd make a *little* more sense if it were nested under the if statement
below.

It would read better to `#define` a `MAX_COMPONENTS` as well.  Could
just alias `MAX_CERTIFICATES` so both use the same value.

>              if (!sig_len) {
> +                check_unsigned_addr(comp_addr, comp_entry);
> +                comp_list_add(&comp_list, comp_entry_idx, cert_entry_idx,
> +                              comp_addr, comp_len, 0x00);
> +
> +                unsigned_count += 1;
> +                comp_entry_idx++;
>                  break;
>              }
>  
> +            check_sclab(comp_addr, comp_len,
> +                        &comp_list.device_entries[comp_entry_idx], &sclab_info);
>              verified = verify_signature(comp_len, comp_addr, sig_len, (uint64_t)sig,
>                                          &cert_len, &cert_table_idx);
>  
> @@ -391,10 +691,20 @@ int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec)
>          }
>      }
>  
> -    if (signed_count == 0) {
> -        zipl_secure_handle("Secure boot is on, but components are not signed");
> +    /* validate load PSW with PSW specified in the final entry */
> +    if (sclab_info.load_psw) {
> +        comp_entry = (comp_entry_idx < MAX_CERTIFICATES) ?
> +                     &comp_list.device_entries[comp_entry_idx] : NULL;

Silently accepting a NULL component makes it seem like an error should
be thrown, or a special case should be handled.

It looks like these `check_*()` functions only need the `cei` field of
the `comp_entry`.  Instead of passing the entire `comp_entry` to these
functions, pass something like an independent `cei_flags` variable. The
functions will set the appropriate error bits where appropriate.  This
also eliminates the various checks for `comp_entry != NULL`.

Then change `comp_list_add()` to accept the `cei_flags` field and set it
in the respective component while still accounting for the
`MAX_CERTIFICATES` check.

> +        check_load_psw(comp_addr_range, addr_range_index,
> +                       sclab_info.load_psw, entry->compdat.load_psw, comp_entry);
> +        comp_list_add(&comp_list, comp_entry_idx, -1,
> +                      entry->compdat.load_psw, 0, 0x00);

Why is the exec component added to the list?  It wasn't added before
this patch.

>      }
>  
> +    check_signed_comp(signed_count, &comp_list);
> +    check_sclab_count(sclab_info.count, &comp_list);
> +    check_global_sclab(sclab_info, unsigned_count, signed_count, &comp_list);
> +
>      update_iirb(&comp_list, &cert_list);
>  
>      *entry_ptr = entry;
> diff --git a/pc-bios/s390-ccw/secure-ipl.h b/pc-bios/s390-ccw/secure-ipl.h
> index 69edfce241..4e9f4f08b9 100644
> --- a/pc-bios/s390-ccw/secure-ipl.h
> +++ b/pc-bios/s390-ccw/secure-ipl.h
> @@ -16,6 +16,38 @@
>  VCStorageSizeBlock *zipl_secure_get_vcssb(void);
>  int zipl_run_secure(ComponentEntry **entry_ptr, uint8_t *tmp_sec);
>  
> +#define S390_SECURE_IPL_SCLAB_FLAG_OPSW    0x8000
> +#define S390_SECURE_IPL_SCLAB_FLAG_OLA     0x4000
> +#define S390_SECURE_IPL_SCLAB_FLAG_NUC     0x2000
> +#define S390_SECURE_IPL_SCLAB_FLAG_SC      0x1000
> +
> +#define S390_SECURE_IPL_SCLAB_MIN_LEN      32
> +
> +struct SecureCodeLoadingAttributesBlock {
> +    uint8_t  format;
> +    uint8_t  reserved1;
> +    uint16_t flags;
> +    uint8_t  reserved2[4];
> +    uint64_t load_psw;
> +    uint64_t load_addr;
> +    uint64_t reserved3[];
> +} __attribute__ ((packed));
> +typedef struct SecureCodeLoadingAttributesBlock SecureCodeLoadingAttributesBlock;
> +
> +struct SclabOriginLocator {
> +    uint8_t reserved[2];
> +    uint16_t len;
> +    uint8_t magic[4];
> +} __attribute__ ((packed));
> +typedef struct SclabOriginLocator SclabOriginLocator;
> +
> +typedef struct SecureIplSclabInfo {
> +    int count;
> +    int global_count;
> +    uint64_t load_psw;
> +    uint16_t flags;
> +} SecureIplSclabInfo;

IIRC, `SecureIplSclabInfo` was invented in these patches to help
consolidate some fields.  Unless I'm wrong, you could add an
`unsigned_count` and `signed_count` fields.  That will clear up some
stuff in `zipl_run_secure()`.

I'd also suggest prepending `global_` to `load_psw` and `flags` here to
make it clear what they represent.

Put a comment above explaining something like "Custom struct used to
consolidate SCLAB overhead".  Anything to signal that this isn't a
documented s390 data structure.

> +
>  typedef struct SecureIplCompAddrRange {
>      bool is_signed;
>      uint64_t start_addr;
> @@ -33,6 +65,16 @@ static inline void zipl_secure_handle(const char *message)
>      }
>  }
>  
> +static inline void set_comp_cei_with_log(IplDeviceComponentEntry *comp_entry,
> +                                         uint32_t flag, const char *message)
> +{
> +    if (comp_entry) {
> +        comp_entry->cei |= flag;
> +    }
> +
> +    zipl_secure_handle(message);
> +}
> +
>  static inline uint64_t diag320(void *data, unsigned long subcode)
>  {
>      register unsigned long addr asm("0") = (unsigned long)data;

Functionally, I think things look sane.  Please look into a way to
simplify the functions that exhibit similar patterns and reduce the
number of parameters required by each function.  SCLAF is a bit of an
abstract concept, so anything to make code simpler will help out a lot.

-- 
Regards,
  Collin