Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31549 )
Change subject: cpu/x86/pae/pgtbl: Add memset with PAE
......................................................................
Patch Set 7:
(13 comments)
Implementation looks good, not fond of the "documentation" ;)
https://review.coreboot.org/#/c/31549/7/Documentation/arch/x86/pae.md
File Documentation/arch/x86/pae.md:
PS7:
This is much too thin to be useful. It raises more
questions than it answers. I instantly felt the urge
to read the code instead.
Also, I would expect and prefer explanations of func-
tion signatures in code comments. Linux manages to
mix that with their Documentation/, maybe worth to
have a look at, how.
If this is the style of documentation moving forward,
I'd prefer to reconsider our decision to document
more. Such documentation shims create maintenance
burden on one side, whilst not providing much value
on the other (beside the links to the source, but I
know `git grep`).
https://review.coreboot.org/#/c/31549/7/Documentation/arch/x86/pae.md@3
PS7, Line 3: Due to missing x86_64 support it's required to use PAE enabled x86_32 code.
If you read this out of context of the patch train (i.e. not
with the memory clearing in mind), it makes no sense.
https://review.coreboot.org/#/c/31549/7/Documentation/arch/x86/pae.md@14
PS7, Line 14: struct pg_table *scratch, void *scratch2);
Without any reasonable description of the parameters (pat,
scratch, scratch2 most of all), this whole section merely
is a "look for this in the source if you want to use it".
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c
File src/cpu/x86/pae/pgtbl.c:
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@114
PS7, Line 114: vmem_addr
documentation shim is already out of sync
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@115
PS7, Line 115: aligned 2 MiB
either `2MiB aligned` or `aligned to 2MiB`, afaik
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@122
PS7, Line 122: * @param pat The pattern to write to the pyhsical memory
AIUI, this is used as a byte pattern? Please mention that in
case, or even make it obvious as `uint8_t pat`.
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@125
PS7, Line 125: size_t
What if `length >= 4GiB`?
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@126
PS7, Line 126: struct pg_table
This `struct pg_table *` makes it look like the caller would
have to care, but actually it shouldn't. Maybe make that clear
with a `void *`?
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@140
PS7, Line 140: ((uintptr_t)dest < (uintptr_t)vmem_addr));
I don't understand this one. They are in different spaces, aren't they?
e.g. if you wanted to erase 1MiB..3GiB (physical), you could still use
0x00200000 (2MiB) as virtual pointer?
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@141
PS7, Line 141: dest <
`dest + length <=` ?
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@142
PS7, Line 142: >
`>=` ?
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@163
PS7, Line 163: PDE_A | PDE_D |
As far as I understood, the processor would not interpret these,
only set them? If we don't interpret them, why set them?
https://review.coreboot.org/#/c/31549/7/src/cpu/x86/pae/pgtbl.c@177
PS7, Line 177: ~((1UL << PDE_IDX_SHIFT) - 1)
PDE_ADDR_MASK?
--
To view, visit https://review.coreboot.org/c/coreboot/+/31549
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I00f7ecf87b5c9227a9d58a0b61eecc38007e1a57
Gerrit-Change-Number: 31549
Gerrit-PatchSet: 7
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Jens Drenhaus <jens.drenhaus(a)9elements.com>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Reviewer: Roy Wen <rgzwen(a)arista.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Wed, 06 Mar 2019 17:02:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Hello Werner Zeh, Aaron Durbin, Julius Werner, Patrick Rudolph, Paul Menzel, David Hendricks, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/29563
to look at the new patch set (#66).
Change subject: security/tpm: Fix TCPA log feature
......................................................................
security/tpm: Fix TCPA log feature
Until now the TCPA log wasn't working correctly.
* Refactor TCPA log code.
* Add TCPA log dump fucntion.
* Make TCPA log available in bootblock.
* Fix TCPA log formatting.
* Add x86 and Cavium memory for early log.
Change-Id: Ic93133531b84318f48940d34bded48cbae739c44
Signed-off-by: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
---
A Documentation/security/index.md
M Documentation/security/vboot/measured_boot.md
M src/arch/x86/car.ld
M src/commonlib/include/commonlib/tcpa_log_serialized.h
M src/include/memlayout.h
M src/security/tpm/Makefile.inc
M src/security/tpm/tspi.h
M src/security/tpm/tspi/log.c
M src/security/tpm/tspi/tspi.c
M src/security/vboot/secdata_tpm.c
M src/security/vboot/symbols.h
M src/security/vboot/vboot_crtm.c
M src/soc/cavium/cn81xx/include/soc/memlayout.ld
M src/soc/imgtec/pistachio/include/soc/memlayout.ld
M src/soc/mediatek/mt8173/include/soc/memlayout.ld
M src/soc/mediatek/mt8183/include/soc/memlayout.ld
M src/soc/nvidia/tegra124/include/soc/memlayout.ld
M src/soc/nvidia/tegra210/include/soc/memlayout.ld
M src/soc/samsung/exynos5250/include/soc/memlayout.ld
M util/cbmem/cbmem.c
20 files changed, 376 insertions(+), 92 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/29563/66
--
To view, visit https://review.coreboot.org/c/coreboot/+/29563
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic93133531b84318f48940d34bded48cbae739c44
Gerrit-Change-Number: 29563
Gerrit-PatchSet: 66
Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki(a)gmail.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Patrick Rudolph
Gerrit-CC: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-MessageType: newpatchset
Hello Patrick Rudolph, build bot (Jenkins), Hannah Williams, Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/29423
to look at the new patch set (#10).
Change subject: soc/intel/braswell: Reserve IOAPIC and ROM resources
......................................................................
soc/intel/braswell: Reserve IOAPIC and ROM resources
The mmio resouces IOAPIC and ROM area not reserved.
Reserve IOAPIC and ROM resources.
BUG=N/A
TEST=Intel CherryHill CRB booting Embedded Linux
Change-Id: I917c30892b46ac1d964e7bab339082d17a1e706d
Signed-off-by: Frans Hendriks <fhendriks(a)eltan.com>
---
M src/soc/intel/braswell/southcluster.c
1 file changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/29423/10
--
To view, visit https://review.coreboot.org/c/coreboot/+/29423
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I917c30892b46ac1d964e7bab339082d17a1e706d
Gerrit-Change-Number: 29423
Gerrit-PatchSet: 10
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: newpatchset
Hello Patrick Rudolph, build bot (Jenkins), Hannah Williams, Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/29423
to look at the new patch set (#9).
Change subject: soc/intel/braswell: Reserve IOAPIC and ROM resources
......................................................................
soc/intel/braswell: Reserve IOAPIC and ROM resources
The mmio resouces IOAPIC and ROM area not reserved.
Reserve ioAPIC and ROM resources. .
BUG=N/A
TEST=Intel CherryHill CRB booting Embedded Linux
Change-Id: I917c30892b46ac1d964e7bab339082d17a1e706d
Signed-off-by: Frans Hendriks <fhendriks(a)eltan.com>
---
M src/soc/intel/braswell/southcluster.c
1 file changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/29423/9
--
To view, visit https://review.coreboot.org/c/coreboot/+/29423
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I917c30892b46ac1d964e7bab339082d17a1e706d
Gerrit-Change-Number: 29423
Gerrit-PatchSet: 9
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: newpatchset
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/28464 )
Change subject: src/drivers/intel/fsp1_1: Configure UART after memory init
......................................................................
Patch Set 2:
Opened bug report at https://github.com/IntelFsp/FSP/issues/15
--
To view, visit https://review.coreboot.org/c/coreboot/+/28464
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ibb6c9e4153b3de58791b211c7f4241be3bceae9d
Gerrit-Change-Number: 28464
Gerrit-PatchSet: 2
Gerrit-Owner: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-CC: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Wed, 06 Mar 2019 12:01:20 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment