Re: [PATCH] [1/9] Handle kernel near memory hole in clear_kernel_mapping
> > +#define overlaps(as, ae, bs, be) ((ae) >= (bs) && (as) <= (be))
>
> inline function please and a bit more intuituive arrangement of
> arguments.
Which one do you prefer? (to be honest the current one is "intuitive"
to me)
>
> > +void __init clear_kernel_mapping(unsigned long address, unsigned long size)
> > +{
> > + int sh = PMD_SHIFT;
> > + unsigned long kernel = __pa(__START_KERNEL_map);
> > +
> > + /*
> > +* Note that we cannot unmap the kernel itself because the unmapped
> > +* holes here are always at least 2MB aligned.
>
> That's not enforced. The unmap code just does not split pages.
It is -- there are BUG_ONs for this in __clear_kernel_mapping
>
> > +* This just applies to the trailing areas of the 40MB kernel mapping.
>
> How is this ensured, that it only affects the end of the 40MB mapping ?
It is enforced in the callers (actually there is only a single caller --
the GART code ) by not calling it overlapping for the kernel itself.
Given that could be checked too, but that would be probably overkill
for an internal function.
>
> > +*/
> > + if (overlaps(kernel >> sh, (kernel + KERNEL_TEXT_SIZE) >> sh,
> > + __pa(address) >> sh, __pa(address + size) >> sh)) {
>
> This checks:
>
> (kernel_end + 1) >= gart_start && kernel_start <= gart_end
>
> One off error: kernel + KERNEL_TEXT_SIZE
> needs to be:kernel + KERNEL_TEXT_SIZE - 1
Ok.
>
> Also there is no sanity check, whether the area is inside real kernel text.
Hmm I can add one, but if that happens the caller is likely seriously
confused and will likely cause other problems anyways.
I don't think it can happen for the GART code which is currently
the only caller.
>
> > + printk(KERN_WARNING
> > + "Kernel mapping at %lx within 2MB of memory hole\n",
> > + kernel);
>
>
> > + __clear_kernel_mapping(__START_KERNEL_map+__pa(address), size);
>
> Doh! This is unmapping the wrong place. According to __phys_addr():
>
> paddr = vaddr - __START_KERNEL_map + phys_base;
Hmm true -- that will only affect relocatable kernels, but for those
it's wrong. Given that the patch was supposed to fix a case
that only happens relocatable kernels that's quite ironic :)
Actually thinking about it again you can just drop it for now.
It is orthogonal to gbpages. I think I added it to the series
when I was planning to do kernel mapping as GB pages, but that
turned out to be a bad idea anyways.
Thanks for the review,
-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [1/9] Handle kernel near memory hole in clear_kernel_mapping
On Tue, 29 Jan 2008, Andi Kleen wrote:
> This was a long standing obscure problem in the relocatable kernel. The
> AMD GART driver needs to unmap part of the GART in the kernel direct mapping
> to
> prevent cache corruption. With the relocatable kernel it is in theory
> possible
> that the separate kernel text mapping straddles that area too.
>
> Normally it should not happen because GART tends to be >= 2GB, and the kernel
> is normally not loaded that high, but it is possible in theory.
This smells fishy.
>
>
> +#define overlaps(as, ae, bs, be) ((ae) >= (bs) && (as) <= (be))
inline function please and a bit more intuituive arrangement of
arguments.
> +void __init clear_kernel_mapping(unsigned long address, unsigned long size)
> +{
> + int sh = PMD_SHIFT;
> + unsigned long kernel = __pa(__START_KERNEL_map);
> +
> + /*
> + * Note that we cannot unmap the kernel itself because the unmapped
> + * holes here are always at least 2MB aligned.
That's not enforced. The unmap code just does not split pages.
> + * This just applies to the trailing areas of the 40MB kernel mapping.
How is this ensured, that it only affects the end of the 40MB mapping ?
> + */
> + if (overlaps(kernel >> sh, (kernel + KERNEL_TEXT_SIZE) >> sh,
> + __pa(address) >> sh, __pa(address + size) >> sh)) {
This checks:
(kernel_end + 1) >= gart_start && kernel_start <= gart_end
One off error: kernel + KERNEL_TEXT_SIZE
needs to be:kernel + KERNEL_TEXT_SIZE - 1
Also there is no sanity check, whether the area is inside real kernel text.
> + printk(KERN_WARNING
> + "Kernel mapping at %lx within 2MB of memory hole\n",
> + kernel);
> + __clear_kernel_mapping(__START_KERNEL_map+__pa(address), size);
Doh! This is unmapping the wrong place. According to __phys_addr():
paddr = vaddr - __START_KERNEL_map + phys_base;
Transformation:
vaddr = padd + __START_KERNEL_map - phys_base;
Sigh. The clear_kernel_mapping() functionality is just another wreckaged
implementation of CPA.
We really should use CPA and have a function to mark entries not
present. Actually the DEBUG_PAGEALLOC code has one already. There are
sanity checks in the CPA code as well, which can be extended to cover
the GART unmap in a safe way.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [1/9] Handle kernel near memory hole in clear_kernel_mapping
On Tue, 29 Jan 2008, Andi Kleen wrote:
This was a long standing obscure problem in the relocatable kernel. The
AMD GART driver needs to unmap part of the GART in the kernel direct mapping
to
prevent cache corruption. With the relocatable kernel it is in theory
possible
that the separate kernel text mapping straddles that area too.
Normally it should not happen because GART tends to be = 2GB, and the kernel
is normally not loaded that high, but it is possible in theory.
This smells fishy.
+#define overlaps(as, ae, bs, be) ((ae) = (bs) (as) = (be))
inline function please and a bit more intuituive arrangement of
arguments.
+void __init clear_kernel_mapping(unsigned long address, unsigned long size)
+{
+ int sh = PMD_SHIFT;
+ unsigned long kernel = __pa(__START_KERNEL_map);
+
+ /*
+ * Note that we cannot unmap the kernel itself because the unmapped
+ * holes here are always at least 2MB aligned.
That's not enforced. The unmap code just does not split pages.
+ * This just applies to the trailing areas of the 40MB kernel mapping.
How is this ensured, that it only affects the end of the 40MB mapping ?
+ */
+ if (overlaps(kernel sh, (kernel + KERNEL_TEXT_SIZE) sh,
+ __pa(address) sh, __pa(address + size) sh)) {
This checks:
(kernel_end + 1) = gart_start kernel_start = gart_end
One off error: kernel + KERNEL_TEXT_SIZE
needs to be:kernel + KERNEL_TEXT_SIZE - 1
Also there is no sanity check, whether the area is inside real kernel text.
+ printk(KERN_WARNING
+ Kernel mapping at %lx within 2MB of memory hole\n,
+ kernel);
+ __clear_kernel_mapping(__START_KERNEL_map+__pa(address), size);
Doh! This is unmapping the wrong place. According to __phys_addr():
paddr = vaddr - __START_KERNEL_map + phys_base;
Transformation:
vaddr = padd + __START_KERNEL_map - phys_base;
Sigh. The clear_kernel_mapping() functionality is just another wreckaged
implementation of CPA.
We really should use CPA and have a function to mark entries not
present. Actually the DEBUG_PAGEALLOC code has one already. There are
sanity checks in the CPA code as well, which can be extended to cover
the GART unmap in a safe way.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [1/9] Handle kernel near memory hole in clear_kernel_mapping
+#define overlaps(as, ae, bs, be) ((ae) = (bs) (as) = (be))
inline function please and a bit more intuituive arrangement of
arguments.
Which one do you prefer? (to be honest the current one is intuitive
to me)
+void __init clear_kernel_mapping(unsigned long address, unsigned long size)
+{
+ int sh = PMD_SHIFT;
+ unsigned long kernel = __pa(__START_KERNEL_map);
+
+ /*
+* Note that we cannot unmap the kernel itself because the unmapped
+* holes here are always at least 2MB aligned.
That's not enforced. The unmap code just does not split pages.
It is -- there are BUG_ONs for this in __clear_kernel_mapping
+* This just applies to the trailing areas of the 40MB kernel mapping.
How is this ensured, that it only affects the end of the 40MB mapping ?
It is enforced in the callers (actually there is only a single caller --
the GART code ) by not calling it overlapping for the kernel itself.
Given that could be checked too, but that would be probably overkill
for an internal function.
+*/
+ if (overlaps(kernel sh, (kernel + KERNEL_TEXT_SIZE) sh,
+ __pa(address) sh, __pa(address + size) sh)) {
This checks:
(kernel_end + 1) = gart_start kernel_start = gart_end
One off error: kernel + KERNEL_TEXT_SIZE
needs to be:kernel + KERNEL_TEXT_SIZE - 1
Ok.
Also there is no sanity check, whether the area is inside real kernel text.
Hmm I can add one, but if that happens the caller is likely seriously
confused and will likely cause other problems anyways.
I don't think it can happen for the GART code which is currently
the only caller.
+ printk(KERN_WARNING
+ Kernel mapping at %lx within 2MB of memory hole\n,
+ kernel);
+ __clear_kernel_mapping(__START_KERNEL_map+__pa(address), size);
Doh! This is unmapping the wrong place. According to __phys_addr():
paddr = vaddr - __START_KERNEL_map + phys_base;
Hmm true -- that will only affect relocatable kernels, but for those
it's wrong. Given that the patch was supposed to fix a case
that only happens relocatable kernels that's quite ironic :)
Actually thinking about it again you can just drop it for now.
It is orthogonal to gbpages. I think I added it to the series
when I was planning to do kernel mapping as GB pages, but that
turned out to be a bad idea anyways.
Thanks for the review,
-Andi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [1/9] Handle kernel near memory hole in clear_kernel_mapping
This was a long standing obscure problem in the relocatable kernel. The
AMD GART driver needs to unmap part of the GART in the kernel direct mapping to
prevent cache corruption. With the relocatable kernel it is in theory possible
that the separate kernel text mapping straddles that area too.
Normally it should not happen because GART tends to be >= 2GB, and the kernel
is normally not loaded that high, but it is possible in theory.
Teach clear_kernel_mapping() about this case.
This will become more important once the kernel mapping uses 1GB pages.
Cc: [EMAIL PROTECTED]
Cc: [EMAIL PROTECTED]
Signed-off-by: Andi Kleen <[EMAIL PROTECTED]>
---
arch/x86/mm/init_64.c | 25 -
1 file changed, 24 insertions(+), 1 deletion(-)
Index: linux/arch/x86/mm/init_64.c
===
--- linux.orig/arch/x86/mm/init_64.c
+++ linux/arch/x86/mm/init_64.c
@@ -438,7 +438,8 @@ void __init paging_init(void)
* address and size must be aligned to 2MB boundaries.
* Does nothing when the mapping doesn't exist.
*/
-void __init clear_kernel_mapping(unsigned long address, unsigned long size)
+static void __init
+__clear_kernel_mapping(unsigned long address, unsigned long size)
{
unsigned long end = address + size;
@@ -475,6 +476,28 @@ void __init clear_kernel_mapping(unsigne
__flush_tlb_all();
}
+#define overlaps(as, ae, bs, be) ((ae) >= (bs) && (as) <= (be))
+
+void __init clear_kernel_mapping(unsigned long address, unsigned long size)
+{
+ int sh = PMD_SHIFT;
+ unsigned long kernel = __pa(__START_KERNEL_map);
+
+ /*
+* Note that we cannot unmap the kernel itself because the unmapped
+* holes here are always at least 2MB aligned.
+* This just applies to the trailing areas of the 40MB kernel mapping.
+*/
+ if (overlaps(kernel >> sh, (kernel + KERNEL_TEXT_SIZE) >> sh,
+ __pa(address) >> sh, __pa(address + size) >> sh)) {
+ printk(KERN_WARNING
+ "Kernel mapping at %lx within 2MB of memory hole\n",
+ kernel);
+ __clear_kernel_mapping(__START_KERNEL_map+__pa(address), size);
+ }
+ __clear_kernel_mapping(address, size);
+}
+
/*
* Memory hotplug specific functions
*/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [1/9] Handle kernel near memory hole in clear_kernel_mapping
This was a long standing obscure problem in the relocatable kernel. The
AMD GART driver needs to unmap part of the GART in the kernel direct mapping to
prevent cache corruption. With the relocatable kernel it is in theory possible
that the separate kernel text mapping straddles that area too.
Normally it should not happen because GART tends to be = 2GB, and the kernel
is normally not loaded that high, but it is possible in theory.
Teach clear_kernel_mapping() about this case.
This will become more important once the kernel mapping uses 1GB pages.
Cc: [EMAIL PROTECTED]
Cc: [EMAIL PROTECTED]
Signed-off-by: Andi Kleen [EMAIL PROTECTED]
---
arch/x86/mm/init_64.c | 25 -
1 file changed, 24 insertions(+), 1 deletion(-)
Index: linux/arch/x86/mm/init_64.c
===
--- linux.orig/arch/x86/mm/init_64.c
+++ linux/arch/x86/mm/init_64.c
@@ -438,7 +438,8 @@ void __init paging_init(void)
* address and size must be aligned to 2MB boundaries.
* Does nothing when the mapping doesn't exist.
*/
-void __init clear_kernel_mapping(unsigned long address, unsigned long size)
+static void __init
+__clear_kernel_mapping(unsigned long address, unsigned long size)
{
unsigned long end = address + size;
@@ -475,6 +476,28 @@ void __init clear_kernel_mapping(unsigne
__flush_tlb_all();
}
+#define overlaps(as, ae, bs, be) ((ae) = (bs) (as) = (be))
+
+void __init clear_kernel_mapping(unsigned long address, unsigned long size)
+{
+ int sh = PMD_SHIFT;
+ unsigned long kernel = __pa(__START_KERNEL_map);
+
+ /*
+* Note that we cannot unmap the kernel itself because the unmapped
+* holes here are always at least 2MB aligned.
+* This just applies to the trailing areas of the 40MB kernel mapping.
+*/
+ if (overlaps(kernel sh, (kernel + KERNEL_TEXT_SIZE) sh,
+ __pa(address) sh, __pa(address + size) sh)) {
+ printk(KERN_WARNING
+ Kernel mapping at %lx within 2MB of memory hole\n,
+ kernel);
+ __clear_kernel_mapping(__START_KERNEL_map+__pa(address), size);
+ }
+ __clear_kernel_mapping(address, size);
+}
+
/*
* Memory hotplug specific functions
*/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
