Re: [PATCH v4 05/28] s390x/diag: Introduce DIAG 320 for certificate store facility

[Eric Farman <farman@linux.ibm.com>]

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 <zycai@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 <zycai@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;
>