On 7/11/25 17:10, Zhuoying Cai wrote:
> DIAG 320 subcode 1 provides information needed to determine
> the amount of storage to store one or more certificates.
>
> The subcode value is denoted by setting the left-most bit
> of an 8-byte field.
This is general DIAG knowledge. Remove this sentence.
>
> The verification-certificate-storage-size block (VCSSB) contains
> the output data when the operation completes successfully.
>
Please add more detail describing where the data that the VCSSB gets
filled with comes from (s390 cert store), how this subcode is useful
(e.g. getting num of certs, knowing how much space may need to be
allocated to store a cert).
There are some #defines for the VCE (cert entries) and VCB (subcode 2
data structure). Please elaborate on them in the commit message.
> Signed-off-by: Zhuoying Cai <zy...@linux.ibm.com>
> ---
> include/hw/s390x/ipl/diag320.h | 23 ++++++++++++++++++++++
> target/s390x/diag.c | 36 +++++++++++++++++++++++++++++++++-
> 2 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/s390x/ipl/diag320.h b/include/hw/s390x/ipl/diag320.h
> index 713570545d..3916a2915e 100644
> --- a/include/hw/s390x/ipl/diag320.h
> +++ b/include/hw/s390x/ipl/diag320.h
> @@ -11,7 +11,30 @@
> #define S390X_DIAG320_H
>
> #define DIAG_320_SUBC_QUERY_ISM 0
> +#define DIAG_320_SUBC_QUERY_VCSI 1
>
> #define DIAG_320_RC_OK 0x0001
>
> +#define VCSSB_MAX_LEN 128
This is just the size of the struct, right? Why not just use sizeof
instead of this define?
> +#define VCE_HEADER_LEN 128
> +#define VCB_HEADER_LEN 64
> +
> +#define DIAG_320_ISM_QUERY_VCSI 0x4000000000000000
FYI: need a bit for subcode 0 (as Eric mentioned)
> +
> +struct VCStorageSizeBlock {
> + uint32_t length;
> + uint8_t reserved0[3];
> + uint8_t version;
> + uint32_t reserved1[6];
> + uint16_t total_vc_ct;
> + uint16_t max_vc_ct;
> + uint32_t reserved3[7];
> + uint32_t max_vce_len;
> + uint32_t reserved4[3];
> + uint32_t max_single_vcb_len;
> + uint32_t total_vcb_len;
> + uint32_t reserved5[10];
> +};
> +typedef struct VCStorageSizeBlock VCStorageSizeBlock;
> +
> #endif
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index 7b9b47a171..1f7d0cb2f6 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -191,9 +191,13 @@ out:
> }
> }
>
> +QEMU_BUILD_BUG_MSG(sizeof(VCStorageSizeBlock) != 128,
> + "size of VCStorageSizeBlock is wrong");
> +
I'm unsure of why this is needed? It's not necessarily up to the build
to determine that the data structure sizing hasn't been tampered with.
See below, which may help clarify where the 128 bytes need to be checked.
> void handle_diag_320(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t
> ra)
> {
> S390CPU *cpu = env_archcpu(env);
> + S390IPLCertificateStore *qcs = s390_ipl_get_certificate_store();
> uint64_t subcode = env->regs[r3];
> uint64_t addr = env->regs[r1];
> int rc;
> @@ -215,13 +219,43 @@ void handle_diag_320(CPUS390XState *env, uint64_t r1,
> uint64_t r3, uintptr_t ra)
>
> switch (subcode) {
> case DIAG_320_SUBC_QUERY_ISM:
> - uint64_t ism = 0;
> + uint64_t ism = cpu_to_be64(DIAG_320_ISM_QUERY_VCSI);
>
> if (s390_cpu_virt_mem_write(cpu, addr, r1, &ism, sizeof(ism))) {
> s390_cpu_virt_mem_handle_exc(cpu, ra);
> return;
> }
>
> + rc = DIAG_320_RC_OK;
> + break;
> + case DIAG_320_SUBC_QUERY_VCSI:
> + VCStorageSizeBlock vcssb;
> +
> + if (!diag_parm_addr_valid(addr, sizeof(VCStorageSizeBlock),
> + true)) {
> + s390_program_interrupt(env, PGM_ADDRESSING, ra);
> + return;
> + }
There is a piece missing of the "negotiation" between here and the
userspace. Userspace must set the length to a "minimum of 128 [bytes]
...; otherwise, a response code of 0x0202 is returned ..."
So, you're going to need to read the VCSSB denoted by addr and check
that the length field is >= 128. Otherwise set that response code.
Check out function `get_vcssb` in the kernel cert_store.c file for guidance.
> +
> + if (!qcs || !qcs->count) {
> + vcssb.length = cpu_to_be32(4);
Please use a #define instead of a literal. 4 is denoted as the length
set if no certs are found.
> + } else {
> + vcssb.length = cpu_to_be32(VCSSB_MAX_LEN);
> + vcssb.version = 0;
> + vcssb.total_vc_ct = cpu_to_be16(qcs->count);
> + vcssb.max_vc_ct = cpu_to_be16(MAX_CERTIFICATES);
> + vcssb.max_vce_len = cpu_to_be32(VCE_HEADER_LEN +
> qcs->max_cert_size);
This field is suppose to represent a constraint imposed by the cert
store that denotes the largest sized cert that it will allow to be
stored, not necessarily the "size of the largest cert currently stored"
(that is reserved for max_single_vcb_len below). I do not think you
have such a limitation in place, so it may be safe to ignore it?
Kernel only prints this field for debugging. Your code later on doesn't
utilize it.
> + vcssb.max_single_vcb_len = cpu_to_be32(VCB_HEADER_LEN +
> VCE_HEADER_LEN +
> + qcs->max_cert_size);
> + vcssb.total_vcb_len = cpu_to_be32(VCB_HEADER_LEN +
> + qcs->count * VCE_HEADER_LEN +
> + qcs->total_bytes);
> + }
> +
> + if (s390_cpu_virt_mem_write(cpu, addr, r1, &vcssb,
> sizeof(VCStorageSizeBlock))) {
> + s390_cpu_virt_mem_handle_exc(cpu, ra);
> + return;
> + }
> rc = DIAG_320_RC_OK;
> break;
> default:
--
Regards,
Collin
