Re: [PATCH v2] mini-os: Use a single start_info_ptr variable

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

Costin Lupu, le mer. 19 août 2020 18:45:36 +0300, a ecrit:
> The second definition of the `start_info_ptr` variable was introduced by 
> commit
> e05eb0 which was part of a series trying to add suspend/resume support to
> mini-os. This patch removes the second definition by reverting some changes of
> the mentioned commit and of commit 892b66.
> 
> However the logic in shutdown.c is still left in an inconsistent state because
> it still doesn't work for ARM. The solution should be part of a future patch.
> 
> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>

Reviewed-by: Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>

> ---
>  arch/x86/setup.c   | 4 ++--
>  include/kernel.h   | 2 +-
>  include/shutdown.h | 2 +-
>  kernel.c           | 4 ++--
>  shutdown.c         | 8 ++++----
>  5 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/setup.c b/arch/x86/setup.c
> index 4fd8e39..64b22c5 100644
> --- a/arch/x86/setup.c
> +++ b/arch/x86/setup.c
> @@ -45,7 +45,7 @@ union start_info_union start_info_union;
>  /*
>   * This pointer holds a reference to the copy of the start_info struct.
>   */
> -static start_info_t *start_info_ptr;
> +start_info_t *start_info_ptr;
>  
>  /*
>   * Shared page for communicating with the hypervisor.
> @@ -221,7 +221,7 @@ arch_init(void *par)
>  #endif
>       start_info_ptr = (start_info_t *)par;
>  
> -     start_kernel((start_info_t *)par);
> +     start_kernel();
>  }
>  
>  void arch_pre_suspend(void)
> diff --git a/include/kernel.h b/include/kernel.h
> index 742abf5..161d757 100644
> --- a/include/kernel.h
> +++ b/include/kernel.h
> @@ -4,7 +4,7 @@
>  #define MAX_CMDLINE_SIZE 1024
>  extern char cmdline[MAX_CMDLINE_SIZE];
>  
> -void start_kernel(void* par);
> +void start_kernel(void);
>  void pre_suspend(void);
>  void post_suspend(int canceled);
>  void do_exit(void) __attribute__((noreturn));
> diff --git a/include/shutdown.h b/include/shutdown.h
> index 88993cb..ba71aeb 100644
> --- a/include/shutdown.h
> +++ b/include/shutdown.h
> @@ -3,7 +3,7 @@
>  
>  #include <mini-os/hypervisor.h>
>  
> -void init_shutdown(start_info_t *si);
> +void init_shutdown(void);
>  void fini_shutdown(void);
>  void kernel_suspend(void);
>  
> diff --git a/kernel.c b/kernel.c
> index 301273d..1f97d8d 100644
> --- a/kernel.c
> +++ b/kernel.c
> @@ -77,7 +77,7 @@ __attribute__((weak)) int app_main(void *p)
>      return 0;
>  }
>  
> -void start_kernel(void* par)
> +void start_kernel(void)
>  {
>      /* Set up events. */
>      init_events();
> @@ -107,7 +107,7 @@ void start_kernel(void* par)
>  
>  #ifdef CONFIG_XENBUS
>      /* Init shutdown thread */
> -    init_shutdown((start_info_t *)par);
> +    init_shutdown();
>  #endif
>  
>      /* Call (possibly overridden) app_main() */
> diff --git a/shutdown.c b/shutdown.c
> index 4c0b13c..0854670 100644
> --- a/shutdown.c
> +++ b/shutdown.c
> @@ -46,7 +46,7 @@
>  #include <mini-os/xmalloc.h>
>  
>  
> -static start_info_t *start_info_ptr;
> +extern start_info_t *start_info_ptr;
>  
>  #ifdef CONFIG_XENBUS
>  static const char *path = "control/shutdown";
> @@ -111,10 +111,8 @@ static void shutdown_thread(void *p)
>      }
>  }
>  
> -void init_shutdown(start_info_t *si)
> +void init_shutdown(void)
>  {
> -    start_info_ptr = si;
> -
>      end_shutdown_thread = 0;
>      create_thread("shutdown", shutdown_thread, NULL);
>  }
> @@ -145,6 +143,8 @@ void kernel_suspend(void)
>      /*
>       * This hypercall returns 1 if the suspend
>       * was cancelled and 0 if resuming in a new domain
> +     *
> +     * TODO Fix this for ARM
>       */
>      rc = HYPERVISOR_suspend(virt_to_mfn(start_info_ptr));
>  
> -- 
> 2.20.1
>