lkml.org 
[lkml]   [2001]   [Oct]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: Patch to read/parse the MPC oem tables
Hi Martin-

Overall this looks like a mostly-clean patch.

Questions and comments below.

~Randy


"Martin J. Bligh" wrote:
>
> This patch will parse the OEM extensions to the mps tables
> (if present). This gives me a mapping to tell which device
> lies in which NUMA node (the current code just guesses).

So these extensions are OEM-specific, not part of the MP spec,
right?


> Patch is against 2.4.13 - if it looks OK, please could you add it?

> diff -urN virgin-2.4.13/arch/i386/kernel/mpparse.c linux-2.4.13/arch/i386/kernel/mpparse.c
> --- virgin-2.4.13/arch/i386/kernel/mpparse.c Thu Oct 4 18:42:54 2001
> +++ linux-2.4.13/arch/i386/kernel/mpparse.c Thu Oct 25 10:13:18 2001
> @@ -118,18 +120,37 @@
> +static int mpc_record;
> +static struct mpc_config_translation *translation_table[MAX_MPC_ENTRY];

Could this array be __initdata or reduced in size some,
for people who don't need it? (more about this below)
E.g., I bet most people don't need this static 4 KB array.

Also, could the array of structs <mp_irqs and mp_ioapics> (in
mpparse.c) be made __initdata, so that they could be discarded
after init?
I tested this idea (without this patch, just by changing
mp_irqs[] and mp_ioapics[] to __initdata, and it booted OK,
and they are put into the .data.init section according to
objdump. Are there some other/different problems doing this, anyone?
OTOH, with a 16 GB system, you won't worry much about saving a few KB,
eh?


> @@ -286,6 +313,62 @@
>
> +static void __init MP_translation_info (struct mpc_config_translation *m)
> +{
> + printk("Translation: record %d, type %d, quad %d, global %d, local %d\n", mpc_record, m->trans_type,
> + m->trans_quad, m->trans_global, m->trans_local);
> +
> + if (mpc_record >= MAX_MPC_ENTRY)
> + printk("MAX_MPC_ENTRY exceeded!\n");

Add "else" here to keep from stepping out of the array bounds.

> + translation_table[mpc_record] = m; /* stash this for later */
> +}
> +
> +/*
> + * Read/parse the MPC oem tables
> + */
> +
> +static void __init smp_read_mpc_oem(struct mp_config_oemtable *oemtable, \
> + unsigned short oemsize)
> +{
> + int count = sizeof (*oemtable); /* the header size */
> + unsigned char *oemptr = ((unsigned char *)oemtable)+count;
> +
> + printk("Found an OEM MPC table at %08lx - parsing it ... \n", (u_long) oemtable);

BTW, "%p" prints pointers also, without casting them.

> + while (count < oemtable->oem_length) {
> + switch (*oemptr) {
> + case MP_TRANSLATION:
> + {
> + struct mpc_config_translation *m=
> + (struct mpc_config_translation *)oemptr;
> + MP_translation_info(m);
> + oemptr += sizeof(*m);
> + count += sizeof(*m);
> + ++mpc_record;
> + break;
> + }

> /*
> * Read/parse the MPC
> */
> @@ -330,6 +413,13 @@
> /* save the local APIC address, it might be non-default */
> mp_lapic_addr = mpc->mpc_lapic;
>
> + if (clustered_apic_mode && mpc->mpc_oemptr) {
> + /* We need to process the oem mpc tables to tell us which quad things are in ... */
> + mpc_record = 0;
> + smp_read_mpc_oem((struct mp_config_oemtable *) mpc->mpc_oemptr, mpc->mpc_oemsize);
> + mpc_record = 0;

What's this =0 for?

> @@ -381,7 +471,13 @@
> count+=sizeof(*m);
> break;
> }
> + default:
> + {
> + count = mpc->mpc_length;
> + break;
> + }
> }
> + ++mpc_record;

And what's this increment for?

> diff -urN virgin-2.4.13/include/asm-i386/mpspec.h linux-2.4.13/include/asm-i386/mpspec.h
> --- virgin-2.4.13/include/asm-i386/mpspec.h Thu Oct 4 18:42:54 2001
> +++ linux-2.4.13/include/asm-i386/mpspec.h Thu Oct 25 14:31:12 2001
> @@ -16,7 +16,13 @@
> /*
> * a maximum of 16 APICs with the current APIC ID architecture.
> */
> +#ifdef CONFIG_MULTIQUAD
> +#define MAX_APICS 256
> +#else /* !CONFIG_MULTIQUAD */
> #define MAX_APICS 16
> +#endif /* CONFIG_MULTIQUAD */
> +
> +#define MAX_MPC_ENTRY 1024

How about #defining MAX_MPC_ENTRY above here (depending on MULTIQUAD),
so that it can be smaller for non-MULTIQUAD targets?

> @@ -55,6 +61,7 @@
> #define MP_IOAPIC 2
> #define MP_INTSRC 3
> #define MP_LINTSRC 4
> +#define MP_TRANSLATION 192

Where does this value (192) come from, and the
mpc_config_oemtable and mpc_config_translation structs?
Not in the MP 1.4 spec, right? (yes, I searched)
So maybe some comment about it being used by IBM would
be good (or even qualified by CONFIG_MULTIQUAD somehow;
that would be easy in the .h file, but not so easy
in mpparse.c -- without being ugly).

Or is it some de facto standard?
Is it used by other large-systems manufacturers for the
same purpose?

> @@ -144,6 +151,27 @@
> +struct mp_config_oemtable
> +{
> + char oem_signature[4];
> +#define MPC_OEM_SIGNATURE "_OEM"
> + unsigned short oem_length; /* Size of table */
> + char oem_rev; /* 0x01 */
> + char oem_checksum;
> + char mpc_oem[8];
> +};
> +
> +struct mpc_config_translation
> +{
> + unsigned char mpc_type;
> + unsigned char trans_len;
> + unsigned char trans_type;
> + unsigned char trans_quad;
> + unsigned char trans_global;
> + unsigned char trans_local;
> + unsigned short trans_reserved;
> +};
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 13:11    [W:0.078 / U:2.572 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site