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

[Claudio Imbrenda]

On Wed,  4 Jan 2023 12:51:10 +0100
Cédric Le Goater <clg@kaod.org> 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;
> +            return;
> +        }
> +

in general yes

however I have noticed that we don't always return a PGM_SPECIFICATION
when PV is not available (we currently do it only for DIAG308_PV_SET). I
think we should return the PGM_SPECIFICATION for all PV subcodes when
the feature is not present (but this is a separate issue)

let me add Janosch in CC since he wrote that code

>          s390_ipl_reset_request(cs, S390_RESET_PV);
>          break;
>      default: