Re: [PATCH v2 03/25] s390x: Guest support for Certificate Store Facility (CS)

[Thomas Huth <thuth@redhat.com>]

On 09/05/2025 00.50, Zhuoying Cai wrote:
> DIAG 320 is supported when the certificate-store (CS) facility
> is installed.
> 
> Availability of CS facility is determined by byte 134 bit 5 of the
> SCLP Read Info block. Byte 134's facilities cannot be represented
> without the availability of the extended-length-SCCB, so add it as
> a check for consistency.
> 
> Note: secure IPL is not available for Secure Execution (SE) guests,
> as their images are already integrity protected, and an additional
> protection of the kernel by secure IPL is not necessary.
> 
> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
> ---
>   target/s390x/cpu_features.c         | 1 +
>   target/s390x/cpu_features_def.h.inc | 1 +
>   target/s390x/cpu_models.c           | 2 ++
>   target/s390x/gen-features.c         | 1 +
>   target/s390x/kvm/kvm.c              | 2 ++
>   5 files changed, 7 insertions(+)
> 
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index 4b5be6798e..99089ab3f5 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -147,6 +147,7 @@ void s390_fill_feat_block(const S390FeatBitmap features, S390FeatType type,
>           break;
>       case S390_FEAT_TYPE_SCLP_FAC134:
>           clear_be_bit(s390_feat_def(S390_FEAT_DIAG_318)->bit, data);
> +        clear_be_bit(s390_feat_def(S390_FEAT_DIAG_320)->bit, data);
>           break;
>       default:
>           return;
> diff --git a/target/s390x/cpu_features_def.h.inc b/target/s390x/cpu_features_def.h.inc
> index e23e603a79..65d38f546d 100644
> --- a/target/s390x/cpu_features_def.h.inc
> +++ b/target/s390x/cpu_features_def.h.inc
> @@ -138,6 +138,7 @@ DEF_FEAT(SIE_IBS, "ibs", SCLP_CONF_CHAR_EXT, 10, "SIE: Interlock-and-broadcast-s
>   
>   /* Features exposed via SCLP SCCB Facilities byte 134 (bit numbers relative to byte-134) */
>   DEF_FEAT(DIAG_318, "diag318", SCLP_FAC134, 0, "Control program name and version codes")
> +DEF_FEAT(DIAG_320, "diag320", SCLP_FAC134, 5, "Provide Certificate Store functions")

Not sure, but "diag320" does not sound like a very good name for something 
related to security that is used in the interface for the users. Maybe 
"cert" or "cstore" would be a better name?

...
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 41840677ce..3d9fbe62ea 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -720,6 +720,7 @@ static uint16_t full_GEN16_GA1[] = {
>       S390_FEAT_PAIE,
>       S390_FEAT_UV_FEAT_AP,
>       S390_FEAT_UV_FEAT_AP_INTR,
> +    S390_FEAT_DIAG_320,
>   };
>   
>   static uint16_t full_GEN17_GA1[] = {
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index b9f1422197..6bad1713d2 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -2487,6 +2487,8 @@ bool kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>           set_bit(S390_FEAT_DIAG_318, model->features);
>       }
>   
> +    set_bit(S390_FEAT_DIAG_320, model->features);
> +
>       /* Test for Ultravisor features that influence secure guest behavior */
>       query_uv_feat_guest(model->features);

Question: Could this feature be enabled of the TCG max CPU, too? If so, it 
might be a good idea to do so, so it could be used in CI tests more easily.

By the way, any chance that you could also add a functional test to this 
patch series, so that we can be sure that there are no regressions in the 
future?

  Thomas