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

[Cyril Bur]

On Mon, 2017-11-13 at 17:33 +1100, Stewart Smith wrote:
> 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?
> 

Sure, I'll add that to a v2 respin. If we're going to do that I'll also
rename the function to nvram_wait_for_ready().

> 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
>