Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1631802 > unrolled thread

Re: [PATCH v3 1/4] x86/vm86/32: Switch to flush_tlb_mm_range() in mark_screen_rdonly()

Started byNadav Amit <namit@vmware.com>
First post2017-04-27 02:00 +0200
Last post2017-04-28 00:20 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 1/4] x86/vm86/32: Switch to flush_tlb_mm_range() in  mark_screen_rdonly() Nadav Amit <namit@vmware.com> - 2017-04-27 02:00 +0200
    Re: [PATCH v3 1/4] x86/vm86/32: Switch to flush_tlb_mm_range() in  mark_screen_rdonly() Nadav Amit <namit@vmware.com> - 2017-04-27 02:10 +0200
      Re: [PATCH v3 1/4] x86/vm86/32: Switch to flush_tlb_mm_range() in mark_screen_rdonly() Andy Lutomirski <luto@kernel.org> - 2017-04-27 18:10 +0200
        Re: [PATCH v3 1/4] x86/vm86/32: Switch to flush_tlb_mm_range() in  mark_screen_rdonly() Stas Sergeev <stsp@list.ru> - 2017-04-28 00:20 +0200
        Re: [PATCH v3 1/4] x86/vm86/32: Switch to flush_tlb_mm_range() in  mark_screen_rdonly() Stas Sergeev <stsp@list.ru> - 2017-04-28 00:20 +0200

#1631802 — Re: [PATCH v3 1/4] x86/vm86/32: Switch to flush_tlb_mm_range() in mark_screen_rdonly()

FromNadav Amit <namit@vmware.com>
Date2017-04-27 02:00 +0200
SubjectRe: [PATCH v3 1/4] x86/vm86/32: Switch to flush_tlb_mm_range() in mark_screen_rdonly()
Message-ID<tAF0J-Ug-1@gated-at.bofh.it>
It may be benign, but I don’t think that flushing the TLB without
holding the ptl or the mmap_sem (for no apparent reason) is a good
practice.

On 4/22/17, 12:01 AM, "Andy Lutomirski" <luto@kernel.org> wrote:

    mark_screen_rdonly() is the last remaining caller of flush_tlb().
    flush_tlb_mm_range() is potentially faster and isn't obsolete.
    
    Compile-tested only because I don't know whether software that uses
    this mechanism even exists.
    
    Cc: Rik van Riel <riel@redhat.com>
    Cc: Dave Hansen <dave.hansen@intel.com>
    Cc: Nadav Amit <namit@vmware.com>
    Cc: Michal Hocko <mhocko@suse.com>
    Cc: Sasha Levin <sasha.levin@oracle.com>
    Cc: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Andy Lutomirski <luto@kernel.org>
    ---
     arch/x86/kernel/vm86_32.c | 2 +-
     1 file changed, 1 insertion(+), 1 deletion(-)
    
    diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
    index 23ee89ce59a9..3eda76b3c835 100644
    --- a/arch/x86/kernel/vm86_32.c
    +++ b/arch/x86/kernel/vm86_32.c
    @@ -193,7 +193,7 @@ static void mark_screen_rdonly(struct mm_struct *mm)
     	pte_unmap_unlock(pte, ptl);
     out:
     	up_write(&mm->mmap_sem);
    -	flush_tlb();
    +	flush_tlb_mm_range(mm, 0xA0000, 0xA0000 + 32*PAGE_SIZE, 0UL);
     }
     
     
    -- 
    2.9.3

[toc] | [next] | [standalone]

#1631803

FromNadav Amit <namit@vmware.com>
Date2017-04-27 02:10 +0200
Message-ID<tAFap-1ct-3@gated-at.bofh.it>
In reply to#1631802
And besides, it looks as if the code was meant to flush the entire
TLB in some cases (e.g., if pgd_none_or_clear_bad() is true).

On 4/26/17, 4:56 PM, "Nadav Amit" <namit@vmware.com> wrote:

    It may be benign, but I don’t think that flushing the TLB without
    holding the ptl or the mmap_sem (for no apparent reason) is a good
    practice.
    
    On 4/22/17, 12:01 AM, "Andy Lutomirski" <luto@kernel.org> wrote:
    
        mark_screen_rdonly() is the last remaining caller of flush_tlb().
        flush_tlb_mm_range() is potentially faster and isn't obsolete.
        
        Compile-tested only because I don't know whether software that uses
        this mechanism even exists.
        
        Cc: Rik van Riel <riel@redhat.com>
        Cc: Dave Hansen <dave.hansen@intel.com>
        Cc: Nadav Amit <namit@vmware.com>
        Cc: Michal Hocko <mhocko@suse.com>
        Cc: Sasha Levin <sasha.levin@oracle.com>
        Cc: Andrew Morton <akpm@linux-foundation.org>
        Signed-off-by: Andy Lutomirski <luto@kernel.org>
        ---
         arch/x86/kernel/vm86_32.c | 2 +-
         1 file changed, 1 insertion(+), 1 deletion(-)
        
        diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
        index 23ee89ce59a9..3eda76b3c835 100644
        --- a/arch/x86/kernel/vm86_32.c
        +++ b/arch/x86/kernel/vm86_32.c
        @@ -193,7 +193,7 @@ static void mark_screen_rdonly(struct mm_struct *mm)
         	pte_unmap_unlock(pte, ptl);
         out:
         	up_write(&mm->mmap_sem);
        -	flush_tlb();
        +	flush_tlb_mm_range(mm, 0xA0000, 0xA0000 + 32*PAGE_SIZE, 0UL);
         }
         
         
        -- 
        2.9.3

[toc] | [prev] | [next] | [standalone]

#1632225 — Re: [PATCH v3 1/4] x86/vm86/32: Switch to flush_tlb_mm_range() in mark_screen_rdonly()

FromAndy Lutomirski <luto@kernel.org>
Date2017-04-27 18:10 +0200
SubjectRe: [PATCH v3 1/4] x86/vm86/32: Switch to flush_tlb_mm_range() in mark_screen_rdonly()
Message-ID<tAU9s-2Zz-13@gated-at.bofh.it>
In reply to#1631803
On Wed, Apr 26, 2017 at 5:02 PM, Nadav Amit <namit@vmware.com> wrote:
> And besides, it looks as if the code was meant to flush the entire
> TLB in some cases (e.g., if pgd_none_or_clear_bad() is true).
>
> On 4/26/17, 4:56 PM, "Nadav Amit" <namit@vmware.com> wrote:
>
>     It may be benign, but I don’t think that flushing the TLB without
>     holding the ptl or the mmap_sem (for no apparent reason) is a good
>     practice.
>
>     On 4/22/17, 12:01 AM, "Andy Lutomirski" <luto@kernel.org> wrote:
>
>         mark_screen_rdonly() is the last remaining caller of flush_tlb().
>         flush_tlb_mm_range() is potentially faster and isn't obsolete.
>
>         Compile-tested only because I don't know whether software that uses
>         this mechanism even exists.
>
>         Cc: Rik van Riel <riel@redhat.com>
>         Cc: Dave Hansen <dave.hansen@intel.com>
>         Cc: Nadav Amit <namit@vmware.com>
>         Cc: Michal Hocko <mhocko@suse.com>
>         Cc: Sasha Levin <sasha.levin@oracle.com>
>         Cc: Andrew Morton <akpm@linux-foundation.org>
>         Signed-off-by: Andy Lutomirski <luto@kernel.org>
>         ---
>          arch/x86/kernel/vm86_32.c | 2 +-
>          1 file changed, 1 insertion(+), 1 deletion(-)
>
>         diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
>         index 23ee89ce59a9..3eda76b3c835 100644
>         --- a/arch/x86/kernel/vm86_32.c
>         +++ b/arch/x86/kernel/vm86_32.c
>         @@ -193,7 +193,7 @@ static void mark_screen_rdonly(struct mm_struct *mm)
>                 pte_unmap_unlock(pte, ptl);
>          out:
>                 up_write(&mm->mmap_sem);
>         -       flush_tlb();
>         +       flush_tlb_mm_range(mm, 0xA0000, 0xA0000 + 32*PAGE_SIZE, 0UL);
>          }
>

Those should probably be pgd_none(), not pgd_none_or_clear_bad().

But this whole function is just garbage.  It mucks with page
protections without even looking up the VMA.  What happens if the
pages are file-backed?  How about chardevs?

I'd like to delete it.  Stas, do you know if there's any code at all
that uses VM86_SCREEN_BITMAP?  Some Googling didn't turn any up at
all.

[toc] | [prev] | [next] | [standalone]

#1632407

FromStas Sergeev <stsp@list.ru>
Date2017-04-28 00:20 +0200
Message-ID<tAZVv-6VL-7@gated-at.bofh.it>
In reply to#1632225
27.04.2017 19:08, Andy Lutomirski пишет:
> Those should probably be pgd_none(), not pgd_none_or_clear_bad().
>
> But this whole function is just garbage.  It mucks with page
> protections without even looking up the VMA.  What happens if the
> pages are file-backed?  How about chardevs?
>
> I'd like to delete it.  Stas, do you know if there's any code at all
> that uses VM86_SCREEN_BITMAP?  Some Googling didn't turn any up at
> all.
dosemu1 has this:
https://sourceforge.net/p/dosemu/code/ci/master/tree/src/env/video/video.c
Scroll down to line 255.
---

#if VIDEO_CHECK_DIRTY
if (!config_dualmon) {
vm86s.flags |= VM86_SCREEN_BITMAP;
}
#endif --- The check expands to "if 0": 
https://sourceforge.net/p/dosemu/code/ci/master/tree/src/include/video.h 
line 27: ---

#define VIDEO_CHECK_DIRTY 0 --- Plus, in video.c you can see the comment 
that basically says that this functionality was of no use (not sure what 
exactly they were saying though). dosemu2 has no traces of this code at 
all. So perfectly fine with me if you remove it. In fact, I've cleaned 
up dosemu2 from any fancy stuff of vm86(), so probably more cleanups are 
possible on kernel side. I even wanted to switch to vm86old() if not for 
the very nasty bug that vm86old() generates SIGTRAP when int3 is called 
in v86. If this is fixed (and its a 1-line fix), we can remove entire 
vm86(). :)

[toc] | [prev] | [next] | [standalone]

#1632409

FromStas Sergeev <stsp@list.ru>
Date2017-04-28 00:20 +0200
Message-ID<tAZVw-6VL-11@gated-at.bofh.it>
In reply to#1632225
27.04.2017 19:08, Andy Lutomirski пишет:
> Those should probably be pgd_none(), not pgd_none_or_clear_bad().
>
> But this whole function is just garbage.  It mucks with page
> protections without even looking up the VMA.  What happens if the
> pages are file-backed?  How about chardevs?
>
> I'd like to delete it.  Stas, do you know if there's any code at all
> that uses VM86_SCREEN_BITMAP?  Some Googling didn't turn any up at
> all.
dosemu1 has this:
https://sourceforge.net/p/dosemu/code/ci/master/tree/src/env/video/video.c
Scroll down to line 255.

---

#if VIDEO_CHECK_DIRTY
if (!config_dualmon) {
vm86s.flags |= VM86_SCREEN_BITMAP;
}
#endif

---


The check expands to "if 0": 
https://sourceforge.net/p/dosemu/code/ci/master/tree/src/include/video.h 
line 27:

---

#define VIDEO_CHECK_DIRTY 0

---

Plus, in video.c you can see the comment that basically says that this 
functionality was of no use (not sure what exactly they were saying 
though). dosemu2 has no traces of this code at all.

So perfectly fine with me if you remove it. In fact, I've cleaned up 
dosemu2 from any fancy stuff of vm86(), so probably more cleanups are 
possible on kernel side. I even wanted to switch to vm86old() if not for 
the very nasty bug that vm86old() generates SIGTRAP when int3 is called 
in v86. If this is fixed (and its a 1-line fix), we can remove entire 
vm86(). :)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web