Re: [PATCH v3 4/4] fs/sysv: Replace kmap() with kmap_local_page()

Re: [PATCH v3 4/4] fs/sysv: Replace kmap() with kmap_local_page()

[Al Viro]

On Fri, Jan 20, 2023 at 05:07:48AM +0000, Al Viro wrote:
> On Fri, Jan 20, 2023 at 04:54:51AM +0000, Matthew Wilcox wrote:
> 
> > > Sure, but... there's also this:
> > > 
> > > static inline void __kunmap_local(const void *addr)
> > > {
> > > #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
> > >         kunmap_flush_on_unmap(addr);
> > > #endif
> > > }
> > > 
> > > Are you sure that the guts of that thing will be happy with address that is not
> > > page-aligned?  I've looked there at some point, got scared of parisc (IIRC)
> > > MMU details and decided not to rely upon that...
> > 
> > Ugh, PA-RISC (the only implementor) definitely will flush the wrong
> > addresses.  I think we should do this, as having bugs that only manifest
> > on one not-well-tested architecture seems Bad.
> > 
> >  static inline void __kunmap_local(const void *addr)
> >  {
> >  #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
> > -       kunmap_flush_on_unmap(addr);
> > +       kunmap_flush_on_unmap(PAGE_ALIGN_DOWN(addr));
> >  #endif
> >  }
> 
> PTR_ALIGN_DOWN(addr, PAGE_SIZE), perhaps?

	Anyway, that's a question to parisc folks; I _think_ pdtlb
quietly ignores the lower bits of address, so that part seems
to be safe, but I wouldn't bet upon that.  And when I got to
flush_kernel_dcache_page_asm I gave up - it's been a long time
since I've dealt with parisc assembler.

Re: [PATCH v3 4/4] fs/sysv: Replace kmap() with kmap_local_page()

[Helge Deller]

On 1/20/23 06:56, Al Viro wrote:
> On Fri, Jan 20, 2023 at 05:07:48AM +0000, Al Viro wrote:
>> On Fri, Jan 20, 2023 at 04:54:51AM +0000, Matthew Wilcox wrote:
>>
>>>> Sure, but... there's also this:
>>>>
>>>> static inline void __kunmap_local(const void *addr)
>>>> {
>>>> #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
>>>>          kunmap_flush_on_unmap(addr);
>>>> #endif
>>>> }
>>>>
>>>> Are you sure that the guts of that thing will be happy with address that is not
>>>> page-aligned?  I've looked there at some point, got scared of parisc (IIRC)
>>>> MMU details and decided not to rely upon that...
>>>
>>> Ugh, PA-RISC (the only implementor) definitely will flush the wrong
>>> addresses.  I think we should do this, as having bugs that only manifest
>>> on one not-well-tested architecture seems Bad.
>>>
>>>   static inline void __kunmap_local(const void *addr)
>>>   {
>>>   #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
>>> -       kunmap_flush_on_unmap(addr);
>>> +       kunmap_flush_on_unmap(PAGE_ALIGN_DOWN(addr));
>>>   #endif
>>>   }
>>
>> PTR_ALIGN_DOWN(addr, PAGE_SIZE), perhaps?
>
> 	Anyway, that's a question to parisc folks; I _think_ pdtlb
> quietly ignores the lower bits of address, so that part seems
> to be safe, but I wouldn't bet upon that.

No, on PA2.0 (64bit) CPUs the lower bits of the address of pdtlb
encodes the amount of memory (page size) to be flushed, see:
http://ftp.parisc-linux.org/docs/arch/parisc2.0.pdf (page 7-106)
So, the proposed page alignment with e.g. PTR_ALIGN_DON() is needed.

Helge

Re: [PATCH v3 4/4] fs/sysv: Replace kmap() with kmap_local_page()

[Ira Weiny]

Helge Deller wrote:
> On 1/20/23 06:56, Al Viro wrote:
> > On Fri, Jan 20, 2023 at 05:07:48AM +0000, Al Viro wrote:
> >> On Fri, Jan 20, 2023 at 04:54:51AM +0000, Matthew Wilcox wrote:
> >>
> >>>> Sure, but... there's also this:
> >>>>
> >>>> static inline void __kunmap_local(const void *addr)
> >>>> {
> >>>> #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
> >>>>          kunmap_flush_on_unmap(addr);
> >>>> #endif
> >>>> }
> >>>>
> >>>> Are you sure that the guts of that thing will be happy with address that is not
> >>>> page-aligned?  I've looked there at some point, got scared of parisc (IIRC)
> >>>> MMU details and decided not to rely upon that...
> >>>
> >>> Ugh, PA-RISC (the only implementor) definitely will flush the wrong
> >>> addresses.  I think we should do this, as having bugs that only manifest
> >>> on one not-well-tested architecture seems Bad.
> >>>
> >>>   static inline void __kunmap_local(const void *addr)
> >>>   {
> >>>   #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
> >>> -       kunmap_flush_on_unmap(addr);
> >>> +       kunmap_flush_on_unmap(PAGE_ALIGN_DOWN(addr));
> >>>   #endif
> >>>   }
> >>
> >> PTR_ALIGN_DOWN(addr, PAGE_SIZE), perhaps?
> >
> > 	Anyway, that's a question to parisc folks; I _think_ pdtlb
> > quietly ignores the lower bits of address, so that part seems
> > to be safe, but I wouldn't bet upon that.
> 
> No, on PA2.0 (64bit) CPUs the lower bits of the address of pdtlb
> encodes the amount of memory (page size) to be flushed, see:
> http://ftp.parisc-linux.org/docs/arch/parisc2.0.pdf (page 7-106)
> So, the proposed page alignment with e.g. PTR_ALIGN_DON() is needed.
> 
> Helge

I'm not sure I completely understand.

First, arn't PAGE_ALIGN_DOWN(addr) and PTR_ALIGN_DOWN(addr, PAGE_SIZE) the
same?

align.h
#define PTR_ALIGN_DOWN(p, a)    ((typeof(p))ALIGN_DOWN((unsigned long)(p), (a)))

mm.h:
#define PAGE_ALIGN_DOWN(addr) ALIGN_DOWN(addr, PAGE_SIZE)

Did parisc redefine it somewhere I'm not seeing?

Second, if the lower bits encode the amount of memory to be flushed is it
required to return the original value returned from page_address()?

Ira

Re: [PATCH v3 4/4] fs/sysv: Replace kmap() with kmap_local_page()

[Helge Deller]

On 1/21/23 09:05, Ira Weiny wrote:
> Helge Deller wrote:
>> On 1/20/23 06:56, Al Viro wrote:
>>> On Fri, Jan 20, 2023 at 05:07:48AM +0000, Al Viro wrote:
>>>> On Fri, Jan 20, 2023 at 04:54:51AM +0000, Matthew Wilcox wrote:
>>>>
>>>>>> Sure, but... there's also this:
>>>>>>
>>>>>> static inline void __kunmap_local(const void *addr)
>>>>>> {
>>>>>> #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
>>>>>>           kunmap_flush_on_unmap(addr);
>>>>>> #endif
>>>>>> }
>>>>>>
>>>>>> Are you sure that the guts of that thing will be happy with address that is not
>>>>>> page-aligned?  I've looked there at some point, got scared of parisc (IIRC)
>>>>>> MMU details and decided not to rely upon that...
>>>>>
>>>>> Ugh, PA-RISC (the only implementor) definitely will flush the wrong
>>>>> addresses.  I think we should do this, as having bugs that only manifest
>>>>> on one not-well-tested architecture seems Bad.
>>>>>
>>>>>    static inline void __kunmap_local(const void *addr)
>>>>>    {
>>>>>    #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
>>>>> -       kunmap_flush_on_unmap(addr);
>>>>> +       kunmap_flush_on_unmap(PAGE_ALIGN_DOWN(addr));
>>>>>    #endif
>>>>>    }
>>>>
>>>> PTR_ALIGN_DOWN(addr, PAGE_SIZE), perhaps?
>>>
>>> 	Anyway, that's a question to parisc folks; I _think_ pdtlb
>>> quietly ignores the lower bits of address, so that part seems
>>> to be safe, but I wouldn't bet upon that.
>>
>> No, on PA2.0 (64bit) CPUs the lower bits of the address of pdtlb
>> encodes the amount of memory (page size) to be flushed, see:
>> http://ftp.parisc-linux.org/docs/arch/parisc2.0.pdf (page 7-106)
>> So, the proposed page alignment with e.g. PTR_ALIGN_DON() is needed.
>>
>
> I'm not sure I completely understand.
>
> First, arn't PAGE_ALIGN_DOWN(addr) and PTR_ALIGN_DOWN(addr, PAGE_SIZE) the
> same?

Yes, they are.

> align.h
> #define PTR_ALIGN_DOWN(p, a)    ((typeof(p))ALIGN_DOWN((unsigned long)(p), (a)))
>
> mm.h:
> #define PAGE_ALIGN_DOWN(addr) ALIGN_DOWN(addr, PAGE_SIZE)
>
> Did parisc redefine it somewhere I'm not seeing?

No, there is nothing special in this regard on parisc.

> Second, if the lower bits encode the amount of memory to be flushed is it
> required to return the original value returned from page_address()?

No.
If the lower bits are zero, then the default page size (4k) is used for the tlb purge.
So, if you simply strip the lower bits (by using PAGE_ALIGN_DOWN() or ALIGN_DOWN())
you are fine.

Helge

Re: [PATCH v3 4/4] fs/sysv: Replace kmap() with kmap_local_page()

[Al Viro]

On Sat, Jan 21, 2023 at 12:05:58AM -0800, Ira Weiny wrote:

> First, arn't PAGE_ALIGN_DOWN(addr) and PTR_ALIGN_DOWN(addr, PAGE_SIZE) the
> same?
> 
> align.h
> #define PTR_ALIGN_DOWN(p, a)    ((typeof(p))ALIGN_DOWN((unsigned long)(p), (a)))
> 
> mm.h:
> #define PAGE_ALIGN_DOWN(addr) ALIGN_DOWN(addr, PAGE_SIZE)

... and ALIGN_DOWN ends up with doing bitwise and on the first argument.
Which doesn't work for pointers, thus the separate variant for those
and typecast to unsigned long in it...

Re: [PATCH v3 4/4] fs/sysv: Replace kmap() with kmap_local_page()

[Fabio M. De Francesco]

On venerdì 20 gennaio 2023 06:56:01 CET Al Viro wrote:
> On Fri, Jan 20, 2023 at 05:07:48AM +0000, Al Viro wrote:
> > On Fri, Jan 20, 2023 at 04:54:51AM +0000, Matthew Wilcox wrote:
> > > > Sure, but... there's also this:
> > > > 
> > > > static inline void __kunmap_local(const void *addr)
> > > > {
> > > > #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
> > > > 
> > > >         kunmap_flush_on_unmap(addr);
> > > > 
> > > > #endif
> > > > }
> > > > 
> > > > Are you sure that the guts of that thing will be happy with address 
that
> > > > is not page-aligned?  I've looked there at some point, got scared of
> > > > parisc (IIRC) MMU details and decided not to rely upon that...
> > > 
> > > Ugh, PA-RISC (the only implementer) definitely will flush the wrong
> > > addresses.  I think we should do this, as having bugs that only manifest
> > > on one not-well-tested architecture seems Bad.
> > > 
> > >  static inline void __kunmap_local(const void *addr)
> > >  {
> > >  #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
> > > 
> > > -       kunmap_flush_on_unmap(addr);
> > > +       kunmap_flush_on_unmap(PAGE_ALIGN_DOWN(addr));
> > > 
> > >  #endif
> > >  }
> > 
> > PTR_ALIGN_DOWN(addr, PAGE_SIZE), perhaps?
> 
> 	Anyway, that's a question to parisc folks; I _think_ pdtlb
> quietly ignores the lower bits of address, so that part seems
> to be safe, but I wouldn't bet upon that.  And when I got to
> flush_kernel_dcache_page_asm I gave up - it's been a long time
> since I've dealt with parisc assembler.

There seems to be consensus that __kunmap_local() needs to be fixed for the 
parisc case (ARCH_HAS_FLUSH_ON_KUNMAP).

Is anyone doing this task?

If you agree, I could make this change and give proper credits for the tip.

Thank you,

Fabio

Re: [PATCH v3 4/4] fs/sysv: Replace kmap() with kmap_local_page()

[Ira Weiny]

Fabio M. De Francesco wrote:
> On venerdì 20 gennaio 2023 06:56:01 CET Al Viro wrote:
> > On Fri, Jan 20, 2023 at 05:07:48AM +0000, Al Viro wrote:
> > > On Fri, Jan 20, 2023 at 04:54:51AM +0000, Matthew Wilcox wrote:
> > > > > Sure, but... there's also this:
> > > > > 
> > > > > static inline void __kunmap_local(const void *addr)
> > > > > {
> > > > > #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
> > > > > 
> > > > >         kunmap_flush_on_unmap(addr);
> > > > > 
> > > > > #endif
> > > > > }
> > > > > 
> > > > > Are you sure that the guts of that thing will be happy with address 
> that
> > > > > is not page-aligned?  I've looked there at some point, got scared of
> > > > > parisc (IIRC) MMU details and decided not to rely upon that...
> > > > 
> > > > Ugh, PA-RISC (the only implementer) definitely will flush the wrong
> > > > addresses.  I think we should do this, as having bugs that only manifest
> > > > on one not-well-tested architecture seems Bad.
> > > > 
> > > >  static inline void __kunmap_local(const void *addr)
> > > >  {
> > > >  #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
> > > > 
> > > > -       kunmap_flush_on_unmap(addr);
> > > > +       kunmap_flush_on_unmap(PAGE_ALIGN_DOWN(addr));
> > > > 
> > > >  #endif
> > > >  }
> > > 
> > > PTR_ALIGN_DOWN(addr, PAGE_SIZE), perhaps?
> > 
> > 	Anyway, that's a question to parisc folks; I _think_ pdtlb
> > quietly ignores the lower bits of address, so that part seems
> > to be safe, but I wouldn't bet upon that.  And when I got to
> > flush_kernel_dcache_page_asm I gave up - it's been a long time
> > since I've dealt with parisc assembler.
> 
> There seems to be consensus that __kunmap_local() needs to be fixed for the 
> parisc case (ARCH_HAS_FLUSH_ON_KUNMAP).
> 
> Is anyone doing this task?

I'm not looking at it.

> 
> If you agree, I could make this change and give proper credits for the tip.

I think that would be great.

Thanks!
Ira

> 
> Thank you,
> 
> Fabio
> 
> 
>