[PATCH] powerpc/mm: Allow user space to map rtas_rmo_buf

[PATCH] powerpc/mm: Allow user space to map rtas_rmo_buf

[Vasant Hegde]

With commit 90a545e9 (restrict /dev/mem to idle io memory ranges) mapping
rtas_rmo_buf from user space is failing. Hence we are not able to make
RTAS syscall.

This patch calls page_is_rtas_user_buf before calling iomem_is_exclusive
in  devmem_is_allowed(). This will allow user space to map rtas_rmo_buf
and we are able to make RTAS syscall.

Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
CC: Dan Williams <dan.j.williams@intel.com>
CC: Nathan Fontenot <nfont@linux.vnet.ibm.com>
CC: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
---
 arch/powerpc/mm/mem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index 22d94c3..d0f0a51 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -560,12 +560,12 @@ subsys_initcall(add_system_ram_resources);
  */
 int devmem_is_allowed(unsigned long pfn)
 {
+	if (page_is_rtas_user_buf(pfn))
+		return 1;
 	if (iomem_is_exclusive(PFN_PHYS(pfn)))
 		return 0;
 	if (!page_is_ram(pfn))
 		return 1;
-	if (page_is_rtas_user_buf(pfn))
-		return 1;
 	return 0;
 }
 #endif /* CONFIG_STRICT_DEVMEM */
-- 
2.1.0

Re: [PATCH] powerpc/mm: Allow user space to map rtas_rmo_buf

[Dan Williams]

On Thu, Jan 21, 2016 at 8:15 AM, Vasant Hegde
<hegdevasant@linux.vnet.ibm.com> wrote:
> With commit 90a545e9 (restrict /dev/mem to idle io memory ranges) mapping
> rtas_rmo_buf from user space is failing. Hence we are not able to make
> RTAS syscall.
>
> This patch calls page_is_rtas_user_buf before calling iomem_is_exclusive
> in  devmem_is_allowed(). This will allow user space to map rtas_rmo_buf
> and we are able to make RTAS syscall.
>
> Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> CC: Dan Williams <dan.j.williams@intel.com>

Acked-by: Dan Williams <dan.j.williams@intel.com>

Re: [PATCH] powerpc/mm: Allow user space to map rtas_rmo_buf

[Michael Ellerman]

On Thu, 2016-01-21 at 21:45 +0530, Vasant Hegde wrote:

> With commit 90a545e9 (restrict /dev/mem to idle io memory ranges) mapping
> rtas_rmo_buf from user space is failing. Hence we are not able to make
> RTAS syscall.
> 
> This patch calls page_is_rtas_user_buf before calling iomem_is_exclusive
> in  devmem_is_allowed(). This will allow user space to map rtas_rmo_buf
> and we are able to make RTAS syscall.

Thanks for the patch.

I'll put it in my fixes branch for next week.


Having said that, why the <expletive deleted> is librtas mapping /dev/mem in
the first place? Unless there is a very good reason, and probably even if there
is, we should fix that to use a sane API.

cheers

Re: [PATCH] powerpc/mm: Allow user space to map rtas_rmo_buf

[Vasant Hegde]

On 01/22/2016 10:59 AM, Michael Ellerman wrote:
> On Thu, 2016-01-21 at 21:45 +0530, Vasant Hegde wrote:
> 
>> With commit 90a545e9 (restrict /dev/mem to idle io memory ranges) mapping
>> rtas_rmo_buf from user space is failing. Hence we are not able to make
>> RTAS syscall.
>>
>> This patch calls page_is_rtas_user_buf before calling iomem_is_exclusive
>> in  devmem_is_allowed(). This will allow user space to map rtas_rmo_buf
>> and we are able to make RTAS syscall.

Michael,

> 
> Thanks for the patch.
> 
> I'll put it in my fixes branch for next week.

Thanks!

> 
> 
> Having said that, why the <expletive deleted> is librtas mapping /dev/mem in
> the first place? Unless there is a very good reason, and probably even if there
> is, we should fix that to use a sane API.

We use rtas system call. We use /dev/mem interface to map the RTAS memory region
(allocated by kernel and information is passed to user space via procfs) so that
we can read/write to RTAS memory.

I do not have historical information. May be Nathan has more information on this.

-Vasant

Re: [PATCH] powerpc/mm: Allow user space to map rtas_rmo_buf

[Michael Ellerman]

On Fri, 2016-01-22 at 11:58 +0530, Vasant Hegde wrote:
> On 01/22/2016 10:59 AM, Michael Ellerman wrote:
> > On Thu, 2016-01-21 at 21:45 +0530, Vasant Hegde wrote:
> > 
> > > With commit 90a545e9 (restrict /dev/mem to idle io memory ranges) mapping
> > > rtas_rmo_buf from user space is failing. Hence we are not able to make
> > > RTAS syscall.
> > 
> > Having said that, why the <expletive deleted> is librtas mapping /dev/mem in
> > the first place? Unless there is a very good reason, and probably even if there
> > is, we should fix that to use a sane API.
> 
> We use rtas system call. We use /dev/mem interface to map the RTAS memory region
> (allocated by kernel and information is passed to user space via procfs) so that
> we can read/write to RTAS memory.
> 
> I do not have historical information. May be Nathan has more information on this.

Yeah, we need to dig into what it's actually doing and why. I had a quick look
but it wasn't obvious.

We should not need 1) a system call, 2) a proc interface, and 3) a mmap of
/dev/mem.

If the syscall's not sufficient and we really need to mmap, we should create a
device which can then be mmapped in a more standard way.

Having said that, Nathan's been moving more of the hotplug logic into the
kernel, so I'm also not clear on how much of the existing API we will need in
the future. So yep hopefully Nathan can chime in.

cheers

Re: [PATCH] powerpc/mm: Allow user space to map rtas_rmo_buf

[Denis Kirjanov]

On 1/22/16, Michael Ellerman <mpe@ellerman.id.au> wrote:
> On Fri, 2016-01-22 at 11:58 +0530, Vasant Hegde wrote:
>> On 01/22/2016 10:59 AM, Michael Ellerman wrote:
>> > On Thu, 2016-01-21 at 21:45 +0530, Vasant Hegde wrote:
>> >
>> > > With commit 90a545e9 (restrict /dev/mem to idle io memory ranges)
>> > > mapping
>> > > rtas_rmo_buf from user space is failing. Hence we are not able to make
>> > > RTAS syscall.
>> >
>> > Having said that, why the <expletive deleted> is librtas mapping
>> > /dev/mem in
>> > the first place? Unless there is a very good reason, and probably even
>> > if there
>> > is, we should fix that to use a sane API.
>>
>> We use rtas system call. We use /dev/mem interface to map the RTAS memory
>> region
>> (allocated by kernel and information is passed to user space via procfs)
>> so that
>> we can read/write to RTAS memory.
>>
>> I do not have historical information. May be Nathan has more information
>> on this.
>
> Yeah, we need to dig into what it's actually doing and why. I had a quick
> look
> but it wasn't obvious.
>
> We should not need 1) a system call, 2) a proc interface, and 3) a mmap of
> /dev/mem.
>
> If the syscall's not sufficient and we really need to mmap, we should create
> a
> device which can then be mmapped in a more standard way.
>
> Having said that, Nathan's been moving more of the hotplug logic into the
> kernel, so I'm also not clear on how much of the existing API we will need
> in
> the future. So yep hopefully Nathan can chime in.

Yeah, but if we're going to move to only one interface to work with
RTAS we can break existing applications.

>
> cheers
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] powerpc/mm: Allow user space to map rtas_rmo_buf

[Nathan Fontenot]

On 01/22/2016 04:41 AM, Denis Kirjanov wrote:
> On 1/22/16, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> On Fri, 2016-01-22 at 11:58 +0530, Vasant Hegde wrote:
>>> On 01/22/2016 10:59 AM, Michael Ellerman wrote:
>>>> On Thu, 2016-01-21 at 21:45 +0530, Vasant Hegde wrote:
>>>>
>>>>> With commit 90a545e9 (restrict /dev/mem to idle io memory ranges)
>>>>> mapping
>>>>> rtas_rmo_buf from user space is failing. Hence we are not able to make
>>>>> RTAS syscall.
>>>>
>>>> Having said that, why the <expletive deleted> is librtas mapping
>>>> /dev/mem in
>>>> the first place? Unless there is a very good reason, and probably even
>>>> if there
>>>> is, we should fix that to use a sane API.
>>>
>>> We use rtas system call. We use /dev/mem interface to map the RTAS memory
>>> region
>>> (allocated by kernel and information is passed to user space via procfs)
>>> so that
>>> we can read/write to RTAS memory.
>>>
>>> I do not have historical information. May be Nathan has more information
>>> on this.
>>
>> Yeah, we need to dig into what it's actually doing and why. I had a quick
>> look
>> but it wasn't obvious.
>>
>> We should not need 1) a system call, 2) a proc interface, and 3) a mmap of
>> /dev/mem.
>>
>> If the syscall's not sufficient and we really need to mmap, we should create
>> a
>> device which can then be mmapped in a more standard way.
>>
>> Having said that, Nathan's been moving more of the hotplug logic into the
>> kernel, so I'm also not clear on how much of the existing API we will need
>> in
>> the future. So yep hopefully Nathan can chime in.
> 
> Yeah, but if we're going to move to only one interface to work with
> RTAS we can break existing applications.
> 

Yes, but I doubt that anything other than librtas is using this. I don't think
this interface was ever documented anywhere.

-Nathan

>>
>> cheers
>>
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/linuxppc-dev
> 

Re: [PATCH] powerpc/mm: Allow user space to map rtas_rmo_buf

[Nathan Fontenot]

On 01/22/2016 02:35 AM, Michael Ellerman wrote:
> On Fri, 2016-01-22 at 11:58 +0530, Vasant Hegde wrote:
>> On 01/22/2016 10:59 AM, Michael Ellerman wrote:
>>> On Thu, 2016-01-21 at 21:45 +0530, Vasant Hegde wrote:
>>>
>>>> With commit 90a545e9 (restrict /dev/mem to idle io memory ranges) mapping
>>>> rtas_rmo_buf from user space is failing. Hence we are not able to make
>>>> RTAS syscall.
>>>
>>> Having said that, why the <expletive deleted> is librtas mapping /dev/mem in
>>> the first place? Unless there is a very good reason, and probably even if there
>>> is, we should fix that to use a sane API.
>>
>> We use rtas system call. We use /dev/mem interface to map the RTAS memory region
>> (allocated by kernel and information is passed to user space via procfs) so that
>> we can read/write to RTAS memory.
>>
>> I do not have historical information. May be Nathan has more information on this.
> 
> Yeah, we need to dig into what it's actually doing and why. I had a quick look
> but it wasn't obvious.

This was done many years ago, going on memory for now...

I will have to dig back into this code further but what I remember is that the
following process is used. At boot time a piece of low memory (buffers passed
to rtas have to be in low memory) is reserved to use for rtas calls. This memory
is made available to userspace, namely the librtas library. The code in librtas
will then reserve pieces of this memory to use for buffers when making rtas
calls that require large buffers. I think the biggest user of this is the rtas
configure-connector call which can require two 1-page buffers.

If I remember correctly this was done to avoid having the kernel do all of the
copying to/from user of the buffers used.


> 
> We should not need 1) a system call, 2) a proc interface, and 3) a mmap of
> /dev/mem.
> 

It seems that we could move to an interface that just uses a syscall and have
the kernel copy the buffers in from userspace into the kernels reserved rtas
buffer space.

> If the syscall's not sufficient and we really need to mmap, we should create a
> device which can then be mmapped in a more standard way.
> 
This would also work if having the kernel copy the buffers is not what we
want to do.

-Nathan

> Having said that, Nathan's been moving more of the hotplug logic into the
> kernel, so I'm also not clear on how much of the existing API we will need in
> the future. So yep hopefully Nathan can chime in.
> 
> cheers
> 

Re: powerpc/mm: Allow user space to map rtas_rmo_buf

[Michael Ellerman]

On Thu, 2016-21-01 at 16:15:31 UTC, Vasant Hegde wrote:
> With commit 90a545e9 (restrict /dev/mem to idle io memory ranges) mapping
> rtas_rmo_buf from user space is failing. Hence we are not able to make
> RTAS syscall.
> 
> This patch calls page_is_rtas_user_buf before calling iomem_is_exclusive
> in  devmem_is_allowed(). This will allow user space to map rtas_rmo_buf
> and we are able to make RTAS syscall.
> 
> Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> CC: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> CC: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> Acked-by: Dan Williams <dan.j.williams@intel.com>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/e256caa7d0515e301f8c8c6e7d

cheers