On 7/11/25 5:10 PM, Zhuoying Cai wrote:
> Introduce Secure-IPL (SIPL) facility.
>
> Use fac_ipl to represent bytes 136 and 137 for IPL device facilities
> of the SCLP Read Info block.
>
> Availability of SIPL facility is determined by byte 136 bit 1 of the
> SCLP Read Info block. Byte 136's facilities cannot be represented
> without the availability of the extended-length-SCCB, so add it as a
> check for consistency.
>
> When SIPL facility is installed, the IPL Parameter Block length must
> contains value that is multiple of 8 bytes.
This has no relevance to the code below.
Add a comment that Secure IPL is not available for guests under
protected virtualization.
>
> Signed-off-by: Zhuoying Cai <zy...@linux.ibm.com>
I have a few nits below to about some code comments, but the code itself
looks functionally sound.
Reviewed-by: Collin Walling <wall...@linux.ibm.com>
@David, would you be able to review wrt adding this to the qemu_max model?
@Christian, @Hendrik, can either of you review wrt that it looks good to
add this feature starting with the gen16a model?
> ---
> hw/s390x/sclp.c | 2 ++
> include/hw/s390x/sclp.h | 4 +++-
> target/s390x/cpu_features.c | 3 +++
> target/s390x/cpu_features.h | 1 +
> target/s390x/cpu_features_def.h.inc | 3 +++
> target/s390x/cpu_models.c | 2 ++
> target/s390x/gen-features.c | 2 ++
> target/s390x/kvm/kvm.c | 3 +++
> 8 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 9718564fa4..69d3328a3d 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -145,6 +145,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
> if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
> s390_get_feat_block(S390_FEAT_TYPE_SCLP_FAC134,
> &read_info->fac134);
> + s390_get_feat_block(S390_FEAT_TYPE_SCLP_FAC_IPL,
> + read_info->fac_ipl);
> }
>
> read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index d32f6180e0..bfd330c340 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -136,7 +136,9 @@ typedef struct ReadInfo {
> uint32_t hmfai;
> uint8_t _reserved7[134 - 128]; /* 128-133 */
> uint8_t fac134;
> - uint8_t _reserved8[144 - 135]; /* 135-143 */
> + uint8_t _reserved8;
> + uint8_t fac_ipl[2]; /* 136-137 */
> + uint8_t _reserved9[144 - 137]; /* 138-143 */
> struct CPUEntry entries[];
> /*
> * When the Extended-Length SCCB (ELS) feature is enabled the
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index 99089ab3f5..3f3d6a80af 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -149,6 +149,9 @@ void s390_fill_feat_block(const S390FeatBitmap features,
> S390FeatType type,
> 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;
> + case S390_FEAT_TYPE_SCLP_FAC_IPL:
> + clear_be_bit(s390_feat_def(S390_FEAT_SIPL)->bit, data);
> + break;
There's a comment above this switch case that lists features which are
not supported under PV. Please add a bullet for "Secure IPL
facilities". This single mention should be sufficient to cover SCLAF as
well.
> default:
> return;
> }
> diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
> index 5635839d03..b038198555 100644
> --- a/target/s390x/cpu_features.h
> +++ b/target/s390x/cpu_features.h
> @@ -24,6 +24,7 @@ typedef enum {
> S390_FEAT_TYPE_SCLP_CONF_CHAR,
> S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
> S390_FEAT_TYPE_SCLP_FAC134,
> + S390_FEAT_TYPE_SCLP_FAC_IPL,
> S390_FEAT_TYPE_SCLP_CPU,
> S390_FEAT_TYPE_MISC,
> S390_FEAT_TYPE_PLO,
> diff --git a/target/s390x/cpu_features_def.h.inc
> b/target/s390x/cpu_features_def.h.inc
> index 7b13a95d98..956bd8a123 100644
> --- a/target/s390x/cpu_features_def.h.inc
> +++ b/target/s390x/cpu_features_def.h.inc
> @@ -140,6 +140,9 @@ DEF_FEAT(SIE_IBS, "ibs", SCLP_CONF_CHAR_EXT, 10, "SIE:
> Interlock-and-broadcast-s
> DEF_FEAT(DIAG_318, "diag318", SCLP_FAC134, 0, "Control program name and
> version codes")
> DEF_FEAT(DIAG_320, "cstore", SCLP_FAC134, 5, "Provide Certificate Store
> functions")
>
> +/* Features exposed via SCLP SCCB Facilities byte 136 - 137 (bit numbers
> relative to byte-136) */
> +DEF_FEAT(SIPL, "sipl", SCLP_FAC_IPL, 1, "Secure-IPL facility")
> +
> /* Features exposed via SCLP CPU info. */
> DEF_FEAT(SIE_F2, "sief2", SCLP_CPU, 4, "SIE: interception format 2 (Virtual
> SIE)")
> DEF_FEAT(SIE_SKEY, "skey", SCLP_CPU, 5, "SIE: Storage-key facility")
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 33ef5c190c..ab46204d9e 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -263,6 +263,7 @@ bool s390_has_feat(S390Feat feat)
> case S390_FEAT_SIE_CMMA:
> case S390_FEAT_SIE_PFMFI:
> case S390_FEAT_SIE_IBS:
> + case S390_FEAT_SIPL:
> case S390_FEAT_CONFIGURATION_TOPOLOGY:
> return false;
> break;
> @@ -507,6 +508,7 @@ static void check_consistency(const S390CPUModel *model)
> { S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL, S390_FEAT_AP },
> { S390_FEAT_DIAG_318, S390_FEAT_EXTENDED_LENGTH_SCCB },
> { S390_FEAT_DIAG_320, S390_FEAT_EXTENDED_LENGTH_SCCB },
> + { S390_FEAT_SIPL, S390_FEAT_EXTENDED_LENGTH_SCCB },
> { S390_FEAT_NNPA, S390_FEAT_VECTOR },
> { S390_FEAT_RDP, S390_FEAT_LOCAL_TLB_CLEARING },
> { S390_FEAT_UV_FEAT_AP, S390_FEAT_AP },
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 86486def23..6ee9bad4c6 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -721,6 +721,7 @@ static uint16_t full_GEN16_GA1[] = {
> S390_FEAT_UV_FEAT_AP,
> S390_FEAT_UV_FEAT_AP_INTR,
> S390_FEAT_DIAG_320,
> + S390_FEAT_SIPL,
> };
>
> static uint16_t full_GEN17_GA1[] = {
> @@ -922,6 +923,7 @@ static uint16_t qemu_MAX[] = {
> S390_FEAT_PRNO_TRNG,
> S390_FEAT_EXTENDED_LENGTH_SCCB,
> S390_FEAT_DIAG_320,
> + S390_FEAT_SIPL,
> };
>
> /****** END FEATURE DEFS ******/
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 840330709b..fc9cad32a1 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -2520,6 +2520,9 @@ bool kvm_s390_get_host_cpu_model(S390CPUModel *model,
> Error **errp)
>
> set_bit(S390_FEAT_DIAG_320, model->features);
>
> + /* Secure-IPL facility is handled entirely within QEMU */
> + set_bit(S390_FEAT_SIPL, model->features);
> +
nit: change this comment to "Some Secure IPL facilities are emulated by
QEMU"
When you introduce SCLAF, you could add that line-of-code directly below
this one since it's in the secure IPL family.
> /* Test for Ultravisor features that influence secure guest behavior */
> query_uv_feat_guest(model->features);
>
--
Regards,
Collin
