Recent fix for pgd_addr_end

Recent fix for pgd_addr_end

[Peter Chubb]

Hi,
  In the recent fix to include/asm-ia64/pgtable.,h, the coode now
sign-extends the top of the pgdindex into the reserved region.

Are you sure this is correct?  Mosberger+Eranian says that these bits
must be zero: page 159, end of first paragraph, `What is unusual is
that the global-directory index is split into a 7-bit low part (pgdi)
and a 3 bit high part (pgdh) that covers bits 22 to 39 and 61 to 63
respectively.  Bits 40 to 60 are unused and must be 0.' (Of course the
bit numbers here are for 8k pages, and have to be adjusted).



-- 
Dr Peter Chubb  http://www.gelato.unsw.edu.au  peterc AT gelato.unsw.edu.au
The technical we do immediately,  the political takes *forever*

RE: Recent fix for pgd_addr_end

[Luck, Tony]

>  In the recent fix to include/asm-ia64/pgtable.,h, the coode now
>sign-extends the top of the pgdindex into the reserved region.
>
>Are you sure this is correct?  Mosberger+Eranian says that these bits
>must be zero: page 159, end of first paragraph, `What is unusual is
>that the global-directory index is split into a 7-bit low part (pgdi)
>and a 3 bit high part (pgdh) that covers bits 22 to 39 and 61 to 63
>respectively.  Bits 40 to 60 are unused and must be 0.' (Of course the
>bit numbers here are for 8k pages, and have to be adjusted).


I don't have the book handy, but the relevent chapter is online.  I see
it saying: http://www.phptr.com/articles/article.asp?p)961&seqNum=2

"The unimplemented portion of the virtual address consists of bits
IMPL_VA_MSB + 1 through 60. Even though they are marked as unimplemented,
the architecture requires that the value in these bits match the value
in bit IMPL_VA_MSB. In other words, the unimplemented bits must correspond
to the sign-extended value of the lower portion of the virtual address.
This restriction has been put in place to ensure that software does not
abuse unimplemented bits for purposes such as type tag bits. Otherwise,
such software might break when running on a machine that implements a
larger number of virtual address bits."

Figure 4.13 just below this text shows the hole in the middle
of the region that this creates.

-Tony

RE: Recent fix for pgd_addr_end

[Peter Chubb]

>>>>> "Tony" = Tony Luck <Luck> writes:

>> In the recent fix to include/asm-ia64/pgtable.,h, the coode now
>> sign-extends the top of the pgdindex into the reserved region.
>> 
>> Are you sure this is correct?  Mosberger+Eranian says that these
>> bits must be zero: page 159, end of first paragraph, `What is
>> unusual is that the global-directory index is split into a 7-bit
>> low part (pgdi) and a 3 bit high part (pgdh) that covers bits 22 to
>> 39 and 61 to 63 respectively.  Bits 40 to 60 are unused and must be
>> 0.' (Of course the bit numbers here are for 8k pages, and have to
>> be adjusted).


Tony> I don't have the book handy, but the relevent chapter is online.
Tony> I see it saying:
Tony> http://www.phptr.com/articles/article.asp?p)961&seqNum=2

Tony> "The unimplemented portion of the virtual address consists of
Tony> bits IMPL_VA_MSB + 1 through 60. Even though they are marked as
Tony> unimplemented, the architecture requires that the value in these
Tony> bits match the value in bit IMPL_VA_MSB. In other words, the
Tony> unimplemented bits must correspond to the sign-extended value of
Tony> the lower portion of the virtual address.  This restriction has
Tony> been put in place to ensure that software does not abuse
Tony> unimplemented bits for purposes such as type tag
Tony> bits. Otherwise, such software might break when running on a
Tony> machine that implements a larger number of virtual address
Tony> bits."

Tony> Figure 4.13 just below this text shows the hole in the middle of
Tony> the region that this creates.

That's the IA64 architecture defined unimplemented bits, which are
sign extended from the most significant implemented bit other than the
region ID in the virtual address.  Go onto the next page:

       http://www.phptr.com/articles/article.asp?p)961&seqNum=3

and look for figure 4.21 and its commentary.



-- 
Dr Peter Chubb  http://www.gelato.unsw.edu.au  peterc AT gelato.unsw.edu.au
The technical we do immediately,  the political takes *forever*

RE: Recent fix for pgd_addr_end

[Peter Chubb]

>>>>> "Peter" = Peter Chubb <peterc@gelato.unsw.edu.au> writes:

(The reason I'm asking all this is that I still see lots of

/home/peterc/kernels/linux-2.5-import/mm/memory.c:105: bad pmd
20000000000acdc8.
/home/peterc/kernels/linux-2.5-import/mm/memory.c:105: bad pmd
ffff004f00000002.
/home/peterc/kernels/linux-2.5-import/mm/memory.c:105: bad pmd
20000000000acdd0.

messages after booting, even with that change).

-- 
Dr Peter Chubb  http://www.gelato.unsw.edu.au  peterc AT gelato.unsw.edu.au
The technical we do immediately,  the political takes *forever*

RE: Recent fix for pgd_addr_end

[Luck, Tony]

>(The reason I'm asking all this is that I still see lots of
>
>/home/peterc/kernels/linux-2.5-import/mm/memory.c:105: bad pmd 20000000000acdc8.
>/home/peterc/kernels/linux-2.5-import/mm/memory.c:105: bad pmd ffff004f00000002.
>/home/peterc/kernels/linux-2.5-import/mm/memory.c:105: bad pmd 20000000000acdd0.
>
>messages after booting, even with that change).

Yes, I still have something wrong ... Dave Miller is still pushing
me towards a solution.

-Tony

RE: Recent fix for pgd_addr_end

[Luck, Tony]

>That's the IA64 architecture defined unimplemented bits, which are
>sign extended from the most significant implemented bit other than the
>region ID in the virtual address.  Go onto the next page:
>
>       http://www.phptr.com/articles/article.asp?p)961&seqNum=3
>
>and look for figure 4.21 and its commentary.

You are right ... I got the implemented bits confused how Linux uses
this stuff.

Thanks.

-Tony

Re: recent fix for pgd_addr_end

[Luck, Tony]

Peter, Dave, and anyone else following along:

How does this look (ignoring for the moment the usage of pgd_addr_end()
in the vmalloc() code path for region 5 ... I'll deal with that next, but
for the moment it is only a theoretical problem as we don't map anywhere
close to enough things in region 5 to trigger the IA64_PGD_OVERFLOW path
in this code).

-Tony

=== include/asm-ia64/pgtable.h 1.55 vs edited ==--- 1.55/include/asm-ia64/pgtable.h	2005-03-16 09:33:36 -08:00
+++ edited/include/asm-ia64/pgtable.h	2005-03-17 16:15:14 -08:00
@@ -557,13 +557,21 @@
  *      +--------+------+--------+-----+-----+--------+
  *      | pgdhi3 | rsvd | pgdlow | pmd | pte | offset |
  *      +--------+------+--------+-----+-----+--------+
- *  The high bit of 'pgdlow' must be sign extended across the 'rsvd' bits.
+ *  'pgdlow' overflows to pgdhi3 (a.k.a. region bits) leaving rsvd=0
  */
-#define IA64_PGD_SIGNEXTEND (PGDIR_SIZE << (PAGE_SHIFT-7))
+#define IA64_PGD_OVERFLOW (PGDIR_SIZE << (PAGE_SHIFT-6))
+
 #define pgd_addr_end(addr, end)						\
 ({	unsigned long __boundary = ((addr) + PGDIR_SIZE) & PGDIR_MASK;	\
-	if (__boundary & IA64_PGD_SIGNEXTEND)				\
-		__boundary |= (RGN_SIZE - 1) & ~(IA64_PGD_SIGNEXTEND-1);\
+	if (__boundary & IA64_PGD_OVERFLOW)				\
+		__boundary += (RGN_SIZE - 1) & ~(IA64_PGD_OVERFLOW - 1);\
+	(__boundary - 1 < (end) - 1)? __boundary: (end);		\
+})
+
+#define pmd_addr_end(addr, end)						\
+({	unsigned long __boundary = ((addr) + PMD_SIZE) & PMD_MASK;	\
+	if (__boundary & IA64_PGD_OVERFLOW)				\
+		__boundary += (RGN_SIZE - 1) & ~(IA64_PGD_OVERFLOW - 1);\
 	(__boundary - 1 < (end) - 1)? __boundary: (end);		\
 })
 

Re: recent fix for pgd_addr_end

[Luck, Tony]

Peter, Dave, and anyone else following along:

How does this look (ignoring for the moment the usage of pgd_addr_end()
in the vmalloc() code path for region 5 ... I'll deal with that next, but
for the moment it is only a theoretical problem as we don't map anywhere
close to enough things in region 5 to trigger the IA64_PGD_OVERFLOW path
in this code).

-Tony

=== include/asm-ia64/pgtable.h 1.55 vs edited ==--- 1.55/include/asm-ia64/pgtable.h	2005-03-16 09:33:36 -08:00
+++ edited/include/asm-ia64/pgtable.h	2005-03-17 16:15:14 -08:00
@@ -557,13 +557,21 @@
  *      +--------+------+--------+-----+-----+--------+
  *      | pgdhi3 | rsvd | pgdlow | pmd | pte | offset |
  *      +--------+------+--------+-----+-----+--------+
- *  The high bit of 'pgdlow' must be sign extended across the 'rsvd' bits.
+ *  'pgdlow' overflows to pgdhi3 (a.k.a. region bits) leaving rsvd=0
  */
-#define IA64_PGD_SIGNEXTEND (PGDIR_SIZE << (PAGE_SHIFT-7))
+#define IA64_PGD_OVERFLOW (PGDIR_SIZE << (PAGE_SHIFT-6))
+
 #define pgd_addr_end(addr, end)						\
 ({	unsigned long __boundary = ((addr) + PGDIR_SIZE) & PGDIR_MASK;	\
-	if (__boundary & IA64_PGD_SIGNEXTEND)				\
-		__boundary |= (RGN_SIZE - 1) & ~(IA64_PGD_SIGNEXTEND-1);\
+	if (__boundary & IA64_PGD_OVERFLOW)				\
+		__boundary += (RGN_SIZE - 1) & ~(IA64_PGD_OVERFLOW - 1);\
+	(__boundary - 1 < (end) - 1)? __boundary: (end);		\
+})
+
+#define pmd_addr_end(addr, end)						\
+({	unsigned long __boundary = ((addr) + PMD_SIZE) & PMD_MASK;	\
+	if (__boundary & IA64_PGD_OVERFLOW)				\
+		__boundary += (RGN_SIZE - 1) & ~(IA64_PGD_OVERFLOW - 1);\
 	(__boundary - 1 < (end) - 1)? __boundary: (end);		\
 })
 

Re: recent fix for pgd_addr_end

[Peter Chubb]

>>>>> "Tony" = Tony Luck <Luck> writes:

Tony> Peter, Dave, and anyone else following along: How does this look
Tony> (ignoring for the moment the usage of pgd_addr_end() in the
Tony> vmalloc() code path for region 5 ... I'll deal with that next,
Tony> but for the moment it is only a theoretical problem as we don't
Tony> map anywhere close to enough things in region 5 to trigger the
Tony> IA64_PGD_OVERFLOW path in this code).

I think it's good.  It even boots :-)

-- 
Dr Peter Chubb  http://www.gelato.unsw.edu.au  peterc AT gelato.unsw.edu.au
The technical we do immediately,  the political takes *forever*

Re: recent fix for pgd_addr_end

[Darren Williams]

Hi Luck,

On Thu, 17 Mar 2005, Luck, Tony wrote:

> Peter, Dave, and anyone else following along:
> 
> How does this look (ignoring for the moment the usage of pgd_addr_end()
> in the vmalloc() code path for region 5 ... I'll deal with that next, but
> for the moment it is only a theoretical problem as we don't map anywhere
> close to enough things in region 5 to trigger the IA64_PGD_OVERFLOW path
> in this code).
> 
> -Tony
> 

Boots on 16way HP Olyimpia:)

[snip]

Thanks
Darren

--------------------------------------------------
Darren Williams <dsw AT gelato.unsw.edu.au>
Gelato@UNSW <www.gelato.unsw.edu.au>
--------------------------------------------------

RE: recent fix for pgd_addr_end

[Luck, Tony]

>Tony> Peter, Dave, and anyone else following along: How does this look
>Tony> (ignoring for the moment the usage of pgd_addr_end() in the
>Tony> vmalloc() code path for region 5 ... I'll deal with that next,
>Tony> but for the moment it is only a theoretical problem as we don't
>Tony> map anywhere close to enough things in region 5 to trigger the
>Tony> IA64_PGD_OVERFLOW path in this code).
>
>I think it's good.  It even boots :-)

Booting is no guarantee of correctness (the last version booted
too, and it was horribly wrong).

Thanks for testing (and for pointing out the problem).

-Tony

Re: recent fix for pgd_addr_end

[Luck, Tony]

Here's the version that handles region 5 correctly too.

-Tony

=== include/asm-ia64/pgtable.h 1.55 vs edited ==--- 1.55/include/asm-ia64/pgtable.h	2005-03-16 09:33:36 -08:00
+++ edited/include/asm-ia64/pgtable.h	2005-03-17 20:46:40 -08:00
@@ -553,17 +553,25 @@
 
 /*
  * Override for pgd_addr_end() to deal with the virtual address space holes
- * in each region.  Virtual address bits are used like this:
+ * in each region.  In regions 0..4 virtual address bits are used like this:
  *      +--------+------+--------+-----+-----+--------+
  *      | pgdhi3 | rsvd | pgdlow | pmd | pte | offset |
  *      +--------+------+--------+-----+-----+--------+
- *  The high bit of 'pgdlow' must be sign extended across the 'rsvd' bits.
+ *  'pgdlow' overflows to pgdhi3 (a.k.a. region bits) leaving rsvd=0
  */
-#define IA64_PGD_SIGNEXTEND (PGDIR_SIZE << (PAGE_SHIFT-7))
+#define IA64_PGD_OVERFLOW (PGDIR_SIZE << (PAGE_SHIFT-6))
+
 #define pgd_addr_end(addr, end)						\
 ({	unsigned long __boundary = ((addr) + PGDIR_SIZE) & PGDIR_MASK;	\
-	if (__boundary & IA64_PGD_SIGNEXTEND)				\
-		__boundary |= (RGN_SIZE - 1) & ~(IA64_PGD_SIGNEXTEND-1);\
+	if (addr < DEFAULT_TASK_SIZE && __boundary & IA64_PGD_OVERFLOW)	\
+		__boundary += (RGN_SIZE - 1) & ~(IA64_PGD_OVERFLOW - 1);\
+	(__boundary - 1 < (end) - 1)? __boundary: (end);		\
+})
+
+#define pmd_addr_end(addr, end)						\
+({	unsigned long __boundary = ((addr) + PMD_SIZE) & PMD_MASK;	\
+	if (addr < DEFAULT_TASK_SIZE && __boundary & IA64_PGD_OVERFLOW)	\
+		__boundary += (RGN_SIZE - 1) & ~(IA64_PGD_OVERFLOW - 1);\
 	(__boundary - 1 < (end) - 1)? __boundary: (end);		\
 })
 

Re: recent fix for pgd_addr_end

[Peter Chubb]

>>>>> "Tony" = Tony Luck <Luck> writes:

Tony> Here's the version that handles region 5 correctly too.  -Tony

Is it always OK to evaluate addr and end more than once?

Or would something like
({ unsigned long __boundary = ((addr) + PGDIR_SIZE) & PGD_MASK;
   unsigend long __region  = __boundary >> 61;
   if (region =5)
      ...
   else
     ...
})
be better?

Re: recent fix for pgd_addr_end

[David Mosberger]

Do we really need to worry about region-crossing in the pgd_addr_end()
macro?  No single vm_area may cros a region boundary.

	--david

Re: recent fix for pgd_addr_end

[David S. Miller]

On Thu, 24 Mar 2005 10:45:09 -0800
David Mosberger <davidm@napali.hpl.hp.com> wrote:

> Do we really need to worry about region-crossing in the pgd_addr_end()
> macro?  No single vm_area may cros a region boundary.

clear_page_range() walks the page tables across the entire
address space and thus across VMA's.

That's why the macro overrides were necessary, to cope with that.

With Hugh Dickin's stuff, we will be doing clear_page_range() via
VMAs and thus eliminate this ugly stuff.

This has all been spelled out pretty explicitly in the various
threads.