diff mbox series

[v3,03/26] target/arm/kvm: Return immediately on error in kvm_arch_init()

Message ID 20241125195626.856992-5-jean-philippe@linaro.org
State New
Headers show
Series arm: Run Arm CCA VMs with KVM | expand

Commit Message

Jean-Philippe Brucker Nov. 25, 2024, 7:56 p.m. UTC 
Returning an error to kvm_init() is fatal anyway, no need to continue
the initialization.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 target/arm/kvm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Philippe Mathieu-Daudé Dec. 5, 2024, 9:47 p.m. UTC | #1 
Hi Jean-Philippe,

On 25/11/24 20:56, Jean-Philippe Brucker wrote:
> Returning an error to kvm_init() is fatal anyway, no need to continue
> the initialization.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>   target/arm/kvm.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 8bdf4abeb6..95bcecf804 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -563,7 +563,7 @@ int kvm_arch_get_default_type(MachineState *ms)
>   
>   int kvm_arch_init(MachineState *ms, KVMState *s)
>   {
> -    int ret = 0;
> +    int ret;

With your change we can reduce this variable scope ...

>       /* For ARM interrupt delivery is always asynchronous,
>        * whether we are using an in-kernel VGIC or not.
>        */
> @@ -585,7 +585,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>           !kvm_check_extension(s, KVM_CAP_ARM_IRQ_LINE_LAYOUT_2)) {
>           error_report("Using more than 256 vcpus requires a host kernel "
>                        "with KVM_CAP_ARM_IRQ_LINE_LAYOUT_2");
> -        ret = -EINVAL;
> +        return -EINVAL;
>       }
>   
>       if (kvm_check_extension(s, KVM_CAP_ARM_NISV_TO_USER)) {
> @@ -607,13 +607,14 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>               warn_report("Eager Page Split support not available");
>           } else if (!(s->kvm_eager_split_size & sizes)) {
>               error_report("Eager Page Split requested chunk size not valid");
> -            ret = -EINVAL;
> +            return -EINVAL;
>           } else {
>               ret = kvm_vm_enable_cap(s, KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE, 0,
>                                       s->kvm_eager_split_size);

... by declaring it here.

Otherwise:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>               if (ret < 0) {
>                   error_report("Enabling of Eager Page Split failed: %s",
>                                strerror(-ret));
> +                return ret;
>               }
>           }
>       }
> @@ -626,7 +627,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>       hw_breakpoints = g_array_sized_new(true, true,
>                                          sizeof(HWBreakpoint), max_hw_bps);
>   
> -    return ret;
> +    return 0;
>   }
>   
>   unsigned long kvm_arch_vcpu_id(CPUState *cpu)
Jean-Philippe Brucker Dec. 10, 2024, 7:06 p.m. UTC | #2 
On Thu, Dec 05, 2024 at 10:47:13PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Jean-Philippe,
> 
> On 25/11/24 20:56, Jean-Philippe Brucker wrote:
> > Returning an error to kvm_init() is fatal anyway, no need to continue
> > the initialization.
> > 
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> >   target/arm/kvm.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> > index 8bdf4abeb6..95bcecf804 100644
> > --- a/target/arm/kvm.c
> > +++ b/target/arm/kvm.c
> > @@ -563,7 +563,7 @@ int kvm_arch_get_default_type(MachineState *ms)
> >   int kvm_arch_init(MachineState *ms, KVMState *s)
> >   {
> > -    int ret = 0;
> > +    int ret;
> 
> With your change we can reduce this variable scope ...
> 
> >       /* For ARM interrupt delivery is always asynchronous,
> >        * whether we are using an in-kernel VGIC or not.
> >        */
> > @@ -585,7 +585,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >           !kvm_check_extension(s, KVM_CAP_ARM_IRQ_LINE_LAYOUT_2)) {
> >           error_report("Using more than 256 vcpus requires a host kernel "
> >                        "with KVM_CAP_ARM_IRQ_LINE_LAYOUT_2");
> > -        ret = -EINVAL;
> > +        return -EINVAL;
> >       }
> >       if (kvm_check_extension(s, KVM_CAP_ARM_NISV_TO_USER)) {
> > @@ -607,13 +607,14 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >               warn_report("Eager Page Split support not available");
> >           } else if (!(s->kvm_eager_split_size & sizes)) {
> >               error_report("Eager Page Split requested chunk size not valid");
> > -            ret = -EINVAL;
> > +            return -EINVAL;
> >           } else {
> >               ret = kvm_vm_enable_cap(s, KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE, 0,
> >                                       s->kvm_eager_split_size);
> 
> ... by declaring it here.

Ah right, but next patch immediately reuses ret:

@@ -627,7 +627,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
     hw_breakpoints = g_array_sized_new(true, true,
                                        sizeof(HWBreakpoint), max_hw_bps);

-    return 0;
+    ret = kvm_arm_rme_init(ms);
+    if (ret) {
+        error_report("Failed to enable RME: %s", strerror(-ret));
+    }
+
+    return ret;
 }


> 
> Otherwise:
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thank you!

> 
> >               if (ret < 0) {
> >                   error_report("Enabling of Eager Page Split failed: %s",
> >                                strerror(-ret));
> > +                return ret;
> >               }
> >           }
> >       }
> > @@ -626,7 +627,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >       hw_breakpoints = g_array_sized_new(true, true,
> >                                          sizeof(HWBreakpoint), max_hw_bps);
> > -    return ret;
> > +    return 0;
> >   }
> >   unsigned long kvm_arch_vcpu_id(CPUState *cpu)
>
diff mbox series

Patch

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 8bdf4abeb6..95bcecf804 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -563,7 +563,7 @@  int kvm_arch_get_default_type(MachineState *ms)
 
 int kvm_arch_init(MachineState *ms, KVMState *s)
 {
-    int ret = 0;
+    int ret;
     /* For ARM interrupt delivery is always asynchronous,
      * whether we are using an in-kernel VGIC or not.
      */
@@ -585,7 +585,7 @@  int kvm_arch_init(MachineState *ms, KVMState *s)
         !kvm_check_extension(s, KVM_CAP_ARM_IRQ_LINE_LAYOUT_2)) {
         error_report("Using more than 256 vcpus requires a host kernel "
                      "with KVM_CAP_ARM_IRQ_LINE_LAYOUT_2");
-        ret = -EINVAL;
+        return -EINVAL;
     }
 
     if (kvm_check_extension(s, KVM_CAP_ARM_NISV_TO_USER)) {
@@ -607,13 +607,14 @@  int kvm_arch_init(MachineState *ms, KVMState *s)
             warn_report("Eager Page Split support not available");
         } else if (!(s->kvm_eager_split_size & sizes)) {
             error_report("Eager Page Split requested chunk size not valid");
-            ret = -EINVAL;
+            return -EINVAL;
         } else {
             ret = kvm_vm_enable_cap(s, KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE, 0,
                                     s->kvm_eager_split_size);
             if (ret < 0) {
                 error_report("Enabling of Eager Page Split failed: %s",
                              strerror(-ret));
+                return ret;
             }
         }
     }
@@ -626,7 +627,7 @@  int kvm_arch_init(MachineState *ms, KVMState *s)
     hw_breakpoints = g_array_sized_new(true, true,
                                        sizeof(HWBreakpoint), max_hw_bps);
 
-    return ret;
+    return 0;
 }
 
 unsigned long kvm_arch_vcpu_id(CPUState *cpu)