[Skiboot] [PATCH] nvram: Fix 'missing' nvram on FSP systems.

Oliver oohall at gmail.com
Tue Nov 14 11:39:32 AEDT 2017


On Tue, Nov 7, 2017 at 5:07 PM, Cyril Bur <cyril.bur at au1.ibm.com> wrote:
> commit ba4d46fdd9eb ("console: Set log level from nvram") wants to read
> from NVRAM rather early. This works fine on BMC based systems as
> nvram_init() is actually synchronous. This is not true for FSP systems
> and it turns out that the query for the console log level simply
> queries blank nvram.
>
> The simple fix is to wait for the NVRAM read to complete before
> performing any query. Unfortunately it turns out that the fsp-nvram
> code does not inform the generic NVRAM layer when the read is complete,
> rather, it must be prompted to do so.
>
> This patch addresses both these problems. This patch adds a check before
> the first read of the NVRAM (for the console log level) that the read
> has completed. The fsp-nvram code has been updated to inform the generic
> layer as soon as the read completes.
>
> The old prompt to the fsp-nvram code has been removed but a check to
> ensure that the NVRAM has been loaded remains. It is conservative but
> if the NVRAM is not done loading before the host is booted it will not
> have an nvram device-tree node which means it won't be able to access
> the NVRAM at all, ever, even after the NVRAM has loaded.
>
> Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> ---
>  core/init.c                  | 11 +++++++++--
>  core/nvram-format.c          |  5 +++++
>  core/nvram.c                 | 35 +++++++++++++++++++++++++++++++++++
>  core/test/run-nvram-format.c |  5 +++++
>  hw/fsp/fsp-nvram.c           | 28 ++++------------------------
>  include/fsp.h                |  1 -
>  include/nvram.h              |  2 ++
>  7 files changed, 60 insertions(+), 27 deletions(-)
>
> diff --git a/core/init.c b/core/init.c
> index 8951e17b..17de9851 100644
> --- a/core/init.c
> +++ b/core/init.c
> @@ -497,7 +497,7 @@ void __noreturn load_and_boot_kernel(bool is_reboot)
>                 /* We wait for the nvram read to complete here so we can
>                  * grab stuff from there such as the kernel arguments
>                  */
> -               fsp_nvram_wait_open();
> +               nvram_wait_for_load();
>
>                 /* Wait for FW VPD data read to complete */
>                 fsp_code_update_wait_vpd(true);
> @@ -987,7 +987,14 @@ void __noreturn __nomcount main_cpu_entry(const void *fdt)
>         /* preload the IMC catalog dtb */
>         imc_catalog_preload();
>
> -       /* Set the console level */
> +       /*
> +        * Set the console level
> +        * We need NVRAM to have been setup by now otherwise we'll
> +        * just read a zeroed buffer.
> +        * Some FSP systems can actually be quite slow to return the
> +        * NVRAM.
> +        */
> +       nvram_wait_for_load();
>         console_log_level();
>
>         /* Secure/Trusted Boot init. We look for /ibm,secureboot in DT */
> diff --git a/core/nvram-format.c b/core/nvram-format.c
> index 923098af..655cb8ee 100644
> --- a/core/nvram-format.c
> +++ b/core/nvram-format.c
> @@ -216,6 +216,11 @@ const char *nvram_query(const char *key)
>         const char *part_end, *start;
>         int key_len = strlen(key);
>

> +       if (!nvram_has_loaded()) {
> +               prlog(PR_WARNING, "NVRAM: Query before is done loading\n");
> +               return NULL;
> +       }

I think this should just call nvram_wait_for_load() rather than
failing here. The problem
with this behaviour is that can end up with nvram_query() failing
intermittently on FSP
systems since loading the nvram is asynchronous there.

Given that we use nvram_query() for switching debug stuff on and off
I'd rather it was
as reliable as possible even if it slows booting slightly.

> +
>         /*
>          * The running OS can modify the NVRAM as it pleases so we need to be
>          * a little paranoid and check that it's ok before we try parse it.
> diff --git a/core/nvram.c b/core/nvram.c
> index 2140706a..de6cbddf 100644
> --- a/core/nvram.c
> +++ b/core/nvram.c
> @@ -122,6 +122,41 @@ void nvram_read_complete(bool success)
>         nvram_ready = true;
>  }
>
> +bool nvram_wait_for_load(void)
> +{
> +       /* Short cut */
> +       if (nvram_ready)
> +               return true;
> +
> +       /* Tell the caller it will never happen */
> +       if (!platform.nvram_info)
> +               return false;
> +
> +       /*
> +        * One of two things has happened here.
> +        * 1. nvram_wait_for_load() was called before nvram_init()
> +        * 2. The read of NVRAM failed.
> +        * Either way, this is quite a bad event.
> +        */
> +       if (!nvram_image && !nvram_size) {
> +               prlog(PR_CRIT, "NVRAM: Possible wait before nvram_init()!\n");
> +               return false;
> +       }
> +
> +       while (!nvram_ready) {
> +               opal_run_pollers();
> +               /* If the read fails, tell the caller */
> +               if (!nvram_image && !nvram_size)
> +                       return false;
> +       }
> +       return true;
> +}
> +
> +bool nvram_has_loaded(void)
> +{
> +       return nvram_ready;
> +}
> +
>  void nvram_init(void)
>  {
>         int rc;
> diff --git a/core/test/run-nvram-format.c b/core/test/run-nvram-format.c
> index 5bd8ea2a..e59fdfb6 100644
> --- a/core/test/run-nvram-format.c
> +++ b/core/test/run-nvram-format.c
> @@ -23,6 +23,11 @@ bool nvram_validate(void)
>         return true;
>  }
>
> +bool nvram_has_loaded(void)
> +{
> +       return true;
> +}
> +
>  static char *nvram_reset(void *nvram_image, int size)
>  {
>         struct chrp_nvram_hdr *h = nvram_image;
> diff --git a/hw/fsp/fsp-nvram.c b/hw/fsp/fsp-nvram.c
> index 85b7e815..52342fcf 100644
> --- a/hw/fsp/fsp-nvram.c
> +++ b/hw/fsp/fsp-nvram.c
> @@ -203,6 +203,10 @@ static void fsp_nvram_rd_complete(struct fsp_msg *msg)
>                  */
>         }
>         unlock(&fsp_nvram_lock);
> +       nvram_read_complete(fsp_nvram_state == NVRAM_STATE_OPEN);
> +       if (fsp_nvram_state != NVRAM_STATE_OPEN)
> +               log_simple_error(&e_info(OPAL_RC_NVRAM_INIT),
> +               "FSP: NVRAM not read, skipping init\n");
>  }
>
>  static void fsp_nvram_send_read(void)
> @@ -387,27 +391,3 @@ int fsp_nvram_write(uint32_t offset, void *src, uint32_t size)
>
>         return 0;
>  }
> -
> -/* This is called right before starting the payload (Linux) to
> - * ensure the initial open & read of nvram has happened before
> - * we transfer control as the guest OS. This is necessary as
> - * Linux will not handle a OPAL_BUSY return properly and treat
> - * it as an error
> - */
> -void fsp_nvram_wait_open(void)
> -{
> -       if (!fsp_present())
> -               return;
> -
> -       while(fsp_nvram_state == NVRAM_STATE_OPENING)
> -               opal_run_pollers();
> -
> -       if (!fsp_nvram_was_read) {
> -               log_simple_error(&e_info(OPAL_RC_NVRAM_INIT),
> -                       "FSP: NVRAM not read, skipping init\n");
> -               nvram_read_complete(false);
> -               return;
> -       }
> -
> -       nvram_read_complete(true);
> -}
> diff --git a/include/fsp.h b/include/fsp.h
> index 1d877d81..e4bcae02 100644
> --- a/include/fsp.h
> +++ b/include/fsp.h
> @@ -776,7 +776,6 @@ extern void fsp_used_by_console(void);
>  extern int fsp_nvram_info(uint32_t *total_size);
>  extern int fsp_nvram_start_read(void *dst, uint32_t src, uint32_t len);
>  extern int fsp_nvram_write(uint32_t offset, void *src, uint32_t size);
> -extern void fsp_nvram_wait_open(void);
>
>  /* RTC */
>  extern void fsp_rtc_init(void);
> diff --git a/include/nvram.h b/include/nvram.h
> index 288b5368..012c107f 100644
> --- a/include/nvram.h
> +++ b/include/nvram.h
> @@ -21,6 +21,8 @@ int nvram_format(void *nvram_image, uint32_t nvram_size);
>  int nvram_check(void *nvram_image, uint32_t nvram_size);
>  void nvram_reinit(void);
>  bool nvram_validate(void);
> +bool nvram_has_loaded(void);
> +bool nvram_wait_for_load(void);
>
>  const char *nvram_query(const char *name);
>  bool nvram_query_eq(const char *key, const char *value);
> --
> 2.15.0
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot


More information about the Skiboot mailing list