[flashrom] [PATCH 07/12] add ICH/PCH flash descriptor decoding
Stefan Tauner
stefan.tauner at student.tuwien.ac.at
Wed Jul 20 18:56:46 CEST 2011
On Thu, 14 Jul 2011 01:17:50 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
> Am 12.06.2011 09:41 schrieb Stefan Tauner:
> > On Sun, 12 Jun 2011 02:15:34 +0200
> > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
> >
> >
> >> Am 08.06.2011 04:55 schrieb Stefan Tauner:
> >>
> >>> +void pretty_print_ich_descriptor_component(void)
> >>> +{
> >>> + const char * str_freq[8] = {
> >>> + "20 MHz", /* 000 */
> >>> + "33 MHz", /* 001 */
> >>> + "reserved/illegal", /* 010 */
> >>> + "reserved/illegal", /* 011 */
> >>> + "50 MHz", /* 100 */
> >>> + "reserved/illegal", /* 101 */
> >>> + "reserved/illegal", /* 110 */
> >>> + "reserved/illegal" /* 111 */
> >>> + };
> >>> + const char * str_mem[8] = {
> >>> + "512kB",
> >>> + "1 MB",
> >>> + "2 MB",
> >>> + "4 MB",
> >>> + "8 MB",
> >>> + "16 MB",
> >>> + "undocumented/illegal",
> >>> + "reserved/illegal"
> >>> + };
> >>> +
> >>> + msg_pdbg("\n");
> >>> + msg_pdbg("=== FCBA ===\n");
> >>> + msg_pdbg("FLCOMP 0x%8.8x\n", fcba.FLCOMP);
> >>> + msg_pdbg("FLILL 0x%8.8x\n", fcba.FLILL );
> >>> + msg_pdbg("\n");
> >>> + msg_pdbg("--- FCBA details ---\n");
> >>> + msg_pdbg("0x%2.2x freq_read_id %s\n",
> >>> + fcba.freq_read_id , str_freq[fcba.freq_read_id ]);
> >>>
> >>>
> >> Out-of-bounds access if fcba.freq_read_id>=8
> >>
> > hm. well if that happens then the compiler is REALLY doing something
> > unexpected because .freq_read_id is defined to be 3 (unsigned) bits
> > wide.
> >
>
> Hm yes. A comment (/* freq_read_id has 3 bits */) or something to that
> effect would be nice and improve code auditability (no need to look up
> stuff elsewhere).
after the changes i have done i am no longer sure this is a good idea.
they dont change anything directly related, but commenting this seems
very redundant if you look at the patch as a whole. it would be
required in quite some places although the definition is only one click
away. this is quite easier to review than for example do_ich9_spi_frap,
which uses an argument to index into similar arrays and does no check
whatsoever.
if you insist though, i can add them anyway.
> Can we add some selftest during flashrom startup which checks for one or
> two special hand-made similar structs if it worked and complains about a
> compiler problem otherwise? That would allow using structs with all
> benefits and without the risk.
>
> To be honest, I had hoped some C expert would chime in and lecture us
> about how to do this correctly.
> Michael?
i could use the structs we introduce with this patch in the startup
checking code.
i would do two tests:
writing to big fields and verify that reading out the matching smaller
fields in the same range are correct, and vice-versa.
we could also add such a check to the makefile tests, which is probably
better because it is a compile-time/compiler check not a runtime check?
side note: doesnt that apply to other startup checks too?
>
> > in some cases we should certainly check the values and this is in the
> > case of redirection; i.e. when we look up an offset to another field in
> > some descriptor register which could potentially be garbage.
> >
>
> Indeed.
done. this was quite some work to get the checked boundaries correct
and tied. now i know why there are so many overflow errors out there...
not just because everyone is lazy. :)
> >>> + }
> >>> + /* straps */
> >>> + /* FIXME: detect chipset correctly */
> >>> + switch (CHIPSET_SERIES_5_IBEX_PEAK) {
> >>>
> >>>
> >> WTF?
> >>
> > yes. :)
> > there is afaik no way to distinguish between the chipsets from the
> > descriptor alone. this is not a big problem for flashrom. it can deduce
> > this from the pci ids of the system. but for the stand alone utility it
> > is a problem.
> > we could add pci id detection directly here but this would make it
> > necessary to either include flashrom's pci stuff (which - from a quick
> > look - is not as good modularized as i would like it be for this case,
> > i have not tried yet to use it though) or to force the user to supply
> > the chipset to use (maybe with a safe - if there is one - default).
> >
>
> Parsing a file should be hardware-independent. Heck, if someone wants to
> use a non-PCI ARM machine that should still work fine. Requiring a
> command line argument is absolutely fine. Use the enum below, and inside
> flashrom map PCI IDs to that enum, and in the separate tool map command
> line parameters to that enum.
inside flashrom we don't need to know (yet), so there is no problem...
i have added a parameter to the tool, that selects the chipset.
> > regarding packing:
> >
> >> If enough space remains, a bit-field that immediately follows another
> >> bit-field in a structure shall be packed into adjacent bits of the same
> >> unit.
> >> If insufficient space remains, whether a bit-field that does not fit is
> >> put into the next unit or overlaps adjacent units is
> >> implementation-defined.
> >>
> > so i think if i use uint32_t for all (overlapping) fields it should be
> > safe?
> >
>
> As long as a field does not span two different uint32_t, it should be
> fine, yes.
fixed.
> >> Why an empty struct?
> >>
> > not done yet. i wanted feedback first ;)
> > this information is not available publicly btw.
> > apart from what mazzoo has supplied, i do only have the data for ibex
> > peak/5 series.
> >
>
> Oh, that's unfortunate. I should ask my Intel contacts if there is
> anything they might be able to share legally. Can you ping me about that
> in a few days (I hope to find the right people by then)?
ping! :)
also what about the authors of the MEI kernel module? i have not
messaged them because i presume they won't be allowed to help us anyway
and did not want to bother them. but since i have written that
letter/blog thingy now i could just spam them anyway. i would like to
hear your opinion on this before i do so because you brought it up
lately yourself...
--
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
More information about the flashrom
mailing list