[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