diff mbox series

Bail out when booting in big core mode

Message ID 20200429031806.2335909-1-joel@jms.id.au
State Superseded
Headers show
Series Bail out when booting in big core mode | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (0f1937ef40fca0c3212a9dff1010b832a24fb063)
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot success Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present

Commit Message

Joel Stanley April 29, 2020, 3:18 a.m. UTC
This is not supported and will result in a misbehaving system later in
boot.

We wait until the console is up so the developer can see what happened.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
I see that there are some patches on the list to support this mode. If
they don't go in soon, I ask that this change be added (and backported)
to help developers who accidentally boot in big core mode.

 core/cpu.c          | 9 +++++++++
 core/init.c         | 3 +++
 include/cpu.h       | 3 +++
 include/processor.h | 2 ++
 4 files changed, 17 insertions(+)

Comments

Vaidyanathan Srinivasan April 29, 2020, 8:05 a.m. UTC | #1
* Joel Stanley <joel@jms.id.au> [2020-04-29 12:48:06]:

> This is not supported and will result in a misbehaving system later in
> boot.
> 
> We wait until the console is up so the developer can see what happened.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> I see that there are some patches on the list to support this mode. If
> they don't go in soon, I ask that this change be added (and backported)
> to help developers who accidentally boot in big core mode.
> 
>  core/cpu.c          | 9 +++++++++
>  core/init.c         | 3 +++
>  include/cpu.h       | 3 +++
>  include/processor.h | 2 ++
>  4 files changed, 17 insertions(+)
> 
> diff --git a/core/cpu.c b/core/cpu.c
> index 37d9f41a8b79..f0c7e881e0f0 100644
> --- a/core/cpu.c
> +++ b/core/cpu.c
> @@ -935,6 +935,15 @@ static void init_cpu_thread(struct cpu_thread *t,
>  	assert(pir == container_of(t, struct cpu_stack, cpu) - cpu_stacks);
>  }
>  
> +void cpu_check_bigcore(void)
> +{
> +	unsigned int pvr = mfspr(SPR_PVR);
> +	if (PVR_TYPE(pvr) == PVR_TYPE_P9 && !(PVR_CHIP_TYPE(pvr) & 0x1)) {
> +		prerror("CPU: Detected unsupported POWER9 in big core mode.\n");
> +		abort();
> +	}
> +}
> +
>  static void enable_attn(void)
>  {
>  	unsigned long hid0;
> diff --git a/core/init.c b/core/init.c
> index 39cfd3fbe1b1..73a6756ebdb3 100644
> --- a/core/init.c
> +++ b/core/init.c
> @@ -1284,6 +1284,9 @@ void __noreturn __nomcount main_cpu_entry(const void *fdt)
>  	/* Install the OPAL Console handlers */
>  	init_opal_console();
>  
> +	/* Now that the console is up, bail out if we do not support the machine */
> +	cpu_check_bigcore();
> +

Hi Joel,

Without the big core enablement patch we hit asset() in
dt_add_cpufeatures() much earlier than this place.

I like the idea of atleast detecting the platform and then printing on
console and then abort().

But I do not know on which platform or combination of setting you
actually get to this point.  Share the PVR you ended up with.

We will actually need to check in Core-Thread-State register bit 63 to
confirm fused-core mode.

Let me split out the fused-core detection logic from the patch series
and add this clean way of aborting with message.  That will solve
cases where we unexpectedly boot in incorrect modes. This is a good
idea.

--Vaidy
Joel Stanley April 30, 2020, 1:09 a.m. UTC | #2
On Wed, 29 Apr 2020 at 08:05, Vaidyanathan Srinivasan
<svaidy@linux.ibm.com> wrote:
>
> * Joel Stanley <joel@jms.id.au> [2020-04-29 12:48:06]:
>
> > This is not supported and will result in a misbehaving system later in
> > boot.
> >
> > We wait until the console is up so the developer can see what happened.
> >
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > ---
> > I see that there are some patches on the list to support this mode. If
> > they don't go in soon, I ask that this change be added (and backported)
> > to help developers who accidentally boot in big core mode.
> >
> >  core/cpu.c          | 9 +++++++++
> >  core/init.c         | 3 +++
> >  include/cpu.h       | 3 +++
> >  include/processor.h | 2 ++
> >  4 files changed, 17 insertions(+)
> >
> > diff --git a/core/cpu.c b/core/cpu.c
> > index 37d9f41a8b79..f0c7e881e0f0 100644
> > --- a/core/cpu.c
> > +++ b/core/cpu.c
> > @@ -935,6 +935,15 @@ static void init_cpu_thread(struct cpu_thread *t,
> >       assert(pir == container_of(t, struct cpu_stack, cpu) - cpu_stacks);
> >  }
> >
> > +void cpu_check_bigcore(void)
> > +{
> > +     unsigned int pvr = mfspr(SPR_PVR);
> > +     if (PVR_TYPE(pvr) == PVR_TYPE_P9 && !(PVR_CHIP_TYPE(pvr) & 0x1)) {
> > +             prerror("CPU: Detected unsupported POWER9 in big core mode.\n");
> > +             abort();
> > +     }
> > +}
> > +
> >  static void enable_attn(void)
> >  {
> >       unsigned long hid0;
> > diff --git a/core/init.c b/core/init.c
> > index 39cfd3fbe1b1..73a6756ebdb3 100644
> > --- a/core/init.c
> > +++ b/core/init.c
> > @@ -1284,6 +1284,9 @@ void __noreturn __nomcount main_cpu_entry(const void *fdt)
> >       /* Install the OPAL Console handlers */
> >       init_opal_console();
> >
> > +     /* Now that the console is up, bail out if we do not support the machine */
> > +     cpu_check_bigcore();
> > +
>
> Hi Joel,
>
> Without the big core enablement patch we hit asset() in
> dt_add_cpufeatures() much earlier than this place.

I don't see that on the hardware I was testing on (witherspoon with
overrides set to boot the machine in big core mode).

I was also testing in mambo with PVR set:

 myconf config processor/initial/PVR 0x4e0202

I understand that is only testing the PVR check, and not other side effects.

> I like the idea of atleast detecting the platform and then printing on
> console and then abort().
>
> But I do not know on which platform or combination of setting you
> actually get to this point.  Share the PVR you ended up with.
>
> We will actually need to check in Core-Thread-State register bit 63 to
> confirm fused-core mode.

The PVR of the witherspoon system is 0x4e0202. This changes when you
set the overrides to boot in big core mode, with no other changes to
the system (same host firmware load, etc).

I haven't checked the other register.

> Let me split out the fused-core detection logic from the patch series
> and add this clean way of aborting with message.  That will solve
> cases where we unexpectedly boot in incorrect modes. This is a good
> idea.

Thanks! I'd like this patch to make its way to the stable tree if
possible, so making it minimal would be ideal.

Cheers,

Joel
diff mbox series

Patch

diff --git a/core/cpu.c b/core/cpu.c
index 37d9f41a8b79..f0c7e881e0f0 100644
--- a/core/cpu.c
+++ b/core/cpu.c
@@ -935,6 +935,15 @@  static void init_cpu_thread(struct cpu_thread *t,
 	assert(pir == container_of(t, struct cpu_stack, cpu) - cpu_stacks);
 }
 
+void cpu_check_bigcore(void)
+{
+	unsigned int pvr = mfspr(SPR_PVR);
+	if (PVR_TYPE(pvr) == PVR_TYPE_P9 && !(PVR_CHIP_TYPE(pvr) & 0x1)) {
+		prerror("CPU: Detected unsupported POWER9 in big core mode.\n");
+		abort();
+	}
+}
+
 static void enable_attn(void)
 {
 	unsigned long hid0;
diff --git a/core/init.c b/core/init.c
index 39cfd3fbe1b1..73a6756ebdb3 100644
--- a/core/init.c
+++ b/core/init.c
@@ -1284,6 +1284,9 @@  void __noreturn __nomcount main_cpu_entry(const void *fdt)
 	/* Install the OPAL Console handlers */
 	init_opal_console();
 
+	/* Now that the console is up, bail out if we do not support the machine */
+	cpu_check_bigcore();
+
 	/*
 	 * Some platforms set a flag to wait for SBE validation to be
 	 * performed by the BMC. If this occurs it leaves the SBE in a
diff --git a/include/cpu.h b/include/cpu.h
index 686310d71676..a24e52c9c5e8 100644
--- a/include/cpu.h
+++ b/include/cpu.h
@@ -291,6 +291,9 @@  void cpu_set_sreset_enable(bool sreset_enabled);
 /* IPI for PM modes is enabled */
 void cpu_set_ipi_enable(bool sreset_enabled);
 
+/* Abort if we do not support CPU mode */
+void cpu_check_bigcore(void);
+
 static inline void cpu_give_self_os(void)
 {
 	__this_cpu->state = cpu_state_os;
diff --git a/include/processor.h b/include/processor.h
index 57c2ee13ad4e..f124caa90e67 100644
--- a/include/processor.h
+++ b/include/processor.h
@@ -175,10 +175,12 @@ 
 #define SPR_PVR_TYPE			0xffff0000
 #define SPR_PVR_VERS_MAJ		0x00000f00
 #define SPR_PVR_VERS_MIN		0x000000ff
+#define SPR_PVR_CHIP_TYPE		0x0000f000
 
 #define PVR_TYPE(_pvr)		GETFIELD(SPR_PVR_TYPE, _pvr)
 #define PVR_VERS_MAJ(_pvr)	GETFIELD(SPR_PVR_VERS_MAJ, _pvr)
 #define PVR_VERS_MIN(_pvr)	GETFIELD(SPR_PVR_VERS_MIN, _pvr)
+#define PVR_CHIP_TYPE(_pvr)	GETFIELD(SPR_PVR_CHIP_TYPE, _pvr)
 
 /* PVR definitions */
 #define PVR_TYPE_P8E	0x004b /* Murano */