Re: [PATCH v2 06/25] s390x/diag: Implement DIAG 320 subcode 1

[Thomas Huth <thuth@redhat.com>]

On 09/05/2025 00.50, 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.
> 
> The verification-certificate-storage-size block (VCSSB) contains
> the output data when the operation completes successfully.
> 
> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
> ---
>   include/hw/s390x/ipl/diag320.h | 25 ++++++++++++++++++++++
>   target/s390x/diag.c            | 38 +++++++++++++++++++++++++++++++++-
>   2 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/s390x/ipl/diag320.h b/include/hw/s390x/ipl/diag320.h
> index 713570545d..e91eb7238c 100644
> --- a/include/hw/s390x/ipl/diag320.h
> +++ b/include/hw/s390x/ipl/diag320.h
> @@ -11,7 +11,32 @@
>   #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 DIAG_320_RC_INVAL_VCSSB_LEN 0x0202
> +
> +#define VCSSB_MAX_LEN   128
> +#define VCE_HEADER_LEN  128
> +#define VCB_HEADER_LEN  64
> +
> +#define DIAG_320_ISM_QUERY_VCSI     0x4000000000000000
> +
> +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];
> +} QEMU_PACKED;

Please try to avoid QEMU_PACKED as long as it is not really necessary. The 
compiler can create better code without it and we have less trouble on 
exotic host architectures that way.

Here, all members seem to have proper natural alignment, so you don't need 
it. You can use something like this to make sure that there is no unexpected 
padding in the structure:

QEMU_BUILD_BUG_MSG(sizeof(VCStorageSizeBlock) != ...,
                           "size of VCStorageSizeBlock is wrong");

> +typedef struct VCStorageSizeBlock \
> +VCStorageSizeBlock;

That looks like it fits into one line.

>   #endif
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index 9d249831b3..0743f5ec0e 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -194,6 +194,7 @@ out:
>   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 +216,48 @@ 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;
> +        }
> +
> +        if (!qcs || !qcs->count) {
> +            vcssb.length = 4;

You missed the cpu_to_be32() here.

> +        } 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);
> +            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 (be32_to_cpu(vcssb.length) > 4 && be32_to_cpu(vcssb.length) < 128) {

How is this supposed to happen? There are only two hard-coded values for 
this field above?

> +            rc = DIAG_320_RC_INVAL_VCSSB_LEN;
> +            break;
> +        }
> +
> +        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:

  Thomas