[Skiboot] [PATCH] nvram: Fix 'missing' nvram on FSP systems.
Cyril Bur
cyril.bur at au1.ibm.com
Fri Nov 17 10:50:50 AEDT 2017
On Tue, 2017-11-14 at 11:39 +1100, Oliver wrote:
> 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.
>
I think this goes back to - when do we call nvram_wait_for_load()...
and great... looks like my entire summary email never made it to the
list... nor can I find it anywhere...
It was a rather long email I don't feel like retyping...
Basically, either we do what Oliver suggests but this hides the (now)
blocking nature of nvram_query() which I was hesitant to do. I feel
like in skiboot it should be fairly obvious that something might block.
Another options is we leave what I have, which makes the blocking quite
obvious but does require the function to be explicitly called before
nvram_query() and more importantly before launching the bootkernel.
Finally, we put nvram_wait_for_load() at the end of nvram_init(), which
is a nicer place to have blocking since its obviously an init function.
The problem being that we might be able to nvram_init() long enough
before we actually need the nvram and we lose the benefit of the
asynchronous load. Having said that, as Oliver points out - we use
nvram_query() to switch debug stuff. We want to do that as early as
possible, so if there is going to be a blocking nvram_query() right
after nvram_init(), then it might as well be in nvram_init().
All this only applies to FSP, BMC nvram_init() is actually synchronous
at the moment (although it doesn't have to be).
Stewart, do you have any strong feelings?
> > +
> > /*
> > * 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://urldefense.proofpoint.com/v2/url?u=https-3A__lists.ozlabs.org_listinfo_skiboot&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=enAa2wJuZFP2PsCNSamrs-fnBSp0sgKE8NVKBN2MoOk&m=T66gB1K3D15DxA1C_88UWikIJnnFzHR2tstA-B9Q7DE&s=gM3LwS7hmgKQV0R9oTgr22KZaNJ6woQPOaFpqGqGF-s&e=
>
>
More information about the Skiboot
mailing list