diff mbox series

[v2,2/5] hw/s390x/pv: Un-inline s390_pv_init()

Message ID 20221217152454.96388-3-philmd@linaro.org
State New
Headers show
Series target/s390x: Header cleanups around "cpu.h" | expand

Commit Message

Philippe Mathieu-Daudé Dec. 17, 2022, 3:24 p.m. UTC 
There is no point in having s390_pv_init() inlined. Directly
call s390_pv_kvm_init() guarded by kvm_enabled() so the compiler
can elide when CONFIG_KVM is not set.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/s390x/pv.c              |  4 +++-
 hw/s390x/s390-virtio-ccw.c |  6 ++++--
 include/hw/s390x/pv.h      | 13 -------------
 3 files changed, 7 insertions(+), 16 deletions(-)

Comments

Richard Henderson Dec. 17, 2022, 5:37 p.m. UTC | #1 
On 12/17/22 07:24, Philippe Mathieu-Daudé wrote:
> @@ -251,7 +251,9 @@ struct S390PVGuestClass {
>   
>   int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>   {
> -    if (!object_dynamic_cast(OBJECT(cgs), TYPE_S390_PV_GUEST)) {
> +    assert(kvm_enabled());
> +
> +    if (!cgs || !object_dynamic_cast(OBJECT(cgs), TYPE_S390_PV_GUEST)) {
>           return 0;
>       }
>   
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 2e64ffab45..d9a96e315e 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -255,8 +255,10 @@ static void ccw_init(MachineState *machine)
>       /* init CPUs (incl. CPU model) early so s390_has_feature() works */
>       s390_init_cpus(machine);
>   
> -    /* Need CPU model to be determined before we can set up PV */
> -    s390_pv_init(machine->cgs, &error_fatal);
> +    if (kvm_enabled()) {
> +        /* Need CPU model to be determined before we can set up PV */
> +        s390_pv_kvm_init(machine->cgs, &error_fatal);
> +    }
>   
>       s390_flic_init();...
> -static inline int s390_pv_init(ConfidentialGuestSupport *cgs, Error **errp)
> -{
> -    if (!cgs) {
> -        return 0;
> -    }
> -    if (kvm_enabled()) {
> -        return s390_pv_kvm_init(cgs, errp);
> -    }
> -
> -    error_setg(errp, "Protected Virtualization requires KVM");
> -    return -1;
> -}

You've lost the error path above.  And it seems like we could just handle null cgs early. 
E.g.

     if (machine->cgs) {
         if (kvm_enabled()) {
             s390_pv_kvm_init(machine->cgs, &error_fatal);
         } else {
             error_report(...);
             exit(EXIT_FAILURE);
         }
     }

(since qabi/error.h says not to use error_setg(&error_fatal, ...)).


r~
Thomas Huth Dec. 29, 2022, 9:23 a.m. UTC | #2 
On 17/12/2022 16.24, Philippe Mathieu-Daudé wrote:
> There is no point in having s390_pv_init() inlined. Directly
> call s390_pv_kvm_init() guarded by kvm_enabled() so the compiler
> can elide when CONFIG_KVM is not set.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/s390x/pv.c              |  4 +++-
>   hw/s390x/s390-virtio-ccw.c |  6 ++++--
>   include/hw/s390x/pv.h      | 13 -------------
>   3 files changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> index 8dfe92d8df..17c658402d 100644
> --- a/hw/s390x/pv.c
> +++ b/hw/s390x/pv.c
> @@ -251,7 +251,9 @@ struct S390PVGuestClass {
>   
>   int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>   {
> -    if (!object_dynamic_cast(OBJECT(cgs), TYPE_S390_PV_GUEST)) {
> +    assert(kvm_enabled());
> +
> +    if (!cgs || !object_dynamic_cast(OBJECT(cgs), TYPE_S390_PV_GUEST)) {
>           return 0;
>       }
>   
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 2e64ffab45..d9a96e315e 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -255,8 +255,10 @@ static void ccw_init(MachineState *machine)
>       /* init CPUs (incl. CPU model) early so s390_has_feature() works */
>       s390_init_cpus(machine);
>   
> -    /* Need CPU model to be determined before we can set up PV */
> -    s390_pv_init(machine->cgs, &error_fatal);
> +    if (kvm_enabled()) {
> +        /* Need CPU model to be determined before we can set up PV */
> +        s390_pv_kvm_init(machine->cgs, &error_fatal);
> +    }

This now slightly changed the order of the checks ... if cgs is set, but kvm 
is disabled, there is now no error message anymore and the code continues 
silently. That sounds wrong?

  Thomas


>       s390_flic_init();
>   
> diff --git a/include/hw/s390x/pv.h b/include/hw/s390x/pv.h
> index 9360aa1091..fad61cc6c6 100644
> --- a/include/hw/s390x/pv.h
> +++ b/include/hw/s390x/pv.h
> @@ -12,7 +12,6 @@
>   #ifndef HW_S390_PV_H
>   #define HW_S390_PV_H
>   
> -#include "qapi/error.h"
>   #include "sysemu/kvm.h"
>   
>   #ifdef CONFIG_KVM
> @@ -78,17 +77,5 @@ static inline int kvm_s390_dump_completion_data(void *buff) { return 0; }
>   #endif /* CONFIG_KVM */
>   
>   int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
> -static inline int s390_pv_init(ConfidentialGuestSupport *cgs, Error **errp)
> -{
> -    if (!cgs) {
> -        return 0;
> -    }
> -    if (kvm_enabled()) {
> -        return s390_pv_kvm_init(cgs, errp);
> -    }
> -
> -    error_setg(errp, "Protected Virtualization requires KVM");
> -    return -1;
> -}
>   
>   #endif /* HW_S390_PV_H */
diff mbox series

Patch

diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
index 8dfe92d8df..17c658402d 100644
--- a/hw/s390x/pv.c
+++ b/hw/s390x/pv.c
@@ -251,7 +251,9 @@  struct S390PVGuestClass {
 
 int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
 {
-    if (!object_dynamic_cast(OBJECT(cgs), TYPE_S390_PV_GUEST)) {
+    assert(kvm_enabled());
+
+    if (!cgs || !object_dynamic_cast(OBJECT(cgs), TYPE_S390_PV_GUEST)) {
         return 0;
     }
 
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 2e64ffab45..d9a96e315e 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -255,8 +255,10 @@  static void ccw_init(MachineState *machine)
     /* init CPUs (incl. CPU model) early so s390_has_feature() works */
     s390_init_cpus(machine);
 
-    /* Need CPU model to be determined before we can set up PV */
-    s390_pv_init(machine->cgs, &error_fatal);
+    if (kvm_enabled()) {
+        /* Need CPU model to be determined before we can set up PV */
+        s390_pv_kvm_init(machine->cgs, &error_fatal);
+    }
 
     s390_flic_init();
 
diff --git a/include/hw/s390x/pv.h b/include/hw/s390x/pv.h
index 9360aa1091..fad61cc6c6 100644
--- a/include/hw/s390x/pv.h
+++ b/include/hw/s390x/pv.h
@@ -12,7 +12,6 @@ 
 #ifndef HW_S390_PV_H
 #define HW_S390_PV_H
 
-#include "qapi/error.h"
 #include "sysemu/kvm.h"
 
 #ifdef CONFIG_KVM
@@ -78,17 +77,5 @@  static inline int kvm_s390_dump_completion_data(void *buff) { return 0; }
 #endif /* CONFIG_KVM */
 
 int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error **errp);
-static inline int s390_pv_init(ConfidentialGuestSupport *cgs, Error **errp)
-{
-    if (!cgs) {
-        return 0;
-    }
-    if (kvm_enabled()) {
-        return s390_pv_kvm_init(cgs, errp);
-    }
-
-    error_setg(errp, "Protected Virtualization requires KVM");
-    return -1;
-}
 
 #endif /* HW_S390_PV_H */