Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30116 )
Change subject: pci_drivers/cpu_drivers: Fix constructed arrays on x86_64
......................................................................
pci_drivers/cpu_drivers: Fix constructed arrays on x86_64
The __pci_driver and __cpu_driver uses variable length arrays which are
constructed by the linker at build-time.
The linker always place the structs at 16-byte boundary, as per
"System V ABI". That's not a problem on x86, as the struct is exactly
16 Bytes in size. On other platforms, like x86_64 it breaks, because the
default data alignment isn't SysV compatible.
Set -malign-data=abi to make x86_64 gcc use the SysV psABI.
Fixes broken __pci_driver and __cpu_driver on x86_64.
Change-Id: I2491d47ed03dcfd8db110dfb181b2c5281449591
Signed-off-by: Patrick Rudolph <siro(a)das-labor.org>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/30116
Reviewed-by: Aaron Durbin <adurbin(a)chromium.org>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M util/xcompile/xcompile
1 file changed, 1 insertion(+), 1 deletion(-)
Approvals:
build bot (Jenkins): Verified
Aaron Durbin: Looks good to me, approved
diff --git a/util/xcompile/xcompile b/util/xcompile/xcompile
index 0fefff2..4a29cdd 100755
--- a/util/xcompile/xcompile
+++ b/util/xcompile/xcompile
@@ -238,7 +238,7 @@
# to use i586 instead.
if [ "${TARCH}" = "x86_64" ]; then
cat <<EOF
- GCC_CFLAGS_${TARCH} += -march=nocona
+ GCC_CFLAGS_${TARCH} += -march=nocona -malign-data=abi
EOF
fi
--
To view, visit https://review.coreboot.org/c/coreboot/+/30116
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I2491d47ed03dcfd8db110dfb181b2c5281449591
Gerrit-Change-Number: 30116
Gerrit-PatchSet: 9
Gerrit-Owner: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Jonathan Neuschäfer <j.neuschaefer(a)gmx.net>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Philipp Hug <philipp(a)hug.cx>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: ron minnich <rminnich(a)gmail.com>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: merged
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29950 )
Change subject: soc/qualcomm/qcs405: Add MMU support
......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/#/c/29950/13/src/soc/qualcomm/qcs405/include/so…
File src/soc/qualcomm/qcs405/include/soc/symbols.h:
https://review.coreboot.org/#/c/29950/13/src/soc/qualcomm/qcs405/include/so…
PS13, Line 23: #define _ssram_size (_essram - _ssram)
This pattern has recently been replaced, please use DECLARE_REGION() (from <symbols.h>) now.
https://review.coreboot.org/#/c/29950/13/src/soc/qualcomm/qcs405/mmu.c
File src/soc/qualcomm/qcs405/mmu.c:
https://review.coreboot.org/#/c/29950/13/src/soc/qualcomm/qcs405/mmu.c@28
PS13, Line 28: _ssram_size
...and then this will become REGION_SIZE(ssram)
--
To view, visit https://review.coreboot.org/c/coreboot/+/29950
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8c6353be2c0379ec94f91223805762a2286de06d
Gerrit-Change-Number: 29950
Gerrit-PatchSet: 13
Gerrit-Owner: nsekar(a)codeaurora.org
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: nsekar(a)codeaurora.org
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Sricharan Ramabadhran <srichara(a)qualcomm.corp-partner.google.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 19:08:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29949 )
Change subject: mainboard/google/mistral: Add support for Mistral
......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/#/c/29949/12/src/mainboard/google/mistral/chrom…
File src/mainboard/google/mistral/chromeos.fmd:
https://review.coreboot.org/#/c/29949/12/src/mainboard/google/mistral/chrom…
PS12, Line 27: RO_PRESERVE {
You should get rid of this now and just directly have RO_DDR_TRAINING(PRESERVE) 8K here (see CB:31707 for details). I'll update Cheza like this as well when I get a chance.
https://review.coreboot.org/#/c/29949/12/src/mainboard/google/mistral/chrom…
PS12, Line 35: RW_ELOG 4K
These four should also have the (PRESERVE) flag now.
--
To view, visit https://review.coreboot.org/c/coreboot/+/29949
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I7ecfad68bb50f42acf36f51bc3433add56597c3d
Gerrit-Change-Number: 29949
Gerrit-PatchSet: 12
Gerrit-Owner: nsekar(a)codeaurora.org
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: nsekar(a)codeaurora.org
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Sricharan Ramabadhran <srichara(a)qualcomm.corp-partner.google.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 19:06:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31767 )
Change subject: soc/qualcomm/qcs405: Support for new SoC
......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31767/1/src/soc/qualcomm/qcs405/Kconfig
File src/soc/qualcomm/qcs405/Kconfig:
https://review.coreboot.org/#/c/31767/1/src/soc/qualcomm/qcs405/Kconfig@19
PS1, Line 19: VBOOT_OPROM_MATTERS
> That is normally used for devices with an "option rom" for graphics initialization. […]
The option is named like this for legacy reasons (and maybe we should rename it). What it really means is "will skip display init on normal mode boot". That is usually true for all normal Arm platforms -- however, since Mistral doesn't have a display, I assume you won't need it here.
--
To view, visit https://review.coreboot.org/c/coreboot/+/31767
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I0e1078b23921f02e505b374ad515ab92260a6dda
Gerrit-Change-Number: 31767
Gerrit-PatchSet: 1
Gerrit-Owner: nsekar(a)codeaurora.org
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Reviewer: nsekar(a)codeaurora.org
Gerrit-Comment-Date: Wed, 06 Mar 2019 18:59:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31550 )
Change subject: arch/x86: Introduce helper to clear memory using PAE
......................................................................
Patch Set 7:
(13 comments)
https://review.coreboot.org/#/c/31550/7/Documentation/arch/x86/pae.md
File Documentation/arch/x86/pae.md:
https://review.coreboot.org/#/c/31550/7/Documentation/arch/x86/pae.md@13
PS7, Line 13: arch
if it's arch-agnostic, it shouldn't be documented here
https://review.coreboot.org/#/c/31550/7/src/arch/x86/Makefile.inc
File src/arch/x86/Makefile.inc:
https://review.coreboot.org/#/c/31550/7/src/arch/x86/Makefile.inc@281
PS7, Line 281: postcar-$(CONFIG_PLATFORM_HAS_DRAM_CLEAR) += memory_clear.c
postcar has global variables in RAM, doesn't it?
why would we want this in postcar?
https://review.coreboot.org/#/c/31550/7/src/arch/x86/include/arch/memory_cl…
File src/arch/x86/include/arch/memory_clear.h:
https://review.coreboot.org/#/c/31550/7/src/arch/x86/include/arch/memory_cl…
PS7, Line 2: * This file is part of the coreboot project.
: *
: * Copyright (C) 2019 9elements Agency GmbH
: * Copyright (C) 2019 Facebook Inc.
: *
: * Redistribution and use in source and binary forms, with or without
: * modification, are permitted provided that the following conditions
: * are met:
: * 1. Redistributions of source code must retain the above copyright
: * notice, this list of conditions and the following disclaimer.
: * 2. Redistributions in binary form must reproduce the above copyright
: * notice, this list of conditions and the following disclaimer in the
: * documentation and/or other materials provided with the distribution.
: * 3. The name of the author may not be used to endorse or promote products
: * derived from this software without specific prior written permission.
: *
: * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
: * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
: * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
: * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
: * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
: * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
: * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
: * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
: * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
: * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
: * SUCH DAMAGE.
> Document in the commit message, why BSD(?) is used?
Unlikely to apply to an interface anyway...
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c
File src/arch/x86/memory_clear.c:
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@38
PS7, Line 38: const struct memranges *mem_soc)
Why not make it simply a pre-filled `struct memranges *`?
Having special cases for low/high mem seems arch specific
and makes it harder to guess the holes and patch things
together.
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@48
PS7, Line 48: causes exception
This should be fixed. What did you try? clearing in PAE mode?
or in regular protected mode?
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@50
PS7, Line 50: /* Hole at 0xa0000 */
This hole may contain RAM. Even security critical RAM (probably
not used in coreboot currently, but some platforms have some
SMM-only accessible crap here).
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@51
PS7, Line 51: memranges_insert(&mem, 0xf0000, 0x10000, BM_MEM_RAM);
Isn't this usually (flash) ROM?
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@56
PS7, Line 56: BM_MEM_RESERVED);
and BSS? and stack?
I would prefer that we only use this in romstage and don't care.
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@61
PS7, Line 61: memranges_each_entry(r, mem_soc) {
: memranges_insert(&mem, range_entry_base(r), range_entry_size(r),
: range_entry_tag(r));
: }
:
Could also use memranges_clone() instead from the beginning.
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@67
PS7, Line 67: * Reserve CBMEM - Used as scratch memory for memcpy_pae.
If we were in postcar, CBMEM would already be initialized?
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@78
PS7, Line 78: if ((uintptr_t)cbmem_top() >= 4ULL * GiB) {
The cast takes precedence, so in 32-bit it flow
over already before the comparison. Use `uint64_t`?
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@82
PS7, Line 82: intptr_t start = (uintptr_t)cbmem_top() - cbmem_size;
: void *scratch2 = (void *)ALIGN_UP(start, 2 * MiB);
: start = (uintptr_t)scratch2 + 2 * MiB;
: void *scratch = (void *)ALIGN_UP(start, 4 * KiB);
This is not easy to follow. Maybe draw some ascii art?
Or a rather alternative approach: Use a constant for
`scratch2`, e.g. 0x200000. And search for the first
RAM range above 0x400000, reserve that for `scratch`
and clear later. That would avoid to reason about
CBMEM and negative offsets and would already check
that the space for `scratch` is actually useable.
https://review.coreboot.org/#/c/31550/7/src/arch/x86/memory_clear.c@85
PS7, Line 85: void *scratch = (void *)ALIGN_UP(start, 4 * KiB);
NB. Weaker alignment makes this a no-op.
--
To view, visit https://review.coreboot.org/c/coreboot/+/31550
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idaadb8fb438e5b95557c0f65a14534e8762fde20
Gerrit-Change-Number: 31550
Gerrit-PatchSet: 7
Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com>
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: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.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: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Wed, 06 Mar 2019 18:49:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: comment
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:
(1 comment)
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@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? […]
However, `pgtbl_buf` and `vmem_addr` must not overlap because of
the live PDE patching.
--
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 18:26:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment