All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
@ 2011-08-25 23:05 Bjorn Helgaas
  2011-08-25 23:05 ` [PATCH 2/2] x86, ioapic: Announce resources reserved " Bjorn Helgaas
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2011-08-25 23:05 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin
  Cc: RalfJungralfjung-e, Cyrill Gorcunov, Yinghai Lu, Suresh Siddha,
	linux-kernel

Previously we reserved 1024 bytes, but that's more space than the IOAPIC
consumes, and it can cause conflicts with nearby devices.  The known
requirement is 68 bytes (sizeof(struct io_apic)), and rounding up to a
power-of-2 gives us 128.

The bug reported below is caused by the following assignments (the IOAPIC
power-on default and the watchdog address recommended in the AMD SP5100
BIOS Developer's Guide):

  IOAPIC[0]        at [mem 0xfec00000-0xfec003ff]
  SP5100 TCO timer at [mem 0xfec000f0-0xfec000f7]

Reported-by: Ralf Jung ralfjung-e@gmx.de
Reference: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=638863
Cc: Cyrill Gorcunov <gorcunov@openvz.org>
Cc: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/x86/include/asm/apicdef.h |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h
index 34595d5..855a18a 100644
--- a/arch/x86/include/asm/apicdef.h
+++ b/arch/x86/include/asm/apicdef.h
@@ -12,10 +12,11 @@
 #define	APIC_DEFAULT_PHYS_BASE		0xfee00000
 
 /*
- * This is the IO-APIC register space as specified
- * by Intel docs:
+ * I/O APICs are accessed indirectly via an index/data pair and an EOI
+ * register.  For example, see sec 13.5.1, "APIC Register Map," in the
+ * Intel ICH10 datasheet and the struct io_apic definition.
  */
-#define IO_APIC_SLOT_SIZE		1024
+#define IO_APIC_SLOT_SIZE		128
 
 #define	APIC_ID		0x20
 


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/2] x86, ioapic: Announce resources reserved for IOAPICs
  2011-08-25 23:05 [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs Bjorn Helgaas
@ 2011-08-25 23:05 ` Bjorn Helgaas
  2011-08-25 23:33 ` [PATCH 1/2] x86, ioapic: Reserve only 128 bytes " Suresh Siddha
       [not found] ` <CAErSpo5kEw=VTVv-=_D3hQg5oRNL9yEyJUnpP0biH=t3WRXMZw@mail.gmail.com>
  2 siblings, 0 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2011-08-25 23:05 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin; +Cc: Suresh Siddha, linux-kernel

Delay the IOAPIC output a bit so we can print the actual resources we
reserve.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 arch/x86/kernel/apic/io_apic.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 8eb863e..914027e 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -3876,6 +3876,7 @@ void __init ioapic_and_gsi_init(void)
 {
 	unsigned long ioapic_phys, idx = FIX_IO_APIC_BASE_0;
 	struct resource *ioapic_res;
+	struct mp_ioapic_gsi *gsi_cfg;
 	int i;
 
 	ioapic_res = ioapic_setup_resources(nr_ioapics);
@@ -3908,6 +3909,13 @@ fake_ioapic_page:
 
 		ioapic_res->start = ioapic_phys;
 		ioapic_res->end = ioapic_phys + IO_APIC_SLOT_SIZE - 1;
+
+		gsi_cfg = mp_ioapic_gsi_routing(i);
+
+		printk(KERN_INFO "IOAPIC[%d]: id %d v%d %pR GSI %d-%d\n",
+		       i, mpc_ioapic_id(i), mpc_ioapic_ver(i), ioapic_res,
+		       gsi_cfg->gsi_base, gsi_cfg->gsi_end);
+
 		ioapic_res++;
 	}
 
@@ -4016,11 +4024,6 @@ void __init mp_register_ioapic(int id, u32 address, u32 gsi_base)
 	if (gsi_cfg->gsi_end >= gsi_top)
 		gsi_top = gsi_cfg->gsi_end + 1;
 
-	printk(KERN_INFO "IOAPIC[%d]: apic_id %d, version %d, address 0x%x, "
-	       "GSI %d-%d\n", idx, mpc_ioapic_id(idx),
-	       mpc_ioapic_ver(idx), mpc_ioapic_addr(idx),
-	       gsi_cfg->gsi_base, gsi_cfg->gsi_end);
-
 	nr_ioapics++;
 }
 


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
  2011-08-25 23:05 [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs Bjorn Helgaas
  2011-08-25 23:05 ` [PATCH 2/2] x86, ioapic: Announce resources reserved " Bjorn Helgaas
@ 2011-08-25 23:33 ` Suresh Siddha
  2011-08-26  0:17   ` Bjorn Helgaas
       [not found] ` <CAErSpo5kEw=VTVv-=_D3hQg5oRNL9yEyJUnpP0biH=t3WRXMZw@mail.gmail.com>
  2 siblings, 1 reply; 18+ messages in thread
From: Suresh Siddha @ 2011-08-25 23:33 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, RalfJungralfjung-e,
	Cyrill Gorcunov, Yinghai Lu, linux-kernel

On Thu, 2011-08-25 at 16:05 -0700, Bjorn Helgaas wrote:
> Previously we reserved 1024 bytes, but that's more space than the IOAPIC
> consumes, and it can cause conflicts with nearby devices.  The known
> requirement is 68 bytes (sizeof(struct io_apic)), and rounding up to a
> power-of-2 gives us 128.
> 

Bjorn, Given the info from Intel that most of its io-apic
implementations has registers up to 0xff offset (reserved), does
reserving just the 128 bytes for the io-apic cause any address conflicts
if the next 128 bytes are allocated (by the OS) for any other device.

Or OS doesn't allocate this range to any other device and its only the
bios which allocates the addresses in this range and OS just ensures
that there are no conflicts?

thanks,
suresh

> The bug reported below is caused by the following assignments (the IOAPIC
> power-on default and the watchdog address recommended in the AMD SP5100
> BIOS Developer's Guide):
> 
>   IOAPIC[0]        at [mem 0xfec00000-0xfec003ff]
>   SP5100 TCO timer at [mem 0xfec000f0-0xfec000f7]
> 
> Reported-by: Ralf Jung ralfjung-e@gmx.de
> Reference: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=638863
> Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  arch/x86/include/asm/apicdef.h |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h
> index 34595d5..855a18a 100644
> --- a/arch/x86/include/asm/apicdef.h
> +++ b/arch/x86/include/asm/apicdef.h
> @@ -12,10 +12,11 @@
>  #define	APIC_DEFAULT_PHYS_BASE		0xfee00000
>  
>  /*
> - * This is the IO-APIC register space as specified
> - * by Intel docs:
> + * I/O APICs are accessed indirectly via an index/data pair and an EOI
> + * register.  For example, see sec 13.5.1, "APIC Register Map," in the
> + * Intel ICH10 datasheet and the struct io_apic definition.
>   */
> -#define IO_APIC_SLOT_SIZE		1024
> +#define IO_APIC_SLOT_SIZE		128
>  
>  #define	APIC_ID		0x20
>  
> 



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
  2011-08-25 23:33 ` [PATCH 1/2] x86, ioapic: Reserve only 128 bytes " Suresh Siddha
@ 2011-08-26  0:17   ` Bjorn Helgaas
  2011-08-26  1:41     ` H. Peter Anvin
  2011-08-26  6:22     ` Cyrill Gorcunov
  0 siblings, 2 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2011-08-26  0:17 UTC (permalink / raw)
  To: Suresh Siddha
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Ralf Jung,
	Cyrill Gorcunov, Yinghai Lu, linux-kernel

On Thu, Aug 25, 2011 at 5:33 PM, Suresh Siddha
<suresh.b.siddha@intel.com> wrote:
> On Thu, 2011-08-25 at 16:05 -0700, Bjorn Helgaas wrote:
>> Previously we reserved 1024 bytes, but that's more space than the IOAPIC
>> consumes, and it can cause conflicts with nearby devices.  The known
>> requirement is 68 bytes (sizeof(struct io_apic)), and rounding up to a
>> power-of-2 gives us 128.
>>
>
> Bjorn, Given the info from Intel that most of its io-apic
> implementations has registers up to 0xff offset (reserved), does
> reserving just the 128 bytes for the io-apic cause any address conflicts
> if the next 128 bytes are allocated (by the OS) for any other device.

If the OS allocated the next 128 bytes to another device, it sounds
like it would cause a conflict on Intel boxes.  This must be an area
that differs between vendors.  I haven't seen a spec that mentions 256
bytes as the required minimum MMIO size for IOAPICs, and apparently
the AMD IOAPIC decodes 240 bytes or fewer.

> Or OS doesn't allocate this range to any other device and its only the
> bios which allocates the addresses in this range and OS just ensures
> that there are no conflicts?

This patch only changes the region marked "busy" by the IOAPIC code.
This is analogous to a driver using request_mem_region() to show what
it's using.

That's different from the information about the range decoded by a
device, e.g,. what we learn from PCI BARs or ACPI _CRS.  We always
need this kind of information so we can avoid handing out that space
to another device, even if the driver isn't loaded or we aren't using
the IOAPICs, e.g., booting with "noapic".

This patch doesn't change the "range decoding" information, so I think
we're fairly safe.  I'm not sure we're completely safe because some
machines have PNP0C01 devices that cover the IOAPIC area, some have
E820 "reserved" areas, and some have both, and we currently ignore
PNP0C01 resources that conflict with E820 ones.

But I think we'd only see a problem if a machine had neither PNP0C01
nor E820 descriptions of that space (in that case, "noapic" is already
broken), if we had an E820 description that only covered *part* of an
IOAPIC (seems unlikely to me), or if we had no E820 but had a PNP0C01
that covered part of it.

Bjorn

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
  2011-08-26  0:17   ` Bjorn Helgaas
@ 2011-08-26  1:41     ` H. Peter Anvin
  2011-08-26  6:18       ` Yinghai Lu
  2011-08-26  6:48       ` Cyrill Gorcunov
  2011-08-26  6:22     ` Cyrill Gorcunov
  1 sibling, 2 replies; 18+ messages in thread
From: H. Peter Anvin @ 2011-08-26  1:41 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Suresh Siddha, Thomas Gleixner, Ingo Molnar, Ralf Jung,
	Cyrill Gorcunov, Yinghai Lu, linux-kernel

On 08/25/2011 05:17 PM, Bjorn Helgaas wrote:
> 
> If the OS allocated the next 128 bytes to another device, it sounds
> like it would cause a conflict on Intel boxes.  This must be an area
> that differs between vendors.  I haven't seen a spec that mentions 256
> bytes as the required minimum MMIO size for IOAPICs, and apparently
> the AMD IOAPIC decodes 240 bytes or fewer.
> 

For what it's worth, it's probably a bad idea on x86 for the OS to
allocate addresses in the 0xFExxxxxx range...

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
  2011-08-26  1:41     ` H. Peter Anvin
@ 2011-08-26  6:18       ` Yinghai Lu
  2011-08-26  6:48       ` Cyrill Gorcunov
  1 sibling, 0 replies; 18+ messages in thread
From: Yinghai Lu @ 2011-08-26  6:18 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Bjorn Helgaas, Suresh Siddha, Thomas Gleixner, Ingo Molnar,
	Ralf Jung, Cyrill Gorcunov, linux-kernel

On 08/25/2011 06:41 PM, H. Peter Anvin wrote:
> On 08/25/2011 05:17 PM, Bjorn Helgaas wrote:
>>
>> If the OS allocated the next 128 bytes to another device, it sounds
>> like it would cause a conflict on Intel boxes.  This must be an area
>> that differs between vendors.  I haven't seen a spec that mentions 256
>> bytes as the required minimum MMIO size for IOAPICs, and apparently
>> the AMD IOAPIC decodes 240 bytes or fewer.
>>
> 
> For what it's worth, it's probably a bad idea on x86 for the OS to
> allocate addresses in the 0xFExxxxxx range...
> 
for system with more than one ioapic controllers, could have ioapic address rather low.
and those address are allocated to some pci devices BAR by BIOS.

Yinghai

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
  2011-08-26  0:17   ` Bjorn Helgaas
  2011-08-26  1:41     ` H. Peter Anvin
@ 2011-08-26  6:22     ` Cyrill Gorcunov
  2011-08-26 16:21       ` Bjorn Helgaas
  1 sibling, 1 reply; 18+ messages in thread
From: Cyrill Gorcunov @ 2011-08-26  6:22 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Suresh Siddha, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	Ralf Jung, Yinghai Lu, linux-kernel

On Thu, Aug 25, 2011 at 06:17:06PM -0600, Bjorn Helgaas wrote:
> On Thu, Aug 25, 2011 at 5:33 PM, Suresh Siddha
> <suresh.b.siddha@intel.com> wrote:
> > On Thu, 2011-08-25 at 16:05 -0700, Bjorn Helgaas wrote:
> >> Previously we reserved 1024 bytes, but that's more space than the IOAPIC
> >> consumes, and it can cause conflicts with nearby devices.  The known
> >> requirement is 68 bytes (sizeof(struct io_apic)), and rounding up to a
> >> power-of-2 gives us 128.
> >>
> >
> > Bjorn, Given the info from Intel that most of its io-apic
> > implementations has registers up to 0xff offset (reserved), does
> > reserving just the 128 bytes for the io-apic cause any address conflicts
> > if the next 128 bytes are allocated (by the OS) for any other device.
> 
> If the OS allocated the next 128 bytes to another device, it sounds
> like it would cause a conflict on Intel boxes.  This must be an area
> that differs between vendors.  I haven't seen a spec that mentions 256
> bytes as the required minimum MMIO size for IOAPICs, and apparently
> the AMD IOAPIC decodes 240 bytes or fewer.
> 

Hi Bjorn,

the former idea (as far as I remember) of all this IO_APIC_SLOT_SIZE
was to be sure the io-apics are allocated with 1K step (which
is requirements for io-apics), but definitely it doesn't consume
that much space neither it decode the whole range.

Which means, I would prefer if we have (since we change IO_APIC_SLOT_SIZE
anyway) some additional check and WARN_ON in this code. Something like

 if (io-apic-base-address & 0x3ff)
   WARN_ON();

Hm? (also we have bad_ioapic() check, probably should put such test
there instead).

	Cyrill

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
  2011-08-26  1:41     ` H. Peter Anvin
  2011-08-26  6:18       ` Yinghai Lu
@ 2011-08-26  6:48       ` Cyrill Gorcunov
  2011-08-26  9:22         ` Ralf Jung
  1 sibling, 1 reply; 18+ messages in thread
From: Cyrill Gorcunov @ 2011-08-26  6:48 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Bjorn Helgaas, Suresh Siddha, Thomas Gleixner, Ingo Molnar,
	Ralf Jung, Yinghai Lu, linux-kernel

On Thu, Aug 25, 2011 at 06:41:57PM -0700, H. Peter Anvin wrote:
> On 08/25/2011 05:17 PM, Bjorn Helgaas wrote:
> > 
> > If the OS allocated the next 128 bytes to another device, it sounds
> > like it would cause a conflict on Intel boxes.  This must be an area
> > that differs between vendors.  I haven't seen a spec that mentions 256
> > bytes as the required minimum MMIO size for IOAPICs, and apparently
> > the AMD IOAPIC decodes 240 bytes or fewer.
> > 
> 
> For what it's worth, it's probably a bad idea on x86 for the OS to
> allocate addresses in the 0xFExxxxxx range...
> 
> 	-hpa
> 

I rather wonder if there a way to assign a different address for the
watchdog Bjorn referring to. I don't have such machine to test but
seems its base address is configurable via pmio. Just curious.

	Cyrill

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
  2011-08-26  6:48       ` Cyrill Gorcunov
@ 2011-08-26  9:22         ` Ralf Jung
  2011-08-26  9:39           ` Cyrill Gorcunov
  0 siblings, 1 reply; 18+ messages in thread
From: Ralf Jung @ 2011-08-26  9:22 UTC (permalink / raw)
  To: Cyrill Gorcunov
  Cc: H. Peter Anvin, Bjorn Helgaas, Suresh Siddha, Thomas Gleixner,
	Ingo Molnar, Yinghai Lu, linux-kernel

Hi,

> I rather wonder if there a way to assign a different address for the
> watchdog Bjorn referring to. I don't have such machine to test but
> seems its base address is configurable via pmio. Just curious.
That would be the watchdog in my laptop then, I guess (I'm the reporter of the 
bug mentioned by Bjorn)? I don't know much about all the inner workings, but 
if you send me patches I can tell you what they do.

Kind regards,
Ralf

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
  2011-08-26  9:22         ` Ralf Jung
@ 2011-08-26  9:39           ` Cyrill Gorcunov
  2011-08-26  9:53             ` Ralf Jung
  0 siblings, 1 reply; 18+ messages in thread
From: Cyrill Gorcunov @ 2011-08-26  9:39 UTC (permalink / raw)
  To: Ralf Jung, Bjorn Helgaas
  Cc: H. Peter Anvin, Suresh Siddha, Thomas Gleixner, Ingo Molnar,
	Yinghai Lu, linux-kernel

On Fri, Aug 26, 2011 at 11:22:32AM +0200, Ralf Jung wrote:
> Hi,
> 
> > I rather wonder if there a way to assign a different address for the
> > watchdog Bjorn referring to. I don't have such machine to test but
> > seems its base address is configurable via pmio. Just curious.
>
> That would be the watchdog in my laptop then, I guess (I'm the reporter of the 
> bug mentioned by Bjorn)? I don't know much about all the inner workings, but 
> if you send me patches I can tell you what they do.
> 
> Kind regards,
> Ralf

Hi Ralf, I can't put that risk on you. Hell knows how it will react
on such reprogramming (which is seems rather a bios duty than kernel).

Bjorn, from where we pick this address at moment, it's obtained via
pci config?

	Cyrill

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
  2011-08-26  9:39           ` Cyrill Gorcunov
@ 2011-08-26  9:53             ` Ralf Jung
  2011-08-26  9:56               ` Cyrill Gorcunov
  0 siblings, 1 reply; 18+ messages in thread
From: Ralf Jung @ 2011-08-26  9:53 UTC (permalink / raw)
  To: Cyrill Gorcunov
  Cc: Bjorn Helgaas, H. Peter Anvin, Suresh Siddha, Thomas Gleixner,
	Ingo Molnar, Yinghai Lu, linux-kernel

Hi,


> Hi Ralf, I can't put that risk on you. Hell knows how it will react
> on such reprogramming (which is seems rather a bios duty than kernel).
Oh, we are talking about that kind of problems? It's my main (and only) work 
laptop, so... thanks for the warning ;-)

Kind regards,
Ralf

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
  2011-08-26  9:53             ` Ralf Jung
@ 2011-08-26  9:56               ` Cyrill Gorcunov
  0 siblings, 0 replies; 18+ messages in thread
From: Cyrill Gorcunov @ 2011-08-26  9:56 UTC (permalink / raw)
  To: Ralf Jung
  Cc: Bjorn Helgaas, H. Peter Anvin, Suresh Siddha, Thomas Gleixner,
	Ingo Molnar, Yinghai Lu, linux-kernel

On Fri, Aug 26, 2011 at 11:53:20AM +0200, Ralf Jung wrote:
> Hi, 
> 
> > Hi Ralf, I can't put that risk on you. Hell knows how it will react
> > on such reprogramming (which is seems rather a bios duty than kernel).
>
> Oh, we are talking about that kind of problems? It's my main (and only) work 
> laptop, so... thanks for the warning ;-)
> 

Yeah, i'm just curious ;) Anyway, Bjorn's patch fixes your problem
and the warn_on I proposed might be added later, on top of this
patch.

	Cyrill

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
  2011-08-26  6:22     ` Cyrill Gorcunov
@ 2011-08-26 16:21       ` Bjorn Helgaas
  2011-08-26 16:24         ` H. Peter Anvin
  2011-08-26 18:09         ` Cyrill Gorcunov
  0 siblings, 2 replies; 18+ messages in thread
From: Bjorn Helgaas @ 2011-08-26 16:21 UTC (permalink / raw)
  To: Cyrill Gorcunov
  Cc: Suresh Siddha, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	Ralf Jung, Yinghai Lu, linux-kernel

On Fri, Aug 26, 2011 at 12:22 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Thu, Aug 25, 2011 at 06:17:06PM -0600, Bjorn Helgaas wrote:
>> On Thu, Aug 25, 2011 at 5:33 PM, Suresh Siddha
>> <suresh.b.siddha@intel.com> wrote:
>> > On Thu, 2011-08-25 at 16:05 -0700, Bjorn Helgaas wrote:
>> >> Previously we reserved 1024 bytes, but that's more space than the IOAPIC
>> >> consumes, and it can cause conflicts with nearby devices.  The known
>> >> requirement is 68 bytes (sizeof(struct io_apic)), and rounding up to a
>> >> power-of-2 gives us 128.
>> >>
>> >
>> > Bjorn, Given the info from Intel that most of its io-apic
>> > implementations has registers up to 0xff offset (reserved), does
>> > reserving just the 128 bytes for the io-apic cause any address conflicts
>> > if the next 128 bytes are allocated (by the OS) for any other device.
>>
>> If the OS allocated the next 128 bytes to another device, it sounds
>> like it would cause a conflict on Intel boxes.  This must be an area
>> that differs between vendors.  I haven't seen a spec that mentions 256
>> bytes as the required minimum MMIO size for IOAPICs, and apparently
>> the AMD IOAPIC decodes 240 bytes or fewer.
>>
>
> Hi Bjorn,
>
> the former idea (as far as I remember) of all this IO_APIC_SLOT_SIZE
> was to be sure the io-apics are allocated with 1K step (which
> is requirements for io-apics), but definitely it doesn't consume
> that much space neither it decode the whole range.
>
> Which means, I would prefer if we have (since we change IO_APIC_SLOT_SIZE
> anyway) some additional check and WARN_ON in this code. Something like
>
>  if (io-apic-base-address & 0x3ff)
>   WARN_ON();
>
> Hm? (also we have bad_ioapic() check, probably should put such test
> there instead).

Is there some spec that requires all IOAPICs to be 1K aligned?  I
don't doubt that's the case; I'd just like to see something more
concrete than folklore.  I'm pretty sure there's some (possibly
secret) "IOAPIC architecture spec," and a section reference to it
would be nice.  Even before my patch, I don't think we actually
checked or enforced any *alignment* -- we only set the size.  I don't
know if it's worth it unless we have a problem it would fix, and it's
conceivable that we'd start warning about a perfectly functional
IOAPIC that's 128-byte aligned.

Bjorn

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
  2011-08-26 16:21       ` Bjorn Helgaas
@ 2011-08-26 16:24         ` H. Peter Anvin
  2011-08-26 18:09         ` Cyrill Gorcunov
  1 sibling, 0 replies; 18+ messages in thread
From: H. Peter Anvin @ 2011-08-26 16:24 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Cyrill Gorcunov, Suresh Siddha, Thomas Gleixner, Ingo Molnar,
	Ralf Jung, Yinghai Lu, linux-kernel

On 08/26/2011 09:21 AM, Bjorn Helgaas wrote:
> 
> Is there some spec that requires all IOAPICs to be 1K aligned?  I
> don't doubt that's the case; I'd just like to see something more
> concrete than folklore.  I'm pretty sure there's some (possibly
> secret) "IOAPIC architecture spec," and a section reference to it
> would be nice.  Even before my patch, I don't think we actually
> checked or enforced any *alignment* -- we only set the size.  I don't
> know if it's worth it unless we have a problem it would fix, and it's
> conceivable that we'd start warning about a perfectly functional
> IOAPIC that's 128-byte aligned.
> 

The PIIX3 IOAPIC decoder could only decode addresses on a 1K
granularity, but I don't think that was architectural.  I will try to ask.

	-hpa
-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
  2011-08-26 16:21       ` Bjorn Helgaas
  2011-08-26 16:24         ` H. Peter Anvin
@ 2011-08-26 18:09         ` Cyrill Gorcunov
  2011-08-26 18:21           ` H. Peter Anvin
  1 sibling, 1 reply; 18+ messages in thread
From: Cyrill Gorcunov @ 2011-08-26 18:09 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Suresh Siddha, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	Ralf Jung, Yinghai Lu, linux-kernel

On Fri, Aug 26, 2011 at 10:21:02AM -0600, Bjorn Helgaas wrote:
...
> >
> > Which means, I would prefer if we have (since we change IO_APIC_SLOT_SIZE
> > anyway) some additional check and WARN_ON in this code. Something like
> >
> >  if (io-apic-base-address & 0x3ff)
> >   WARN_ON();
> >
> > Hm? (also we have bad_ioapic() check, probably should put such test
> > there instead).
> 
> Is there some spec that requires all IOAPICs to be 1K aligned?  I
> don't doubt that's the case; I'd just like to see something more
> concrete than folklore.  I'm pretty sure there's some (possibly
> secret) "IOAPIC architecture spec," and a section reference to it
> would be nice.  Even before my patch, I don't think we actually
> checked or enforced any *alignment* -- we only set the size.  I don't
> know if it's worth it unless we have a problem it would fix, and it's
> conceivable that we'd start warning about a perfectly functional
> IOAPIC that's 128-byte aligned.
> 
> Bjorn

Yes, one of the spec is Intel's MP specification (as far as I remember).
Letme re-check...

 | 3.6.5 APIC Memory Mapping
 |
 | "Unlike the local APICs, the I/O APICs are mapped to give shared access from all
 | processors, providing full symmetric I/O access. The default base address for the
 | first I/O APIC is 0FEC0_0000h. Subsequent I/O APIC addresses are assigned in
 | 4K increments. For example, the second I/O APIC is at 0FEC0_1000h. 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."

Ie -- 4K increment with 1K base address. If I find other sources I have in mind
I'll ping you.

	Cyrill

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
  2011-08-26 18:09         ` Cyrill Gorcunov
@ 2011-08-26 18:21           ` H. Peter Anvin
  2011-08-26 19:15             ` Cyrill Gorcunov
  0 siblings, 1 reply; 18+ messages in thread
From: H. Peter Anvin @ 2011-08-26 18:21 UTC (permalink / raw)
  To: Cyrill Gorcunov
  Cc: Bjorn Helgaas, Suresh Siddha, Thomas Gleixner, Ingo Molnar,
	Ralf Jung, Yinghai Lu, linux-kernel

On 08/26/2011 11:09 AM, Cyrill Gorcunov wrote:
> 
> Yes, one of the spec is Intel's MP specification (as far as I remember).
> Letme re-check...
> 
>  | 3.6.5 APIC Memory Mapping
>  |
>  | "Unlike the local APICs, the I/O APICs are mapped to give shared access from all
>  | processors, providing full symmetric I/O access. The default base address for the
>  | first I/O APIC is 0FEC0_0000h. Subsequent I/O APIC addresses are assigned in
>  | 4K increments. For example, the second I/O APIC is at 0FEC0_1000h. 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."
> 
> Ie -- 4K increment with 1K base address. If I find other sources I have in mind
> I'll ping you.
> 

OK, so that explicitly specifies a 1K alignment.  The 4K bit seems to be
a default policy and not a requirement.

	-hpa


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
  2011-08-26 18:21           ` H. Peter Anvin
@ 2011-08-26 19:15             ` Cyrill Gorcunov
  0 siblings, 0 replies; 18+ messages in thread
From: Cyrill Gorcunov @ 2011-08-26 19:15 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Bjorn Helgaas, Suresh Siddha, Thomas Gleixner, Ingo Molnar,
	Ralf Jung, Yinghai Lu, linux-kernel

On Fri, Aug 26, 2011 at 11:21:53AM -0700, H. Peter Anvin wrote:
...
> > 
> > Ie -- 4K increment with 1K base address. If I find other sources I have in mind
> > I'll ping you.
> > 
> 
> OK, so that explicitly specifies a 1K alignment.  The 4K bit seems to be
> a default policy and not a requirement.
> 
> 	-hpa
> 

Yeah, and I seem to overdid with warning proposal ;)
So patch is fune, thanks a lot Bjorn!

	Cyrill

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: Fwd: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
       [not found] ` <CAErSpo5kEw=VTVv-=_D3hQg5oRNL9yEyJUnpP0biH=t3WRXMZw@mail.gmail.com>
@ 2011-08-26 21:09   ` Ralf Jung
  0 siblings, 0 replies; 18+ messages in thread
From: Ralf Jung @ 2011-08-26 21:09 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Cyrill Gorcunov,
	Yinghai Lu, Suresh Siddha, linux-kernel

Tested-by: Ralf Jung <ralfjung-e@gmx.de>

Thanks a lot!
Ralf

On Friday 26 August 2011 01:08:19 Bjorn Helgaas wrote:
> FYI.  I botched your email addr in the commit log.
> 
> 
> ---------- Forwarded message ----------
> From: Bjorn Helgaas <bhelgaas@google.com>
> Date: Thu, Aug 25, 2011 at 5:05 PM
> Subject: [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs
> To: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar
> <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>
> Cc: RalfJungralfjung-e@gmx.de, Cyrill Gorcunov <gorcunov@openvz.org>,
> Yinghai Lu <yinghai@kernel.org>, Suresh Siddha
> <suresh.b.siddha@intel.com>, linux-kernel@vger.kernel.org
> 
> 
> Previously we reserved 1024 bytes, but that's more space than the IOAPIC
> consumes, and it can cause conflicts with nearby devices.  The known
> requirement is 68 bytes (sizeof(struct io_apic)), and rounding up to a
> power-of-2 gives us 128.
> 
> The bug reported below is caused by the following assignments (the IOAPIC
> power-on default and the watchdog address recommended in the AMD SP5100
> BIOS Developer's Guide):
> 
>  IOAPIC[0]        at [mem 0xfec00000-0xfec003ff]
>  SP5100 TCO timer at [mem 0xfec000f0-0xfec000f7]
> 
> Reported-by: Ralf Jung ralfjung-e@gmx.de
> Reference: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=638863
> Cc: Cyrill Gorcunov <gorcunov@openvz.org>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  arch/x86/include/asm/apicdef.h |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/include/asm/apicdef.h
> b/arch/x86/include/asm/apicdef.h index 34595d5..855a18a 100644
> --- a/arch/x86/include/asm/apicdef.h
> +++ b/arch/x86/include/asm/apicdef.h
> @@ -12,10 +12,11 @@
>  #define        APIC_DEFAULT_PHYS_BASE          0xfee00000
> 
>  /*
> - * This is the IO-APIC register space as specified
> - * by Intel docs:
> + * I/O APICs are accessed indirectly via an index/data pair and an EOI
> + * register.  For example, see sec 13.5.1, "APIC Register Map," in the
> + * Intel ICH10 datasheet and the struct io_apic definition.
>  */
> -#define IO_APIC_SLOT_SIZE              1024
> +#define IO_APIC_SLOT_SIZE              128
> 
>  #define        APIC_ID         0x20

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2011-08-26 21:09 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-25 23:05 [PATCH 1/2] x86, ioapic: Reserve only 128 bytes for IOAPICs Bjorn Helgaas
2011-08-25 23:05 ` [PATCH 2/2] x86, ioapic: Announce resources reserved " Bjorn Helgaas
2011-08-25 23:33 ` [PATCH 1/2] x86, ioapic: Reserve only 128 bytes " Suresh Siddha
2011-08-26  0:17   ` Bjorn Helgaas
2011-08-26  1:41     ` H. Peter Anvin
2011-08-26  6:18       ` Yinghai Lu
2011-08-26  6:48       ` Cyrill Gorcunov
2011-08-26  9:22         ` Ralf Jung
2011-08-26  9:39           ` Cyrill Gorcunov
2011-08-26  9:53             ` Ralf Jung
2011-08-26  9:56               ` Cyrill Gorcunov
2011-08-26  6:22     ` Cyrill Gorcunov
2011-08-26 16:21       ` Bjorn Helgaas
2011-08-26 16:24         ` H. Peter Anvin
2011-08-26 18:09         ` Cyrill Gorcunov
2011-08-26 18:21           ` H. Peter Anvin
2011-08-26 19:15             ` Cyrill Gorcunov
     [not found] ` <CAErSpo5kEw=VTVv-=_D3hQg5oRNL9yEyJUnpP0biH=t3WRXMZw@mail.gmail.com>
2011-08-26 21:09   ` Fwd: " Ralf Jung

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.