Re: [PATCH 4/5] s390x/pv: Introduce a s390_pv_check() helper for runtime

[Thomas Huth]

On 04/01/2023 12.51, Cédric Le Goater wrote:
> From: Cédric Le Goater <clg@redhat.com>
>
> If a secure kernel is started in a non-protected VM, the OS will hang
> during boot without giving a proper error message to the user.
>
> Perform the checks on Confidential Guest support at runtime with an
> helper called from the service call switching the guest to protected
> mode.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   include/hw/s390x/pv.h |  2 ++
>   hw/s390x/pv.c         | 14 ++++++++++++++
>   target/s390x/diag.c   |  7 +++++++
>   3 files changed, 23 insertions(+)
>
> diff --git a/include/hw/s390x/pv.h b/include/hw/s390x/pv.h
> index 9360aa1091..ca7dac2e20 100644
> --- a/include/hw/s390x/pv.h
> +++ b/include/hw/s390x/pv.h
> @@ -55,6 +55,7 @@ int kvm_s390_dump_init(void);
>   int kvm_s390_dump_cpu(S390CPU *cpu, void *buff);
>   int kvm_s390_dump_mem_state(uint64_t addr, size_t len, void *dest);
>   int kvm_s390_dump_completion_data(void *buff);
> +bool s390_pv_check(Error **errp);
>   #else /* CONFIG_KVM */
>   static inline bool s390_is_pv(void) { return false; }
>   static inline int s390_pv_query_info(void) { return 0; }
> @@ -75,6 +76,7 @@ static inline int kvm_s390_dump_cpu(S390CPU *cpu, void *buff) 
> { return 0; }
>   static inline int kvm_s390_dump_mem_state(uint64_t addr, size_t len,
>                                             void *dest) { return 0; }
>   static inline int kvm_s390_dump_completion_data(void *buff) { return 0; }
> +static inline bool s390_pv_check(Error **errp) { return false; }
>   #endif /* CONFIG_KVM */
>   
>   int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> index 8d0d3f4adc..96c0728ec9 100644
> --- a/hw/s390x/pv.c
> +++ b/hw/s390x/pv.c
> @@ -307,6 +307,20 @@ static bool s390_pv_guest_check(const Object *obj, Error 
> **errp)
>       return s390_pv_check_cpus(errp) && s390_pv_check_host(errp);
>   }
>   
> +bool s390_pv_check(Error **errp)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    Object *obj = OBJECT(ms->cgs);
> +
> +    if (!obj) {
> +        error_setg(errp, "Protected VM started without a Confidential"
> +                   " Guest support interface");
> +        return false;
> +    }
> +
> +    return s390_pv_guest_check(obj, errp);
> +}
> +
>   OBJECT_DEFINE_TYPE_WITH_INTERFACES(S390PVGuest,
>                                      s390_pv_guest,
>                                      S390_PV_GUEST,
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index 76b01dcd68..9b16e25930 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -79,6 +79,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, 
> uint64_t r3, uintptr_t ra)
>       uint64_t addr =  env->regs[r1];
>       uint64_t subcode = env->regs[r3];
>       IplParameterBlock *iplb;
> +    Error *local_err = NULL;
>   
>       if (env->psw.mask & PSW_MASK_PSTATE) {
>           s390_program_interrupt(env, PGM_PRIVILEGED, ra);
> @@ -176,6 +177,12 @@ out:
>               return;
>           }
>   
> +        if (!s390_pv_check(&local_err)) {
> +            error_report_err(local_err);
> +            env->regs[r1 + 1] = DIAG_308_RC_INVAL_FOR_PV;

I hope someone from IBM can double-check whether that return code is fine in 
this case here.

If so, the patch looks fine to me.

 Thomas


> +            return;
> +        }
> +
>           s390_ipl_reset_request(cs, S390_RESET_PV);
>           break;
>       default: