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

Cyrill Gorcunov

unread,

Nov 8, 2009, 11:00:02 AM11/8/09

to

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 <yin...@kernel.org>
CC: "Maciej W. Rozycki" <ma...@linux-mips.org>
Signed-off-by: Cyrill Gorcunov <gorc...@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++;
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

tip-bot for Cyrill Gorcunov

unread,

Nov 8, 2009, 11:20:02 AM11/8/09

to

Commit-ID: 46dc281b1bb02527195fe2ad50a3af6d7f7f7325
Gitweb: http://git.kernel.org/tip/46dc281b1bb02527195fe2ad50a3af6d7f7f7325
Author: Cyrill Gorcunov <gorc...@openvz.org>
AuthorDate: Sun, 8 Nov 2009 18:53:56 +0300
Committer: Ingo Molnar <mi...@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 <gorc...@openvz.org>
Cc: Yinghai Lu <yin...@kernel.org>
Cc: Maciej W. Rozycki <ma...@linux-mips.org>
LKML-Reference: <20091108155356.GB25940@lenovo>
Signed-off-by: Ingo Molnar <mi...@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:

Maciej W. Rozycki

unread,

Nov 8, 2009, 12:30:03 PM11/8/09

to

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 <yin...@kernel.org>
> CC: "Maciej W. Rozycki" <ma...@linux-mips.org>
> Signed-off-by: Cyrill Gorcunov <gorc...@openvz.org>
> ---

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

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

Maciej

Cyrill Gorcunov

unread,

Nov 8, 2009, 12:40:01 PM11/8/09

to

[Maciej W. Rozycki - Sun, Nov 08, 2009 at 05:28:05PM +0000]

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

|
| Maciej
|
-- Cyrill

Ingo Molnar

unread,

Nov 8, 2009, 1:10:01 PM11/8/09

to

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

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

Ingo

H. Peter Anvin

unread,

Nov 9, 2009, 6:10:02 PM11/9/09

to

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 <yin...@kernel.org>
> CC: "Maciej W. Rozycki" <ma...@linux-mips.org>
> Signed-off-by: Cyrill Gorcunov <gorc...@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

Maciej W. Rozycki

unread,

Nov 9, 2009, 7:20:01 PM11/9/09

to

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

Cyrill Gorcunov

unread,

Nov 9, 2009, 11:50:02 PM11/9/09

to

On 11/10/09, Maciej W. Rozycki <ma...@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.