On 7/11/25 17:10, Zhuoying Cai wrote:
> DIAGNOSE 320 is introduced to support certificate store facility,
> which includes operations such as query certificate storage
> information and provide certificates in the certificate store.
>
> Currently, only subcode 0 is supported with this patch, which is
> used to query a bitmap of which subcodes are supported.
Change to: "used to query the Installed Subcodes Mask (ISM)."
Based on my feedback below, make sure to include a statement that this
subcode is only supported when the Certificate Store facility is enabled.
>
> Signed-off-by: Zhuoying Cai <zy...@linux.ibm.com>
> ---
> include/hw/s390x/ipl/diag320.h | 17 ++++++++++++++
> target/s390x/diag.c | 41 ++++++++++++++++++++++++++++++++++
> target/s390x/kvm/kvm.c | 14 ++++++++++++
> target/s390x/s390x-internal.h | 2 ++
> target/s390x/tcg/misc_helper.c | 7 ++++++
> 5 files changed, 81 insertions(+)
> create mode 100644 include/hw/s390x/ipl/diag320.h
>
> diff --git a/include/hw/s390x/ipl/diag320.h b/include/hw/s390x/ipl/diag320.h
> new file mode 100644
> index 0000000000..713570545d
> --- /dev/null
> +++ b/include/hw/s390x/ipl/diag320.h
> @@ -0,0 +1,17 @@
> +/*
> + * S/390 DIAGNOSE 320 definitions and structures
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Zhuoying Cai <zy...@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef S390X_DIAG320_H
> +#define S390X_DIAG320_H
> +
> +#define DIAG_320_SUBC_QUERY_ISM 0
> +
> +#define DIAG_320_RC_OK 0x0001
> +
> +#endif
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index cff9fbc4b0..d33c5daf38 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -18,6 +18,7 @@
> #include "hw/watchdog/wdt_diag288.h"
> #include "system/cpus.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"
> @@ -191,3 +192,43 @@ out:
> break;
> }
> }
> +
> +void handle_diag_320(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t
> ra)
> +{
> + S390CPU *cpu = env_archcpu(env);
> + uint64_t subcode = env->regs[r3];
> + uint64_t addr = env->regs[r1];
> + int rc;
Even though "rc" likely means "response code" in the context of DIAG, it
screams "return code" in a void function, which looks a little odd. I'd
remove this variable and simply set `env->regs[r1 + 1] = DIAG_320_RC_*`
instead, similar to how DIAG 308 handles this.
> +
> + if (env->psw.mask & PSW_MASK_PSTATE) {
> + s390_program_interrupt(env, PGM_PRIVILEGED, ra);
> + return;
> + }
> +
> + if (!s390_has_feat(S390_FEAT_DIAG_320)) {
> + s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> + return;
> + }
> +
> + if (r1 & 1) {
> + s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> + return;
> + }
> +
> + switch (subcode) {
In patch 4, you introduce the feature bit for DIAG 320. It is
documented that subcodes 0-3 are only supported if this feature bit is
on. Please add a check for this feature. Subcode 0 (and later 1 and 2)
should not be handled if this feature bit is not present.
> + case DIAG_320_SUBC_QUERY_ISM:
> + uint64_t ism = 0;
Technically, this subcode is suppose to write to an 8-word installed
subcodes block (ISB). Though I can't imagine that we'll grow beyond
even the first word for quite some time. I guess it's up to the caller
to provide a proper ISB anyway.
I'd suggest the following:
change from `uint64_t` to `uint32_t ism_word0`
Add a comment:
/*
* The Installed Subcode Block (ISB) can be up 8 words in size,
* but the current set of subcodes can fit within a single word
* for now.
*/
> +
> + 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;
env->regs[r1 + 1] = DIAG_320_RC_OK;
> + break;
> + default:
> + s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> + return;
As specified in the documentation, "response code 0x0102 indicates that
the subcode is reserved or not supported on this model". There is no
program specification when an incorrect subcode has been provided.
> + }
> + env->regs[r1 + 1] = rc;
Remove this line.
> +}
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 8f655a4b7f..d5b3694600 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -98,6 +98,7 @@
> #define DIAG_TIMEREVENT 0x288
> #define DIAG_IPL 0x308
> #define DIAG_SET_CONTROL_PROGRAM_CODES 0x318
> +#define DIAG_CERT_STORE 0x320
> #define DIAG_KVM_HYPERCALL 0x500
> #define DIAG_KVM_BREAKPOINT 0x501
>
> @@ -1560,6 +1561,16 @@ static void handle_diag_318(S390CPU *cpu, struct
> kvm_run *run)
> }
> }
>
> +static void kvm_handle_diag_320(S390CPU *cpu, struct kvm_run *run)
> +{
> + uint64_t r1, r3;
> +
> + r1 = (run->s390_sieic.ipa & 0x00f0) >> 4;
> + r3 = run->s390_sieic.ipa & 0x000f;
> +
> + handle_diag_320(&cpu->env, r1, r3, RA_IGNORED);
> +}
> +
> #define DIAG_KVM_CODE_MASK 0x000000000000ffff
>
> static int handle_diag(S390CPU *cpu, struct kvm_run *run, uint32_t ipb)
> @@ -1590,6 +1601,9 @@ static int handle_diag(S390CPU *cpu, struct kvm_run
> *run, uint32_t ipb)
> case DIAG_KVM_BREAKPOINT:
> r = handle_sw_breakpoint(cpu, run);
> break;
> + case DIAG_CERT_STORE:
> + kvm_handle_diag_320(cpu, run);
> + break;
> default:
> trace_kvm_insn_diag(func_code);
> kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
> diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
> index a4ba6227ab..86a652f833 100644
> --- a/target/s390x/s390x-internal.h
> +++ b/target/s390x/s390x-internal.h
> @@ -400,6 +400,8 @@ int mmu_translate_real(CPUS390XState *env, target_ulong
> raddr, int rw,
> int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3);
> void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3,
> uintptr_t ra);
> +void handle_diag_320(CPUS390XState *env, uint64_t r1, uint64_t r3,
> + uintptr_t ra);
>
>
> /* translate.c */
> diff --git a/target/s390x/tcg/misc_helper.c b/target/s390x/tcg/misc_helper.c
> index f7101be574..412c34ed93 100644
> --- a/target/s390x/tcg/misc_helper.c
> +++ b/target/s390x/tcg/misc_helper.c
> @@ -142,6 +142,13 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1,
> uint32_t r3, uint32_t num)
> /* time bomb (watchdog) */
> r = handle_diag_288(env, r1, r3);
> break;
> + case 0x320:
> + /* cert store */
> + bql_lock();
> + handle_diag_320(env, r1, r3, GETPC());
> + bql_unlock();
> + r = 0;
> + break;
> default:
> r = -1;
> break;
--
Regards,
Collin
