[coreboot] Multiboot

[Mathias Krause]

Hi again,

Robert Millan wrote:
> On Tue, Sep 23, 2008 at 10:10:31AM +0200, Mathias Krause wrote:
>> Robert Millan wrote:
>>>   - Add "edx" to clobber list.  Together with eax and ecx which are already
>>>     in input list, this makes the call able to return for any payload that
>>>     complies with "cdecl" calling convention.
>> ...
>>
>>> +
>>> +static int run_address_multiboot(void *f)
>>> +{
>>> +	int ret;
>>> +	__asm__ __volatile__ ("call *%3" : "=a" (ret) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f) : "edx");
>>> +	return ret;
>>> +}
>>> +
>> this assembler inline is not quite correct to be able to let the call
>> return because the compiler assumes that after this assembler statement
>> %ecx still holds the value f which hasn't to be the case. It assumes the
>> same for %ebx but this should be no problem since the calling function
>> must preserve it's value if it follows the C calling convention. But
>> %ecx isn't one of the registers that needs to be preserved, so the
>> assumption the compiler makes here is wrong.
>>
>> How about this one:
>>
>> | static int run_address_multiboot(void *f)
>> | {
>> |     int ret, dummy;
>> |     __asm__ __volatile__ ("call *%4" : "=a" (ret), "=c" (dummy) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f) : "edx", "memory");
>> |     return ret;
>> | }
> 
> I see.  But why not just add "ecx" to clobber list instead?  Then a dummy
> variable isn't needed.

When doing so the compiler is unable to find another register called
%ecx to assign the value of f as input value. He cannot use %ecx because
by mentioning it in the clobber list you already told him that register
may change it's value at any time within the asm statement. That for the
"kudge" with the dummy output variable is needed but it should get
optimized away since it's value is never used. It's only usage is to
tell the compiler that the value of %ecx has changed.


>> The memory clobber is needed since you cannot know what the called
>> function will actually do with the memory and to ensure all cached
>> values are actually written back to memory before calling f().
> 
> Is this really a problem?  If the payload expects to return, it isn't
> supposed to modify coreboot's memory at all.  If it does, I'd say it's
> normal that things break.

Not the return of f() is the problem but missing memory writes _before_
calling it. If coreboot does some memory modifications that are relevant
for f() they should be taken out of registers and written back to memory
before calling f(). Otherwise f() may break.