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

[Stewart Smith]

Cyril Bur <cyril.bur at au1.ibm.com> writes:
> 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;
> +	}
> +
>  	/*
>  	 * 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;

here I think we should return true instead, as it's as 'ready' as it's
ever going to be, it's just not ever going to be usable.

So for the purpose of "do we need to log some error about not being able
to wait for nvram to be ready", the answer is no, which is pretty much
what the return value of this function is saying.

make sense?

Of course, what doesn't make sense is that I now have a meeting and thus
I haven't looked at the rest of this patch yet

-- 
Stewart Smith
OPAL Architect, IBM.