coreboot July 2016

coreboot@coreboot.org

47 participants
135 discussions

Re: [coreboot] Turns out I never knew what intuitive meant.
by Zoran Stojsavljevic 28 Jul '16

28 Jul '16

For heavy headache, the best medicine in The World (at least, helps me... .. . Especially when I am mortally/dead drunk): [image: Inline image 1] ;-) Zoran On Thu, Jul 28, 2016 at 8:30 PM, ron minnich <rminnich(a)gmail.com> wrote: > Oh, thanks for that, it cured the problem that I did not have a headache > yet today, but now I do. > > :-) > > ron > > On Wed, Jul 27, 2016 at 9:14 PM Zoran Stojsavljevic < > zoran.stojsavljevic(a)gmail.com> wrote: > >> Hello Ron, >> >> Here it is according to ASUS: >> https://www.asus.com/Motherboards/AM1IA/ >> >> New UEFI BIOS – friendlier and more intuitiveExclusive EZ Mode adds >> useful info to become even easier >> >> Seems, it points to very user friendly (EZ mode) GUI = intuitive GUI. >> >> [image: Inline image 1] >> >> Zoran >> >> On Thu, Jul 28, 2016 at 5:21 AM, ron minnich <rminnich(a)gmail.com> wrote: >> >>> "ASUS AM1I-A is an AM1 Socket-based Mini-ITX motherboard. It has an >>> intuitive UEFI BIOS " >>> >>> er, what? What's the word intuitive mean again? >>> >>> ron >>> >>> -- >>> coreboot mailing list: coreboot(a)coreboot.org >>> https://www.coreboot.org/mailman/listinfo/coreboot >>> >>

1 0

Re: [coreboot] Turns out I never knew what intuitive meant.
by ron minnich 28 Jul '16

28 Jul '16

Oh, thanks for that, it cured the problem that I did not have a headache yet today, but now I do. :-) ron On Wed, Jul 27, 2016 at 9:14 PM Zoran Stojsavljevic < zoran.stojsavljevic(a)gmail.com> wrote: > Hello Ron, > > Here it is according to ASUS: > https://www.asus.com/Motherboards/AM1IA/ > > New UEFI BIOS – friendlier and more intuitiveExclusive EZ Mode adds > useful info to become even easier > > Seems, it points to very user friendly (EZ mode) GUI = intuitive GUI. > > [image: Inline image 1] > > Zoran > > On Thu, Jul 28, 2016 at 5:21 AM, ron minnich <rminnich(a)gmail.com> wrote: > >> "ASUS AM1I-A is an AM1 Socket-based Mini-ITX motherboard. It has an >> intuitive UEFI BIOS " >> >> er, what? What's the word intuitive mean again? >> >> ron >> >> -- >> coreboot mailing list: coreboot(a)coreboot.org >> https://www.coreboot.org/mailman/listinfo/coreboot >> >

1 0

Re: [coreboot] Microcode problem with Braswell CPU
by Alexander Böcken 28 Jul '16

28 Jul '16

Hi Zoran, yes, that’s the function. I also found the same information in the Intel docs. The docs say, the processor starts up with all caching disabled. So, the function marks the top 4 MB as write-protected memory and thus enables caching for this region. The MTTR_DEF_TYPE MSR set the default memory configuration for all memory regions that were not explicitly configured by fixed/variable MTTRs. Intel recommends to use uncachable memory for all non-existing physical memory. As we haven’t initialized DDR memory at this point, this seems fine to me. However, TempRamInit also seems to fuss about caching, regarding the FSP specification. So the code seems to interfere somehow, at least on my CPU. I don’t know. Best regards, Alex Von: Zoran Stojsavljevic [mailto:zoran.stojsavljevic@gmail.com] Gesendet: Donnerstag, 28. Juli 2016 06:47 An: Alexander Böcken Cc: cheng yichen; coreboot; york.yang(a)intel.com Betreff: Re: [coreboot] Microcode problem with Braswell CPU > The function bootblock_cpu_init() (/src/soc/intel/braswell/bootblock/bootblock.c) contains a call to enable_rom_caching() This is the function, you are talking about: static void enable_rom_caching(void) { msr_t msr; disable_cache(); /* Why only top 4MiB ? */ set_var_mtrr(1, 0xffc00000, 4*1024*1024, MTRR_TYPE_WRPROT); enable_cache(); /* Enable Variable MTRRs */ msr.hi = 0x00000000; msr.lo = 0x00000800; wrmsr(MTRR_DEF_TYPE_MSR, msr); } I went to the Intel® 64 and IA-32 Architectures Software Developer Manuals<https://www.google.de/url?sa=t&rct=j&q=&esrc=s&source=web&cd=1&cad=rja&uact…> to try to understand what really MTRR_DEF_TYPE_MSR refister (MSR 0x2FF) really means. After reading the appropriate parts to actually analyze this register, I got confused. [Inline image 1] First, I do not understand (as comment says correctly: Why only top 4MiB?), why not all 8MiB? Or maybe 16MiB, actual size of board flash? Second. I see that the type of this memory (msr.lo = 0x00000800;) is E - MTRR enable, FE disabled, type uncacheable - 0x00 (instead MTRR_TYPE_WRPROT - 0x05). Seems that this function was blindly copy/pasted from some other place... While creating BSW Coreboot context. Best Regards, Zoran

1 0

Re: [coreboot] Microcode problem with Braswell CPU
by Alexander Böcken 28 Jul '16

28 Jul '16

Hi all, I made a discovery yesterday that somehow solves my initial problem: The function bootblock_cpu_init() (/src/soc/intel/braswell/bootblock/bootblock.c) contains a call to enable_rom_caching(). If I remove this call then TempRamInit returns successfully and coreboot is able to call cache_as_ram_main(). Hence, TempRamInit must have returned a valid (cache) memory range and I have a stack now. I don’t yet understand the implications of this “fix”, nor how it relates to TempRamInit. Maybe, someone from Intel can shed light on this. Meanwhile I’m learning about memory type range registers (MTRRs) because they are being accessed in enable_rom_caching(). Also, cheng, can you confirm that this works for you? Best regards, Alex

3 2

Re: [coreboot] Turns out I never knew what intuitive meant.
by Zoran Stojsavljevic 28 Jul '16

28 Jul '16

Hello Ron, Here it is according to ASUS: https://www.asus.com/Motherboards/AM1IA/ New UEFI BIOS – friendlier and more intuitiveExclusive EZ Mode adds useful info to become even easier Seems, it points to very user friendly (EZ mode) GUI = intuitive GUI. [image: Inline image 1] Zoran On Thu, Jul 28, 2016 at 5:21 AM, ron minnich <rminnich(a)gmail.com> wrote: > "ASUS AM1I-A is an AM1 Socket-based Mini-ITX motherboard. It has an > intuitive UEFI BIOS " > > er, what? What's the word intuitive mean again? > > ron > > -- > coreboot mailing list: coreboot(a)coreboot.org > https://www.coreboot.org/mailman/listinfo/coreboot >

1 0

Turns out I never knew what intuitive meant.
by ron minnich 28 Jul '16

28 Jul '16

"ASUS AM1I-A is an AM1 Socket-based Mini-ITX motherboard. It has an intuitive UEFI BIOS " er, what? What's the word intuitive mean again? ron

1 0

ASUS KFSN4-DRE (K8) Automated Test Failure [master]
by Raptor Engineering Automated Coreboot Test Stand 28 Jul '16

28 Jul '16

The ASUS KFSN4-DRE (K8) fails verification for branch master as of commit e3e2bb0a892bc185a52f210bcae15db268c1d034 The following tests failed: BOOT_FAILURE Commits since last successful test: e3e2bb0 util: Correct typo in MSR_EBC_SOFT_POWERON 7ab98fb util/msrtool: update register for Pentium4_later e5a5084 msrtool/README: Remove trailing spaces 3339861 soc/intel/skylake: Use init_vbnv_cmos from vboot vbnv 0faf401 soc/intel/broadwell: Use init_vbnv_cmos from vboot vbnv <9 commits skipped> af8ef2a drivers/intel/fsp2_0: Update MRC cache with dead version in recovery c319737 soc/intel/common: Store MRC data in next available slot in the cache bc24b85 mainboard/google/slippy: remove unobtainable mainboard 139314b mainboard/google/bolt: remove unobtainable mainboard 60cc75d soc/intel/apollolake: Disable monitor mwait See attached log for details This message was automatically generated from Raptor Engineering's ASUS KFSN4-DRE (K8) test stand Want to test on your own equipment? Check out https://www.raptorengineeringinc.com/content/REACTS/intro.html Raptor Engineering also offers coreboot consulting services! Please visit https://www.raptorengineeringinc.com for more information Please contact Timothy Pearson at Raptor Engineering <tpearson(a)raptorengineeringinc.com> regarding any issues stemming from this notification

1 0

Re: [coreboot] WG: What is the best way to treat warnings reported by checkpatch.pl
by Martin Roth 28 Jul '16

28 Jul '16

Hey Werner, Thanks for bringing up this topic. It's something we've discussed a bunch of times, but hadn't done anything about until now. Here's what I've done: I've pushed the .checkpatch.conf from the Chrome OS coreboot tree https://review.coreboot.org/#/c/15919/ I had initially pushed a patch to update the pre-commit git hook to say "If this failure is invalid, bypass it with 'git commit -n'.", but i decided I'd rather do that inside checkpatch.pl so everyone who has already run make gitconfig will get the fix, so I abandoned that change. It needed some additional fixes anyway. I'll push another commit shortly. I've also added a jenkins build to run checkpatch on every commit, but that doesn't give any feedback in the commit, so it doesn't seem super useful right now. You have to go into the console output for the jenkins build to see the results. Here's an example: https://qa.coreboot.org/job/coreboot-checkpatch/30/console What other changes to checkpatch are needed? Martin On Tue, Jul 26, 2016 at 10:34 PM, Zeh, Werner <werner.zeh(a)siemens.com> wrote: >>Does git commit --no-verify (or -n for short) allow you to commit? >> > > Yes, one can go this way and I did it already earlier. But I just wanted to point it out here. > We do not need a check script for every commit if we simply disable the check when it comes to issues. > I wanted to start this discussion to improve the situation with this script we currently have. > >>I think we should try to do a little of option 1 within reason, not by forking the script but rather by disabling more check steps that don't match the coreboot code style (by >having our wrapper pass --ignore XXX flags to checkpatch) and possibly upstreaming checkpatch.pl patches with new features we need to the Linux kernel (to make the >detection more accurate or add a more specific --ignore flag we can turn on). In the Chromium fork of coreboot we're already using a bunch of those flags that we should >probably use in upstream coreboot as well: >> >>--ignore C99_COMMENTS >>--ignore GLOBAL_INITIALISERS >>--ignore INITIALISED_STATIC >>--ignore LINE_SPACING >>--ignore NEW_TYPEDEFS >>--ignore PREFER_ALIGNED >>--ignore PREFER_PACKED >>--ignore PREFER_PRINTF >>--ignore SPLIT_STRING >> > > If we really did not touch the contents of the script, I totally agree with you. We can disable warnings that do not match our coding style while updating the script from its origin from time to time. If we have already started tailoring this script, than we should do it the right way and end this tailoring process to match our needs. I admit that the later one will end up in more work if one wants to synchronize this script with origin again one day. > > Werner > -- > coreboot mailing list: coreboot(a)coreboot.org > https://www.coreboot.org/mailman/listinfo/coreboot

1 0

Re: [coreboot] What is the best way to treat warnings reported by checkpatch.pl
by Julius Werner 27 Jul '16

27 Jul '16

> typedef struct dmar_atsr_entry { > u16 type; > u16 length; > u8 flags; > u8 reserved; > u16 segment; > } __attribute__ ((packed)) dmar_atsr_entry_t; Looking at the original example here, I would still recommend not to use the packed attribute. It forces the compiler to use accesses that would work on unaligned data... for x86 that doesn't matter (and granted, this sounds x86-specific, but it's worth applying the same principles to all code), but for other architectures it may generate very inefficient code (even on ARM where unaligned accesses are okay after you turn on caching, GCC keeps doing this for all packed structures and there's no way to convince it otherwise). In this case, the members are all correctly aligned so there's no need to insert padding, so you can (and therefore should) leave out the packed attribute.

1 0

Re: [coreboot] What is the best way to treat warnings reported by checkpatch.pl
by ron minnich 27 Jul '16

27 Jul '16

Ah well, Werner, I've had the error of my ways pointed out to me, turns out this packed stuff is a standard practice in coreboot now. I must have missed the memo. So, I'm not a fan but if that's how we do it, it's how we do it. thanks and apologies ron On Wed, Jul 27, 2016 at 10:07 AM ron minnich <rminnich(a)gmail.com> wrote: > On Tue, Jul 26, 2016 at 9:25 PM Zeh, Werner <werner.zeh(a)siemens.com> > wrote: > >> >> >> >> In the case of ACPI we need to provide a table which has constrains on >> the used data types and alignment >> as the contents of the table will be interpreted by the ACPI interpreter >> of the OS. >> So if we omit the usage of packed it may result in gaps in between the >> single members of the data structure >> which will in turn lead to errors while the OS interprets the contents. >> >> So in my point of view, we really need this structure to be packed in >> this case. >> >> >> > > no, please don't do this. We and others have made this mistake before and > it took a lot of work to unwind it where we did it. Using packed structs to > create aligned data in memory is a frequent source of bugs. > > If you're creating a desired data layout in memory, from a struct, using > packed will fail badly should we ever have a big endian machine, but it can > even have weird problems between gcc versions. When you pack data into > memory with a specified alignment, byte order, and padding, you are > converting it to an external data representation, i.e. XDR. You need to do > that explicitly, not by using packed structs. > > We've got code in the repo which does this. Please look at util/cbfstool/ > for example code, or see the mptable generation code. just git grep xdr to > see some usage. > > But using packed structs to try to force layout of data in memory is > almost always going to end badly. For a fun read, check this out > https://sourceware.org/bugzilla/show_bug.cgi?id=5070 > > This one section is applicable: > "Tom Hunter 2007-10-02 05:45:39 UTC > ARM is one of the architectures supported by glibc. You may not like it, > but it > is a fact. > > Independently of the architecture, the padding done is not valid. You > can't > (and shouldn't) make any assumption about the alignment and associated > padding > done by the compiler for any architecture. GCC is free to change the > alignment > rules in any future versions. It seems rather silly to have the assert() > which > is meant to verify at runtime that your invalid assumption holds true. > > I would also suggest that you don't use structures to format packets for > networking. Packets for networking should be treated as byte streams to > avoid > any alignment/padding/byte-order issues. A standard way of doing this is > something like this: > > unsigned char buf[MaxPacketSize]; > unsigned char *bp; > int skt; > size_t len; > > ... > bp = buf; > *bp++ = ...; > *bp++ = ...; > *bp++ = ...; > ... > len = send(skt, buf, bp - buf, 0); > " > > The key realization is that Tom is right even when you're packing to > memory. > > ron >

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

2004

2003

2002

coreboot July 2016