LinuxLists.cc - [PATCH] x86/mm: fix regression with huge pages on PAE

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

On Mon, Nov 09, 2015 at 04:43:11PM -0700, Toshi Kani wrote:
> On Tue, 2015-11-10 at 01:18 +0200, Kirill A. Shutemov wrote:
> > Recent PAT patchset has caused issue on 32-bit PAE machines:
> :
> > The problem is in pmd_pfn_mask() and pmd_flags_mask(). These helpers use
> > PMD_PAGE_MASK to calculate resulting mask. PMD_PAGE_MASK is 'unsigned
> > long', not 'unsigned long long' as physaddr_t. As result upper bits of
> > resulting mask is truncated.
> >
> > The patch reworks code to use PMD_SHIFT as base of mask calculation
> > instead of PMD_PAGE_MASK.
> >
> > pud_pfn_mask() and pud_flags_mask() aren't problematic since we don't
> > have PUD page table level on 32-bit systems, but they reworked too to be
> > consistent with PMD counterpart.
> >
> > Signed-off-by: Kirill A. Shutemov <[email protected]>
> > Reported-and-Tested-by: Boris Ostrovsky <[email protected]>
> > Fixes: f70abb0fc3da ("x86/asm: Fix pud/pmd interfaces to handle large PAT
> > bit")
> > Cc: Toshi Kani <[email protected]>
> > ---
> > arch/x86/include/asm/pgtable_types.h | 14 ++++----------
> > 1 file changed, 4 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/pgtable_types.h
> > b/arch/x86/include/asm/pgtable_types.h
> > index dd5b0aa9dd2f..c1e797266ce9 100644
> > --- a/arch/x86/include/asm/pgtable_types.h
> > +++ b/arch/x86/include/asm/pgtable_types.h
> > @@ -279,17 +279,14 @@ static inline pmdval_t native_pmd_val(pmd_t pmd)
> > static inline pudval_t pud_pfn_mask(pud_t pud)
> > {
> > if (native_pud_val(pud) & _PAGE_PSE)
> > - return PUD_PAGE_MASK & PHYSICAL_PAGE_MASK;
> > + return ~((1ULL << PUD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
>
> Thanks for the fix! Should we fix the PMD/PUD MASK/SIZE macros, so that we do
> not hit the same issue again when they are used?

I don't this so. PAGE_SIZE is not 'unsigned long long'. And all *PAGE_MASK
are usually applied to virtual addresses which are 'unsigned long'.
I think it's safer to leave them as they are.

>
> --- a/arch/x86/include/asm/page_types.h
> +++ b/arch/x86/include/asm/page_types.h
> @@ -17,10 +17,10 @@
> (ie, 32-bit PAE). */
> #define PHYSICAL_PAGE_MASK (((signed long)PAGE_MASK) & __PHYSICAL_MASK)
>
> -#define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT)
> +#define PMD_PAGE_SIZE (_AC(1, ULL) << PMD_SHIFT)
> #define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1))
>
> -#define PUD_PAGE_SIZE (_AC(1, UL) << PUD_SHIFT)
> +#define PUD_PAGE_SIZE (_AC(1, ULL) << PUD_SHIFT)
> #define PUD_PAGE_MASK (~(PUD_PAGE_SIZE-1))
>
> Thanks,
> -Toshi
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

--
Kirill A. Shutemov

2015-11-10 00:16:59

by Kani, Toshimitsu

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

On Tue, 2015-11-10 at 01:57 +0200, Kirill A. Shutemov wrote:
> On Mon, Nov 09, 2015 at 04:43:11PM -0700, Toshi Kani wrote:
> > On Tue, 2015-11-10 at 01:18 +0200, Kirill A. Shutemov wrote:
> > > Recent PAT patchset has caused issue on 32-bit PAE machines:
> > :
> > > The problem is in pmd_pfn_mask() and pmd_flags_mask(). These helpers use
> > > PMD_PAGE_MASK to calculate resulting mask. PMD_PAGE_MASK is 'unsigned
> > > long', not 'unsigned long long' as physaddr_t. As result upper bits of
> > > resulting mask is truncated.
> > >
> > > The patch reworks code to use PMD_SHIFT as base of mask calculation
> > > instead of PMD_PAGE_MASK.
> > >
> > > pud_pfn_mask() and pud_flags_mask() aren't problematic since we don't
> > > have PUD page table level on 32-bit systems, but they reworked too to be
> > > consistent with PMD counterpart.
> > >
> > > Signed-off-by: Kirill A. Shutemov <[email protected]>
> > > Reported-and-Tested-by: Boris Ostrovsky <[email protected]>
> > > Fixes: f70abb0fc3da ("x86/asm: Fix pud/pmd interfaces to handle large PAT
> > > bit")
> > > Cc: Toshi Kani <[email protected]>
> > > ---
> > > arch/x86/include/asm/pgtable_types.h | 14 ++++----------
> > > 1 file changed, 4 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/pgtable_types.h
> > > b/arch/x86/include/asm/pgtable_types.h
> > > index dd5b0aa9dd2f..c1e797266ce9 100644
> > > --- a/arch/x86/include/asm/pgtable_types.h
> > > +++ b/arch/x86/include/asm/pgtable_types.h
> > > @@ -279,17 +279,14 @@ static inline pmdval_t native_pmd_val(pmd_t pmd)
> > > static inline pudval_t pud_pfn_mask(pud_t pud)
> > > {
> > > if (native_pud_val(pud) & _PAGE_PSE)
> > > - return PUD_PAGE_MASK & PHYSICAL_PAGE_MASK;
> > > + return ~((1ULL << PUD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
> >
> > Thanks for the fix! Should we fix the PMD/PUD MASK/SIZE macros, so that we
> > do not hit the same issue again when they are used?
>
> I don't this so. PAGE_SIZE is not 'unsigned long long'. And all *PAGE_MASK
> are usually applied to virtual addresses which are 'unsigned long'.
> I think it's safer to leave them as they are.

Thanks for the explanation! That makes sense.

Reviewed-by: Toshi Kani <[email protected]>

-Toshi

2015-11-10 12:42:48

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

On Tue, Nov 10, 2015 at 01:18:10AM +0200, Kirill A. Shutemov wrote:
> diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
> index dd5b0aa9dd2f..c1e797266ce9 100644
> --- a/arch/x86/include/asm/pgtable_types.h
> +++ b/arch/x86/include/asm/pgtable_types.h
> @@ -279,17 +279,14 @@ static inline pmdval_t native_pmd_val(pmd_t pmd)
> static inline pudval_t pud_pfn_mask(pud_t pud)
> {
> if (native_pud_val(pud) & _PAGE_PSE)
> - return PUD_PAGE_MASK & PHYSICAL_PAGE_MASK;
> + return ~((1ULL << PUD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;

In file included from ./arch/x86/include/asm/boot.h:5:0,
from ./arch/x86/boot/boot.h:26,
from arch/x86/realmode/rm/wakemain.c:2:
./arch/x86/include/asm/pgtable_types.h: In function ‘pud_pfn_mask’:
./arch/x86/include/asm/pgtable_types.h:282:10: warning: large integer implicitly truncated to unsigned type [-Woverflow]
return ~((1ULL << PUD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
^
./arch/x86/include/asm/pgtable_types.h: In function ‘pmd_pfn_mask’:
./arch/x86/include/asm/pgtable_types.h:300:10: warning: large integer implicitly truncated to unsigned type [-Woverflow]
return ~((1ULL << PMD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
^
In file included from ./arch/x86/include/asm/boot.h:5:0,
from arch/x86/realmode/rm/../../boot/boot.h:26,
from arch/x86/realmode/rm/../../boot/video-mode.c:18,
from arch/x86/realmode/rm/video-mode.c:1:
./arch/x86/include/asm/pgtable_types.h: In function ‘pud_pfn_mask’:
./arch/x86/include/asm/pgtable_types.h:282:10: warning: large integer implicitly truncated to unsigned type [-Woverflow]
return ~((1ULL << PUD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
^
...

That's a 64-bit config.

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

2015-11-10 13:53:09

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

On Tue, Nov 10, 2015 at 01:34:29PM +0100, Borislav Petkov wrote:
> On Tue, Nov 10, 2015 at 01:18:10AM +0200, Kirill A. Shutemov wrote:
> > diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
> > index dd5b0aa9dd2f..c1e797266ce9 100644
> > --- a/arch/x86/include/asm/pgtable_types.h
> > +++ b/arch/x86/include/asm/pgtable_types.h
> > @@ -279,17 +279,14 @@ static inline pmdval_t native_pmd_val(pmd_t pmd)
> > static inline pudval_t pud_pfn_mask(pud_t pud)
> > {
> > if (native_pud_val(pud) & _PAGE_PSE)
> > - return PUD_PAGE_MASK & PHYSICAL_PAGE_MASK;
> > + return ~((1ULL << PUD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
>
> In file included from ./arch/x86/include/asm/boot.h:5:0,
> from ./arch/x86/boot/boot.h:26,
> from arch/x86/realmode/rm/wakemain.c:2:
> ./arch/x86/include/asm/pgtable_types.h: In function ‘pud_pfn_mask’:
> ./arch/x86/include/asm/pgtable_types.h:282:10: warning: large integer implicitly truncated to unsigned type [-Woverflow]
> return ~((1ULL << PUD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
> ^
> ./arch/x86/include/asm/pgtable_types.h: In function ‘pmd_pfn_mask’:
> ./arch/x86/include/asm/pgtable_types.h:300:10: warning: large integer implicitly truncated to unsigned type [-Woverflow]
> return ~((1ULL << PMD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
> ^
> In file included from ./arch/x86/include/asm/boot.h:5:0,
> from arch/x86/realmode/rm/../../boot/boot.h:26,
> from arch/x86/realmode/rm/../../boot/video-mode.c:18,
> from arch/x86/realmode/rm/video-mode.c:1:
> ./arch/x86/include/asm/pgtable_types.h: In function ‘pud_pfn_mask’:
> ./arch/x86/include/asm/pgtable_types.h:282:10: warning: large integer implicitly truncated to unsigned type [-Woverflow]
> return ~((1ULL << PUD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
> ^
> ...
>
> That's a 64-bit config.

Oh.. pmdval_t/pudval_t is 'unsinged long' on 64 bit. But realmode code
uses -m16 which makes 'unsigned long' 32-bit therefore truncation warning.

These helpers not really used in realmode code. I see few ways out:

- convert helpers to macros to avoid their translation;

- wrap the code into not-for-realmode ifdef. (Do we have any?);

- convert pudval_t/pmdval_t to u64 for 64-bit. I'm not sure what side
effects would it have.

Any opinions?

--
Kirill A. Shutemov

2015-11-10 14:47:04

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

On Tue, Nov 10, 2015 at 03:53:03PM +0200, Kirill A. Shutemov wrote:
> Oh.. pmdval_t/pudval_t is 'unsinged long' on 64 bit. But realmode code
> uses -m16 which makes 'unsigned long' 32-bit therefore truncation warning.
>
> These helpers not really used in realmode code.

Hrrm, yeah, that's just the nasty include hell causing it. The diff
below fixes it with my config but it'll probably need a more careful
analysis and reshuffling of includes/defines.

Certainly better to do that than accomodating realmode to not throw
warnings with ifdeffery...

---
diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index 0033e96c3f09..9011a88353de 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -23,7 +23,6 @@
#include <stdarg.h>
#include <linux/types.h>
#include <linux/edd.h>
-#include <asm/boot.h>
#include <asm/setup.h>
#include "bitops.h"
#include "ctype.h"
diff --git a/arch/x86/boot/video-mode.c b/arch/x86/boot/video-mode.c
index aa8a96b052e3..896077ed3381 100644
--- a/arch/x86/boot/video-mode.c
+++ b/arch/x86/boot/video-mode.c
@@ -19,6 +19,9 @@
#include "video.h"
#include "vesa.h"

+#define NORMAL_VGA 0xffff /* 80x25 mode */
+#define EXTENDED_VGA 0xfffe /* 80x50 mode */
+
/*
* Common variables
*/
diff --git a/arch/x86/boot/video.c b/arch/x86/boot/video.c
index 05111bb8d018..a839448038b6 100644
--- a/arch/x86/boot/video.c
+++ b/arch/x86/boot/video.c
@@ -17,6 +17,8 @@
#include "video.h"
#include "vesa.h"

+#define ASK_VGA 0xfffd /* ask for it at bootup */
+
static u16 video_segment;

static void store_cursor_position(void)
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 48d34d28f5a6..cd0fc0cc78bc 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -1,7 +1,6 @@
#ifndef _ASM_X86_PLATFORM_H
#define _ASM_X86_PLATFORM_H

-#include <asm/pgtable_types.h>
#include <asm/bootparam.h>

struct mpc_bus;

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

2015-11-10 15:07:17

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

On Tue, Nov 10, 2015 at 03:46:48PM +0100, Borislav Petkov wrote:
> On Tue, Nov 10, 2015 at 03:53:03PM +0200, Kirill A. Shutemov wrote:
> > Oh.. pmdval_t/pudval_t is 'unsinged long' on 64 bit. But realmode code
> > uses -m16 which makes 'unsigned long' 32-bit therefore truncation warning.
> >
> > These helpers not really used in realmode code.
>
> Hrrm, yeah, that's just the nasty include hell causing it. The diff
> below fixes it with my config but it'll probably need a more careful
> analysis and reshuffling of includes/defines.
>
> Certainly better to do that than accomodating realmode to not throw
> warnings with ifdeffery...

Yeah. Looks good to me.

> ---
> diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
> index 0033e96c3f09..9011a88353de 100644
> --- a/arch/x86/boot/boot.h
> +++ b/arch/x86/boot/boot.h
> @@ -23,7 +23,6 @@
> #include <stdarg.h>
> #include <linux/types.h>
> #include <linux/edd.h>
> -#include <asm/boot.h>
> #include <asm/setup.h>
> #include "bitops.h"
> #include "ctype.h"
> diff --git a/arch/x86/boot/video-mode.c b/arch/x86/boot/video-mode.c
> index aa8a96b052e3..896077ed3381 100644
> --- a/arch/x86/boot/video-mode.c
> +++ b/arch/x86/boot/video-mode.c
> @@ -19,6 +19,9 @@
> #include "video.h"
> #include "vesa.h"
>
> +#define NORMAL_VGA 0xffff /* 80x25 mode */
> +#define EXTENDED_VGA 0xfffe /* 80x50 mode */
> +
> /*
> * Common variables
> */
> diff --git a/arch/x86/boot/video.c b/arch/x86/boot/video.c
> index 05111bb8d018..a839448038b6 100644
> --- a/arch/x86/boot/video.c
> +++ b/arch/x86/boot/video.c
> @@ -17,6 +17,8 @@
> #include "video.h"
> #include "vesa.h"
>
> +#define ASK_VGA 0xfffd /* ask for it at bootup */
> +
> static u16 video_segment;
>
> static void store_cursor_position(void)
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index 48d34d28f5a6..cd0fc0cc78bc 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -1,7 +1,6 @@
> #ifndef _ASM_X86_PLATFORM_H
> #define _ASM_X86_PLATFORM_H
>
> -#include <asm/pgtable_types.h>
> #include <asm/bootparam.h>
>
> struct mpc_bus;
>
> --
> Regards/Gruss,
> Boris.
>
> ECO tip #101: Trim your mails when you reply.

--
Kirill A. Shutemov

2015-11-10 17:05:03

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

On Tue, Nov 10, 2015 at 05:07:13PM +0200, Kirill A. Shutemov wrote:
> Yeah. Looks good to me.

It gets even cleaner. Let me run it through the *config build tests.

---
diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index 0033e96c3f09..9011a88353de 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -23,7 +23,6 @@
#include <stdarg.h>
#include <linux/types.h>
#include <linux/edd.h>
-#include <asm/boot.h>
#include <asm/setup.h>
#include "bitops.h"
#include "ctype.h"
diff --git a/arch/x86/boot/video-mode.c b/arch/x86/boot/video-mode.c
index aa8a96b052e3..95c7a818c0ed 100644
--- a/arch/x86/boot/video-mode.c
+++ b/arch/x86/boot/video-mode.c
@@ -19,6 +19,8 @@
#include "video.h"
#include "vesa.h"

+#include <uapi/asm/boot.h>
+
/*
* Common variables
*/
diff --git a/arch/x86/boot/video.c b/arch/x86/boot/video.c
index 05111bb8d018..77780e386e9b 100644
--- a/arch/x86/boot/video.c
+++ b/arch/x86/boot/video.c
@@ -13,6 +13,8 @@
* Select video mode
*/

+#include <uapi/asm/boot.h>
+
#include "boot.h"
#include "video.h"
#include "vesa.h"
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 48d34d28f5a6..cd0fc0cc78bc 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -1,7 +1,6 @@
#ifndef _ASM_X86_PLATFORM_H
#define _ASM_X86_PLATFORM_H

-#include <asm/pgtable_types.h>
#include <asm/bootparam.h>

struct mpc_bus;

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

2015-11-11 09:51:23

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

On Tue, Nov 10, 2015 at 06:04:47PM +0100, Borislav Petkov wrote:
> On Tue, Nov 10, 2015 at 05:07:13PM +0200, Kirill A. Shutemov wrote:
> > Yeah. Looks good to me.
>
> It gets even cleaner. Let me run it through the *config build tests.

Hohumm, it passes. Here's what I ended up committing:

---
From: "Kirill A. Shutemov" <[email protected]>
Date: Tue, 10 Nov 2015 01:18:10 +0200
Subject: [PATCH] x86/mm: Fix regression with huge pages on PAE
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Recent PAT patchset has caused issue on 32-bit PAE machines:

page:eea45000 count:0 mapcount:-128 mapping: (null) index:0x0
flags: 0x40000000()
page dumped because: VM_BUG_ON_PAGE(page_mapcount(page) < 0)
------------[ cut here ]------------
kernel BUG at /home/build/linux-boris/mm/huge_memory.c:1485!
invalid opcode: 0000 [#1] SMP
Modules linked in: ahci libahci ata_generic skge r8169 firewire_ohci mii libata qla2xxx(+) scsi_transport_fc scsi_mod radeon tpm_infineon ttm backlight wmi acpi_cpufreq tpm_tis
CPU: 2 PID: 1758 Comm: modprobe Not tainted 4.3.0upstream-09269-gce5c2d2 #1
Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./To be filled by O.E.M., BIOS 080014 07/18/2008
task: ed84e600 ti: f6458000 task.ti: f6458000
EIP: 0060:[<c11bde80>] EFLAGS: 00010246 CPU: 2
EIP is at zap_huge_pmd+0x240/0x260
EAX: 00000000 EBX: f6459eb0 ECX: 00000292 EDX: 00000292
ESI: f6634d98 EDI: eea45000 EBP: f6459dc8 ESP: f6459d98
ata1: SATA link down (SStatus 0 SControl 300)
ata2: SATA link down (SStatus 0 SControl 300)
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 8005003b CR2: b75b21a0 CR3: 3655b880 CR4: 000006f0
Stack:
...
Call Trace:
unmap_single_vma
? __wake_up
unmap_vmas
unmap_region
do_munmap
vm_munmap
SyS_munmap
do_fast_syscall_32
? __do_page_fault
sysenter_past_esp
Code: 00 e9 05 fe ff ff 90 8d 74 26 00 0f 0b eb fe ba 4c e1 7a c1 89 f8 e8 f0 91 fd ff 0f 0b eb fe ba 6c e1 7a c1 89 f8 e8 e0 91 fd ff <0f> 0b eb fe ba c4 e1 7a c1 89 f8 e8 d0 91 fd ff 0f 0b eb fe 8d
EIP: [<c11bde80>] zap_huge_pmd+0x240/0x260 SS:ESP 0068:f6459d98
---[ end trace cba8fb1fc2e2e78a ]---

The problem is in pmd_pfn_mask() and pmd_flags_mask(). These helpers use
PMD_PAGE_MASK to calculate resulting mask. PMD_PAGE_MASK is 'unsigned
long', not 'unsigned long long' as phys_addr_t. As result upper bits of
resulting mask is truncated.

The patch reworks code to use PMD_SHIFT as base of mask calculation
instead of PMD_PAGE_MASK.

pud_pfn_mask() and pud_flags_mask() aren't problematic since we don't
have PUD page table level on 32-bit systems, but they are reworked too
to be consistent with PMD counterpart.

Reported-and-Tested-by: Boris Ostrovsky <[email protected]>
Signed-off-by: Kirill A. Shutemov <[email protected]>
Reviewed-by: Toshi Kani <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Cc: "H. Peter Anvin" <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Jürgen Gross <[email protected]>
Cc: [email protected]
Cc: linux-mm <[email protected]>
Cc: Mel Gorman <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Fixes: f70abb0fc3da ("x86/asm: Fix pud/pmd interfaces to handle large PAT bit")
Link: http://lkml.kernel.org/r/[email protected]
[ Fix -Woverflow warnings from the realmode code. ]
Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/boot/boot.h | 1 -
arch/x86/boot/video-mode.c | 2 ++
arch/x86/boot/video.c | 2 ++
arch/x86/include/asm/pgtable_types.h | 14 ++++----------
arch/x86/include/asm/x86_init.h | 1 -
5 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index 0033e96c3f09..9011a88353de 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -23,7 +23,6 @@
#include <stdarg.h>
#include <linux/types.h>
#include <linux/edd.h>
-#include <asm/boot.h>
#include <asm/setup.h>
#include "bitops.h"
#include "ctype.h"
diff --git a/arch/x86/boot/video-mode.c b/arch/x86/boot/video-mode.c
index aa8a96b052e3..95c7a818c0ed 100644
--- a/arch/x86/boot/video-mode.c
+++ b/arch/x86/boot/video-mode.c
@@ -19,6 +19,8 @@
#include "video.h"
#include "vesa.h"

+#include <uapi/asm/boot.h>
+
/*
* Common variables
*/
diff --git a/arch/x86/boot/video.c b/arch/x86/boot/video.c
index 05111bb8d018..77780e386e9b 100644
--- a/arch/x86/boot/video.c
+++ b/arch/x86/boot/video.c
@@ -13,6 +13,8 @@
* Select video mode
*/

+#include <uapi/asm/boot.h>
+
#include "boot.h"
#include "video.h"
#include "vesa.h"
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index dd5b0aa9dd2f..c1e797266ce9 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -279,17 +279,14 @@ static inline pmdval_t native_pmd_val(pmd_t pmd)
static inline pudval_t pud_pfn_mask(pud_t pud)
{
if (native_pud_val(pud) & _PAGE_PSE)
- return PUD_PAGE_MASK & PHYSICAL_PAGE_MASK;
+ return ~((1ULL << PUD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
else
return PTE_PFN_MASK;
}

static inline pudval_t pud_flags_mask(pud_t pud)
{
- if (native_pud_val(pud) & _PAGE_PSE)
- return ~(PUD_PAGE_MASK & (pudval_t)PHYSICAL_PAGE_MASK);
- else
- return ~PTE_PFN_MASK;
+ return ~pud_pfn_mask(pud);
}

static inline pudval_t pud_flags(pud_t pud)
@@ -300,17 +297,14 @@ static inline pudval_t pud_flags(pud_t pud)
static inline pmdval_t pmd_pfn_mask(pmd_t pmd)
{
if (native_pmd_val(pmd) & _PAGE_PSE)
- return PMD_PAGE_MASK & PHYSICAL_PAGE_MASK;
+ return ~((1ULL << PMD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
else
return PTE_PFN_MASK;
}

static inline pmdval_t pmd_flags_mask(pmd_t pmd)
{
- if (native_pmd_val(pmd) & _PAGE_PSE)
- return ~(PMD_PAGE_MASK & (pmdval_t)PHYSICAL_PAGE_MASK);
- else
- return ~PTE_PFN_MASK;
+ return ~pmd_pfn_mask(pmd);
}

static inline pmdval_t pmd_flags(pmd_t pmd)
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 48d34d28f5a6..cd0fc0cc78bc 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -1,7 +1,6 @@
#ifndef _ASM_X86_PLATFORM_H
#define _ASM_X86_PLATFORM_H

-#include <asm/pgtable_types.h>
#include <asm/bootparam.h>

struct mpc_bus;
--
2.3.5

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

2015-11-12 07:49:00

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

* Borislav Petkov <[email protected]> wrote:

> --- a/arch/x86/include/asm/pgtable_types.h
> +++ b/arch/x86/include/asm/pgtable_types.h
> @@ -279,17 +279,14 @@ static inline pmdval_t native_pmd_val(pmd_t pmd)
> static inline pudval_t pud_pfn_mask(pud_t pud)
> {
> if (native_pud_val(pud) & _PAGE_PSE)
> - return PUD_PAGE_MASK & PHYSICAL_PAGE_MASK;
> + return ~((1ULL << PUD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
> else
> return PTE_PFN_MASK;
> }

> static inline pmdval_t pmd_pfn_mask(pmd_t pmd)
> {
> if (native_pmd_val(pmd) & _PAGE_PSE)
> - return PMD_PAGE_MASK & PHYSICAL_PAGE_MASK;
> + return ~((1ULL << PMD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
> else
> return PTE_PFN_MASK;
> }

So instead of uglifying the code, why not fix the real bug: change the
PMD_PAGE_MASK/PUD_PAGE_MASK definitions to be 64-bit everywhere?

Thanks,

Ingo

2015-11-12 07:58:06

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

On Thu, Nov 12, 2015 at 08:48:54AM +0100, Ingo Molnar wrote:
>
> * Borislav Petkov <[email protected]> wrote:
>
> > --- a/arch/x86/include/asm/pgtable_types.h
> > +++ b/arch/x86/include/asm/pgtable_types.h
> > @@ -279,17 +279,14 @@ static inline pmdval_t native_pmd_val(pmd_t pmd)
> > static inline pudval_t pud_pfn_mask(pud_t pud)
> > {
> > if (native_pud_val(pud) & _PAGE_PSE)
> > - return PUD_PAGE_MASK & PHYSICAL_PAGE_MASK;
> > + return ~((1ULL << PUD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
> > else
> > return PTE_PFN_MASK;
> > }
>
> > static inline pmdval_t pmd_pfn_mask(pmd_t pmd)
> > {
> > if (native_pmd_val(pmd) & _PAGE_PSE)
> > - return PMD_PAGE_MASK & PHYSICAL_PAGE_MASK;
> > + return ~((1ULL << PMD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
> > else
> > return PTE_PFN_MASK;
> > }
>
> So instead of uglifying the code, why not fix the real bug: change the
> PMD_PAGE_MASK/PUD_PAGE_MASK definitions to be 64-bit everywhere?

*PAGE_MASK are usually applied to virtual addresses. I don't think it
should anything but 'unsigned long'. This is odd use case really.

--
Kirill A. Shutemov

2015-11-12 08:01:05

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

* Kirill A. Shutemov <[email protected]> wrote:

> On Thu, Nov 12, 2015 at 08:48:54AM +0100, Ingo Molnar wrote:
> >
> > * Borislav Petkov <[email protected]> wrote:
> >
> > > --- a/arch/x86/include/asm/pgtable_types.h
> > > +++ b/arch/x86/include/asm/pgtable_types.h
> > > @@ -279,17 +279,14 @@ static inline pmdval_t native_pmd_val(pmd_t pmd)
> > > static inline pudval_t pud_pfn_mask(pud_t pud)
> > > {
> > > if (native_pud_val(pud) & _PAGE_PSE)
> > > - return PUD_PAGE_MASK & PHYSICAL_PAGE_MASK;
> > > + return ~((1ULL << PUD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
> > > else
> > > return PTE_PFN_MASK;
> > > }
> >
> > > static inline pmdval_t pmd_pfn_mask(pmd_t pmd)
> > > {
> > > if (native_pmd_val(pmd) & _PAGE_PSE)
> > > - return PMD_PAGE_MASK & PHYSICAL_PAGE_MASK;
> > > + return ~((1ULL << PMD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
> > > else
> > > return PTE_PFN_MASK;
> > > }
> >
> > So instead of uglifying the code, why not fix the real bug: change the
> > PMD_PAGE_MASK/PUD_PAGE_MASK definitions to be 64-bit everywhere?
>
> *PAGE_MASK are usually applied to virtual addresses. I don't think it
> should anything but 'unsigned long'. This is odd use case really.

So we already have PHYSICAL_PAGE_MASK, why not introduce PHYSICAL_PMD_MASK et al,
instead of uglifying the code?

But, what problems do you expect with having a wider mask than its primary usage?
If it's used for 32-bit values it will be truncated down safely. (But I have not
tested it, so I might be missing some complication.)

Thanks,

Ingo

2015-11-12 08:46:54

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

Ingo Molnar wrote:
>
> * Kirill A. Shutemov <[email protected]> wrote:
>
> > On Thu, Nov 12, 2015 at 08:48:54AM +0100, Ingo Molnar wrote:
> > >
> > > * Borislav Petkov <[email protected]> wrote:
> > >
> > > > --- a/arch/x86/include/asm/pgtable_types.h
> > > > +++ b/arch/x86/include/asm/pgtable_types.h
> > > > @@ -279,17 +279,14 @@ static inline pmdval_t native_pmd_val(pmd_t pmd)
> > > > static inline pudval_t pud_pfn_mask(pud_t pud)
> > > > {
> > > > if (native_pud_val(pud) & _PAGE_PSE)
> > > > - return PUD_PAGE_MASK & PHYSICAL_PAGE_MASK;
> > > > + return ~((1ULL << PUD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
> > > > else
> > > > return PTE_PFN_MASK;
> > > > }
> > >
> > > > static inline pmdval_t pmd_pfn_mask(pmd_t pmd)
> > > > {
> > > > if (native_pmd_val(pmd) & _PAGE_PSE)
> > > > - return PMD_PAGE_MASK & PHYSICAL_PAGE_MASK;
> > > > + return ~((1ULL << PMD_SHIFT) - 1) & PHYSICAL_PAGE_MASK;
> > > > else
> > > > return PTE_PFN_MASK;
> > > > }
> > >
> > > So instead of uglifying the code, why not fix the real bug: change the
> > > PMD_PAGE_MASK/PUD_PAGE_MASK definitions to be 64-bit everywhere?
> >
> > *PAGE_MASK are usually applied to virtual addresses. I don't think it
> > should anything but 'unsigned long'. This is odd use case really.
>
> So we already have PHYSICAL_PAGE_MASK, why not introduce PHYSICAL_PMD_MASK et al,
> instead of uglifying the code?

Okay, makes sense. Check out the patch below.

> But, what problems do you expect with having a wider mask than its primary usage?
> If it's used for 32-bit values it will be truncated down safely. (But I have not
> tested it, so I might be missing some complication.)

Yeah, I basically worry about non realized side effect.

And these masks are defined via {PMD,PUD}_PAGE_SIZE. Should we change them
to 'unsigned long long' too or leave alone? What about PAGE_SIZE and
PAGE_MASK? Need to be converted too?

>From 96362f38d47d6ef9a0ebb9a92c6b4db9337f1565 Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <[email protected]>
Date: Thu, 12 Nov 2015 10:39:00 +0200
Subject: [PATCH] x86/mm: fix regression with huge pages on PAE
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Recent PAT patchset has caused issue on 32-bit PAE machines:

page:eea45000 count:0 mapcount:-128 mapping: (null) index:0x0
flags: 0x40000000()
page dumped because: VM_BUG_ON_PAGE(page_mapcount(page) < 0)
------------[ cut here ]------------
kernel BUG at /home/build/linux-boris/mm/huge_memory.c:1485!
invalid opcode: 0000 [#1] SMP
Modules linked in: ahci libahci ata_generic skge r8169 firewire_ohci mii libata qla2xxx(+) scsi_transport_fc scsi_mod radeon tpm_infineon ttm backlight wmi acpi_cpufreq tpm_tis
CPU: 2 PID: 1758 Comm: modprobe Not tainted 4.3.0upstream-09269-gce5c2d2 #1
Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./To be filled by O.E.M., BIOS 080014 07/18/2008
task: ed84e600 ti: f6458000 task.ti: f6458000
EIP: 0060:[<c11bde80>] EFLAGS: 00010246 CPU: 2
EIP is at zap_huge_pmd+0x240/0x260
EAX: 00000000 EBX: f6459eb0 ECX: 00000292 EDX: 00000292
ESI: f6634d98 EDI: eea45000 EBP: f6459dc8 ESP: f6459d98
ata1: SATA link down (SStatus 0 SControl 300)
ata2: SATA link down (SStatus 0 SControl 300)
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 8005003b CR2: b75b21a0 CR3: 3655b880 CR4: 000006f0
Stack:
...
Call Trace:
unmap_single_vma
? __wake_up
unmap_vmas
unmap_region
do_munmap
vm_munmap
SyS_munmap
do_fast_syscall_32
? __do_page_fault
sysenter_past_esp
Code: 00 e9 05 fe ff ff 90 8d 74 26 00 0f 0b eb fe ba 4c e1 7a c1 89 f8 e8 f0 91 fd ff 0f 0b eb fe ba 6c e1 7a c1 89 f8 e8 e0 91 fd ff <0f> 0b eb fe ba c4 e1 7a c1 89 f8 e8 d0 91 fd ff 0f 0b eb fe 8d
EIP: [<c11bde80>] zap_huge_pmd+0x240/0x260 SS:ESP 0068:f6459d98
---[ end trace cba8fb1fc2e2e78a ]---

The problem is in pmd_pfn_mask() and pmd_flags_mask(). These helpers use
PMD_PAGE_MASK to calculate resulting mask. PMD_PAGE_MASK is 'unsigned
long', not 'unsigned long long' as phys_addr_t. As result upper bits of
resulting mask is truncated.

pud_pfn_mask() and pud_flags_mask() aren't problematic since we don't
have PUD page table level on 32-bit systems, but it's reasonable to keep
them consitent with PMD counterpart.

The patch introduces PHYSICAL_PMD_PAGE_MASK and PHYSICAL_PUD_PAGE_MASK
in addition to existing PHYSICAL_PAGE_MASK and rework helpers to use
them.

Reported-and-Tested-by: Boris Ostrovsky <[email protected]>
Signed-off-by: Kirill A. Shutemov <[email protected]>
Reviewed-by: Toshi Kani <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: [email protected]
Cc: "H. Peter Anvin" <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Jürgen Gross <[email protected]>
Cc: [email protected]
Cc: linux-mm <[email protected]>
Cc: Mel Gorman <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Fixes: f70abb0fc3da ("x86/asm: Fix pud/pmd interfaces to handle large PAT bit")
Link: http://lkml.kernel.org/r/[email protected]
[ Fix -Woverflow warnings from the realmode code. ]
Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/boot/boot.h | 1 -
arch/x86/boot/video-mode.c | 2 ++
arch/x86/boot/video.c | 2 ++
arch/x86/include/asm/page_types.h | 16 +++++++++-------
arch/x86/include/asm/pgtable_types.h | 14 ++++----------
arch/x86/include/asm/x86_init.h | 1 -
6 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index 0033e96c3f09..9011a88353de 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -23,7 +23,6 @@
#include <stdarg.h>
#include <linux/types.h>
#include <linux/edd.h>
-#include <asm/boot.h>
#include <asm/setup.h>
#include "bitops.h"
#include "ctype.h"
diff --git a/arch/x86/boot/video-mode.c b/arch/x86/boot/video-mode.c
index aa8a96b052e3..95c7a818c0ed 100644
--- a/arch/x86/boot/video-mode.c
+++ b/arch/x86/boot/video-mode.c
@@ -19,6 +19,8 @@
#include "video.h"
#include "vesa.h"

+#include <uapi/asm/boot.h>
+
/*
* Common variables
*/
diff --git a/arch/x86/boot/video.c b/arch/x86/boot/video.c
index 05111bb8d018..77780e386e9b 100644
--- a/arch/x86/boot/video.c
+++ b/arch/x86/boot/video.c
@@ -13,6 +13,8 @@
* Select video mode
*/

+#include <uapi/asm/boot.h>
+
#include "boot.h"
#include "video.h"
#include "vesa.h"
diff --git a/arch/x86/include/asm/page_types.h b/arch/x86/include/asm/page_types.h
index c5b7fb2774d0..cc071c6f7d4d 100644
--- a/arch/x86/include/asm/page_types.h
+++ b/arch/x86/include/asm/page_types.h
@@ -9,19 +9,21 @@
#define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
#define PAGE_MASK (~(PAGE_SIZE-1))

+#define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT)
+#define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1))
+
+#define PUD_PAGE_SIZE (_AC(1, UL) << PUD_SHIFT)
+#define PUD_PAGE_MASK (~(PUD_PAGE_SIZE-1))
+
#define __PHYSICAL_MASK ((phys_addr_t)((1ULL << __PHYSICAL_MASK_SHIFT) - 1))
#define __VIRTUAL_MASK ((1UL << __VIRTUAL_MASK_SHIFT) - 1)

-/* Cast PAGE_MASK to a signed type so that it is sign-extended if
+/* Cast *PAGE_MASK to a signed type so that it is sign-extended if
virtual addresses are 32-bits but physical addresses are larger
(ie, 32-bit PAE). */
#define PHYSICAL_PAGE_MASK (((signed long)PAGE_MASK) & __PHYSICAL_MASK)
-
-#define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT)
-#define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1))
-
-#define PUD_PAGE_SIZE (_AC(1, UL) << PUD_SHIFT)
-#define PUD_PAGE_MASK (~(PUD_PAGE_SIZE-1))
+#define PHYSICAL_PMD_PAGE_MASK (((signed long)PMD_PAGE_MASK) & __PHYSICAL_MASK)
+#define PHYSICAL_PUD_PAGE_MASK (((signed long)PUD_PAGE_MASK) & __PHYSICAL_MASK)

#define HPAGE_SHIFT PMD_SHIFT
#define HPAGE_SIZE (_AC(1,UL) << HPAGE_SHIFT)
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 116fc4ee586f..d1b76f88ccd1 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -277,17 +277,14 @@ static inline pmdval_t native_pmd_val(pmd_t pmd)
static inline pudval_t pud_pfn_mask(pud_t pud)
{
if (native_pud_val(pud) & _PAGE_PSE)
- return PUD_PAGE_MASK & PHYSICAL_PAGE_MASK;
+ return PHYSICAL_PUD_PAGE_MASK;
else
return PTE_PFN_MASK;
}

static inline pudval_t pud_flags_mask(pud_t pud)
{
- if (native_pud_val(pud) & _PAGE_PSE)
- return ~(PUD_PAGE_MASK & (pudval_t)PHYSICAL_PAGE_MASK);
- else
- return ~PTE_PFN_MASK;
+ return ~pud_pfn_mask(pud);
}

static inline pudval_t pud_flags(pud_t pud)
@@ -298,17 +295,14 @@ static inline pudval_t pud_flags(pud_t pud)
static inline pmdval_t pmd_pfn_mask(pmd_t pmd)
{
if (native_pmd_val(pmd) & _PAGE_PSE)
- return PMD_PAGE_MASK & PHYSICAL_PAGE_MASK;
+ return PHYSICAL_PMD_PAGE_MASK;
else
return PTE_PFN_MASK;
}

static inline pmdval_t pmd_flags_mask(pmd_t pmd)
{
- if (native_pmd_val(pmd) & _PAGE_PSE)
- return ~(PMD_PAGE_MASK & (pmdval_t)PHYSICAL_PAGE_MASK);
- else
- return ~PTE_PFN_MASK;
+ return ~pmd_pfn_mask(pmd);
}

static inline pmdval_t pmd_flags(pmd_t pmd)
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 48d34d28f5a6..cd0fc0cc78bc 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -1,7 +1,6 @@
#ifndef _ASM_X86_PLATFORM_H
#define _ASM_X86_PLATFORM_H

-#include <asm/pgtable_types.h>
#include <asm/bootparam.h>

struct mpc_bus;
--
2.6.2

2015-11-12 08:54:24

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

* Kirill A. Shutemov <[email protected]> wrote:

> diff --git a/arch/x86/include/asm/page_types.h b/arch/x86/include/asm/page_types.h
> index c5b7fb2774d0..cc071c6f7d4d 100644
> --- a/arch/x86/include/asm/page_types.h
> +++ b/arch/x86/include/asm/page_types.h
> @@ -9,19 +9,21 @@
> #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
> #define PAGE_MASK (~(PAGE_SIZE-1))
>
> +#define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT)
> +#define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1))
> +
> +#define PUD_PAGE_SIZE (_AC(1, UL) << PUD_SHIFT)
> +#define PUD_PAGE_MASK (~(PUD_PAGE_SIZE-1))
> +
> #define __PHYSICAL_MASK ((phys_addr_t)((1ULL << __PHYSICAL_MASK_SHIFT) - 1))
> #define __VIRTUAL_MASK ((1UL << __VIRTUAL_MASK_SHIFT) - 1)
>
> -/* Cast PAGE_MASK to a signed type so that it is sign-extended if
> +/* Cast *PAGE_MASK to a signed type so that it is sign-extended if
> virtual addresses are 32-bits but physical addresses are larger
> (ie, 32-bit PAE). */
> #define PHYSICAL_PAGE_MASK (((signed long)PAGE_MASK) & __PHYSICAL_MASK)
> -
> -#define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT)
> -#define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1))
> -
> -#define PUD_PAGE_SIZE (_AC(1, UL) << PUD_SHIFT)
> -#define PUD_PAGE_MASK (~(PUD_PAGE_SIZE-1))
> +#define PHYSICAL_PMD_PAGE_MASK (((signed long)PMD_PAGE_MASK) & __PHYSICAL_MASK)
> +#define PHYSICAL_PUD_PAGE_MASK (((signed long)PUD_PAGE_MASK) & __PHYSICAL_MASK)

that's a really odd way of writing it, 'long' is signed by default ...

There seems to be 150+ such cases in the kernel source though - weird.

More importantly, how does this improve things on 32-bit PAE kernels? If I follow
the values correctly then PMD_PAGE_MASK is 'UL' i.e. 32-bit:

> +#define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT)
> +#define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1))

thus PHYSICAL_PMD_PAGE_MASK is 32-bit too:

> +#define PHYSICAL_PMD_PAGE_MASK (((signed long)PMD_PAGE_MASK) & __PHYSICAL_MASK)

so how is the bug fixed?

Thanks,

Ingo

2015-11-12 08:55:33

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

* Kirill A. Shutemov <[email protected]> wrote:

> > But, what problems do you expect with having a wider mask than its primary usage?
> > If it's used for 32-bit values it will be truncated down safely. (But I have not
> > tested it, so I might be missing some complication.)
>
> Yeah, I basically worry about non realized side effect.

Such a worry is prudent, the best way would be to double check a disassembly of a
before/after vmlinux and see in which functions they differ and why.

We might have similar bugs elsewhere - silently fixed by such a change.

Thanks,

Ingo

2015-11-12 09:00:23

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

On Thu, Nov 12, 2015 at 09:54:18AM +0100, Ingo Molnar wrote:
>
> * Kirill A. Shutemov <[email protected]> wrote:
>
> > diff --git a/arch/x86/include/asm/page_types.h b/arch/x86/include/asm/page_types.h
> > index c5b7fb2774d0..cc071c6f7d4d 100644
> > --- a/arch/x86/include/asm/page_types.h
> > +++ b/arch/x86/include/asm/page_types.h
> > @@ -9,19 +9,21 @@
> > #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
> > #define PAGE_MASK (~(PAGE_SIZE-1))
> >
> > +#define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT)
> > +#define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1))
> > +
> > +#define PUD_PAGE_SIZE (_AC(1, UL) << PUD_SHIFT)
> > +#define PUD_PAGE_MASK (~(PUD_PAGE_SIZE-1))
> > +
> > #define __PHYSICAL_MASK ((phys_addr_t)((1ULL << __PHYSICAL_MASK_SHIFT) - 1))
> > #define __VIRTUAL_MASK ((1UL << __VIRTUAL_MASK_SHIFT) - 1)
> >
> > -/* Cast PAGE_MASK to a signed type so that it is sign-extended if
> > +/* Cast *PAGE_MASK to a signed type so that it is sign-extended if
> > virtual addresses are 32-bits but physical addresses are larger
> > (ie, 32-bit PAE). */
> > #define PHYSICAL_PAGE_MASK (((signed long)PAGE_MASK) & __PHYSICAL_MASK)
> > -
> > -#define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT)
> > -#define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1))
> > -
> > -#define PUD_PAGE_SIZE (_AC(1, UL) << PUD_SHIFT)
> > -#define PUD_PAGE_MASK (~(PUD_PAGE_SIZE-1))
> > +#define PHYSICAL_PMD_PAGE_MASK (((signed long)PMD_PAGE_MASK) & __PHYSICAL_MASK)
> > +#define PHYSICAL_PUD_PAGE_MASK (((signed long)PUD_PAGE_MASK) & __PHYSICAL_MASK)
>
> that's a really odd way of writing it, 'long' is signed by default ...

See the comment above (it was there before the patch). 'signed' can be
considered as documentation -- we want sign-extension here.

> There seems to be 150+ such cases in the kernel source though - weird.
>
> More importantly, how does this improve things on 32-bit PAE kernels? If I follow
> the values correctly then PMD_PAGE_MASK is 'UL' i.e. 32-bit:
>
> > +#define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT)
> > +#define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1))
>
> thus PHYSICAL_PMD_PAGE_MASK is 32-bit too:
>
> > +#define PHYSICAL_PMD_PAGE_MASK (((signed long)PMD_PAGE_MASK) & __PHYSICAL_MASK)
>
> so how is the bug fixed?

Again, see the comment.
I've checked that it generates correct value (using kernel/bounds.c).

--
Kirill A. Shutemov

2015-11-12 13:30:53

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

* Kirill A. Shutemov <[email protected]> wrote:

> On Thu, Nov 12, 2015 at 09:54:18AM +0100, Ingo Molnar wrote:
> >
> > * Kirill A. Shutemov <[email protected]> wrote:
> >
> > > diff --git a/arch/x86/include/asm/page_types.h b/arch/x86/include/asm/page_types.h
> > > index c5b7fb2774d0..cc071c6f7d4d 100644
> > > --- a/arch/x86/include/asm/page_types.h
> > > +++ b/arch/x86/include/asm/page_types.h
> > > @@ -9,19 +9,21 @@
> > > #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
> > > #define PAGE_MASK (~(PAGE_SIZE-1))
> > >
> > > +#define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT)
> > > +#define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1))
> > > +
> > > +#define PUD_PAGE_SIZE (_AC(1, UL) << PUD_SHIFT)
> > > +#define PUD_PAGE_MASK (~(PUD_PAGE_SIZE-1))
> > > +
> > > #define __PHYSICAL_MASK ((phys_addr_t)((1ULL << __PHYSICAL_MASK_SHIFT) - 1))
> > > #define __VIRTUAL_MASK ((1UL << __VIRTUAL_MASK_SHIFT) - 1)
> > >
> > > -/* Cast PAGE_MASK to a signed type so that it is sign-extended if
> > > +/* Cast *PAGE_MASK to a signed type so that it is sign-extended if
> > > virtual addresses are 32-bits but physical addresses are larger
> > > (ie, 32-bit PAE). */
> > > #define PHYSICAL_PAGE_MASK (((signed long)PAGE_MASK) & __PHYSICAL_MASK)
> > > -
> > > -#define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT)
> > > -#define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1))
> > > -
> > > -#define PUD_PAGE_SIZE (_AC(1, UL) << PUD_SHIFT)
> > > -#define PUD_PAGE_MASK (~(PUD_PAGE_SIZE-1))
> > > +#define PHYSICAL_PMD_PAGE_MASK (((signed long)PMD_PAGE_MASK) & __PHYSICAL_MASK)
> > > +#define PHYSICAL_PUD_PAGE_MASK (((signed long)PUD_PAGE_MASK) & __PHYSICAL_MASK)
> >
> > that's a really odd way of writing it, 'long' is signed by default ...
>
> See the comment above (it was there before the patch). 'signed' can be
> considered as documentation -- we want sign-extension here.
>
> > There seems to be 150+ such cases in the kernel source though - weird.
> >
> > More importantly, how does this improve things on 32-bit PAE kernels? If I follow
> > the values correctly then PMD_PAGE_MASK is 'UL' i.e. 32-bit:
> >
> > > +#define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT)
> > > +#define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1))
> >
> > thus PHYSICAL_PMD_PAGE_MASK is 32-bit too:
> >
> > > +#define PHYSICAL_PMD_PAGE_MASK (((signed long)PMD_PAGE_MASK) & __PHYSICAL_MASK)
> >
> > so how is the bug fixed?
>
> Again, see the comment.

Ah, indeed! That should in fact work even better than using u64 or so, as it does
the obvious thing for masks.

The concept will only break down once TBPAGES (well, 512 GB pages) are introduced.

Thanks,

Ingo

2015-11-12 19:29:35

by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

On Thu, Nov 12, 2015 at 12:00 AM, Ingo Molnar <[email protected]> wrote:
>
> So we already have PHYSICAL_PAGE_MASK, why not introduce PHYSICAL_PMD_MASK et al,
> instead of uglifying the code?

I think that's the right thing here.

> But, what problems do you expect with having a wider mask than its primary usage?
> If it's used for 32-bit values it will be truncated down safely. (But I have not
> tested it, so I might be missing some complication.)

No, it will not necessarily be truncated down. If we were to make the
regular PAGE_MASK etc that are normally used for virtual addresses be
"ull", it might easily make some calcyulations be done in 64 bits
instead. Sure, they'll probably be truncated down *eventually* when
you actually store them to some 32-bit thing, but I'd worry about it.

An example of a situation where over-long types cause problems is
simply in variadic functions (typically printk, but they do happen in
other places). Writing

printk("page offset = %ul\n", address & PAGE_MASK);

makes sense. In the VM, addresses really are "unsigned long". But just
imagine how wrong the above goes if PAGE_MASK was made "ull".

So no, widening masks to the maximal possible type is not the answer.
They need to be the natural size.

Another possibility would be to simply make masks be _signed_ longs.
That can has its own set of problems, but it does mean that when the
mask has high bits set and it gets expanded to a wider type, the
normal C rules just do the RightThing(tm).

We've occasionally done that very explicitly. Just see how
PHYSICAL_PAGE_MASK is defined in terms of a signed PAGE_MASK.

I have this dim memory of us playing around with just making PAGE_SIZE
(and thus PAGE_MASK) always be signed, but that it caused other
problems. Signed types have downsides too.

Linus

2015-11-13 09:01:30

by Dan Williams

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

On Thu, Nov 12, 2015 at 11:29 AM, Linus Torvalds
<[email protected]> wrote:
> On Thu, Nov 12, 2015 at 12:00 AM, Ingo Molnar <[email protected]> wrote:
[..]
> I have this dim memory of us playing around with just making PAGE_SIZE
> (and thus PAGE_MASK) always be signed, but that it caused other
> problems. Signed types have downsides too.

FWIW, I ran into this recently with the pfn_t patch. mips and powerpc
have PAGE_MASK as a signed int.

2015-11-24 15:02:04

by Boris Ostrovsky

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

On 11/12/2015 04:00 AM, Kirill A. Shutemov wrote:
> On Thu, Nov 12, 2015 at 09:54:18AM +0100, Ingo Molnar wrote:
>> * Kirill A. Shutemov <[email protected]> wrote:
>>
>>> diff --git a/arch/x86/include/asm/page_types.h b/arch/x86/include/asm/page_types.h
>>> index c5b7fb2774d0..cc071c6f7d4d 100644
>>> --- a/arch/x86/include/asm/page_types.h
>>> +++ b/arch/x86/include/asm/page_types.h

Kirill, where are we with this patch?

-boris

>>> @@ -9,19 +9,21 @@
>>> #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
>>> #define PAGE_MASK (~(PAGE_SIZE-1))
>>>
>>> +#define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT)
>>> +#define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1))
>>> +
>>> +#define PUD_PAGE_SIZE (_AC(1, UL) << PUD_SHIFT)
>>> +#define PUD_PAGE_MASK (~(PUD_PAGE_SIZE-1))
>>> +
>>> #define __PHYSICAL_MASK ((phys_addr_t)((1ULL << __PHYSICAL_MASK_SHIFT) - 1))
>>> #define __VIRTUAL_MASK ((1UL << __VIRTUAL_MASK_SHIFT) - 1)
>>>
>>> -/* Cast PAGE_MASK to a signed type so that it is sign-extended if
>>> +/* Cast *PAGE_MASK to a signed type so that it is sign-extended if
>>> virtual addresses are 32-bits but physical addresses are larger
>>> (ie, 32-bit PAE). */
>>> #define PHYSICAL_PAGE_MASK (((signed long)PAGE_MASK) & __PHYSICAL_MASK)
>>> -
>>> -#define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT)
>>> -#define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1))
>>> -
>>> -#define PUD_PAGE_SIZE (_AC(1, UL) << PUD_SHIFT)
>>> -#define PUD_PAGE_MASK (~(PUD_PAGE_SIZE-1))
>>> +#define PHYSICAL_PMD_PAGE_MASK (((signed long)PMD_PAGE_MASK) & __PHYSICAL_MASK)
>>> +#define PHYSICAL_PUD_PAGE_MASK (((signed long)PUD_PAGE_MASK) & __PHYSICAL_MASK)
>> that's a really odd way of writing it, 'long' is signed by default ...
> See the comment above (it was there before the patch). 'signed' can be
> considered as documentation -- we want sign-extension here.
>
>> There seems to be 150+ such cases in the kernel source though - weird.
>>
>> More importantly, how does this improve things on 32-bit PAE kernels? If I follow
>> the values correctly then PMD_PAGE_MASK is 'UL' i.e. 32-bit:
>>
>>> +#define PMD_PAGE_SIZE (_AC(1, UL) << PMD_SHIFT)
>>> +#define PMD_PAGE_MASK (~(PMD_PAGE_SIZE-1))
>> thus PHYSICAL_PMD_PAGE_MASK is 32-bit too:
>>
>>> +#define PHYSICAL_PMD_PAGE_MASK (((signed long)PMD_PAGE_MASK) & __PHYSICAL_MASK)
>> so how is the bug fixed?
> Again, see the comment.
> I've checked that it generates correct value (using kernel/bounds.c).
>

2015-11-24 20:14:53

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

On Tue, Nov 24, 2015 at 09:59:27AM -0500, Boris Ostrovsky wrote:
> On 11/12/2015 04:00 AM, Kirill A. Shutemov wrote:
> >On Thu, Nov 12, 2015 at 09:54:18AM +0100, Ingo Molnar wrote:
> >>* Kirill A. Shutemov <[email protected]> wrote:
> >>
> >>>diff --git a/arch/x86/include/asm/page_types.h b/arch/x86/include/asm/page_types.h
> >>>index c5b7fb2774d0..cc071c6f7d4d 100644
> >>>--- a/arch/x86/include/asm/page_types.h
> >>>+++ b/arch/x86/include/asm/page_types.h
>
>
> Kirill, where are we with this patch?

I haven't seen any actionable objections to the updated patch.
Not sure why it's not applied.

--
Kirill A. Shutemov

2015-11-25 10:27:44

[permalink] [raw]

Subject: Re: [PATCH] x86/mm: fix regression with huge pages on PAE

On Tue, Nov 24, 2015 at 10:14:49PM +0200, Kirill A. Shutemov wrote:
> I haven't seen any actionable objections to the updated patch.
> Not sure why it's not applied.

It is now.

Thanks.

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

2015-11-27 10:16:09