Re: [Xen-devel] [PATCH v7 01/10] x86/hypervisor: make hypervisor_ap_setup return an error code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.02.2020 16:36, Wei Liu wrote:
> @@ -215,18 +220,19 @@ static void init_evtchn(void)
>      rc = xen_hypercall_set_evtchn_upcall_vector(this_cpu(vcpu_id),
>                                                  evtchn_upcall_vector);
>      if ( rc )
> -        panic("Unable to set evtchn upcall vector: %d\n", rc);
> +    {
> +        printk("Unable to set evtchn upcall vector: %d\n", rc);
> +        goto out;

There's no need for "goto" here - "return rc" is all you need
instead. As stated elsewhere, when there's complex cleanup or
a fair risk of leaving out an important cleanup step, I can
live with "goto" getting used. But I don't think it should be
used to replace a simple "return".

With this
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one more (optional!) suggestion and one more remark:

> @@ -254,14 +260,20 @@ static void __init setup(void)
>                 XEN_LEGACY_MAX_VCPUS);
>      }
>  
> -    init_evtchn();
> +    BUG_ON(init_evtchn());
>  }
>  
> -static void ap_setup(void)
> +static int ap_setup(void)
>  {
> +    int rc;
> +
>      set_vcpu_id();
> -    map_vcpuinfo();
> -    init_evtchn();
> +
> +    rc = map_vcpuinfo();
> +    if ( rc )
> +        return rc;
> +
> +    return init_evtchn();
>  }

To avoid a local variable, how about

    return map_vcpuinfo() ?: init_evtchn();

?

> @@ -283,8 +295,8 @@ int xg_free_unused_page(mfn_t mfn)
>  
>  static void ap_resume(void *unused)
>  {
> -    map_vcpuinfo();
> -    init_evtchn();
> +    BUG_ON(map_vcpuinfo());
> +    BUG_ON(init_evtchn());
>  }

Current code structure calls for this, but in principle I don't
think AP failure on resume should be any different from AP
failure during boot. Nothing to be address here and now, of
course.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel