[PATCH] powerpc/mm: Invalidate partition table cache on host proc tbl base update

[PATCH] powerpc/mm: Invalidate partition table cache on host proc tbl base update

[Suraj Jitindar Singh]

The host process table base is stored in the partition table by calling
the function native_register_process_table(). Currently this just sets
the entry in memory and is missing a proceeding cache invalidation
instruction. Any update to the partition table should be followed by a
cache invalidation instruction specifying invalidation of the caching of
any partition table entries (RIC = 2, PRS = 0).

We already have a function to update the partition table with the
required cache invalidation instructions - mmu_partition_table_set_entry().
Update the native_register_process_table() function to call
mmu_partition_table_set_entry(), this ensures all appropriate
invalidation will be performed.

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 arch/powerpc/mm/pgtable-radix.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
index 671a45d..1d5178f 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c
@@ -33,7 +33,8 @@ static int native_register_process_table(unsigned long base, unsigned long pg_sz
 {
 	unsigned long patb1 = base | table_size | PATB_GR;
 
-	partition_tb->patb1 = cpu_to_be64(patb1);
+	mmu_partition_table_set_entry(0, be64_to_cpu(partition_tb->patb0),
+				      patb1);
 	return 0;
 }
 
-- 
2.9.4

Re: [PATCH] powerpc/mm: Invalidate partition table cache on host proc tbl base update

[Michael Ellerman]

Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:

> The host process table base is stored in the partition table by calling
> the function native_register_process_table(). Currently this just sets
> the entry in memory and is missing a proceeding cache invalidation
> instruction. Any update to the partition table should be followed by a
> cache invalidation instruction specifying invalidation of the caching of
> any partition table entries (RIC = 2, PRS = 0).
>
> We already have a function to update the partition table with the
> required cache invalidation instructions - mmu_partition_table_set_entry().
> Update the native_register_process_table() function to call
> mmu_partition_table_set_entry(), this ensures all appropriate
> invalidation will be performed.

Without this patch the kernel will:
 [ ] work normally
 [ ] randomly crash
 [ ] catch fire

?

cheers

Re: [PATCH] powerpc/mm: Invalidate partition table cache on host proc tbl base update

[Benjamin Herrenschmidt]

On Thu, 2017-08-03 at 16:30 +1000, Michael Ellerman wrote:
> Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:
> 
> > The host process table base is stored in the partition table by calling
> > the function native_register_process_table(). Currently this just sets
> > the entry in memory and is missing a proceeding cache invalidation
> > instruction. Any update to the partition table should be followed by a
> > cache invalidation instruction specifying invalidation of the caching of
> > any partition table entries (RIC = 2, PRS = 0).
> > 
> > We already have a function to update the partition table with the
> > required cache invalidation instructions - mmu_partition_table_set_entry().
> > Update the native_register_process_table() function to call
> > mmu_partition_table_set_entry(), this ensures all appropriate
> > invalidation will be performed.
> 
> Without this patch the kernel will:
>  [ ] work normally
>  [ ] randomly crash
>  [ ] catch fire

I think we get lucky because OPAL added a "flush the whole world" to
opal_reinit_cpus() but this patch seems to improve general code
"correctness".

Cheers,
Ben.

Re: [PATCH] powerpc/mm: Invalidate partition table cache on host proc tbl base update

[Suraj Jitindar Singh]

On Thu, 2017-08-03 at 17:35 +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2017-08-03 at 16:30 +1000, Michael Ellerman wrote:
> > Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:
> > 
> > > The host process table base is stored in the partition table by
> > > calling
> > > the function native_register_process_table(). Currently this just
> > > sets
> > > the entry in memory and is missing a proceeding cache
> > > invalidation
> > > instruction. Any update to the partition table should be followed
> > > by a
> > > cache invalidation instruction specifying invalidation of the
> > > caching of
> > > any partition table entries (RIC = 2, PRS = 0).
> > > 
> > > We already have a function to update the partition table with the
> > > required cache invalidation instructions -
> > > mmu_partition_table_set_entry().
> > > Update the native_register_process_table() function to call
> > > mmu_partition_table_set_entry(), this ensures all appropriate
> > > invalidation will be performed.
> > 
> > Without this patch the kernel will:
> >  [ ] work normally
> >  [ ] randomly crash
> >  [ ] catch fire
> 
> I think we get lucky because OPAL added a "flush the whole world" to
> opal_reinit_cpus() but this patch seems to improve general code
> "correctness".
> 
> Cheers,
> Ben.
> 

I guess there's the possibility of:
[x] randomly crash

This is required to run a powernv kernel as a guest because we need to
know when it's updated its process table location.

Re: [PATCH] powerpc/mm: Invalidate partition table cache on host proc tbl base update

[Benjamin Herrenschmidt]

On Fri, 2017-08-04 at 11:02 +1000, Suraj Jitindar Singh wrote:
> 
> I guess there's the possibility of:
> [x] randomly crash
> 
> This is required to run a powernv kernel as a guest because we need to
> know when it's updated its process table location.

You mean in qemu full emu ? powernv kernels don't run as guest do they
?

Cheers,
Ben.

Re: [PATCH] powerpc/mm: Invalidate partition table cache on host proc tbl base update

[Suraj Jitindar Singh]

On Fri, 2017-08-04 at 11:31 +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2017-08-04 at 11:02 +1000, Suraj Jitindar Singh wrote:
> > 
> > I guess there's the possibility of:
> > [x] randomly crash
> > 
> > This is required to run a powernv kernel as a guest because we need
> > to
> > know when it's updated its process table location.
> 
> You mean in qemu full emu ? powernv kernels don't run as guest do
> they
> ?

For now they don't... :p

> 
> Cheers,
> Ben.
> 

Re: [PATCH] powerpc/mm: Invalidate partition table cache on host proc tbl base update

[Michael Ellerman]

Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:

> On Thu, 2017-08-03 at 17:35 +1000, Benjamin Herrenschmidt wrote:
>> On Thu, 2017-08-03 at 16:30 +1000, Michael Ellerman wrote:
>> > Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:
>> >=20
>> > > The host process table base is stored in the partition table by
>> > > calling
>> > > the function native_register_process_table(). Currently this just
>> > > sets
>> > > the entry in memory and is missing a proceeding cache
>> > > invalidation
>> > > instruction. Any update to the partition table should be followed
>> > > by a
>> > > cache invalidation instruction specifying invalidation of the
>> > > caching of
>> > > any partition table entries (RIC =3D 2, PRS =3D 0).
>> > >=20
>> > > We already have a function to update the partition table with the
>> > > required cache invalidation instructions -
>> > > mmu_partition_table_set_entry().
>> > > Update the native_register_process_table() function to call
>> > > mmu_partition_table_set_entry(), this ensures all appropriate
>> > > invalidation will be performed.
>> >=20
>> > Without this patch the kernel will:
>> > =C2=A0[ ] work normally
>> > =C2=A0[ ] randomly crash
>> > =C2=A0[ ] catch fire
>>=20
>> I think we get lucky because OPAL added a "flush the whole world" to
>> opal_reinit_cpus() but this patch seems to improve general code
>> "correctness".
>
> I guess there's the possibility of:
> [x] randomly crash

But you haven't actually see any right?

> This is required to run a powernv kernel as a guest because we need to
> know when it's updated its process table location.

Which doesn't currently work for other reasons :)

cheers

Re: [PATCH] powerpc/mm: Invalidate partition table cache on host proc tbl base update

[Aneesh Kumar K.V]

Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:

> The host process table base is stored in the partition table by calling
> the function native_register_process_table(). Currently this just sets
> the entry in memory and is missing a proceeding cache invalidation
> instruction. Any update to the partition table should be followed by a
> cache invalidation instruction specifying invalidation of the caching of
> any partition table entries (RIC = 2, PRS = 0).
>
> We already have a function to update the partition table with the
> required cache invalidation instructions - mmu_partition_table_set_entry().
> Update the native_register_process_table() function to call
> mmu_partition_table_set_entry(), this ensures all appropriate
> invalidation will be performed.
>

Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  arch/powerpc/mm/pgtable-radix.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
> index 671a45d..1d5178f 100644
> --- a/arch/powerpc/mm/pgtable-radix.c
> +++ b/arch/powerpc/mm/pgtable-radix.c
> @@ -33,7 +33,8 @@ static int native_register_process_table(unsigned long base, unsigned long pg_sz
>  {
>  	unsigned long patb1 = base | table_size | PATB_GR;
>
> -	partition_tb->patb1 = cpu_to_be64(patb1);
> +	mmu_partition_table_set_entry(0, be64_to_cpu(partition_tb->patb0),
> +				      patb1);
>  	return 0;
>  }
>
> -- 
> 2.9.4

Re: [PATCH] powerpc/mm: Invalidate partition table cache on host proc tbl base update

[Michael Ellerman]

Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:

> The host process table base is stored in the partition table by calling
> the function native_register_process_table(). Currently this just sets
> the entry in memory and is missing a proceeding cache invalidation
> instruction. Any update to the partition table should be followed by a
> cache invalidation instruction specifying invalidation of the caching of
> any partition table entries (RIC = 2, PRS = 0).
>
> We already have a function to update the partition table with the
> required cache invalidation instructions - mmu_partition_table_set_entry().
> Update the native_register_process_table() function to call
> mmu_partition_table_set_entry(), this ensures all appropriate
> invalidation will be performed.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  arch/powerpc/mm/pgtable-radix.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
> index 671a45d..1d5178f 100644
> --- a/arch/powerpc/mm/pgtable-radix.c
> +++ b/arch/powerpc/mm/pgtable-radix.c
> @@ -33,7 +33,8 @@ static int native_register_process_table(unsigned long base, unsigned long pg_sz
>  {
>  	unsigned long patb1 = base | table_size | PATB_GR;
>  
> -	partition_tb->patb1 = cpu_to_be64(patb1);
> +	mmu_partition_table_set_entry(0, be64_to_cpu(partition_tb->patb0),
> +				      patb1);

This is really a bit gross.

Can we agree on whether partition_tb is an array or not?

How about ...

cheers

diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
index c1185c8ecb22..5d8be076f8e5 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c
@@ -28,9 +28,13 @@
 static int native_register_process_table(unsigned long base, unsigned long pg_sz,
                                         unsigned long table_size)
 {
-       unsigned long patb1 = base | table_size | PATB_GR;
+       unsigned long patb0, patb1;
+
+       patb0 = be64_to_cpu(partition_tb[0].patb0);
+       patb1 = base | table_size | PATB_GR;
+
+       mmu_partition_table_set_entry(0, patb0, patb1);
 
-       partition_tb->patb1 = cpu_to_be64(patb1);
        return 0;
 }

Re: powerpc/mm: Invalidate partition table cache on host proc tbl base update

[Michael Ellerman]

On Thu, 2017-08-03 at 04:15:51 UTC, Suraj Jitindar Singh wrote:
> The host process table base is stored in the partition table by calling
> the function native_register_process_table(). Currently this just sets
> the entry in memory and is missing a proceeding cache invalidation
> instruction. Any update to the partition table should be followed by a
> cache invalidation instruction specifying invalidation of the caching of
> any partition table entries (RIC = 2, PRS = 0).
> 
> We already have a function to update the partition table with the
> required cache invalidation instructions - mmu_partition_table_set_entry().
> Update the native_register_process_table() function to call
> mmu_partition_table_set_entry(), this ensures all appropriate
> invalidation will be performed.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/7cd2a8695ef9c31e8f51773f0de9e6

cheers

Re: [PATCH] powerpc/mm: Invalidate partition table cache on host proc tbl base update

[Suraj Jitindar Singh]

On Wed, 2017-08-09 at 20:30 +1000, Michael Ellerman wrote:
> Suraj Jitindar Singh <sjitindarsingh@gmail.com> writes:
> 
> > The host process table base is stored in the partition table by
> > calling
> > the function native_register_process_table(). Currently this just
> > sets
> > the entry in memory and is missing a proceeding cache invalidation
> > instruction. Any update to the partition table should be followed
> > by a
> > cache invalidation instruction specifying invalidation of the
> > caching of
> > any partition table entries (RIC = 2, PRS = 0).
> > 
> > We already have a function to update the partition table with the
> > required cache invalidation instructions -
> > mmu_partition_table_set_entry().
> > Update the native_register_process_table() function to call
> > mmu_partition_table_set_entry(), this ensures all appropriate
> > invalidation will be performed.
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  arch/powerpc/mm/pgtable-radix.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/mm/pgtable-radix.c
> > b/arch/powerpc/mm/pgtable-radix.c
> > index 671a45d..1d5178f 100644
> > --- a/arch/powerpc/mm/pgtable-radix.c
> > +++ b/arch/powerpc/mm/pgtable-radix.c
> > @@ -33,7 +33,8 @@ static int native_register_process_table(unsigned
> > long base, unsigned long pg_sz
> >  {
> >  	unsigned long patb1 = base | table_size | PATB_GR;
> >  
> > -	partition_tb->patb1 = cpu_to_be64(patb1);
> > +	mmu_partition_table_set_entry(0, be64_to_cpu(partition_tb-
> > >patb0),
> > +				      patb1);
> 
> This is really a bit gross.
> 
> Can we agree on whether partition_tb is an array or not?

Well it is an array, it's just we only ever want the first element in
this function. That being said we might as well access it as an array
to make that clear.

> 
> How about ...
> 
> cheers
> 
> diff --git a/arch/powerpc/mm/pgtable-radix.c
> b/arch/powerpc/mm/pgtable-radix.c
> index c1185c8ecb22..5d8be076f8e5 100644
> --- a/arch/powerpc/mm/pgtable-radix.c
> +++ b/arch/powerpc/mm/pgtable-radix.c
> @@ -28,9 +28,13 @@
>  static int native_register_process_table(unsigned long base,
> unsigned long pg_sz,
>                                          unsigned long table_size)
>  {
> -       unsigned long patb1 = base | table_size | PATB_GR;
> +       unsigned long patb0, patb1;
> +
> +       patb0 = be64_to_cpu(partition_tb[0].patb0);
> +       patb1 = base | table_size | PATB_GR;
> +
> +       mmu_partition_table_set_entry(0, patb0, patb1);
>  
> -       partition_tb->patb1 = cpu_to_be64(patb1);
>         return 0;
>  }

Looks good :)