[flashrom] [PATCH 1/8] ichspi: add ICH/PCH flash descriptor decoding via FDOC/FDOD

[Carl-Daniel Hailfinger]

Am 03.09.2011 23:43 schrieb Stefan Tauner:
> On Sat, 03 Sep 2011 14:46:00 +0200
> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
>
>   
>> Am 03.09.2011 12:33 schrieb Stefan Tauner:
>>     
>>> On Fri, 02 Sep 2011 21:33:33 +0200
>>> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote
>>>       
>>>> Am 20.08.2011 12:39 schrieb Stefan Tauner:
>>>>     
>>>>         
>>>>> because this patch relies on (compiler) implementation-specific
>>>>> layouting of bit-fields, it checks for basic compiler support in the
>>>>> makefile and for correct layout before taking any action on runtime.  
>>>>>       
>>>>>           
>>>> Checking for presence of bitfield support in the compiler is the way to
>>>> madness. Next we have to test for C99 capability and other fun stuff.
>>>> Just assume that bitfields are supported, otherwise compilation will
>>>> blow up or the runtime checks will catch a miscompilation. That
>>>> compile-time check (including the #if BITFIELDS) is nacked.
>>>>     
>>>>         
>>> well, for now bit-fields are only needed in one place that is seldom
>>> needed. why break compilation unnecessarily for all other cases too?
>>>   
>>>       
>> Would it really break compilation? AFAICS not.
>>     
> it could due to:
> "A bit-field shall have a type that is a qualified or unqualified
> version of _Bool, signed int, unsigned int, or some other
> implementation-defined type."
>
> maybe i don't understand the "qualified" part of this definition
> (i can not remember what this means in the specs right now), but i
> would think that uint32_t is not guaranteed to work as a bit-field type?
>   

I thought that refers to the bitfield members, not the whole bitfield.
We have to ask a C99 expert about that.


>> Bitfield support is available at least in all recent gcc and clang, and
>> probably other compilers as well. 
>>     
> under the premise that all used compilers can deal with bit-fields the
> way we want, even the run time test is unnecessary ;)
>   

You have to differentiate between the functionality mandated by C99 (I
assume that as a given) and the functionality labeled as implementation
defined by C99 (needs to be tested at runtime).


>> No need to test for that at compile time.
>>     
> well it could be tested when host=target arch, but sure this is not the
> way to go (and i did not go there).
>   

Indeed, this would screw up cross compilation.


>>> basically i agree, but since everyone thinks bit-fields break all the
>>> time i thought it is a good idea to integrate it in the least
>>> interfering way possible... :)  
>>>       
>> Bitfields have various implementation-defined aspects which may explode
>> in your face unless you test for them at runtime, but it is technically
>> impossible to test those at compile time.
>>     
> if it would not possibly break compilation, i would totally agree with
> you. i am not insisting on keeping the test + ifdefs, but you have been
> warned now ;)
> please confirm what i should do.
>   

If bitfields ever break compilation, we can handle the failure with some
compiler dependent #ifdefs. Until then I'd rather avoid conditional code.
Could you comment on my "bitfield checker" mail? I know that endianness
should not be complained about and rather be automatically fixed, but at
least the code in there hopefully detects all possible bitfield layout
variants allowed by C99.

Semi-related question: Can we compile flashrom with gcc 2.95 (may break
due to anonymous unions)? What about gcc 3.0/3.3?


>>>> […]
>>>>     
>>>>         
>>>>> +
>>>>> +#define RET_OK		0
>>>>> +#define RET_ERR		-1
>>>>> +#define RET_WARN	-2
>>>>> +#define RET_PARAM	-3
>>>>> +#define RET_OOB		-4
>>>>>   
>>>>>       
>>>>>           
>>>> Too generic names.
>>>>     
>>>>         
>>> not at all, but they should be moved to wherever we want the generic
>>> return macros to be eventually ;)
>>> can we do that and/or introduce the universal values before/with merge
>>> of this patch?
>>> some additional FL_ prefix or so would be ok with me.
>>> i could of course rename them now, and then again, but we should add
>>> the universal return values soon-ish anyway.
>>>   
>>>       
>> You mean like the various (and partially unmerged) patches which added
>> definitions like ERROR_PTR and TIMEOUT_ERROR? Much more important than
>> the name is deciding on which errors we want to handle as separate
>> return codes and whether a warning should indeed be a return code
>> different from OK.
>>     
> error_ptr was a macro to indicate errors when NULL is a valid response
> iirc... but timeout_error and the fatal thingy tadas/uwe introduced
> today are such things yes.
> of course the naming is not the most important point to
> discuss, but also discussing the issues you named needs to be started
> at some point... :)
>
> if not you will continue to see randomly named error macro definitions
> in patches with funny values... forever!
>   

Can you start a separate discussion thread about error numbers? I see
that Uwe has started assigning unique return codes to various ft2232_spi
errors, and I was not really happy with those.


>>>> How does FDOVR (or whatever the correct acronym for Flash Descriptor
>>>> Override is) relate to this?
>>>>     
>>>>         
>>> Flash Descriptor Override Pin-Strap Status (FDOPSS).
>>> it is later called flash descriptor *security* override pin, and this
>>> hints at its true purpose: overriding the region access rights declared
>>> in the descriptor.
>>>
>>> quote ich8 datasheet description of the respective pin (GPIO33):
>>> "If sampled low, the Flash Descriptor Security will be overridden. If
>>> high, the security measures defined in the Flash Descriptor will be in effect."
>>>   
>>>       
>> Excellent, this clears up one FIXME in the ICH code.
>>     
> mhm nomnomnom, i like FIXMEs!</coreboot-status>
>
>   
>>> even then the whole descriptor is of interest imo... and this will most
>>> likely never occur in practice anyway.
>>>   
>>>       
>> A possible use case is allowing reflashing of a dev board with an
>> additional jumper.
>>     
> sure, but even then (or maybe then even more) the user would like to
> know the contents of the descriptor. the flag does indicate that the
> descriptor is valid only, nothing else. i don't see a problem.
>   

Agreed. My comment was about the "likely never occur" aspect of your
statement. We might want to handle FDOPSS by disregarding region
protection registers, but I don't know if the documentation matches the
hardware.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/