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

[Thomas Huth]

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 */