Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33821 )
Change subject: src: Remove variable length arrays
......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/#/c/33821/8/src/drivers/spi/spi_flash.c
File src/drivers/spi/spi_flash.c:
https://review.coreboot.org/#/c/33821/8/src/drivers/spi/spi_flash.c@103
PS8, Line 103: #pragma GCC diagnostic ignored "-Wvla"
> This driver is used in all stages, so no malloc(). In theory this should be really easy to solve because our SPI API is a pure byte pushing API that controls CS independently, so xfer(cmd <concatenated with> data); should just be equivalent to { xfer(cmd); xfer(data); }. In practice, however, there are a lot of weird Intel and AMD controllers that are not really generic SPI controllers but rather specific SPI NOR flash controllers but were still implemented using the generic controller API. They expect that every xfer() they get sent is a whole SPI command (e.g. they inspect dout[0] and expect that it must be a SPI flash opcode).
>
> I think the only clean solution here would be to make all those drivers that aren't actually SPI controllers stop using the generic SPI API, and instead make them implement custom flash read/write ops by overriding .flash_probe() (like some more recent drivers for special controllers such as Intel fast_spi or the MT8173 flash controller already do). Then we rewrite this cleanly and mandate that from then on forward, drivers shouldn't implement the generic SPI API unless they can actually handle it. But there are a lot of old drivers like this (I think basically everything that implements xfer_vector, which we could remove if we fix them all), so this would be a lot of work (although most of these were copied from each other and are very similar... I think for the most part there's just 6 versions of the Intel controller and 4 versions of the AMD controller).
>
> A hackier but less complicated option might be to add a new SPI_CNTRLR flag for all these controllers and then make only them run the dangerous code, while other ones could run a safer version.
>
> +Aaron, what do you think?
I think the answer is the 2nd paragraph. For the fixed function spi flash controllers make sure they are implemented according to their semantics. i.e. struct spi_flash->ops should be filled in with special ops corresponding to the controllers' implementations. e.g.
src/soc/intel/common/block/fast_spi/fast_spi_flash.c
I think the list of things to cutover are any users of spi_flash_vector_helper(). The xfer_vector as you noted is a good proxy as well. The cn81xx spi driver is the odd one out:
$ git grep -l xfer_vector
src/drivers/spi/spi-generic.c
src/drivers/spi/spi_flash.c
src/include/spi-generic.h
src/soc/amd/stoneyridge/spi.c
src/soc/cavium/cn81xx/spi.c
src/soc/intel/baytrail/spi.c
src/soc/intel/braswell/spi.c
src/soc/intel/broadwell/spi.c
src/soc/intel/fsp_baytrail/spi.c
src/soc/qualcomm/qcs405/spi.c
src/southbridge/amd/agesa/hudson/spi.c
src/southbridge/amd/cimx/sb800/spi.c
src/southbridge/amd/sb700/spi.c
src/southbridge/intel/common/spi.c
src/southbridge/intel/fsp_rangeley/spi.c
$ git grep -l spi_flash_vector_helper
src/drivers/spi/spi_flash.c
src/include/spi_flash.h
src/soc/amd/stoneyridge/spi.c
src/soc/intel/baytrail/spi.c
src/soc/intel/braswell/spi.c
src/soc/intel/broadwell/spi.c
src/soc/intel/fsp_baytrail/spi.c
src/soc/qualcomm/qcs405/spi.c
src/southbridge/amd/agesa/hudson/spi.c
src/southbridge/amd/cimx/sb800/spi.c
src/southbridge/amd/sb700/spi.c
src/southbridge/intel/common/spi.c
src/southbridge/intel/fsp_rangeley/spi.c
I think having cn81xx just implement xfer would be the fix for its xfer_vector usage.
Added a tracking bug here: https://ticket.coreboot.org/issues/217
--
To view, visit https://review.coreboot.org/c/coreboot/+/33821
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I7d9d1ddadbf1cee5f695165bbe3f0effb7bd32b9
Gerrit-Change-Number: 33821
Gerrit-PatchSet: 8
Gerrit-Owner: Jacob Garber <jgarber1(a)ualberta.ca>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Jacob Garber <jgarber1(a)ualberta.ca>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Mon, 01 Jul 2019 20:59:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Comment-In-Reply-To: Jacob Garber <jgarber1(a)ualberta.ca>
Gerrit-MessageType: comment
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33859
Change subject: Kconfig: Enable RAMPAYLOAD for x86
......................................................................
Kconfig: Enable RAMPAYLOAD for x86
This patch makes CONFIG_RAMPAYLOAD default enable upon selection
of HAVE_RAMPAYLOAD kconfig from mainboard for x86 platform.
Without this CL, CONFIG_RAMPAYLOAD is still disable although
mainboard has selected CONFIG_HAVE_RAMPAYLOAD.
Change-Id: I40308bbf970a0dbe5f7e2086ed8a7a70c2a3a32c
Signed-off-by: Subrata Banik <subrata.banik(a)intel.com>
---
M src/Kconfig
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/33859/1
diff --git a/src/Kconfig b/src/Kconfig
index 72d826f..919d257 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -290,7 +290,7 @@
config RAMPAYLOAD
bool "Enable coreboot flow without executing ramstage"
- default n
+ default y if ARCH_X86
depends on HAVE_RAMPAYLOAD
help
If this option is enabled, coreboot flow will skip ramstage
--
To view, visit https://review.coreboot.org/c/coreboot/+/33859
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I40308bbf970a0dbe5f7e2086ed8a7a70c2a3a32c
Gerrit-Change-Number: 33859
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-MessageType: newchange