Patchwork Improve i82830 MBI SMI Handler

login
register
about
Submitter Joseph Smith
Date 2010-05-21 05:25:03
Message ID <4BF6192F.2080508@settoplinux.org>
Download mbox | patch
Permalink /patch/1367/
State Superseded, archived
Headers show

Comments

Joseph Smith - 2010-05-21 05:25:03
This patch improves the i82830 MBI SMI Handler. It is now able to load 
Intel vbios VBT and Flexaim modules. Build and boot tested.

Signed-off-by: Joseph Smith <joe@settoplinux.org>
Peter Stuge - 2010-05-21 16:27:55
Joseph Smith wrote:
> This patch improves the i82830 MBI SMI Handler. It is now able to load 
> Intel vbios VBT and Flexaim modules. Build and boot tested.
>
> Signed-off-by: Joseph Smith <joe@settoplinux.org>

One issue, but other than that it is

Acked-by: Peter Stuge <peter@stuge.se>


> +++ src/northbridge/intel/i82830/i82830_smihandler.c	(working copy)
> @@ -32,7 +32,7 @@
>  extern unsigned char *mbi;
>  extern u32 mbi_len;
>  
> -// #define DEBUG_SMI_I82830
> +#define DEBUG_SMI_I82830

Really enable debugging here by default? Should this maybe come from
Kconfig somehow. It could e.g. depend on some existing value in
Kconfig.


//Peter
Stefan Reinauer - 2010-05-21 19:17:51
On 5/21/10 7:25 AM, Joseph Smith wrote:
> This patch improves the i82830 MBI SMI Handler. It is now able to load
> Intel vbios VBT and Flexaim modules. Build and boot tested.
>
> Signed-off-by: Joseph Smith <joe@settoplinux.org>

God catch...
However, please don't commit this... it breaks the implementation of
MBI, the code should stay completely module agnostic. There should be a
solution that works without adding an exception for every module supported
(plus, I think the rules are based on the length of the name, so any
module 203 and 204 with different names will break your assumptions.

Stefan
Joseph Smith - 2010-05-21 20:20:36
On 05/21/2010 03:17 PM, Stefan Reinauer wrote:
> On 5/21/10 7:25 AM, Joseph Smith wrote:
>> This patch improves the i82830 MBI SMI Handler. It is now able to load
>> Intel vbios VBT and Flexaim modules. Build and boot tested.
>>
>> Signed-off-by: Joseph Smith <joe@settoplinux.org>
>
> God catch...
> However, please don't commit this... it breaks the implementation of
> MBI, the code should stay completely module agnostic. There should be a
> solution that works without adding an exception for every module supported
> (plus, I think the rules are based on the length of the name, so any
> module 203 and 204 with different names will break your assumptions.
>
Hmm. I have gone through a bunch of VBT's (type 203) and dozens of 
Flexaims (type 204) (every version available). All the VBT's (type 203) 
have the same data structure of a certain size and all the Flexaims 
(type 204) have the same data structures of a certain size. The sizes of 
the data structures are different. With the existing code you can get 
one or the other working but not both. Hence the "type" switch to 
distinguish between the two data structure sizes.

As far as I have seen for the i830 these are the only two types of MBI 
modules, VBT's (type 203) and Flexaims (type 204). Unfortunately i don't 
know what the implementation of MBI is supposed to look like, I am just 
going by what works (thinking outside the box).

If you have a completely module agnostic way to determine the different 
data structure sizes, I am all ears.

 > I think the rules are based on the length of the name, so any
 > module 203 and 204 with different names will break your assumptions.

No, it shouldn't. All VBT's (type 203) only have "VBT" as the name and 
The way I setup the name for Flexaims (type 204) is based on the 
name_len so it is actually module agnostic.
Joseph Smith - 2010-05-21 20:51:52
On 05/21/2010 03:17 PM, Stefan Reinauer wrote:
> On 5/21/10 7:25 AM, Joseph Smith wrote:
>> This patch improves the i82830 MBI SMI Handler. It is now able to load
>> Intel vbios VBT and Flexaim modules. Build and boot tested.
>>
>> Signed-off-by: Joseph Smith <joe@settoplinux.org>
>
> God catch...
> However, please don't commit this... it breaks the implementation of
> MBI, the code should stay completely module agnostic. There should be a
> solution that works without adding an exception for every module supported
> (plus, I think the rules are based on the length of the name, so any
> module 203 and 204 with different names will break your assumptions.
>
Even the existing code is not "completely module agnostic". There are 
little + _some_number_ through out the code to adjust the data structure 
sizes.......
Joseph Smith - 2010-05-21 21:01:35
On 05/21/2010 12:27 PM, Peter Stuge wrote:
> Joseph Smith wrote:
>> This patch improves the i82830 MBI SMI Handler. It is now able to load
>> Intel vbios VBT and Flexaim modules. Build and boot tested.
>>
>> Signed-off-by: Joseph Smith<joe@settoplinux.org>
>
> One issue, but other than that it is
>
> Acked-by: Peter Stuge<peter@stuge.se>
>
>
>> +++ src/northbridge/intel/i82830/i82830_smihandler.c	(working copy)
>> @@ -32,7 +32,7 @@
>>   extern unsigned char *mbi;
>>   extern u32 mbi_len;
>>
>> -// #define DEBUG_SMI_I82830
>> +#define DEBUG_SMI_I82830
>
> Really enable debugging here by default? Should this maybe come from
> Kconfig somehow. It could e.g. depend on some existing value in
> Kconfig.
>
 >
Oops, thanks for the catch. I was actually going to use Kconfig to 
enable it in the next patch.
Joseph Smith - 2010-05-22 15:40:56
On 05/21/2010 04:20 PM, Joseph Smith wrote:
> On 05/21/2010 03:17 PM, Stefan Reinauer wrote:
>> On 5/21/10 7:25 AM, Joseph Smith wrote:
>>> This patch improves the i82830 MBI SMI Handler. It is now able to load
>>> Intel vbios VBT and Flexaim modules. Build and boot tested.
>>>
>>> Signed-off-by: Joseph Smith <joe@settoplinux.org>
>>
>> God catch...
>> However, please don't commit this... it breaks the implementation of
>> MBI, the code should stay completely module agnostic. There should be a
>> solution that works without adding an exception for every module
>> supported
>> (plus, I think the rules are based on the length of the name, so any
>> module 203 and 204 with different names will break your assumptions.
>>
> Hmm. I have gone through a bunch of VBT's (type 203) and dozens of
> Flexaims (type 204) (every version available). All the VBT's (type 203)
> have the same data structure of a certain size and all the Flexaims
> (type 204) have the same data structures of a certain size. The sizes of
> the data structures are different. With the existing code you can get
> one or the other working but not both. Hence the "type" switch to
> distinguish between the two data structure sizes.
>
> As far as I have seen for the i830 these are the only two types of MBI
> modules, VBT's (type 203) and Flexaims (type 204). Unfortunately i don't
> know what the implementation of MBI is supposed to look like, I am just
> going by what works (thinking outside the box).
>
> If you have a completely module agnostic way to determine the different
> data structure sizes, I am all ears.
>
>  > I think the rules are based on the length of the name, so any
>  > module 203 and 204 with different names will break your assumptions.
>
> No, it shouldn't. All VBT's (type 203) only have "VBT" as the name and
> The way I setup the name for Flexaims (type 204) is based on the
> name_len so it is actually module agnostic.
>
>
OK, let's get technical on why the switch(mbi_header->type) _HAS_ to 
happen. First lets take a look at the VBT (type 203). The first data 
structure is the mbi_header:

|- MBI_GetObjectHeader
|  |- objnum = 0
|  |- header_id = f6f0
|  |- attributes = 9750
|  |- size = 7c0
|  |- name_len = 3
|  |- type = 203
|  |- header_ext = 0
|  |- name[0] = 56


F0 F6 50 97 7C 00 03 FF 03 02 00 00 00 00 00 00

This data structure will always be 16 bytes len. Pretty basic.

Ok, now lets look at the second data structure mbi_header->name_len. The 
mbi_header reports that this is 3 bytes long but the space allocated 
will actually always be 16 bytes long:

|  |- MBI module 'VBT' found.

56 42 54 00 00 00 00 00 00 00 00 00 00 00 00 00

The first three bytes signify the mbi name "VBT", and the rest of the 
bytes are just padding, but again the space allocated for 
mbi_header->name_len will always be 16 bytes long. In this case there 
needs to be a slight hack to signify the 16 bytes. This situation only 
happens in VBT (type 203) thus the need for the switch.

Next the:

|  |- headerlen = 32

Reports the total byte size of the mbi header. This signify's how many 
bytes get copied to the bios area.

And last the mbi object area. This is the area of the mbi module minus 
the headerlen where the real data/code begins. This is reported from the 
mbi header size:

|  |- objectlen = 7c0 or  |  |- size = 7c0

This signify's how many bytes get copied to the bios area.

-----------------------

Ok now let's take a look at the Flexaims (type 204) and compare the 
differences.

|- MBI_GetObjectHeader
|  |- objnum = 1
|  |- header_id = f6f0
|  |- attributes = 9750
|  |- size = 810
|  |- name_len = 40
|  |- type = 204
|  |- header_ext = 0
|  |- name[0] = 24
|  |- MBI module '$AIM@' found.

F0 F6 50 97 81 00 40 FF 04 02 00 00 00 00 00 00

This data structure will always be 16 bytes len. Pretty basic.

Ok, now lets look at the second data structure mbi_header->name_len. The 
mbi_header reports that this is 64(0x40) bytes long, which is correct:

24 41 49 4D 40 00 00 00 09 08 00 00 00 03 00 00
01 00 00 00 00 00 05 00 06 00 43 58 2D 32 35 38
00 00 00 00 00 00 00 00 00 00 00 00 02 00 02 00
9C EE 00 00 00 00 00 00 00 00 00 00 00 00 00 00


Next the:

|  |- headerlen = 80

Reports the total byte size of the mbi header (16 + 64). This signify's 
how many bytes get copied to the bios area.

And last the mbi object area. This is the area of the mbi module minus 
the headerlen where the real data/code begins. This is reported from the 
mbi header size:

|  |- objectlen = 810 or  |  |- size = 810

This signify's how many bytes get copied to the bios area.

----------------

So hopefully now you can clearly see the main point of the 
switch(mbi_header->type) is to determine if the module is a VBT (type 
203) there needs to be a slight hack to the second data structure 
mbi_header->name_len to make sure it allocates the full 16 byte area. If 
not it throws everything else off.

And we can't just say always use 16 bytes for this area for everything 
because that will throw the Flexaims (type 204) off because the 
mbi_header->name_len could be different sizes.

Again, I have confirmed this with many different actual raw data of the 
i830 mbi modules. The only other solution I can come up with is a simple 
if statement:

if (mbi_header->type == 0x0203) {
	mbi_header->name_len = 16;
}

If that works for you please let me know and I will submit a new patch.

Patch

Index: src/northbridge/intel/i82830/i82830_smihandler.c
===================================================================
--- src/northbridge/intel/i82830/i82830_smihandler.c	(revision 5575)
+++ src/northbridge/intel/i82830/i82830_smihandler.c	(working copy)
@@ -32,7 +32,7 @@ 
 extern unsigned char *mbi;
 extern u32 mbi_len;
 
-// #define DEBUG_SMI_I82830
+#define DEBUG_SMI_I82830
 
 /* If YABEL is enabled and it's not running at 0x00000000, we have to add some
  * offset to all our mbi object memory accesses
@@ -196,6 +196,15 @@ 
 			}
 
 			mbi_header = (mbi_header_t *)&mbi[i];
+#ifdef DEBUG_SMI_I82830
+			printk(BIOS_DEBUG, "|  |- header_id = %x\n", mbi_header->header_id);
+			printk(BIOS_DEBUG, "|  |- attributes = %x\n", mbi_header->attributes);
+			printk(BIOS_DEBUG, "|  |- size = %x\n", (mbi_header->size * 16));
+			printk(BIOS_DEBUG, "|  |- name_len = %x\n", mbi_header->name_len);
+			printk(BIOS_DEBUG, "|  |- type = %x\n", mbi_header->type);
+			printk(BIOS_DEBUG, "|  |- header_ext = %x\n", mbi_header->header_ext);
+			printk(BIOS_DEBUG, "|  |- name[0] = %x\n", mbi_header->name[0]);
+#endif
 			len = ALIGN((mbi_header->size * 16) + sizeof(mbi_header) + mbi_header->name_len, 16);
 
 			if (obj_header->objnum == count) {
@@ -205,19 +214,33 @@ 
 					break;
 				}
 #endif
-				int headerlen = ALIGN(sizeof(mbi_header) + mbi_header->name_len + 15, 16);
+				int headerlen;
+
+				switch(mbi_header->type) {
+				case 0x0203: {
+					headerlen = ALIGN(sizeof(mbi_header) + mbi_header->name_len + 15, 16);
+					break;
+				}
+				case 0x0204: {
+					headerlen = ALIGN(sizeof(mbi_header) + mbi_header->name_len, 16);
+					break;
+				}
+				default:
+					printk(BIOS_DEBUG, "|  |-- Unknown MBI Header!\n");
+					break;
+				}
 #ifdef DEBUG_SMI_I82830
 				printk(BIOS_DEBUG, "|  |- headerlen = %d\n", headerlen);
 #endif
 				memcpy(&obj_header->header, mbi_header, headerlen);
 				obj_header->banner.retsts = MSH_OK;
-				printk(BIOS_DEBUG, "|     |- MBI module '");
+				printk(BIOS_DEBUG, "|  |- MBI module '");
 				int j;
 				for (j=0; j < mbi_header->name_len && mbi_header->name[j]; j++)
 					printk(BIOS_DEBUG, "%c",  mbi_header->name[j]);
 				printk(BIOS_DEBUG, "' found.\n");
 #ifdef DEBUG_SMI_I82830
-				dump(banner_id, sizeof(obj_header_t) + 16);
+				dump(banner_id, sizeof(obj_header_t) + mbi_header->name_len);
 #endif
 				break;
 			}
@@ -225,7 +248,7 @@ 
 			count++;
 		}
 		if (obj_header->banner.retsts == MSH_IF_NOT_FOUND)
-			printk(BIOS_DEBUG, "|     |- MBI object #%d not found.\n", obj_header->objnum);
+			printk(BIOS_DEBUG, "|  |- MBI object #%d not found.\n", obj_header->objnum);
 		break;
 	}
 	case 0x0203: {
@@ -233,7 +256,7 @@ 
 		mbi_header_t *mbi_header = NULL;
 		printk(BIOS_DEBUG, "|- MBI_GetObject\n");
 #ifdef DEBUG_SMI_I82830
-		printk(BIOS_DEBUG, "|  |- handle = %016lx\n", getobj->handle);
+		printk(BIOS_DEBUG, "|  |- handle = %Lx\n", getobj->handle);
 #endif
 		printk(BIOS_DEBUG, "|  |- objnum = %d\n", getobj->objnum);
 		printk(BIOS_DEBUG, "|  |- start = %x\n", getobj->start);
@@ -256,14 +279,30 @@ 
 			len = ALIGN((mbi_header->size * 16) + sizeof(mbi_header) + mbi_header->name_len, 16);
 
 			if (getobj->objnum == count) {
-				printk(BIOS_DEBUG, "|  |- len = %x\n", len);
-				memcpy((void *)(getobj->buffer + OBJ_OFFSET),
-						((char *)mbi_header) + 0x20 , (len > getobj->buflen) ? getobj->buflen : len);
 
+				int objectlen = ALIGN((mbi_header->size * 16), 16);
+				printk(BIOS_DEBUG, "|  |- objectlen = %x\n", objectlen);
+
+				switch(mbi_header->type) {
+				case 0x0203: {
+					memcpy((void *)(getobj->buffer + OBJ_OFFSET),
+							((char *)mbi_header) + 0x20 , (objectlen > getobj->buflen) ? getobj->buflen : objectlen);
+					break;
+				}
+				case 0x0204: {
+					memcpy((void *)(getobj->buffer + OBJ_OFFSET),
+							((char *)mbi_header) + (mbi_header->name_len + 0x10), (objectlen > getobj->buflen) ? getobj->buflen : objectlen);
+					break;
+				}
+				default:
+					printk(BIOS_DEBUG, "|  |-- Unknown MBI Object!\n");
+					break;
+				}
+
 				getobj->banner.retsts = MSH_OK;
 #ifdef DEBUG_SMI_I82830
 				dump(banner_id, sizeof(*getobj));
-				dump(getobj->buffer + OBJ_OFFSET, len);
+				dump(getobj->buffer + OBJ_OFFSET, objectlen);
 #endif
 				break;
 			}