Re: [coreboot] What is the best way to treat warnings reported by checkpatch.pl

[Vadim Bendebury]

I have not read the entire thread, but just in case it was not
mentioned before:  even when the structure elements come together very
snugly on their own, without __packed(), it still is required if the
structure comes over a wire or from a file, etc. as __packed()
guarantees that there is no unaligned accesses.

Also, what seems to be missing in checkpatch.pl (or is ti there, I
just don't know about it?) is the ability to suppress a warning for
just a certain line of code, so that when the style deviations are
necessary, they are clearly marked/explained.

-v


On Thu, Jul 28, 2016 at 1:14 PM, Julius Werner  wrote:
>> The point is, that without ((packed)) there is no guarantee that gcc
>> won't choose a different alignment over what you and I think would make
>> sense.
>
> In practice it is very predictable and there should be no sane reason
> to do it otherwise. Like with many of the other "it's not guaranteed
> in the standard but GCC and Clang have been doing it this way since
> forever" issues where being standards-compliant would incur a serious
> performance or readability hit, I'd advocate being pragmatic. Everyone
> knows the standard is stupid, but we still need to generate the
> binaries we want somehow. (And, of course, __attribute__((packed)) is
> also a GCC extension anyway.)
>
>> What other changes to checkpatch are needed?
>
> I'd really suggest that we stick with the upstream checkpatch.pl and
> use command line flags to tune it to coreboot (submitting patches for
> new flags or cleaner detection upstream if necessary). The tool gets
> updated frequently and we'd quickly fall out of sync with that if we
> don't keep a clean copy that can just be uprevved regularly. Also,
> this is what the Chromium tree does so maintaining a different fork of
> checkpatch.pl for upstream coreboot would just make lives harder for
> everyone working on Chromium (or, more likely, would just make them
> commit with --no-verify all the time). Working together to improve a
> common upstream would make all our lives easier.
>
> --
> coreboot mailing list: coreboot@coreboot.org
> https://www.coreboot.org/mailman/listinfo/coreboot

-- 
coreboot mailing list: coreboot@coreboot.org
https://www.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] What is the best way to treat warnings reported by checkpatch.pl

[Julius Werner]

> The point is, that without ((packed)) there is no guarantee that gcc
> won't choose a different alignment over what you and I think would make
> sense.

In practice it is very predictable and there should be no sane reason
to do it otherwise. Like with many of the other "it's not guaranteed
in the standard but GCC and Clang have been doing it this way since
forever" issues where being standards-compliant would incur a serious
performance or readability hit, I'd advocate being pragmatic. Everyone
knows the standard is stupid, but we still need to generate the
binaries we want somehow. (And, of course, __attribute__((packed)) is
also a GCC extension anyway.)

> What other changes to checkpatch are needed?

I'd really suggest that we stick with the upstream checkpatch.pl and
use command line flags to tune it to coreboot (submitting patches for
new flags or cleaner detection upstream if necessary). The tool gets
updated frequently and we'd quickly fall out of sync with that if we
don't keep a clean copy that can just be uprevved regularly. Also,
this is what the Chromium tree does so maintaining a different fork of
checkpatch.pl for upstream coreboot would just make lives harder for
everyone working on Chromium (or, more likely, would just make them
commit with --no-verify all the time). Working together to improve a
common upstream would make all our lives easier.

-- 
coreboot mailing list: coreboot@coreboot.org
https://www.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] initrd in 4.4 versus head

[Stefan Reinauer]

* Trammell Hudson  [160727 13:58]:
> I see a difference in the way 4.4 handles initrd images for linux
> payloads versus the way it is done in head.  With 4.4 my Linux
> kernel can not find the external initrd, so it is necessary to
> build it as part of the kernel.  With head it works fine.
> 
> It looks like 4.4 is adding the initrd as a separate section
> named "(empty)" with type "null" and the kernel can't find it:

(empty) is indeed what it claims to be, empty space in the image.
There is no initrd in there.

> performing operation on 'COREBOOT' region...
> Name   Offset Type Size
> cbfs master header 0x0cbfs header  32
> cpu_microcode_blob.bin 0x80   microcode22528
> cmos.default   0x5900 cmos_default 256
> cmos_layout.bin0x5a40 cmos_layout  1948
> fallback/dsdt.aml  0x6240 raw  13847
> (empty)0x98c0 null 26264
> fallback/romstage  0xff80 stage74020
> (empty)0x22140null 56664
> mrc.cache  0x2fec0mrc_cache65536
> fallback/ramstage  0x3ff00stage84790
> fallback/payload   0x54a80payload  1618769
> (empty)0x1dfe40   null 2226328
> bootblock  0x3ff700   bootblock1952
> 
> While in head it is bundling them together into the payload
> region (3.9 MB == bzImage + initrd.img) -- the kernel can
> find the image and use it:
> 
> Performing operation on 'COREBOOT' region...
> Name   Offset Type Size
> cbfs master header 0x0cbfs header  32
> fallback/romstage  0x80   stage14620
> cpu_microcode_blob.bin 0x3a00 microcode22528
> fallback/ramstage  0x9280 stage43781
> cmos_layout.bin0x13dc0cmos_layout  1948
> fallback/dsdt.aml  0x145c0raw  4021
> fallback/payload   0x155c0payload  3906169
> (empty)0x3cf080   null 199256
> bootblock  0x3ffb00   bootblock960
> 
> I don't see any changes in the util/cbfstool/cbfs-payload-linux.c
> file between these two versions.  Is there something else
> that changed?

It seems these two images are significantly different from each other,
apart from the payload. Almost all the stages are about twice as big in
the first image.

Did you compare the .config files for both images? Did you use the same
compiler to produce them? Are these for the same mainboard? Which one?

Stefan




-- 
coreboot mailing list: coreboot@coreboot.org
https://www.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] What is the best way to treat warnings reported by checkpatch.pl

[Stefan Reinauer]

* Julius Werner  [160727 23:41]:
> > 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.

The point is, that without ((packed)) there is no guarantee that gcc
won't choose a different alignment over what you and I think would make
sense.

Stefan

-- 
coreboot mailing list: coreboot@coreboot.org
https://www.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] re-enabling boot for ChromeOS

[Matt DeVillier]

On Thu, Jul 28, 2016 at 7:36 AM, Matthias Apitz  wrote:

> El día Wednesday, July 20, 2016 a las 11:00:28PM -0500, Matt DeVillier
> escribió:
>
> My C720 normally boots via SeaBIOS a FreeBSD operating system and there
> are no such tools gbb_utility and flashrom. Is there any image which could
> be copied with dd(1) onto an USB stick to boot from and have such
> utilities?
>

you can either:
1) use my Firmware Utility Script as noted before (website w/full
instructions is now up), which will download flashrom, cbfstool,
gbb_utility as needed, or
2) boot ChromiumOS, one of Arnold the Bat's builds would work fine


>
> Another question related to this: One of my C720 (I have two) from one
> day to another does not do any beep with the pc-speaker, all other sound
> is working fine. I asked in the FreeBSD mailing lists and one of the
> hints was to check the BIOS configuration. I do not see any screens for
> this in
> SeaBIOS, only one to select the boot device number if there are two
> devices visible, the SSD and another USB device. Is there something I
> could check or modify in the SeaBIOS?
>

coreboot/SeaBIOS do not have any user-accessible settings like a
traditional PC BIOS/UEFI setup.  Furthermore, since you're running the same
(stock?) firmware, any settings related to the onboard sound would be
identical.  I suspect the difference is either hardware or OS related


>
> Thanks
>
>
> matthias
>
> --
> Matthias Apitz, ✉ g...@unixarea.de, ⌂ http://www.unixarea.de/  ☎
> +49-176-38902045
> "Wer übersieht, dass wir uns den anderen weggenommen haben und sie uns
> wiederhaben wollen,
> kann von den Kämpfen der letzten Tage keinen verstehen. Und kann natürlich
> auch keinen
> dieser Kämpfe bestehen." Hermann Kant in jW 1.10.1989
>
-- 
coreboot mailing list: coreboot@coreboot.org
https://www.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] re-enabling boot for ChromeOS

[Matthias Apitz]

El día Wednesday, July 20, 2016 a las 11:00:28PM -0500, Matt DeVillier escribió:

> Julius,
> 
> wouldn't he also need gbb_utility to set the flags, in addition to 
> (CrOS) flashrom to
> be able to read/write just the GBB region?  That's why I suggested he use my
> ChromeOS Firmware Utility Script, since it would automatically download the
> required binaries as well as ensure the flags were set to a sane value.
> 
> But as you, no need to change the flags as set to boot ChromeOS

Thanks for the helping hints.

My C720 normally boots via SeaBIOS a FreeBSD operating system and there
are no such tools gbb_utility and flashrom. Is there any image which could
be copied with dd(1) onto an USB stick to boot from and have such
utilities?

Another question related to this: One of my C720 (I have two) from one
day to another does not do any beep with the pc-speaker, all other sound
is working fine. I asked in the FreeBSD mailing lists and one of the
hints was to check the BIOS configuration. I do not see any screens for this in
SeaBIOS, only one to select the boot device number if there are two
devices visible, the SSD and another USB device. Is there something I
could check or modify in the SeaBIOS?

Thanks


matthias

-- 
Matthias Apitz, ✉ g...@unixarea.de, ⌂ http://www.unixarea.de/  ☎ 
+49-176-38902045
"Wer übersieht, dass wir uns den anderen weggenommen haben und sie uns 
wiederhaben wollen,
kann von den Kämpfen der letzten Tage keinen verstehen. Und kann natürlich auch 
keinen
dieser Kämpfe bestehen." Hermann Kant in jW 1.10.1989

-- 
coreboot mailing list: coreboot@coreboot.org
https://www.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] Microcode problem with Braswell CPU

[Alexander Böcken]

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.stojsavlje...@gmail.com]
Gesendet: Donnerstag, 28. Juli 2016 06:47
An: Alexander Böcken
Cc: cheng yichen; coreboot; york.y...@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, 0xffc0, 4*1024*1024, MTRR_TYPE_WRPROT);
enable_cache();

/* Enable Variable MTRRs */
msr.hi = 0x;
msr.lo = 0x0800;
wrmsr(MTRR_DEF_TYPE_MSR, msr);
}

I went to the Intel® 64 and IA-32 Architectures Software Developer 
Manuals
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 = 0x0800;) 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
-- 
coreboot mailing list: coreboot@coreboot.org
https://www.coreboot.org/mailman/listinfo/coreboot