On 5/5/26 16:18, Zhuoying Cai wrote:
[...]
> +
> +#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
The key types, format, and hash types might be good as enumerations.
It'd be especially helpful when going through the debugger and seeing
explicit values printed out.
[...]
> +
> +static int build_vce_header(VCEntry *vce, const S390IPLCertificate *cert,
> int idx)
> +{
> + int key_type;
> +
> + vce->vce_hdr.len = cpu_to_be32(sizeof(VCEntryHeader));
> + vce->vce_hdr.cert_idx = cpu_to_be16(idx + 1);
> + memcpy(vce->vce_hdr.name, cert->name, CERT_NAME_MAX_LEN);
> +
> + key_type = get_key_type(cert);
> + if (key_type == -1) {
> + return -1;
> + }
Move the is_cert_valid check to here.
> + vce->vce_hdr.key_type = key_type;
> +
> + return 0;
> +}
> +
[...]
> +
> + for (int i = cs_start_index; i <= cs_end_index; i++) {
> + VCEntry *vce;
What about using g_autofree here and then removing the g_free's below?
> + 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);
> +
> + /*
> + * If there is no more space to store the cert,
> + * set the remaining verification cert count and
> + * break early.
> + */
> + if (remaining_space < vce->vce_hdr.len) {
> + vcb->vcb_hdr.remain_ct = cpu_to_be16(last_vc_index - i);
> + g_free(vce);
> + break;
> + }
> +
> + /* Write VCE */
> + if (s390_cpu_virt_mem_write(cpu, addr + vcb->vcb_hdr.out_len, r1,
> + vce, vce->vce_hdr.len)) {
> + s390_cpu_virt_mem_handle_exc(cpu, ra);
> + g_free(vce);
> + return -1;
> + }
> +
> + vcb->vcb_hdr.out_len += vce->vce_hdr.len;
> + remaining_space -= vce->vce_hdr.len;
> + vcb->vcb_hdr.stored_ct++;
> +
> + g_free(vce);
> + }
> + vcb->vcb_hdr.stored_ct = cpu_to_be16(vcb->vcb_hdr.stored_ct);
> +
> +out:
> + vcb->vcb_hdr.out_len = cpu_to_be32(vcb->vcb_hdr.out_len);
> +
> + if (s390_cpu_virt_mem_write(cpu, addr, r1, vcb, sizeof(VCBlockHeader))) {
> + s390_cpu_virt_mem_handle_exc(cpu, ra);
> + return -1;
> + }
> +
> + return DIAG_320_RC_OK;
> +}
[...]
Aside from the endian conversion mentioned by Farhan, I think the rest
of this patch looks good. I appreciate the inline comments. It has
made coming back to these reviews a lot easier to understand your
approach :)
--
Regards,
Collin
