diff mbox

powerpc: Fix booting P9 hash with CONFIG_PPC_RADIX_MMU=N

Message ID 87bmqir5tl.fsf@skywalker.in.ibm.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Aneesh Kumar K.V May 24, 2017, 8:56 a.m. UTC 
Michael Neuling <mikey@neuling.org> writes:

> Currently if you disable CONFIG_PPC_RADIX_MMU you'll crash on boot on
> a P9. This is because we still set MMU_FTR_TYPE_RADIX via
> ibm,pa-features and MMU_FTR_TYPE_RADIX is what's used for code patching
> in much of the asm code (ie. slb_miss_realmode)
>
> This patch fixes the problem by stopping MMU_FTR_TYPE_RADIX from being
> set from ibm.pa-features.
>
> We may eventually end up removing the CONFIG_PPC_RADIX_MMU option
> completely but until then this fixes the issue.
>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> ---
>  arch/powerpc/kernel/prom.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 40c4887c27..f830562974 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -161,7 +161,9 @@ static struct ibm_pa_feature {
>  	{ .pabyte = 0,  .pabit = 3, .cpu_features  = CPU_FTR_CTRL },
>  	{ .pabyte = 0,  .pabit = 6, .cpu_features  = CPU_FTR_NOEXECUTE },
>  	{ .pabyte = 1,  .pabit = 2, .mmu_features  = MMU_FTR_CI_LARGE_PAGE },
> +#ifdef CONFIG_PPC_RADIX_MMU
>  	{ .pabyte = 40, .pabit = 0, .mmu_features  = MMU_FTR_TYPE_RADIX },
> +#endif
>  	{ .pabyte = 1,  .pabit = 1, .invert = 1, .cpu_features = CPU_FTR_NODSISRALIGN },
>  	{ .pabyte = 5,  .pabit = 0, .cpu_features  = CPU_FTR_REAL_LE,
>  				    .cpu_user_ftrs = PPC_FEATURE_TRUE_LE },
> -- 
> 2.11.0

Instead can we do that feature removal in mmu_early_init_devtree. ie, something like

Comments

Michael Neuling May 25, 2017, 12:13 a.m. UTC | #1 
On Wed, 2017-05-24 at 14:26 +0530, Aneesh Kumar K.V wrote:
> Michael Neuling <mikey@neuling.org> writes:
> 
> > Currently if you disable CONFIG_PPC_RADIX_MMU you'll crash on boot on
> > a P9. This is because we still set MMU_FTR_TYPE_RADIX via
> > ibm,pa-features and MMU_FTR_TYPE_RADIX is what's used for code patching
> > in much of the asm code (ie. slb_miss_realmode)
> > 
> > This patch fixes the problem by stopping MMU_FTR_TYPE_RADIX from being
> > set from ibm.pa-features.
> > 
> > We may eventually end up removing the CONFIG_PPC_RADIX_MMU option
> > completely but until then this fixes the issue.
> > 
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > ---
> >  arch/powerpc/kernel/prom.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> > index 40c4887c27..f830562974 100644
> > --- a/arch/powerpc/kernel/prom.c
> > +++ b/arch/powerpc/kernel/prom.c
> > @@ -161,7 +161,9 @@ static struct ibm_pa_feature {
> >  	{ .pabyte = 0,  .pabit = 3, .cpu_features  = CPU_FTR_CTRL },
> >  	{ .pabyte = 0,  .pabit = 6, .cpu_features  = CPU_FTR_NOEXECUTE },
> >  	{ .pabyte = 1,  .pabit = 2, .mmu_features  = MMU_FTR_CI_LARGE_PAGE
> > },
> > +#ifdef CONFIG_PPC_RADIX_MMU
> >  	{ .pabyte = 40, .pabit = 0, .mmu_features  = MMU_FTR_TYPE_RADIX },
> > +#endif
> >  	{ .pabyte = 1,  .pabit = 1, .invert = 1, .cpu_features =
> > CPU_FTR_NODSISRALIGN },
> >  	{ .pabyte = 5,  .pabit = 0, .cpu_features  = CPU_FTR_REAL_LE,
> >  				    .cpu_user_ftrs = PPC_FEATURE_TRUE_LE },
> > -- 
> > 2.11.0
> 
> Instead can we do that feature removal in mmu_early_init_devtree. ie,
> something like

It looks like mmu_early_init_devtree() gets called after parsing ibm,pa_features
so that should work.

> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
> index 8f6f2a173e47..8f43f3827bac 100644
> --- a/arch/powerpc/mm/init_64.c
> +++ b/arch/powerpc/mm/init_64.c
> @@ -394,7 +394,7 @@ void __init mmu_early_init_devtree(v
> oid)
>  {
>  	/* Disable radix mode based on kernel command line. */
>  	/* We don't yet have the machinery to do radix as a guest. */
> -	if (disable_radix || !(mfmsr() & MSR_HV))
> > +	if (!IS_ENABLED(CONFIG_PPC_RADIX_MMU) || disable_radix || !(mfmsr() & MSR_HV))

What tree is this patch against?  Linus' tree doesn't look like that.

Mikey


>  		cur_cpu_spec->mmu_features &= ~MMU_FTR_TYPE_RADIX;
>  
>  	/*
>
Aneesh Kumar K.V May 25, 2017, 2:53 a.m. UTC | #2 
Michael Neuling <mikey@neuling.org> writes:

> On Wed, 2017-05-24 at 14:26 +0530, Aneesh Kumar K.V wrote:
>> Michael Neuling <mikey@neuling.org> writes:
>> 
>> > Currently if you disable CONFIG_PPC_RADIX_MMU you'll crash on boot on
>> > a P9. This is because we still set MMU_FTR_TYPE_RADIX via
>> > ibm,pa-features and MMU_FTR_TYPE_RADIX is what's used for code patching
>> > in much of the asm code (ie. slb_miss_realmode)
>> > 
>> > This patch fixes the problem by stopping MMU_FTR_TYPE_RADIX from being
>> > set from ibm.pa-features.
>> > 
>> > We may eventually end up removing the CONFIG_PPC_RADIX_MMU option
>> > completely but until then this fixes the issue.
>> > 
>> > Signed-off-by: Michael Neuling <mikey@neuling.org>
>> > ---
>> >  arch/powerpc/kernel/prom.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> > 
>> > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> > index 40c4887c27..f830562974 100644
>> > --- a/arch/powerpc/kernel/prom.c
>> > +++ b/arch/powerpc/kernel/prom.c
>> > @@ -161,7 +161,9 @@ static struct ibm_pa_feature {
>> >  	{ .pabyte = 0,  .pabit = 3, .cpu_features  = CPU_FTR_CTRL },
>> >  	{ .pabyte = 0,  .pabit = 6, .cpu_features  = CPU_FTR_NOEXECUTE },
>> >  	{ .pabyte = 1,  .pabit = 2, .mmu_features  = MMU_FTR_CI_LARGE_PAGE
>> > },
>> > +#ifdef CONFIG_PPC_RADIX_MMU
>> >  	{ .pabyte = 40, .pabit = 0, .mmu_features  = MMU_FTR_TYPE_RADIX },
>> > +#endif
>> >  	{ .pabyte = 1,  .pabit = 1, .invert = 1, .cpu_features =
>> > CPU_FTR_NODSISRALIGN },
>> >  	{ .pabyte = 5,  .pabit = 0, .cpu_features  = CPU_FTR_REAL_LE,
>> >  				    .cpu_user_ftrs = PPC_FEATURE_TRUE_LE },
>> > -- 
>> > 2.11.0
>> 
>> Instead can we do that feature removal in mmu_early_init_devtree. ie,
>> something like
>
> It looks like mmu_early_init_devtree() gets called after parsing ibm,pa_features
> so that should work.
>
>> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
>> index 8f6f2a173e47..8f43f3827bac 100644
>> --- a/arch/powerpc/mm/init_64.c
>> +++ b/arch/powerpc/mm/init_64.c
>> @@ -394,7 +394,7 @@ void __init mmu_early_init_devtree(v
>> oid)
>>  {
>>  	/* Disable radix mode based on kernel command line. */
>>  	/* We don't yet have the machinery to do radix as a guest. */
>> -	if (disable_radix || !(mfmsr() & MSR_HV))
>> > +	if (!IS_ENABLED(CONFIG_PPC_RADIX_MMU) || disable_radix || !(mfmsr() & MSR_HV))
>
> What tree is this patch against?  Linus' tree doesn't look like that.
>

mpe/next at the time of this merge window. Do you want me to send a
complete patch against latest mpe/next ?

-aneesh
Michael Ellerman May 25, 2017, 6:16 a.m. UTC | #3 
Michael Neuling <mikey@neuling.org> writes:

> On Wed, 2017-05-24 at 14:26 +0530, Aneesh Kumar K.V wrote:
>> Michael Neuling <mikey@neuling.org> writes:
>> 
>> > Currently if you disable CONFIG_PPC_RADIX_MMU you'll crash on boot on
>> > a P9. This is because we still set MMU_FTR_TYPE_RADIX via
>> > ibm,pa-features and MMU_FTR_TYPE_RADIX is what's used for code patching
>> > in much of the asm code (ie. slb_miss_realmode)
>> > 
>> > This patch fixes the problem by stopping MMU_FTR_TYPE_RADIX from being
>> > set from ibm.pa-features.
>> > 
>> > We may eventually end up removing the CONFIG_PPC_RADIX_MMU option
>> > completely but until then this fixes the issue.
>> > 
>> > Signed-off-by: Michael Neuling <mikey@neuling.org>
>> > ---
>> >  arch/powerpc/kernel/prom.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> > 
>> > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> > index 40c4887c27..f830562974 100644
>> > --- a/arch/powerpc/kernel/prom.c
>> > +++ b/arch/powerpc/kernel/prom.c
>> > @@ -161,7 +161,9 @@ static struct ibm_pa_feature {
>> >  	{ .pabyte = 0,  .pabit = 3, .cpu_features  = CPU_FTR_CTRL },
>> >  	{ .pabyte = 0,  .pabit = 6, .cpu_features  = CPU_FTR_NOEXECUTE },
>> >  	{ .pabyte = 1,  .pabit = 2, .mmu_features  = MMU_FTR_CI_LARGE_PAGE
>> > },
>> > +#ifdef CONFIG_PPC_RADIX_MMU
>> >  	{ .pabyte = 40, .pabit = 0, .mmu_features  = MMU_FTR_TYPE_RADIX },
>> > +#endif
>> >  	{ .pabyte = 1,  .pabit = 1, .invert = 1, .cpu_features =
>> > CPU_FTR_NODSISRALIGN },
>> >  	{ .pabyte = 5,  .pabit = 0, .cpu_features  = CPU_FTR_REAL_LE,
>> >  				    .cpu_user_ftrs = PPC_FEATURE_TRUE_LE },
>> > -- 
>> > 2.11.0
>> 
>> Instead can we do that feature removal in mmu_early_init_devtree. ie,
>> something like
>
> It looks like mmu_early_init_devtree() gets called after parsing ibm,pa_features
> so that should work.

But why is doing it later preferable?

Mikey's patch means it will never be set at any point during boot, which
seems obviously better to me.

cheers
Aneesh Kumar K.V May 25, 2017, 6:22 a.m. UTC | #4 
On Thursday 25 May 2017 11:46 AM, Michael Ellerman wrote:
> Michael Neuling <mikey@neuling.org> writes:
> 
>> On Wed, 2017-05-24 at 14:26 +0530, Aneesh Kumar K.V wrote:
>>> Michael Neuling <mikey@neuling.org> writes:
>>>
>>>> Currently if you disable CONFIG_PPC_RADIX_MMU you'll crash on boot on
>>>> a P9. This is because we still set MMU_FTR_TYPE_RADIX via
>>>> ibm,pa-features and MMU_FTR_TYPE_RADIX is what's used for code patching
>>>> in much of the asm code (ie. slb_miss_realmode)
>>>>
>>>> This patch fixes the problem by stopping MMU_FTR_TYPE_RADIX from being
>>>> set from ibm.pa-features.
>>>>
>>>> We may eventually end up removing the CONFIG_PPC_RADIX_MMU option
>>>> completely but until then this fixes the issue.
>>>>
>>>> Signed-off-by: Michael Neuling <mikey@neuling.org>
>>>> ---
>>>>   arch/powerpc/kernel/prom.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>>>> index 40c4887c27..f830562974 100644
>>>> --- a/arch/powerpc/kernel/prom.c
>>>> +++ b/arch/powerpc/kernel/prom.c
>>>> @@ -161,7 +161,9 @@ static struct ibm_pa_feature {
>>>>   	{ .pabyte = 0,  .pabit = 3, .cpu_features  = CPU_FTR_CTRL },
>>>>   	{ .pabyte = 0,  .pabit = 6, .cpu_features  = CPU_FTR_NOEXECUTE },
>>>>   	{ .pabyte = 1,  .pabit = 2, .mmu_features  = MMU_FTR_CI_LARGE_PAGE
>>>> },
>>>> +#ifdef CONFIG_PPC_RADIX_MMU
>>>>   	{ .pabyte = 40, .pabit = 0, .mmu_features  = MMU_FTR_TYPE_RADIX },
>>>> +#endif
>>>>   	{ .pabyte = 1,  .pabit = 1, .invert = 1, .cpu_features =
>>>> CPU_FTR_NODSISRALIGN },
>>>>   	{ .pabyte = 5,  .pabit = 0, .cpu_features  = CPU_FTR_REAL_LE,
>>>>   				    .cpu_user_ftrs = PPC_FEATURE_TRUE_LE },
>>>> -- 
>>>> 2.11.0
>>>
>>> Instead can we do that feature removal in mmu_early_init_devtree. ie,
>>> something like
>>
>> It looks like mmu_early_init_devtree() gets called after parsing ibm,pa_features
>> so that should work.
> 
> But why is doing it later preferable?
> 
> Mikey's patch means it will never be set at any point during boot, which
> seems obviously better to me.

My suggestion was w.r.t consolidating different ways of clearing 
RADIX_MMU feature flag into one place. Also I was not sure we want to 
conditionally parse the pa-feature device tree. Instead we parse it 
without any #ifdef in there and clear things which are not supported by 
the kernel either via command line or via Kconfig.

-aneesh
Michael Neuling May 25, 2017, 7:12 a.m. UTC | #5 
On Thu, 2017-05-25 at 08:23 +0530, Aneesh Kumar K.V wrote:
> Michael Neuling <mikey@neuling.org> writes:
> 
> > On Wed, 2017-05-24 at 14:26 +0530, Aneesh Kumar K.V wrote:
> > > Michael Neuling <mikey@neuling.org> writes:
> > > 
> > > > Currently if you disable CONFIG_PPC_RADIX_MMU you'll crash on boot on
> > > > a P9. This is because we still set MMU_FTR_TYPE_RADIX via
> > > > ibm,pa-features and MMU_FTR_TYPE_RADIX is what's used for code patching
> > > > in much of the asm code (ie. slb_miss_realmode)
> > > > 
> > > > This patch fixes the problem by stopping MMU_FTR_TYPE_RADIX from being
> > > > set from ibm.pa-features.
> > > > 
> > > > We may eventually end up removing the CONFIG_PPC_RADIX_MMU option
> > > > completely but until then this fixes the issue.
> > > > 
> > > > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > > > ---
> > > >  arch/powerpc/kernel/prom.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> > > > index 40c4887c27..f830562974 100644
> > > > --- a/arch/powerpc/kernel/prom.c
> > > > +++ b/arch/powerpc/kernel/prom.c
> > > > @@ -161,7 +161,9 @@ static struct ibm_pa_feature {
> > > >  	{ .pabyte = 0,  .pabit = 3, .cpu_features  = CPU_FTR_CTRL },
> > > >  	{ .pabyte = 0,  .pabit = 6, .cpu_features  = CPU_FTR_NOEXECUTE
> > > > },
> > > >  	{ .pabyte = 1,  .pabit = 2, .mmu_features  =
> > > > MMU_FTR_CI_LARGE_PAGE
> > > > },
> > > > +#ifdef CONFIG_PPC_RADIX_MMU
> > > >  	{ .pabyte = 40, .pabit = 0, .mmu_features  = MMU_FTR_TYPE_RADIX
> > > > },
> > > > +#endif
> > > >  	{ .pabyte = 1,  .pabit = 1, .invert = 1, .cpu_features =
> > > > CPU_FTR_NODSISRALIGN },
> > > >  	{ .pabyte = 5,  .pabit = 0, .cpu_features  = CPU_FTR_REAL_LE,
> > > >  				    .cpu_user_ftrs =
> > > > PPC_FEATURE_TRUE_LE },
> > > > -- 
> > > > 2.11.0
> > > 
> > > Instead can we do that feature removal in mmu_early_init_devtree. ie,
> > > something like
> > 
> > It looks like mmu_early_init_devtree() gets called after parsing
> > ibm,pa_features
> > so that should work.
> > 
> > > diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
> > > index 8f6f2a173e47..8f43f3827bac 100644
> > > --- a/arch/powerpc/mm/init_64.c
> > > +++ b/arch/powerpc/mm/init_64.c
> > > @@ -394,7 +394,7 @@ void __init mmu_early_init_devtree(v
> > > oid)
> > >  {
> > >  	/* Disable radix mode based on kernel command line. */
> > >  	/* We don't yet have the machinery to do radix as a guest. */
> > > -	if (disable_radix || !(mfmsr() & MSR_HV))
> > > > +	if (!IS_ENABLED(CONFIG_PPC_RADIX_MMU) || disable_radix ||
> > > > !(mfmsr() & MSR_HV))
> > 
> > What tree is this patch against?  Linus' tree doesn't look like that.
> > 
> 
> mpe/next at the time of this merge window. Do you want me to send a
> complete patch against latest mpe/next ?

If you can test the broken case to make sure it fixes it... Sure!

Thanks,
Mikey
diff mbox

Patch

diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
index 8f6f2a173e47..8f43f3827bac 100644
--- a/arch/powerpc/mm/init_64.c
+++ b/arch/powerpc/mm/init_64.c
@@ -394,7 +394,7 @@  void __init mmu_early_init_devtree(void)
 {
 	/* Disable radix mode based on kernel command line. */
 	/* We don't yet have the machinery to do radix as a guest. */
-	if (disable_radix || !(mfmsr() & MSR_HV))
+	if (!IS_ENABLED(CONFIG_PPC_RADIX_MMU) || disable_radix || !(mfmsr() & MSR_HV))
 		cur_cpu_spec->mmu_features &= ~MMU_FTR_TYPE_RADIX;
 
 	/*