diff mbox series

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

Message ID 20230104115111.3240594-5-clg@kaod.org (mailing list archive)
State New, archived
Headers show
Series s390x/pv: Improve protected VM support | expand

Commit Message

Cédric Le Goater Jan. 4, 2023, 11:51 a.m. UTC 
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(+)

Comments

Thomas Huth Jan. 5, 2023, 12:33 p.m. UTC | #1 
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:
Claudio Imbrenda Jan. 5, 2023, 2:24 p.m. UTC | #2 
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:
diff mbox series

Patch

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;
+        }
+
         s390_ipl_reset_request(cs, S390_RESET_PV);
         break;
     default: