On Mon, 2025-07-21 at 17:26 -0400, Collin Walling wrote:
> 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.
Yes to this, but subcode is limited to one byte so we still need to do a spec
exception if r3 is
greater than 255 (you can do it before the switch, like is done to r1).
>
> > + }
> > + 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;
>
