[coreboot] [PATCH] Improve i82830 MBI SMI Handler

[Joseph Smith]

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 at 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.


-- 
Thanks,
Joseph Smith
Set-Top-Linux
www.settoplinux.org