x86: some more patches

PATCH In review Jeremy Fitzhardinge 5 participants 28 messages Jan 15, 2008 – Jan 18, 2008

Subsystems (2)

x86 x86 MM

lore

Share & tools

lore

Jeremy Fitzhardinge

author

2008-01-15 22:27 UTC #0 lore raw

PATCH 1/1 x86: some more patches

Hi Ingo,

More patches:

- rearrange paravirt.h to remove duplicate code, and to make it easy to drop in 4th level pagetable functions.
- fix a warning I got from clear_bit in pgtable.h
- fix up some bogosity in pte_modify
- mask NX from pte_pfn

The last two may help with the problem that Andi's seeing.

Thanks,
J

Jeremy Fitzhardinge

author

2008-01-15 22:27 UTC #1 lore raw

PATCH 1/1 x86: mask NX from pte_pfn

In 32-bit PAE, mask NX from pte_pfn, since it isn't part of the PFN. This code is due for unification anyway, but this fixes a latent bug.

Signed-off-by Jeremy Fitzhardinge <[email protected]>

---
 include/asm-x86/pgtable-3level.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/asm-x86/pgtable-3level.h +1 -1

@@ -139,7 +139,7 @@ static inline int pte_none(pte_t pte)
 
 static inline unsigned long pte_pfn(pte_t pte)
 {
-	return pte_val(pte) >> PAGE_SHIFT;
+	return (pte_val(pte) >> PAGE_SHIFT) & ~_PAGE_NX;
 }
 
 /*

Hugh Dickins

2008-01-18 13:54 UTC #2 lore raw

Re: [PATCH 4 of 4] x86: mask NX from pte_pfn

Quoted 13 lines · Jeremy Fitzhardinge

In 32-bit PAE, mask NX from pte_pfn, since it isn't part of the PFN. This code is due for unification anyway, but this fixes a latent bug.
Signed-off-by Jeremy Fitzhardinge <[email protected]>
---
 include/asm-x86/pgtable-3level.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/asm-x86/pgtable-3level.h static inline int pte_none(pte_t pte) · line 145

Quoted 8 lines

include/asm-x86/pgtable-3level.h +1 -1

@@ -139,7 +139,7 @@ static inline int pte_none(pte_t pte)
 
 static inline unsigned long pte_pfn(pte_t pte)
 {
-	return pte_val(pte) >> PAGE_SHIFT;
+	return (pte_val(pte) >> PAGE_SHIFT) & ~_PAGE_NX;
 }
 
 /*

Shouldn't that be

	return (pte_val(pte) & ~_PAGE_NX) >> PAGE_SHIFT;

? Hugh

Ingo Molnar

2008-01-18 14:01 UTC #3 lore raw

Re: [PATCH 4 of 4] x86: mask NX from pte_pfn

Quoted 6 lines · Hugh Dickins

include/asm-x86/pgtable-3level.h static inline int pte_none(pte_t pte) · line 142
Quoted 2 lines
include/asm-x86/pgtable-3level.h +1 -1
-	return pte_val(pte) >> PAGE_SHIFT;
+	return (pte_val(pte) >> PAGE_SHIFT) & ~_PAGE_NX;
Shouldn't that be
	return (pte_val(pte) & ~_PAGE_NX) >> PAGE_SHIFT;

ouch! Sharp eyes! Fixed it.

Ingo

Jeremy Fitzhardinge

author

2008-01-18 15:55 UTC #4 lore raw

Re: [PATCH 4 of 4] x86: mask NX from pte_pfn

In reply to Hugh Dickins

Quoted 4 lines · Hugh Dickins

Shouldn't that be

	return (pte_val(pte) & ~_PAGE_NX) >> PAGE_SHIFT;

Yes, it should be.

Thanks,
J

Jeremy Fitzhardinge

author

2008-01-15 22:27 UTC #5 lore raw

PATCH 1/1 x86: fix warning

In reply to Jeremy Fitzhardinge

&ptep->pte isn't always an unsigned long *, so cast it to avoid a warning.

Signed-off-by Jeremy Fitzhardinge <[email protected]>

---
 include/asm-x86/pgtable.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/asm-x86/pgtable.h +1 -1

@@ -340,7 +340,7 @@ static inline pte_t ptep_get_and_clear_f
 #define __HAVE_ARCH_PTEP_SET_WRPROTECT
 static inline void ptep_set_wrprotect(struct mm_struct *mm, unsigned long addr, pte_t *ptep)
 {
-	clear_bit(_PAGE_BIT_RW, &ptep->pte);
+	clear_bit(_PAGE_BIT_RW, (unsigned long *)&ptep->pte);
 	pte_update(mm, addr, ptep);
 }

Jeremy Fitzhardinge

author

2008-01-15 22:28 UTC #6 lore raw

PATCH 1/1 x86: clean up pte_modify

In reply to Jeremy Fitzhardinge

pte_modify() got mushed in an apparent mismerge. Fix it up.

Signed-off-by Jeremy Fitzhardinge <[email protected]>

---
 include/asm-x86/pgtable.h |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

include/asm-x86/pgtable.h +1 -4

@@ -176,14 +176,11 @@ static inline pte_t pte_modify(pte_t pte
 {
 	pteval_t val = pte_val(pte);
 
-	val &= _PAGE_CHG_MASK;
-	val |= pgprot_val(newprot);
-
 	/*
 	 * Chop off the NX bit (if present), and add the NX portion of
 	 * the newprot (if present):
 	 */
-	val &= ~_PAGE_NX;
+	val &= _PAGE_CHG_MASK & ~_PAGE_NX;
 	val |= pgprot_val(newprot) & __supported_pte_mask;
 
 	return __pte(val);

Andi Kleen

2008-01-16 00:43 UTC #7 lore raw

Re: [PATCH 3 of 4] x86: clean up pte_modify

Quoted 1 line · On Tuesday 15 January 2008 23:17:08 Jeremy Fitzhardinge wrote:

pte_modify() got mushed in an apparent mismerge. Fix it up.

Note it's not just a clean up -- it would actually drop bits of the PFN.

-Andi

Jeremy Fitzhardinge

author

2008-01-15 22:28 UTC #8 lore raw

PATCH 1/1 x86: refactor mmu ops in paravirt.h

In reply to Jeremy Fitzhardinge

Rearrange mmu ops in paravirt.h to remove duplication and to make adding 4th level operations simple.

Signed-off-by Jeremy Fitzhardinge <[email protected]>

Cc Eduardo Habkost <[email protected]>

---
 include/asm-x86/page.h     |   10 +
 include/asm-x86/paravirt.h |  223 +++++++++++++++++++++++++-------------------
 2 files changed, 137 insertions(+), 96 deletions(-)

include/asm-x86/page.h +10 -0

@@ -91,6 +91,11 @@ static inline pudval_t native_pud_val(pu
 }
 #else	/* PAGETABLE_LEVELS == 3 */
 #include <asm-generic/pgtable-nopud.h>
+
+static inline pudval_t native_pud_val(pud_t pud)
+{
+	return native_pgd_val(pud.pgd);
+}
 #endif	/* PAGETABLE_LEVELS == 4 */
 
 typedef struct { pmdval_t pmd; } pmd_t;

@@ -106,6 +111,11 @@ static inline pmdval_t native_pmd_val(pm
 }
 #else  /* PAGETABLE_LEVELS == 2 */
 #include <asm-generic/pgtable-nopmd.h>
+
+static inline pmdval_t native_pmd_val(pmd_t pmd)
+{
+	return native_pgd_val(pmd.pud.pgd);
+}
 #endif	/* PAGETABLE_LEVELS >= 3 */
 
 static inline pte_t native_make_pte(pteval_t val)

include/asm-x86/paravirt.h +127 ~~-96~~

281 lines not on this page · raw message

Ingo Molnar

2008-01-15 22:36 UTC #9 lore raw

In reply to Jeremy Fitzhardinge

Quoted 11 lines · Jeremy Fitzhardinge

Hi Ingo,
More patches:
- rearrange paravirt.h to remove duplicate code, and to make it easy to drop in 4th level pagetable functions.
- fix a warning I got from clear_bit in pgtable.h
- fix up some bogosity in pte_modify
- mask NX from pte_pfn
The last two may help with the problem that Andi's seeing.

unfortunately they dont solve it:

 [   92.042586] Freeing unused kernel memory: 328k freed
 [   92.091838] khelper used greatest stack depth: 6244 bytes left
 [   92.281738] init[1]: segfault at 00000004 ip 49471cbb sp bff8dbb0 error 4
 [   92.288761] init[1]: segfault at 00000004 ip 49471cbb sp bff8dbb0 error 4
 [   92.295763] init[1]: segfault at 00000004 ip 49471cbb sp bff8dbb0 error 4
 [...]
 [   97.312484] printk: 611046 messages suppressed.

32-bit, PAE and RAM above 4GB seems to be enough to trigger that bug.

Ingo

Jeremy Fitzhardinge

author

2008-01-15 23:28 UTC #10 lore raw

Quoted 10 lines · Ingo Molnar

unfortunately they dont solve it:

 [   92.042586] Freeing unused kernel memory: 328k freed
 [   92.091838] khelper used greatest stack depth: 6244 bytes left
 [   92.281738] init[1]: segfault at 00000004 ip 49471cbb sp bff8dbb0 error 4
 [   92.288761] init[1]: segfault at 00000004 ip 49471cbb sp bff8dbb0 error 4
 [   92.295763] init[1]: segfault at 00000004 ip 49471cbb sp bff8dbb0 error 4
 [...]
 [   97.312484] printk: 611046 messages suppressed.

Quoted 2 lines

32-bit, PAE and RAM above 4GB seems to be enough to trigger that bug.

Bum. We established that once you fix the accessors+sign extension bug, there's another bug somewhere later on in the series. Any chance you could move "x86/pgtable: fix constant sign extension problem" to be just after "x86/pgtable: unify pagetable accessors" and bisect the following patches?

Thanks,
J

Andi Kleen

2008-01-16 00:44 UTC #11 lore raw

Quoted 11 lines · On Wednesday 16 January 2008 00:28:23 Jeremy Fitzhardinge wrote:

Quoted 10 lines · Ingo Molnar

unfortunately they dont solve it:

 [   92.042586] Freeing unused kernel memory: 328k freed
 [   92.091838] khelper used greatest stack depth: 6244 bytes left
 [   92.281738] init[1]: segfault at 00000004 ip 49471cbb sp bff8dbb0 error 4
 [   92.288761] init[1]: segfault at 00000004 ip 49471cbb sp bff8dbb0 error 4
 [   92.295763] init[1]: segfault at 00000004 ip 49471cbb sp bff8dbb0 error 4
 [...]
 [   97.312484] printk: 611046 messages suppressed.

Quoted 8 lines

Quoted 2 lines
32-bit, PAE and RAM above 4GB seems to be enough to trigger that bug.
Bum. We established that once you fix the accessors+sign extension bug, there's another bug somewhere later on in the series. Any chance you could move "x86/pgtable: fix constant sign extension problem" to be just after "x86/pgtable: unify pagetable accessors" and bisect the following patches?

Bisecting is somewhat difficult because there seem to be quite a lot of problems in the current git-x86 stream. I found that Jan's ioremap fix also causes silent hangs here during earlier bisecting (not sure why)

-Andi

Ingo Molnar

2008-01-16 07:26 UTC #12 lore raw

Quoted 2 lines · Andi Kleen

[...] I found that Jan's ioremap fix also causes silent hangs here during earlier bisecting (not sure why)

please send a fuller bugreport - all these problems on your testbox might be interrelated. Jan's patch is undone in a later part of the series so it's a bisection artifact. I've fixed that up.

Ingo

------------------->
Subject: x86: fix 64-bit ioremap()
From: From: Jan Beulich <[email protected]>

Quoted 4 lines

Yeah, that may be true, but this particular tree is weird, and I'm trying to understand what's going on here. Specifically, 64-bit ioremap()s *don't* set _PAGE_GLOBAL, which appears to be an accident resulting from the strange definitions of __PAGE_KERNEL_* vs PAGE_KERNEL_*.

ioremap() should set G agreed.

Quoted 6 lines

For example, ioremap_64.c:__ioremap() creates a vma for the io mapping, and explicitly sets _PAGE_GLOBAL in the vma's version of pgprot - but then it calls ioremap_page_range() to actually create the mapping, which ends up making a non-global mapping, because its rolling its own version of PAGE_KERNEL by using pgprot(__PAGE_KERNEL) - which is not the actual definition of PAGE_KERNEL.

That should not really matter because ioremap_change_attr()->c_p_a is only called when flags is != 0 and that means it is already different from PAGE_KERNEL.

Quoted 2 lines

I think there's a bug around here, but I think its currently being hidden

There's one Jan pointed out: iounmap does not subtract the guard page size so it ends up resetting one page too much. That is probably what causes your problem. But again you should be passing in G in the first place.

Additionally I found it necessary to fix ioremap_64.c's use of change_page_attr_addr():

[ [email protected]: fixed coding style errors ]

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/mm/ioremap_64.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

arch/x86/mm/ioremap_64.c +6 -4

Index: linux-x86.q/arch/x86/mm/ioremap_64.c
===================================================================
@@ -45,10 +45,12 @@ ioremap_change_attr(unsigned long phys_a
 		unsigned long vaddr = (unsigned long) __va(phys_addr);
 
 		/*
- 		 * Must use a address here and not struct page because the phys addr
-		 * can be a in hole between nodes and not have an memmap entry.
+		 * Must use an address here and not struct page because the
+		 * phys addr can be a in hole between nodes and not have an
+		 * memmap entry:
 		 */
-		err = change_page_attr_addr(vaddr,npages,__pgprot(__PAGE_KERNEL|flags));
+		err = change_page_attr_addr(vaddr, npages,
+					    MAKE_GLOBAL(__PAGE_KERNEL|flags));
 		if (!err)
 			global_flush_tlb();
 	}

@@ -181,7 +183,7 @@ void iounmap(volatile void __iomem *addr
 
 	/* Reset the direct mapping. Can block */
 	if (p->flags >> 20)
-		ioremap_change_attr(p->phys_addr, p->size, 0);
+		ioremap_change_attr(p->phys_addr, get_vm_area_size(p), 0);
 
 	/* Finally remove it */
 	o = remove_vm_area((void *)addr);

Ingo Molnar

2008-01-16 14:23 UTC #13 lore raw

In reply to Jeremy Fitzhardinge

Quoted 11 lines · Jeremy Fitzhardinge

Quoted 10 lines · Ingo Molnar

unfortunately they dont solve it:

 [   92.042586] Freeing unused kernel memory: 328k freed
 [   92.091838] khelper used greatest stack depth: 6244 bytes left
 [   92.281738] init[1]: segfault at 00000004 ip 49471cbb sp bff8dbb0 error 4
 [   92.288761] init[1]: segfault at 00000004 ip 49471cbb sp bff8dbb0 error 4
 [   92.295763] init[1]: segfault at 00000004 ip 49471cbb sp bff8dbb0 error 4
 [...]
 [   97.312484] printk: 611046 messages suppressed.

Quoted 8 lines

Quoted 2 lines
32-bit, PAE and RAM above 4GB seems to be enough to trigger that bug.
Bum. We established that once you fix the accessors+sign extension bug, there's another bug somewhere later on in the series. Any chance you could move "x86/pgtable: fix constant sign extension problem" to be just after "x86/pgtable: unify pagetable accessors" and bisect the following patches?

after a day of debugging i finally tracked it down to another unsigned-type and masking mismatch on PAE, caused by:

Subject: x86: unify pgtable accessors which use supported_pte_mask
From: Jeremy Fitzhardinge <[email protected]>

the fix is below.

I have to say, i'm less than impressed about the structure of that patch. The patch too was way too coarse for easy bisection. You did multiple nontrivial steps in the same patch.

I'm not going to apply such all-in-one patches to x86.git anymore, and will start straight with another patch you sent yesterday:

Subject: x86: refactor mmu ops in paravirt.h

  2 files changed, 129 insertions(+), 88 deletions(-)

If anyone finds a problem in that patch we'll have a hard time figuring out what went wrong. Please split that patch up properly. To demonstrate the kind of splitup we need for such patches, i've done a sample splitup of the following patch in x86.git:

Subject: x86/pgtable: unify pagetable accessors
From: Jeremy Fitzhardinge <[email protected]>

and turned it into 6 easy-to-review and easy-to-validate patches. Each step is _provably trivial_, and even if something goes wrong, it's easy to figure out where the problem comes from. At each step, document in the patch description whether the patch is intended to cause any code changes or not.

Ingo

include/asm-x86/page.h +1 -1

Index: linux-x86.q/include/asm-x86/page.h
===================================================================
@@ -11,7 +11,7 @@
 #ifdef __KERNEL__
 
 #define PHYSICAL_PAGE_MASK	(PAGE_MASK & __PHYSICAL_MASK)
-#define PTE_MASK		PHYSICAL_PAGE_MASK
+#define PTE_MASK		(_AT(long, PHYSICAL_PAGE_MASK))
 
 #define LARGE_PAGE_SIZE		(_AC(1,UL) << PMD_SHIFT)
 #define LARGE_PAGE_MASK		(~(LARGE_PAGE_SIZE-1))

Andi Kleen

2008-01-16 14:44 UTC #14 lore raw

include/asm-x86/page.h line 13

Quoted 9 lines

Index: linux-x86.q/include/asm-x86/page.h
===================================================================
include/asm-x86/page.h +0 -0
@@ -11,7 +11,7 @@
 #ifdef __KERNEL__
 
 #define PHYSICAL_PAGE_MASK	(PAGE_MASK & __PHYSICAL_MASK)

I haven't tested yet, but we looked at that one earlier and I thought it was ok because

#define __PHYSICAL_MASK _AT(phys_addr_t, (_AC(1,ULL) << __PHYSICAL_MASK_SHIFT) - 1)

and

typedef u64 phys_addr_t;

for PAE. So the expression above should have been already 64bit.

include/asm-x86/page.h line 14

Quoted 2 lines

include/asm-x86/page.h +1 -1

-#define PTE_MASK		PHYSICAL_PAGE_MASK
+#define PTE_MASK		(_AT(long, PHYSICAL_PAGE_MASK))

That would mean it is 32bit again because 32bit long is 32bit, but on 32bit PAE PTE_MASK definitely needs to be 64bit.

So I would be surprised if the patch works.

The problem seems to be rather that PAGE_MASK does not include the bits above 32bit. Something like

#define PHYSICAL_PAGE_MASK (__PHYSICAL_MASK & ~(phys_addr_t)(PAGE_SIZE-1))

should work (untested)

-Andi

Ingo Molnar

2008-01-16 14:54 UTC #15 lore raw

Quoted 6 lines · Andi Kleen

Quoted 1 line
#define PHYSICAL_PAGE_MASK (PAGE_MASK & __PHYSICAL_MASK)
I haven't tested yet, but we looked at that one earlier and I thought it was ok because

Quoted 7 lines

#define __PHYSICAL_MASK _AT(phys_addr_t, (_AC(1,ULL) << __PHYSICAL_MASK_SHIFT) - 1)
and
typedef u64 phys_addr_t;
for PAE. So the expression above should have been already 64bit.

no. The problem is that PAGE_MASK is:

  #define PAGE_MASK        (~(PAGE_SIZE-1))

  #define PAGE_SIZE        (_AC(1,UL) << PAGE_SHIFT)

that's u32 on PAE, and __PHYSICAL_MASK is u64. So PAGE_MASK gets zero-extended to u64. So the combined mask:

  #define PHYSICAL_PAGE_MASK      (PAGE_MASK & __PHYSICAL_MASK)

has the high bits chopped off. Please try my patch.

(PHYSICAL_PAGE_MASK is broken too in the same way, i just fixed that in my tree - but it's not used by anything on 32-bit PAE but by PAGE_MASK)

Quoted 1 line

So I would be surprised if the patch works.

try it ...

Ingo

Ingo Molnar

2008-01-16 15:18 UTC #16 lore raw

today's x86.git now has all the fixes included - can you confirm that it boots fine on your box?

Ingo

Andi Kleen

2008-01-16 15:26 UTC #17 lore raw

In reply to Ingo Molnar

Quoted 11 lines · On Wednesday 16 January 2008 15:54:27 Ingo Molnar wrote:

Quoted 8 lines · Andi Kleen
Quoted 1 line
#define PHYSICAL_PAGE_MASK (PAGE_MASK & __PHYSICAL_MASK)
I haven't tested yet, but we looked at that one earlier and I thought it was ok because
#define __PHYSICAL_MASK _AT(phys_addr_t, (_AC(1,ULL) << __PHYSICAL_MASK_SHIFT) - 1)

Quoted 7 lines

Quoted 5 lines
and
typedef u64 phys_addr_t;
for PAE. So the expression above should have been already 64bit.
no. The problem is that PAGE_MASK is:

I covered that in the end of the email.

Quoted 4 lines

(PHYSICAL_PAGE_MASK is broken too in the same way, i just fixed that in my tree - but it's not used by anything on 32-bit PAE but by PAGE_MASK)

Yes, but if you cast to long the result will be 32bit again. Or do you rely on it being always used in 64bit signed context with sign extension? While that might work it would seem rather fragile to me, just asking for similar future bugs.

-Andi

Jan Beulich

2008-01-16 15:42 UTC #18 lore raw

Quoted 4 lines

Yes, but if you cast to long the result will be 32bit again. Or do you rely on it being always used in 64bit signed context with sign extension? While that might work it would seem rather fragile to me, just asking for similar future bugs.

Even if conversion is to 64-bit unsigned, the value (being signed) will be sign-extended first (to preserve its value modulo 2**<target type width>).

Jan

Ingo Molnar

2008-01-16 15:47 UTC #19 lore raw

Quoted 8 lines · Jan Beulich

Quoted 4 lines
Yes, but if you cast to long the result will be 32bit again. Or do you rely on it being always used in 64bit signed context with sign extension? While that might work it would seem rather fragile to me, just asking for similar future bugs.
Even if conversion is to 64-bit unsigned, the value (being signed) will be sign-extended first (to preserve its value modulo 2**<target type width>).

yes. It would be nice if Andi could test my fix instead of arguing why he thinks it's unlikely to work ;-)

(i've got the cleanup patch below as well ontop of today's x86.git which includes the first fix, but it's an RFC as it has wider impact and might break stuff.)

Ingo

--------------->
Subject: x86: PAGE_MASK cleanup
From: Ingo Molnar <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 include/asm-x86/page.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

include/asm-x86/page.h +2 -2

Index: linux-x86.q/include/asm-x86/page.h
===================================================================
@@ -6,12 +6,12 @@
 /* PAGE_SHIFT determines the page size */
 #define PAGE_SHIFT	12
 #define PAGE_SIZE	(_AC(1,UL) << PAGE_SHIFT)
-#define PAGE_MASK	(~(PAGE_SIZE-1))
+#define PAGE_MASK	(_AT(phys_addr_t, ~(PAGE_SIZE-1)))
 
 #ifdef __KERNEL__
 
 #define PHYSICAL_PAGE_MASK	(PAGE_MASK & __PHYSICAL_MASK)
-#define PTE_MASK		(_AT(long, PHYSICAL_PAGE_MASK))
+#define PTE_MASK		PHYSICAL_PAGE_MASK
 
 #define LARGE_PAGE_SIZE		(_AC(1,UL) << PMD_SHIFT)
 #define LARGE_PAGE_MASK		(~(LARGE_PAGE_SIZE-1))

Ingo Molnar

2008-01-16 16:06 UTC #20 lore raw

include/asm-x86/page.h line 9

Quoted 2 lines · Ingo Molnar

include/asm-x86/page.h +1 -1

-#define PAGE_MASK	(~(PAGE_SIZE-1))
+#define PAGE_MASK	(_AT(phys_addr_t, ~(PAGE_SIZE-1)))

that should be:

#define PAGE_MASK (_AT(phys_addr_t, PAGE_SIZE)-1)

but still - PAGE_MASK is used in a lot of places so i'm nervous about this change.

Ingo

Jeremy Fitzhardinge

author

2008-01-16 17:06 UTC #21 lore raw

In reply to Ingo Molnar

Quoted 28 lines · Ingo Molnar

Quoted 4 lines · Jan Beulich
Quoted 2 lines
Yes, but if you cast to long the result will be 32bit again. Or do you rely on it being always used in 64bit signed context with sign extension? While that might work it would seem rather fragile to me, just asking for similar future bugs.
Even if conversion is to 64-bit unsigned, the value (being signed) will be sign-extended first (to preserve its value modulo 2**<target type width>).
yes. It would be nice if Andi could test my fix instead of arguing why he thinks it's unlikely to work ;-)
(i've got the cleanup patch below as well ontop of today's x86.git which includes the first fix, but it's an RFC as it has wider impact and might break stuff.)
Ingo
--------------->
Subject: x86: PAGE_MASK cleanup
From: Ingo Molnar <[email protected]>
Signed-off-by Ingo Molnar <[email protected]>
---
 include/asm-x86/page.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Index: linux-x86.q/include/asm-x86/page.h
===================================================================

include/asm-x86/page.h line 10

Quoted 6 lines

include/asm-x86/page.h +1 -1

@@ -6,12 +6,12 @@
 /* PAGE_SHIFT determines the page size */
 #define PAGE_SHIFT	12
 #define PAGE_SIZE	(_AC(1,UL) << PAGE_SHIFT)
-#define PAGE_MASK	(~(PAGE_SIZE-1))
+#define PAGE_MASK	(_AT(phys_addr_t, ~(PAGE_SIZE-1)))

Hm, this seems fairly wide-ranging. How about just making it signed? In fact, all these masks which are of the form "all high bits set, some low bits clear" (or perhaps "all set except some") should be signed, so that the "all high bits set" property is maintained when its extended to larger types.

Also, Andi, do you want to get rid of the PHYSICAL_MASK stuff?

Andi Kleen

2008-01-16 17:14 UTC #22 lore raw

Quoted 2 lines

Also, Andi, do you want to get rid of the PHYSICAL_MASK stuff?

That would be fine, just all the accessors need to mask out NX explicitely. I didn't have plans to code that up myself though.

-Andi

Andi Kleen

2008-01-16 17:41 UTC #23 lore raw

In reply to Ingo Molnar

git-x86 upto a9f7faa5fd229a65747f02ab0f2d45ee35856760 boots, but gives

VFS: Mounted root (nfs filesystem).
Freeing unused kernel memory: 264k freed
eth0: no IPv6 routers present
Clocksource tsc unstable (delta = 18747555652 ns)

BUG: soft lockup - CPU#2 stuck for 55s! [swapper:0]

Pid: 0, comm: swapper Not tainted (2.6.24-rc8-ga9f7faa5 #23)
EIP: 0060:[<c0103b71>] EFLAGS: 00000206 CPU: 2
EIP is at default_idle+0x36/0x58
EAX: 6f179d4d EBX: 6f179d4d ECX: 000001c9 EDX: 0000006a
ESI: 0000006a EDI: 00000000 EBP: 00000000 ESP: f7c49fa4
DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
CR0: 8005003b CR2: bfaaf990 CR3: 3749f000 CR4: 000006f0
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff0ff0 DR7: 00000400

 [<c0103b3b>] default_idle+0x0/0x58
 [<c0103a7e>] cpu_idle+0xad/0xcd
 =======================

-Andi

Ingo Molnar

2008-01-16 20:23 UTC #24 lore raw

Quoted 18 lines · Andi Kleen

git-x86 upto a9f7faa5fd229a65747f02ab0f2d45ee35856760 boots, but gives
VFS: Mounted root (nfs filesystem).
Freeing unused kernel memory: 264k freed
eth0: no IPv6 routers present
Clocksource tsc unstable (delta = 18747555652 ns)
BUG: soft lockup - CPU#2 stuck for 55s! [swapper:0]
Pid: 0, comm: swapper Not tainted (2.6.24-rc8-ga9f7faa5 #23)
EIP: 0060:[<c0103b71>] EFLAGS: 00000206 CPU: 2
EIP is at default_idle+0x36/0x58
EAX: 6f179d4d EBX: 6f179d4d ECX: 000001c9 EDX: 0000006a
ESI: 0000006a EDI: 00000000 EBP: 00000000 ESP: f7c49fa4
DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
CR0: 8005003b CR2: bfaaf990 CR3: 3749f000 CR4: 000006f0
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff0ff0 DR7: 00000400

Quoted 3 lines

 [<c0103b3b>] default_idle+0x0/0x58
 [<c0103a7e>] cpu_idle+0xad/0xcd
 =======================

truly stuck, or just an annoying message?

Ingo

Andi Kleen

2008-01-16 21:05 UTC #25 lore raw

Quoted 3 lines

truly stuck, or just an annoying message?

Just annoying message; system works after that for simple login etc. (haven't run anything complicated)

-Andi

Ingo Molnar

2008-01-16 21:07 UTC #26 lore raw

Quoted 4 lines · Andi Kleen

Quoted 1 line
truly stuck, or just an annoying message?
Just annoying message; system works after that for simple login etc. (haven't run anything complicated)

ok, if you see no other failures and if you have some time it would be nice to figure out why that message is happening. Is NOHZ enabled perhaps?

Ingo

Andi Kleen

2008-01-17 02:13 UTC #27 lore raw

Quoted 11 lines · On Wednesday 16 January 2008 22:06:46 Ingo Molnar wrote:

Quoted 4 lines · Andi Kleen
Quoted 1 line
truly stuck, or just an annoying message?
Just annoying message; system works after that for simple login etc. (haven't run anything complicated)
ok, if you see no other failures and if you have some time it would be nice to figure out why that message is happening. Is NOHZ enabled perhaps?

I updated now to latest git-x86 and the problem seems to have gone away.

But NO_HZ was enabled yes and the system also had a very unsynchronous TSC (dual K8 opteron with cpufreq)

Also I should say my patches were applied, but I didn't change anything in this area.

-Andi