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

[Jean-Philippe Brucker]

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-phili...@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é <phi...@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)
>