[PATCH -tip] x86,apic: Use PAGE_SIZE instead of numbers

[PATCH -tip] x86,apic: Use PAGE_SIZE instead of numbers

[Cyrill Gorcunov]

The whole page is reserved for IO-APIC fixmap
due to non-cacheable requirement. So lets note
this explicitly instead of playing with numbers.

CC: Yinghai Lu <yinghai@kernel.org>
CC: "Maciej W. Rozycki" <macro@linux-mips.org>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---

Perhaps I miss something and (4 * 1024) have some special
meaning?

 arch/x86/kernel/apic/io_apic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.git/arch/x86/kernel/apic/io_apic.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic/io_apic.c
+++ linux-2.6.git/arch/x86/kernel/apic/io_apic.c
@@ -4144,7 +4144,7 @@ fake_ioapic_page:
 		idx++;
 
 		ioapic_res->start = ioapic_phys;
-		ioapic_res->end = ioapic_phys + (4 * 1024) - 1;
+		ioapic_res->end = ioapic_phys + PAGE_SIZE - 1;
 		ioapic_res++;
 	}
 }

[tip:x86/apic] x86, apic: Use PAGE_SIZE instead of numbers

[tip-bot for Cyrill Gorcunov]

Commit-ID:  46dc281b1bb02527195fe2ad50a3af6d7f7f7325
Gitweb:     http://git.kernel.org/tip/46dc281b1bb02527195fe2ad50a3af6d7f7f7325
Author:     Cyrill Gorcunov <gorcunov@openvz.org>
AuthorDate: Sun, 8 Nov 2009 18:53:56 +0300
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 8 Nov 2009 17:06:22 +0100

x86, apic: Use PAGE_SIZE instead of numbers

The whole page is reserved for IO-APIC fixmap
due to non-cacheable requirement. So lets note
this explicitly instead of playing with numbers.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Maciej W. Rozycki <macro@linux-mips.org>
LKML-Reference: <20091108155356.GB25940@lenovo>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/apic/io_apic.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 31e9db3..9ee1c16 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -4111,7 +4111,7 @@ fake_ioapic_page:
 		idx++;
 
 		ioapic_res->start = ioapic_phys;
-		ioapic_res->end = ioapic_phys + (4 * 1024) - 1;
+		ioapic_res->end = ioapic_phys + PAGE_SIZE-1;
 		ioapic_res++;
 	}
 }

Re: [PATCH -tip] x86,apic: Use PAGE_SIZE instead of numbers

[Maciej W. Rozycki]

On Sun, 8 Nov 2009, Cyrill Gorcunov wrote:

> The whole page is reserved for IO-APIC fixmap
> due to non-cacheable requirement. So lets note
> this explicitly instead of playing with numbers.
> 
> CC: Yinghai Lu <yinghai@kernel.org>
> CC: "Maciej W. Rozycki" <macro@linux-mips.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> ---

Acked-by: Maciej W. Rozycki <macro@linux-mips.org>

 Is PAGE_SIZE still always 4kB for the x86 these days?  If so, then your 
change makes sense to me.

  Maciej

Re: [PATCH -tip] x86,apic: Use PAGE_SIZE instead of numbers

[Cyrill Gorcunov]

[Maciej W. Rozycki - Sun, Nov 08, 2009 at 05:28:05PM +0000]
| On Sun, 8 Nov 2009, Cyrill Gorcunov wrote:
| 
| > The whole page is reserved for IO-APIC fixmap
| > due to non-cacheable requirement. So lets note
| > this explicitly instead of playing with numbers.
| > 
| > CC: Yinghai Lu <yinghai@kernel.org>
| > CC: "Maciej W. Rozycki" <macro@linux-mips.org>
| > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
| > ---
| 
| Acked-by: Maciej W. Rozycki <macro@linux-mips.org>
| 
|  Is PAGE_SIZE still always 4kB for the x86 these days?  If so, then your 
| change makes sense to me.

Yeah, as far as I know. I didn't find any sign if we going to
change PAGE_SHIFT.

| 
|   Maciej
| 
	-- Cyrill

Re: [PATCH -tip] x86,apic: Use PAGE_SIZE instead of numbers

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> [Maciej W. Rozycki - Sun, Nov 08, 2009 at 05:28:05PM +0000]
> | On Sun, 8 Nov 2009, Cyrill Gorcunov wrote:
> | 
> | > The whole page is reserved for IO-APIC fixmap
> | > due to non-cacheable requirement. So lets note
> | > this explicitly instead of playing with numbers.
> | > 
> | > CC: Yinghai Lu <yinghai@kernel.org>
> | > CC: "Maciej W. Rozycki" <macro@linux-mips.org>
> | > Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> | > ---
> | 
> | Acked-by: Maciej W. Rozycki <macro@linux-mips.org>
> | 
> |  Is PAGE_SIZE still always 4kB for the x86 these days?  If so, then your 
> | change makes sense to me.
> 
> Yeah, as far as I know. I didn't find any sign if we going to change 
> PAGE_SHIFT.

Not in the next 10-20 years or so. (and any prediction beyond that is 
pointless i suspect)

	Ingo

Re: [PATCH -tip] x86,apic: Use PAGE_SIZE instead of numbers

[H. Peter Anvin]

On 11/08/2009 07:53 AM, Cyrill Gorcunov wrote:
> The whole page is reserved for IO-APIC fixmap
> due to non-cacheable requirement. So lets note
> this explicitly instead of playing with numbers.
> 
> CC: Yinghai Lu <yinghai@kernel.org>
> CC: "Maciej W. Rozycki" <macro@linux-mips.org>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> ---
> 
> Perhaps I miss something and (4 * 1024) have some special
> meaning?
> 
>  arch/x86/kernel/apic/io_apic.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6.git/arch/x86/kernel/apic/io_apic.c
> =====================================================================
> --- linux-2.6.git.orig/arch/x86/kernel/apic/io_apic.c
> +++ linux-2.6.git/arch/x86/kernel/apic/io_apic.c
> @@ -4144,7 +4144,7 @@ fake_ioapic_page:
>  		idx++;
>  
>  		ioapic_res->start = ioapic_phys;
> -		ioapic_res->end = ioapic_phys + (4 * 1024) - 1;
> +		ioapic_res->end = ioapic_phys + PAGE_SIZE - 1;
>  		ioapic_res++;
>  	}
>  }

In theory we could have more than one ioapic packed into a single page,
and it is also entirely plausible we'll support other page sizes in x86
at some point.  However, it's probably easier to flag something as
PAGE_SIZE and have to fix it up later than have magic constants, so I
think it's probably the right thing to do.

	-hpa

Re: [PATCH -tip] x86,apic: Use PAGE_SIZE instead of numbers

[Maciej W. Rozycki]

On Mon, 9 Nov 2009, H. Peter Anvin wrote:

> In theory we could have more than one ioapic packed into a single page,
> and it is also entirely plausible we'll support other page sizes in x86
> at some point.  However, it's probably easier to flag something as
> PAGE_SIZE and have to fix it up later than have magic constants, so I
> think it's probably the right thing to do.

 Hmm, the MPS said in Chapter 3.6.5 "APIC Memory Mapping":

 "Non-default APIC base addresses can be used if the MP configuration 
table is provided. (Refer to Chapter 4.)  However, the local APIC base 
address must be aligned on a 4K boundary, and the I/O APIC base address 
must be aligned on a 1K boundary."

This probably still stands; I suppose it would be safer to define 
IOAPIC_SLOT_SIZE to 1024 and use it by default, expanding all reservations 
as needed where less than PAGE_SIZE / IOAPIC_SLOT_SIZE I/O APICs would be 
mapped in a page.  This is relatively recent a piece of code -- how much 
has it been tested?

 Well, actually not much as a quick search has revealed this message:

http://marc.info/?l=linux-kernel&m=118114792006520

which shows page alignment of I/O APICs clearly does not stand, and 
moreover there are two pairs of I/O APIC in the system reported which 
share a page each.  In this case the ranges requested do not make sense 
and some resource insertions will silently fail.  And also while page 
aliases created in fixmaps here should not harm, they make me feel a 
little bit chilly...

 Overall this piece of code needs an overhaul, fixing resource allocation 
and reusing fixmaps where possible.

  Maciej 

Re: [PATCH -tip] x86,apic: Use PAGE_SIZE instead of numbers

[Cyrill Gorcunov]

On 11/10/09, Maciej W. Rozycki <macro@linux-mips.org> wrote:
> On Mon, 9 Nov 2009, H. Peter Anvin wrote:
>
>> In theory we could have more than one ioapic packed into a single page,
>> and it is also entirely plausible we'll support other page sizes in x86
>> at some point.  However, it's probably easier to flag something as
>> PAGE_SIZE and have to fix it up later than have magic constants, so I
>> think it's probably the right thing to do.
>
>  Hmm, the MPS said in Chapter 3.6.5 "APIC Memory Mapping":
>
>  "Non-default APIC base addresses can be used if the MP configuration
> table is provided. (Refer to Chapter 4.)  However, the local APIC base
> address must be aligned on a 4K boundary, and the I/O APIC base address
> must be aligned on a 1K boundary."
>
> This probably still stands; I suppose it would be safer to define
> IOAPIC_SLOT_SIZE to 1024 and use it by default, expanding all reservations
> as needed where less than PAGE_SIZE / IOAPIC_SLOT_SIZE I/O APICs would be

yes, it would be even more clear, i'll take care

> mapped in a page.  This is relatively recent a piece of code -- how much
> has it been tested?
>
>  Well, actually not much as a quick search has revealed this message:
>
> http://marc.info/?l=linux-kernel&m=118114792006520
>
> which shows page alignment of I/O APICs clearly does not stand, and
> moreover there are two pairs of I/O APIC in the system reported which
> share a page each.  In this case the ranges requested do not make sense
> and some resource insertions will silently fail.  And also while page
> aliases created in fixmaps here should not harm, they make me feel a
> little bit chilly...
>
>  Overall this piece of code needs an overhaul, fixing resource allocation
> and reusing fixmaps where possible.

Ok

>
>   Maciej
>

thanks, Peter, Maciej for comments!
I must admit (to be fair) i was concerning about fixmap itself so that
forgot about 1k alignment requirement :/ Will fix.