Re: [PATCH] avb: Extend support to non-eMMC interfaces
On Mon, Oct 10, 2022 at 7:53 AM Mattijs Korpershoek wrote: > > Hi Alistair, > > Thank you for your patch. > > On lun., sept. 26, 2022 at 22:02, Alistair Delva wrote: > > > From: Jiyong Park > > > > Previously Android AVB supported block devices only on eMMC. This change > > eliminates the restriction by using the generic block driver model. > > > > The `avb init' command is modified to accept another parameter which > > specifies the interface type. e.g., `avb init virtio 0' initializes > > avb for the first (0) disk that is accessible via the virtio interface. > > > > [adelva: The "avb init" command is updated directly, as this is > > considered a "debug command" that can't be usefully used in u-boot > > scripts.] > > It seems that: > * include/configs/ti_omap5_common.h > * include/configs/meson64_android.h > * configs/total_compute_defconfig > > All call "avb init" > > Aren't we breaking these boot scripts with this change? > > Since all of them used mmc before, it should be trivial to patch these > environments as well. > If you do so, please cc me in next version so I can give this a try on > the khadas vim3l board. Sure, I'll do that and send a v2. > Maybe those devices are doing the wrong thing though. since this is > considered a debug command I imagine we should not be calling this at > all. > If so, do you have any alternatives in mind? Yes, sorry. My rationale was confusing. We have more patches to upstream which will explain the reasoning better: "data from boot and vendor_boot partitions are loaded only once for the verification. After the verification is done, the read data is directly copied to the load addresses instead of doing the I/O again from the disk. This is to prevent a TOCTOU issue where attacker provides different data for verification and actual booting." So yes, what these env files do for now isn't safe, but there isn't a good upstream alternative. Anyway, this problem isn't related to what this patch is doing. So, I should update them for now. > > > > Signed-off-by: Alistair Delva > > Cc: Igor Opaniuk > > Cc: Ram Muthiah > > Cc: Jiyong Park > > Cc: Simon Glass > > --- > > cmd/avb.c| 16 --- > > common/Kconfig | 1 - > > common/avb_verify.c | 105 +-- > > include/avb_verify.h | 31 - > > Should we also update the documentation in doc/android/avb2.rst ? Will do. > I also think this might break: > test/py/tests/test_android/test_avb.py I'll update it. > > 4 files changed, 69 insertions(+), 84 deletions(-) > > > > diff --git a/cmd/avb.c b/cmd/avb.c > > index 783f51b816..8bffe49011 100644 > > --- a/cmd/avb.c > > +++ b/cmd/avb.c > > @@ -10,24 +10,25 @@ > > #include > > #include > > #include > > -#include > > > > #define AVB_BOOTARGS "avb_bootargs" > > static struct AvbOps *avb_ops; > > > > int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const > > argv[]) > > { > > - unsigned long mmc_dev; > > + const char *iface; > > + const char *devnum; > > > > - if (argc != 2) > > + if (argc != 3) > > return CMD_RET_USAGE; > > > > - mmc_dev = hextoul(argv[1], NULL); > > + iface = argv[1]; > > + devnum = argv[2]; > > > > if (avb_ops) > > avb_ops_free(avb_ops); > > > > - avb_ops = avb_ops_alloc(mmc_dev); > > + avb_ops = avb_ops_alloc(iface, devnum); > > if (avb_ops) > > return CMD_RET_SUCCESS; > > > > @@ -419,7 +420,7 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int > > flag, int argc, > > } > > > > static struct cmd_tbl cmd_avb[] = { > > - U_BOOT_CMD_MKENT(init, 2, 0, do_avb_init, "", ""), > > + U_BOOT_CMD_MKENT(init, 3, 0, do_avb_init, "", ""), > > U_BOOT_CMD_MKENT(read_rb, 2, 0, do_avb_read_rb, "", ""), > > U_BOOT_CMD_MKENT(write_rb, 3, 0, do_avb_write_rb, "", ""), > > U_BOOT_CMD_MKENT(is_unlocked, 1, 0, do_avb_is_unlocked, "", ""), > > @@ -455,7 +456,8 @@ static int do_avb(struct cmd_tbl *cmdtp, int flag, int > > argc, char *const argv[]) > > U_BOOT_CMD( > > avb, 29, 0, do_avb, > > "Provides commands for testing Android Verified Boot 2.0 > > functionality", > > - "init - initialize avb2 for \n" > > + &q
Re: [PATCH] Makefile: Disable PLATFORM_LIBGCC for LLVM toolchain
Hi Tom, On Tue, Sep 27, 2022 at 8:57 AM Tom Rini wrote: > > On Mon, Sep 26, 2022 at 08:47:47PM +, Alistair Delva wrote: > > > The LLVM toolchain does not have or need libgcc, so do not require > > it to exist on the library path. Even if "-print-libgcc-file-name" > > returned the empty string, -lgcc would be specified. > > > > This leaves CONFIG_USE_PRIVATE_LIBGCC alone because I did not have > > a target/toolchain combination available for testing. > > > > Signed-off-by: Alistair Delva > > Cc: Simon Glass > > Cc: Tom Rini > > Cc: Nick Desaulniers > > --- > > Makefile | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/Makefile b/Makefile > > index 8af67ebd63..af06b7aa19 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -874,8 +874,10 @@ u-boot-main := $(libs-y) > > ifeq ($(CONFIG_USE_PRIVATE_LIBGCC),y) > > PLATFORM_LIBGCC = arch/$(ARCH)/lib/lib.a > > else > > +ifneq ($(cc-name),clang) > > PLATFORM_LIBGCC := -L $(shell dirname `$(CC) $(c_flags) > > -print-libgcc-file-name`) -lgcc > > endif > > +endif > > PLATFORM_LIBS += $(PLATFORM_LIBGCC) > > > > ifdef CONFIG_CC_COVERAGE > > So this one isn't quite right, and will result in some platforms / > architectures just failing to build as the handful of functions we get > provided by CONFIG_USE_PRIVATE_LIBGCC end up being missing. > > It's also the case that this is a badly named option, and something we > really should figure out how to remove and then always provide the > required functions for. What configuration did you end up having this > problem on exactly? We have CI set up for qemu_arm64_defconfig, qemu-x86_64_defconfig, am57xx_evm_defconfig and rock-pi-4-rk3399_defconfig. I think all were affected, since they don't use CONFIG_USE_PRIVATE_LIBGCC, so they all hit this fallback. AFAIK, for a long time, Android used an LLVM toolchain built with some workarounds such as knowledge of a corresponding GCC-based toolchain, so -print-libgcc-file-name could provide a path to libgcc.a. However, our newer toolchains no longer do this and the symbols are either automatically provided or provided by compiler-rt. In U-Boot's case, I don't think our targets ever required libgcc but because the fall-through expands to "-L . -lgcc" when -print-libgcc-file-name returns the empty string, the link will fail because it will look for libgcc but not find it. Alistair.
[PATCH] x86: Fix i8259 ifdef include guard
When building U-Boot with clang, it notices that the i8259.h include guard does not work correctly due to a typo. Fix it. Signed-off-by: Alistair Delva Cc: Simon Glass Cc: Bin Meng Cc: Nick Desaulniers --- arch/x86/include/asm/i8259.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/include/asm/i8259.h b/arch/x86/include/asm/i8259.h index b73052a6d2..90f2d3866c 100644 --- a/arch/x86/include/asm/i8259.h +++ b/arch/x86/include/asm/i8259.h @@ -7,7 +7,7 @@ /* i8259.h i8259 PIC Registers */ #ifndef _ASMI386_I8259_H_ -#define _ASMI386_I8959_H_ +#define _ASMI386_I8259_H_ /* PIC I/O mapped registers */ #define IRR0x0 /* Interrupt Request Register */ @@ -73,4 +73,4 @@ int i8259_init(void); -#endif /* _ASMI386_I8959_H_ */ +#endif /* _ASMI386_I8259_H_ */ -- 2.30.2
[PATCH] avb: Extend support to non-eMMC interfaces
From: Jiyong Park Previously Android AVB supported block devices only on eMMC. This change eliminates the restriction by using the generic block driver model. The `avb init' command is modified to accept another parameter which specifies the interface type. e.g., `avb init virtio 0' initializes avb for the first (0) disk that is accessible via the virtio interface. [adelva: The "avb init" command is updated directly, as this is considered a "debug command" that can't be usefully used in u-boot scripts.] Signed-off-by: Alistair Delva Cc: Igor Opaniuk Cc: Ram Muthiah Cc: Jiyong Park Cc: Simon Glass --- cmd/avb.c| 16 --- common/Kconfig | 1 - common/avb_verify.c | 105 +-- include/avb_verify.h | 31 - 4 files changed, 69 insertions(+), 84 deletions(-) diff --git a/cmd/avb.c b/cmd/avb.c index 783f51b816..8bffe49011 100644 --- a/cmd/avb.c +++ b/cmd/avb.c @@ -10,24 +10,25 @@ #include #include #include -#include #define AVB_BOOTARGS "avb_bootargs" static struct AvbOps *avb_ops; int do_avb_init(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { - unsigned long mmc_dev; + const char *iface; + const char *devnum; - if (argc != 2) + if (argc != 3) return CMD_RET_USAGE; - mmc_dev = hextoul(argv[1], NULL); + iface = argv[1]; + devnum = argv[2]; if (avb_ops) avb_ops_free(avb_ops); - avb_ops = avb_ops_alloc(mmc_dev); + avb_ops = avb_ops_alloc(iface, devnum); if (avb_ops) return CMD_RET_SUCCESS; @@ -419,7 +420,7 @@ int do_avb_write_pvalue(struct cmd_tbl *cmdtp, int flag, int argc, } static struct cmd_tbl cmd_avb[] = { - U_BOOT_CMD_MKENT(init, 2, 0, do_avb_init, "", ""), + U_BOOT_CMD_MKENT(init, 3, 0, do_avb_init, "", ""), U_BOOT_CMD_MKENT(read_rb, 2, 0, do_avb_read_rb, "", ""), U_BOOT_CMD_MKENT(write_rb, 3, 0, do_avb_write_rb, "", ""), U_BOOT_CMD_MKENT(is_unlocked, 1, 0, do_avb_is_unlocked, "", ""), @@ -455,7 +456,8 @@ static int do_avb(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) U_BOOT_CMD( avb, 29, 0, do_avb, "Provides commands for testing Android Verified Boot 2.0 functionality", - "init - initialize avb2 for \n" + "init - initialize avb2 for the disk which\n" + "is on the interface \n" "avb read_rb - read rollback index at location \n" "avb write_rb - write rollback index to \n" "avb is_unlocked - returns unlock status of the device\n" diff --git a/common/Kconfig b/common/Kconfig index ebee856e56..a66060767c 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -703,7 +703,6 @@ config HASH config AVB_VERIFY bool "Build Android Verified Boot operations" depends on LIBAVB - depends on MMC depends on PARTITION_UUIDS help This option enables compilation of bootloader-dependent operations, diff --git a/common/avb_verify.c b/common/avb_verify.c index 0520a71455..d30bbb5726 100644 --- a/common/avb_verify.c +++ b/common/avb_verify.c @@ -253,10 +253,10 @@ char *avb_set_enforce_verity(const char *cmdline) /** * - * IO(mmc) auxiliary functions + * IO auxiliary functions * */ -static unsigned long mmc_read_and_flush(struct mmc_part *part, +static unsigned long blk_read_and_flush(struct avb_part *part, lbaint_t start, lbaint_t sectors, void *buffer) @@ -291,7 +291,7 @@ static unsigned long mmc_read_and_flush(struct mmc_part *part, tmp_buf = buffer; } - blks = blk_dread(part->mmc_blk, + blks = blk_dread(part->blk, start, sectors, tmp_buf); /* flush cache after read */ flush_cache((ulong)tmp_buf, sectors * part->info.blksz); @@ -302,7 +302,7 @@ static unsigned long mmc_read_and_flush(struct mmc_part *part, return blks; } -static unsigned long mmc_write(struct mmc_part *part, lbaint_t start, +static unsigned long blk_write(struct avb_part *part, lbaint_t start, lbaint_t sectors, void *buffer) { void *tmp_buf; @@ -330,69 +330,59 @@ static unsigned long mmc_write(struct mmc_part *part, lbaint_t start, tmp_buf = buffer; } - return blk_dwrite(part->mmc_blk, + return blk_dwrite(part->blk, start, sectors, tmp_
[PATCH] spl: atf: Fix clang -Wasm-operand-widths warning
common/spl/spl_atf.c:187:51: warning: value size does not match register size specified by the constraint and modifier [-Wasm-operand-widths] __asm__ __volatile__("msr DAIF, %0\n\t" : : "r" (daif) : "memory"); ^ common/spl/spl_atf.c:187:34: note: use constraint modifier "w" __asm__ __volatile__("msr DAIF, %0\n\t" : : "r" (daif) : "memory"); ^~ %w0 Use %x0 to match what Linux does in write_sysreg(). Signed-off-by: Alistair Delva Cc: Kever Yang Cc: Michael Walle Cc: Simon Glass Cc: Tom Rini Cc: Nick Desaulniers --- common/spl/spl_atf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/spl/spl_atf.c b/common/spl/spl_atf.c index e1b68dd561..bae5c010c8 100644 --- a/common/spl/spl_atf.c +++ b/common/spl/spl_atf.c @@ -184,7 +184,7 @@ __weak struct bl_params *bl2_plat_get_bl31_params_v2(uintptr_t bl32_entry, static inline void raw_write_daif(unsigned int daif) { - __asm__ __volatile__("msr DAIF, %0\n\t" : : "r" (daif) : "memory"); + __asm__ __volatile__("msr DAIF, %x0\n\t" : : "r" (daif) : "memory"); } typedef void (*atf_entry_t)(struct bl31_params *params, void *plat_params); -- 2.30.2
[PATCH] Makefile: Disable PLATFORM_LIBGCC for LLVM toolchain
The LLVM toolchain does not have or need libgcc, so do not require it to exist on the library path. Even if "-print-libgcc-file-name" returned the empty string, -lgcc would be specified. This leaves CONFIG_USE_PRIVATE_LIBGCC alone because I did not have a target/toolchain combination available for testing. Signed-off-by: Alistair Delva Cc: Simon Glass Cc: Tom Rini Cc: Nick Desaulniers --- Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Makefile b/Makefile index 8af67ebd63..af06b7aa19 100644 --- a/Makefile +++ b/Makefile @@ -874,8 +874,10 @@ u-boot-main := $(libs-y) ifeq ($(CONFIG_USE_PRIVATE_LIBGCC),y) PLATFORM_LIBGCC = arch/$(ARCH)/lib/lib.a else +ifneq ($(cc-name),clang) PLATFORM_LIBGCC := -L $(shell dirname `$(CC) $(c_flags) -print-libgcc-file-name`) -lgcc endif +endif PLATFORM_LIBS += $(PLATFORM_LIBGCC) ifdef CONFIG_CC_COVERAGE -- 2.30.2
[PATCH] Makefile: apply dynamic relocations for LLD
From: Nick Desaulniers It seems that for aarch64, unless we apply dynamic relocations to the location being relocated, we fail to boot. As Fangrui notes: For dynamic relocations using the RELA format (readelf -Wr), GNU ld sets the initial content to r_addend; ld.lld doesn't do that by default (needs --apply-dynamic-relocs). Otherwise .rodata appears to be full of NUL-bytes before relocation, causing crashes when trying to invoke the function pointers in init_sequence_f from initcall_run_list(). Link: https://reviews.llvm.org/D42797 Suggested-by: Fangrui Song Signed-off-by: Nick Desaulniers Signed-off-by: Alistair Delva Cc: Simon Glass Cc: Tom Rini Cc: Nick Desaulniers --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 010ace9d7c..af06b7aa19 100644 --- a/Makefile +++ b/Makefile @@ -1022,7 +1022,7 @@ LDFLAGS_u-boot += $(LDFLAGS_FINAL) LDFLAGS_u-boot += $(call ld-option, --no-dynamic-linker) # ld.lld support -LDFLAGS_u-boot += -z notext +LDFLAGS_u-boot += -z notext $(call ld-option,--apply-dynamic-relocs) LDFLAGS_u-boot += --build-id=none -- 2.30.2
[PATCH] examples: standalone: Fix build with LLVM toolchain
When building the standalone example with llvm, the link step fails: examples/standalone/libstubs.o: In function `dummy': include/_exports.h:10: undefined reference to `jt' include/_exports.h:11: undefined reference to `jt' include/_exports.h:12: undefined reference to `jt' include/_exports.h:13: undefined reference to `jt' include/_exports.h:14: undefined reference to `jt' examples/standalone/libstubs.o:include/_exports.h:15: more undefined references to `jt' follow Indeed, the standalone libstubs.o does use the jt symbol, but it was marked 'static' in stubs.c. It's strange how gcc builds are working. Signed-off-by: Alistair Delva Cc: Rick Chen Cc: Simon Glass Cc: Tom Rini Cc: Nick Desaulniers --- examples/standalone/stubs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/standalone/stubs.c b/examples/standalone/stubs.c index ce05f41b0c..65115570e8 100644 --- a/examples/standalone/stubs.c +++ b/examples/standalone/stubs.c @@ -14,7 +14,7 @@ struct cmd_tbl; * from flash memory. The global_data address is passed as argv[-1] * to the application program. */ -static struct jt_funcs *jt; +struct jt_funcs *jt; gd_t *global_data; #define EXPORT_FUNC(f, a, x, ...) \ -- 2.30.2
Re: Commit 4f2e2280862a ("RFC: arm: pci: Add PCI cam support to PCI-E ecam driver")
Hi Pali, Sorry for the late reply.. On Thu, Jan 13, 2022 at 4:34 AM Pali Rohár wrote: > > Hello! > > Now I see that you have merged commit 4f2e2280862a ("RFC: arm: pci: Add > PCI cam support to PCI-E ecam driver"). It adds some "PCI cam support" > with generic DT binding "pci-host-cam-generic". > > I have tried to find some information about it, but in PCIe > specification there is nothing like PCI CAM. And neither in old PCI > local bus 2.x or 3.x specs. I can't really help you with documentation, but "pci-host-cam-generic" isn't something we made up, it is the same name used upstream by Linux: https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/pci-host-generic.c#L60 We don't have specs, we just reverse engineered what was happening in the crosvm vm manager emulation of this device (https://chromium.googlesource.com/chromiumos/platform/crosvm/+/refs/heads/master/aarch64/src/fdt.rs). > This access looks like a mix of "PCI Configuration Mechanism #1" and > "PCI Configuration Mechanism #2" from PCI Local Bus Specification > (rev 2.1, sections 3.7.4.1 and 3.7.4.2) and incompatible with both of > them. It has layout similar to Mechanism #1 and access similar to #2. > > PCI Configuration Mechanism #1 uses two registers, one which select > config address and second for accessing config space (selected address). > But that U-Boot "PCI CAM" is implemented as memory mapped address space, > something similar to PCI Configuration Mechanism #2 but with different > layout. Also that "PCI CAM" does not set "enable" bit which is per PCI > Configuration Mechanism #1 required to access PCI config space. > > Recently I converted all PCI drivers in U-Boot which uses PCI > Configuration Mechanism #1 to use PCI_CONF1_ADDRESS() macro for > accessing PCI config space. Basically every HW which uses PCI > Configuration Mechanism #1 requires to set "enable" bit like it is > described in PCI local bus spec. There is only one exception pci_msc01.c > which requires to have "enable" bit unset. And I'm not sure if this is > not rather bug in U-Boot driver (but it is in U-Boot in this state for a > long time). > > Do you have some references to this "PCI CAM" specification? Because for > me it looks like some vendor/proprietary undocumented API and > incompatible with everything which I saw. > > Therefore I would suggest to not call it "pci-host-cam-generic" or > TYPE_PCI as it is not generic for sure (like PCIe ECAM which is > documented in PCIe base spec) and also because it is not PCI type (does > not match neither PCI Mechanism #1 nor Mechanism #2). > > Anyway, I would like to know, which hardware uses this unusual PCI > config space access? I don't know what real hardware uses it, but it is used by crosvm (https://chromium.googlesource.com/chromiumos/platform/crosvm) > Btw, that commit probably does not work. It uses construction: > > (PCI_FUNC(bdf) << 8) | offset > > offset passed by U-Boot is number between 0..4095 and therefore it > overlaps with PCI function number. Either shift by 8 is wrong and it > should be shift by 12 or offset needs to be limited just to 0..255. But > then there would be no access to PCIe extended space (256..4095), only > PCI and I doubt that somebody in 2022 is still doing new development for > Conventional PCI local bus hardware. I think that's the case for this device, unfortunately. Perhaps we should cap offset between 0..255. Our change does work; without it, U-Boot can't see any PCI devices. With it, they are all shown. The other shifts in the change look the same as the Linux driver which adjusts the shift from 20 to 16 here: https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/pci-host-generic.c#L18 I admit, the added logic looks different though: https://elixir.bootlin.com/linux/latest/source/drivers/pci/ecam.c#L187 > Also in my opinion as this "PCI CAM" does not set "enable" bit, there is > needed some other code which sets it via vendor-specific API. What should we do for now? Do you need any help getting set up with this environment? I think we could look at adding the pcie ecam device to crosvm in parallel.
[PATCH] arm64: relocate-rela: Add support for ld.lld
Cap end of relocations by the binary size. Linkers like to insert some auxiliary sections between .rela.dyn and .bss_start. These sections don't make their way to the final binary, but reloc_rela still tries to relocate them, resulting in attempted read past the end of file. When linking U-Boot with ld.lld, the STATIC_RELA feature (enabled by default on arm64) breaks the build. After this patch, U-Boot can be linked successfully with and without CONFIG_STATIC_RELA. Originally-from: Elena Petrova Signed-off-by: Alistair Delva Cc: David Brazdil Cc: Scott Wood Cc: Tom Rini --- tools/relocate-rela.c | 20 +--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/tools/relocate-rela.c b/tools/relocate-rela.c index 6a524014b7..f0bc548617 100644 --- a/tools/relocate-rela.c +++ b/tools/relocate-rela.c @@ -63,7 +63,7 @@ int main(int argc, char **argv) { FILE *f; int i, num; - uint64_t rela_start, rela_end, text_base; + uint64_t rela_start, rela_end, text_base, file_size; if (argc != 5) { fprintf(stderr, "Statically apply ELF rela relocations\n"); @@ -87,8 +87,7 @@ int main(int argc, char **argv) return 3; } - if (rela_start > rela_end || rela_start < text_base || - (rela_end - rela_start) % sizeof(Elf64_Rela)) { + if (rela_start > rela_end || rela_start < text_base) { fprintf(stderr, "%s: bad rela bounds\n", argv[0]); return 3; } @@ -96,6 +95,21 @@ int main(int argc, char **argv) rela_start -= text_base; rela_end -= text_base; + fseek(f, 0, SEEK_END); + file_size = ftell(f); + rewind(f); + + if (rela_end > file_size) { + // Most likely compiler inserted some section that didn't get + // objcopy-ed into the final binary + rela_end = file_size; + } + + if ((rela_end - rela_start) % sizeof(Elf64_Rela)) { + fprintf(stderr, "%s: rela size isn't a multiple of Elf64_Rela\n", argv[0]); + return 3; + } + num = (rela_end - rela_start) / sizeof(Elf64_Rela); for (i = 0; i < num; i++) { -- 2.30.2