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

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[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 <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Michael Ellerman-2
Suraj Jitindar Singh <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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 <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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 <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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.
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Michael Ellerman-2
In reply to this post by Suraj Jitindar Singh
Suraj Jitindar Singh <[hidden email]> 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 <[hidden email]> 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".
>
> 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Aneesh Kumar K.V-3
In reply to this post by Suraj Jitindar Singh
Suraj Jitindar Singh <[hidden email]> 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 <[hidden email]>

> Signed-off-by: Suraj Jitindar Singh <[hidden email]>
> ---
>  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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Michael Ellerman-2
In reply to this post by Suraj Jitindar Singh
Suraj Jitindar Singh <[hidden email]> 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 <[hidden email]>
> ---
>  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;
 }
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Michael Ellerman-3
In reply to this post by Suraj Jitindar Singh
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 <[hidden email]>
> Reviewed-by: Aneesh Kumar K.V <[hidden email]>

Applied to powerpc next, thanks.

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

cheers
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Suraj Jitindar Singh
In reply to this post by Michael Ellerman-2
On Wed, 2017-08-09 at 20:30 +1000, Michael Ellerman wrote:

> Suraj Jitindar Singh <[hidden email]> 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 <[hidden email]>
> > ---
> >  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 :)
Loading...