Re: [PATCH 1/2] lib: optee: remove the duplicate CONFIG_OPTEE
Hello Alex and Patrick,
(my apologies for my previous malformed post)
On Fri, 3 Sept 2021 at 18:43, Alex G. wrote:
>
> Hi Patrick
>
> On 9/2/21 4:56 AM, Patrick Delaunay wrote:
> > The configuration CONFIG_OPTEE is defined 2 times:
> > 1- in lib/optee/Kconfig for support of OPTEE images loaded by bootm command
> > 2- in drivers/tee/optee/Kconfig for support of OP-TEE driver.
> >
> > It is abnormal to have the same CONFIG define for 2 purpose;
> > and it is difficult to managed correctly their dependencies.
+1
> >
> > Moreover CONFIG_SPL_OPTEE is defined in common/spl/Kconfig
> > to manage OPTEE image load in SPL.
> >
> > This definition causes an issue with the macro CONFIG_IS_ENABLED(OPTEE)
> > to test the availability of the OP-TEE driver.
> >
> > This patch cleans the configuration dependency with:
> > - CONFIG_OPTEE_IMAGE (renamed) => support of OP-TEE image in U-Boot
> > - CONFIG_SPL_OPTEE_IMAGE (renamed) => support of OP-TEE image in SPL
> > - CONFIG_OPTEE (same) => support of OP-TEE driver in U-Boot
> > - CONFIG_OPTEE_LIB (new) => support of OP-TEE library
> >
> > After this patch, the macro have the correct behavior:
> > - CONFIG_IS_ENABLED(OPTEE_IMAGE) => Load of OP-TEE image is supported
> > - CONFIG_IS_ENABLED(OPTEE) => OP-TEE driver is supported
>
> It seems a little odd to have both OPTEE_LIB and OPTEE_IMAGE, since they
> are both used together to support booting with OP-TEE. What also seems
> odd is that "OP-TEE driver in U-Boot" does not depend on "OP-TEE library".
>
> Introducing OPTEE_LIB then, makes sense to me, provided that OPTEE
> depends on OPTEE_LIB, but I'm not sure about OPTEE_IMAGE.
>
> > diff --git a/lib/optee/optee.c b/lib/optee/optee.c
> > index 672690dc53..5676785cb5 100644
> > --- a/lib/optee/optee.c
> > +++ b/lib/optee/optee.c
> > @@ -20,6 +20,7 @@
> > "\n\theader lo=0x%08x hi=0x%08x size=0x%08lx arch=0x%08x" \
> > "\n\tuimage params 0x%08lx-0x%08lx\n"
> >
> > +#if defined(CONFIG_OPTEE_IMAGE)
> > int optee_verify_image(struct optee_header *hdr, unsigned long
> > tzdram_start,
> > unsigned long tzdram_len, unsigned long image_len)
> > {
> > @@ -70,6 +71,7 @@ error:
> >
> > return ret;
> > }
> > +#endif
>
> One the idea of having CONFIGs is to include/exclude code via
> obj-$(CONFIG_FOO)+=code.c. This prevents the proliferation of #ifdefs.
> It's fairly counterintuitive to have a CONFIG_OPTEE_IMAGE in a file
> controlled by CONFIG_OPTEE_LIB.
>
> Going to optee_verify_image() itself. It essentially checks against
> OPTEE_TZDRAM_(BASE/SIZE). But those are a derived from devicetree, not
> Kconfig. So it seems the motivation behing optee_verify_bootm_image() is
> flawed. Also the error message is not very helpful.
The 2 functions are related to CONFIG_BOOTM_OPTEE. They could depend on.
My 2 cents.
If preserving the optee_verify_xxx() functions, they could move to a
specific source lib/optee/optee_image.c
>
> In fact, the SPL boot path for OP-TEE doesn't use this function. That's
> intentional.
>
> Here's what I suggest:
>- Remove OPTEE_TZDRAM_BASE and _SIZE
There is some legacy here, board/warp7and board/technexion/pico-imx7d.
regards,
etienne
>
>- Remove optee_verify_bootm_image()
>
>- No need for CONFIG_OPTEE_IMAGE
>
>
>
> Alex
[PATCH v2 1/1] riscv: show code leading to exception
To make analyzing exceptions easier output the code that leads to it.
We already do the same on the ARM platform.
Here is an example:
=> exception ebreak
Unhandled exception: Breakpoint
EPC: 8ff5d50e RA: 8ff5d62c TVAL:
EPC: 8020b50e RA: 8020b62c reloc adjusted
Code: 2785 0693 07a0 dce3 fef6 47a5 d563 00e7 (9002)
To disassemble the code we can use the decodecode script:
$ echo 'Code: 2785 0693 07a0 dce3 fef6 47a5 d563 00e7 (9002)' | \
CROSS_COMPILE=riscv64-linux-gnu- scripts/decodecode
Code: 2785 0693 07a0 dce3 fef6 47a5 d563 00e7 (9002)
All code
0: 2785addiw a5,a5,1
2: 07a00693li a3,122
6: fef6dce3bge a3,a5,0xfffe
a: 47a5li a5,9
c: 00e7d563bge a5,a4,0x16
10:* 9002ebreak <-- trapping instruction
...
Code starting with the faulting instruction
===
0: 9002ebreak
...
As it is not always clear if the first 16 bits are at the start or in the
middle of a 32bit instruction it may become necessary to strip the first
u16 from the output before calling decodecode to get the correct
disassembled code.
Signed-off-by: Heinrich Schuchardt
---
v2:
remove support for instructions longer than 32 bit as these are
not yet specified
---
arch/riscv/lib/interrupts.c | 33 +
1 file changed, 33 insertions(+)
diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c
index 7525c152b8..100be2e966 100644
--- a/arch/riscv/lib/interrupts.c
+++ b/arch/riscv/lib/interrupts.c
@@ -51,6 +51,38 @@ static void show_regs(struct pt_regs *regs)
#endif
}
+/**
+ * instr_len() - get instruction length
+ *
+ * @i: low 16 bits of the instruction
+ * Return: number of u16 in instruction
+ */
+static int instr_len(u16 i)
+{
+ if ((i & 0x03) != 0x03)
+ return 1;
+ /* Instructions with more than 32 bits are not yet specified */
+ return 2;
+}
+
+/**
+ * show_code() - display code leading to exception
+ *
+ * @epc: program counter
+ */
+static void show_code(ulong epc)
+{
+ u16 *pos = (u16 *)(epc & ~1UL);
+ int i, len = instr_len(*pos);
+
+ printf("\nCode: ");
+ for (i = -8; i; ++i)
+ printf("%04x ", pos[i]);
+ printf("(");
+ for (i = 0; i < len; ++i)
+ printf("%04x%s", pos[i], i + 1 == len ? ")\n" : " ");
+}
+
static void _exit_trap(ulong code, ulong epc, ulong tval, struct pt_regs *regs)
{
static const char * const exception_code[] = {
@@ -85,6 +117,7 @@ static void _exit_trap(ulong code, ulong epc, ulong tval,
struct pt_regs *regs)
epc - gd->reloc_off, regs->ra - gd->reloc_off);
show_regs(regs);
+ show_code(epc);
show_efi_loaded_images(epc);
panic("\n");
}
--
2.30.2
[PATCH v2 1/1] configs: qemu-riscvXX_spl_defconfig enable CMD_SBI
Both for 64bit and 32bit at least on one board we should compile the sbi command. Enabling it on QEMU will allow to write a test for it. Signed-off-by: Heinrich Schuchardt --- v2: additionally enable 32bit --- configs/qemu-riscv32_spl_defconfig | 1 + configs/qemu-riscv64_spl_defconfig | 1 + 2 files changed, 2 insertions(+) diff --git a/configs/qemu-riscv32_spl_defconfig b/configs/qemu-riscv32_spl_defconfig index 28ac2b3b53..ee81e55272 100644 --- a/configs/qemu-riscv32_spl_defconfig +++ b/configs/qemu-riscv32_spl_defconfig @@ -10,6 +10,7 @@ CONFIG_FIT=y CONFIG_SPL_LOAD_FIT_ADDRESS=0x8020 CONFIG_DISPLAY_CPUINFO=y CONFIG_DISPLAY_BOARDINFO=y +CONFIG_CMD_SBI=y # CONFIG_CMD_MII is not set CONFIG_OF_PRIOR_STAGE=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y diff --git a/configs/qemu-riscv64_spl_defconfig b/configs/qemu-riscv64_spl_defconfig index 78cfc410a3..429d4d814e 100644 --- a/configs/qemu-riscv64_spl_defconfig +++ b/configs/qemu-riscv64_spl_defconfig @@ -11,6 +11,7 @@ CONFIG_FIT=y CONFIG_SPL_LOAD_FIT_ADDRESS=0x8020 CONFIG_DISPLAY_CPUINFO=y CONFIG_DISPLAY_BOARDINFO=y +CONFIG_CMD_SBI=y # CONFIG_CMD_MII is not set CONFIG_OF_PRIOR_STAGE=y CONFIG_SYS_RELOC_GD_ENV_ADDR=y -- 2.30.2
Pull request for efi-2021-10-rc4
Dear Tom, The following changes since commit 94509b79b13e69c209199af0757afbde8d2ebd6d: btrfs: Use default subvolume as filesystem root (2021-09-01 10:11:24 -0400) are available in the Git repository at: https://source.denx.de/u-boot/custodians/u-boot-efi.git tags/efi-2021-10-rc4 for you to fetch changes up to 1dfa494610c5469cc28cf1f8538abf4be6c00324: efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check (2021-09-04 09:15:09 +0200) Pull request for efi-2021-10-rc4 Documentation: Remove invalid reference to configuration variable in UEFI doc UEFI: Parameter checks for the EFI_TCG2_PROTOCOL Improve support of preseeding UEFI variables. Correct the calculation of the size of loaded images. Allow for UEFI images with zero VirtualSize Heinrich Schuchardt (5): efi_loader: sections with zero VirtualSize efi_loader: rounding of image size efi_loader: don't load signature database from file efi_loader: efi_auth_var_type for AuditMode, DeployedMode efi_loader: correct determination of secure boot state Masahisa Kojima (3): efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api efi_loader: fix boot_service_capability_min calculation efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check include/efi_tcg2.h| 4 +++- include/efi_variable.h| 6 +- lib/efi_loader/efi_image_loader.c | 35 +-- lib/efi_loader/efi_tcg2.c | 21 ++- lib/efi_loader/efi_var_common.c | 43 ++- lib/efi_loader/efi_var_file.c | 41 ++--- lib/efi_loader/efi_variable.c | 6 +++--- 7 files changed, 118 insertions(+), 38 deletions(-)
Re: Pull request for efi-2021-10-rc4
Hello Tom, the doc patch was missing I have updated tag: The following changes since commit 94509b79b13e69c209199af0757afbde8d2ebd6d: btrfs: Use default subvolume as filesystem root (2021-09-01 10:11:24 -0400) are available in the Git repository at: https://source.denx.de/u-boot/custodians/u-boot-efi.git tags/efi-2021-10-rc4 for you to fetch changes up to 538c0f2d3798261161a28a05e445d0c85af56276: efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check (2021-09-04 12:03:57 +0200) Pull request for efi-2021-10-rc4 Documentation: Remove invalid reference to configuration variable in UEFI doc UEFI: Parameter checks for the EFI_TCG2_PROTOCOL Improve support of preseeding UEFI variables. Correct the calculation of the size of loaded images. Allow for UEFI images with zero VirtualSize Heinrich Schuchardt (5): efi_loader: sections with zero VirtualSize efi_loader: rounding of image size efi_loader: don't load signature database from file efi_loader: efi_auth_var_type for AuditMode, DeployedMode efi_loader: correct determination of secure boot state Masahisa Kojima (3): efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api efi_loader: fix boot_service_capability_min calculation efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check Michal Simek (1): doc: Remove information about CAPSULE_FMP_HEADER doc/develop/uefi/uefi.rst | 1 - include/efi_tcg2.h| 4 +++- include/efi_variable.h| 6 +- lib/efi_loader/efi_image_loader.c | 35 +-- lib/efi_loader/efi_tcg2.c | 21 ++- lib/efi_loader/efi_var_common.c | 43 ++- lib/efi_loader/efi_var_file.c | 41 ++--- lib/efi_loader/efi_variable.c | 6 +++--- 8 files changed, 118 insertions(+), 39 deletions(-)
Re: [PATCH] sunxi: mmc: A20: Fix MMC optimisation
Andre Przywara wrote on 9/4/21 12:49 AM: > Some SoCs (as seen on A20) seem to misreport the MMC FIFO level if the > FIFO is completely full: the level size reads as zero, but the FIFO_FULL > bit is set. We won't do a single iteration of the read loop in this > case, so will be stuck forever. > > Check for this situation and use a safe minimal FIFO size instead when > we hit this case. > > This fixes MMC boot on A20 devices after the MMC FIFO optimisation > (9faae5457f52). > > Signed-off-by: Andre Przywara Reviewed-by: Jaehoon Chung Best Regards, Jaehoon Chung > --- > drivers/mmc/sunxi_mmc.c | 10 +++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c > index 178b8cf106..aaab0cf866 100644 > --- a/drivers/mmc/sunxi_mmc.c > +++ b/drivers/mmc/sunxi_mmc.c > @@ -349,10 +349,14 @@ static int mmc_trans_data_by_cpu(struct sunxi_mmc_priv > *priv, struct mmc *mmc, >* register without checking the status register after every >* read. That saves half of the costly MMIO reads, effectively >* doubling the read performance. > + * Some SoCs (A20) report a level of 0 if the FIFO is > + * completely full (value masked out?). Use a safe minimal > + * FIFO size in this case. >*/ > - for (in_fifo = SUNXI_MMC_STATUS_FIFO_LEVEL(status); > - in_fifo > 0; > - in_fifo--) > + in_fifo = SUNXI_MMC_STATUS_FIFO_LEVEL(status); > + if (in_fifo == 0 && (status & SUNXI_MMC_STATUS_FIFO_FULL)) > + in_fifo = 32; > + for (; in_fifo > 0; in_fifo--) > buff[i++] = readl_relaxed(&priv->reg->fifo); > dmb(); > }
Re: Pull request for efi-2021-10-rc4
On Sat, Sep 04, 2021 at 11:56:47AM +0200, Heinrich Schuchardt wrote: > Dear Tom, > > The following changes since commit 94509b79b13e69c209199af0757afbde8d2ebd6d: > > btrfs: Use default subvolume as filesystem root (2021-09-01 10:11:24 > -0400) > > are available in the Git repository at: > > https://source.denx.de/u-boot/custodians/u-boot-efi.git > tags/efi-2021-10-rc4 > > for you to fetch changes up to 1dfa494610c5469cc28cf1f8538abf4be6c00324: > > efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check > (2021-09-04 09:15:09 +0200) > > > Pull request for efi-2021-10-rc4 > > Documentation: > > Remove invalid reference to configuration variable in UEFI doc > > UEFI: > > Parameter checks for the EFI_TCG2_PROTOCOL > Improve support of preseeding UEFI variables. > Correct the calculation of the size of loaded images. > Allow for UEFI images with zero VirtualSize > > > Heinrich Schuchardt (5): > efi_loader: sections with zero VirtualSize > efi_loader: rounding of image size > efi_loader: don't load signature database from file > efi_loader: efi_auth_var_type for AuditMode, DeployedMode > efi_loader: correct determination of secure boot state > > Masahisa Kojima (3): > efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api > efi_loader: fix boot_service_capability_min calculation > efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check And I don't see Simon's revert in here either. And he asked you about that yesterday: https://lore.kernel.org/r/capnjgz3erdjf0jb9s-cjk6y+feuyrywf0hnkf2trib4dr4u...@mail.gmail.com/ So at this point, are you asserting there is nothing to revert? -- Tom signature.asc Description: PGP signature
Re: Pull request for efi-2021-10-rc4
Am 4. September 2021 15:01:11 MESZ schrieb Tom Rini : >On Sat, Sep 04, 2021 at 11:56:47AM +0200, Heinrich Schuchardt wrote: > >> Dear Tom, >> >> The following changes since commit 94509b79b13e69c209199af0757afbde8d2ebd6d: >> >> btrfs: Use default subvolume as filesystem root (2021-09-01 10:11:24 >> -0400) >> >> are available in the Git repository at: >> >> https://source.denx.de/u-boot/custodians/u-boot-efi.git >> tags/efi-2021-10-rc4 >> >> for you to fetch changes up to 1dfa494610c5469cc28cf1f8538abf4be6c00324: >> >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check >> (2021-09-04 09:15:09 +0200) >> >> >> Pull request for efi-2021-10-rc4 >> >> Documentation: >> >> Remove invalid reference to configuration variable in UEFI doc >> >> UEFI: >> >> Parameter checks for the EFI_TCG2_PROTOCOL >> Improve support of preseeding UEFI variables. >> Correct the calculation of the size of loaded images. >> Allow for UEFI images with zero VirtualSize >> >> >> Heinrich Schuchardt (5): >> efi_loader: sections with zero VirtualSize >> efi_loader: rounding of image size >> efi_loader: don't load signature database from file >> efi_loader: efi_auth_var_type for AuditMode, DeployedMode >> efi_loader: correct determination of secure boot state >> >> Masahisa Kojima (3): >> efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api >> efi_loader: fix boot_service_capability_min calculation >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check > >And I don't see Simon's revert in here either. And he asked you about >that yesterday: >https://lore.kernel.org/r/capnjgz3erdjf0jb9s-cjk6y+feuyrywf0hnkf2trib4dr4u...@mail.gmail.com/ > >So at this point, are you asserting there is nothing to revert? > >-- >Tom Never. Simons "revert" is breaking functionality. The concept for suporting blobs in devicetrees supplied by a prior bootstage has not been defined yet. See the discussion on the ML. Best regards Heinrich
Re: Pull request for efi-2021-10-rc4
On Sat, Sep 04, 2021 at 03:08:38PM +0200, Heinrich Schuchardt wrote: > > > Am 4. September 2021 15:01:11 MESZ schrieb Tom Rini : > >On Sat, Sep 04, 2021 at 11:56:47AM +0200, Heinrich Schuchardt wrote: > > > >> Dear Tom, > >> > >> The following changes since commit > >> 94509b79b13e69c209199af0757afbde8d2ebd6d: > >> > >> btrfs: Use default subvolume as filesystem root (2021-09-01 10:11:24 > >> -0400) > >> > >> are available in the Git repository at: > >> > >> https://source.denx.de/u-boot/custodians/u-boot-efi.git > >> tags/efi-2021-10-rc4 > >> > >> for you to fetch changes up to 1dfa494610c5469cc28cf1f8538abf4be6c00324: > >> > >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check > >> (2021-09-04 09:15:09 +0200) > >> > >> > >> Pull request for efi-2021-10-rc4 > >> > >> Documentation: > >> > >> Remove invalid reference to configuration variable in UEFI doc > >> > >> UEFI: > >> > >> Parameter checks for the EFI_TCG2_PROTOCOL > >> Improve support of preseeding UEFI variables. > >> Correct the calculation of the size of loaded images. > >> Allow for UEFI images with zero VirtualSize > >> > >> > >> Heinrich Schuchardt (5): > >> efi_loader: sections with zero VirtualSize > >> efi_loader: rounding of image size > >> efi_loader: don't load signature database from file > >> efi_loader: efi_auth_var_type for AuditMode, DeployedMode > >> efi_loader: correct determination of secure boot state > >> > >> Masahisa Kojima (3): > >> efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api > >> efi_loader: fix boot_service_capability_min calculation > >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check > > > >And I don't see Simon's revert in here either. And he asked you about > >that yesterday: > >https://lore.kernel.org/r/capnjgz3erdjf0jb9s-cjk6y+feuyrywf0hnkf2trib4dr4u...@mail.gmail.com/ > > > >So at this point, are you asserting there is nothing to revert? > > Never. Simons "revert" is breaking functionality. The concept for suporting > blobs in devicetrees supplied by a prior bootstage has not been defined yet. > > See the discussion on the ML. Yes, I have been following the discussion, which is why I'm wondering why there's still not been a revert in your tree, to bring things back to the state of the previous release, until everyone is in something closer to agreement about how it should be handled moving forward. -- Tom signature.asc Description: PGP signature
Re: [PATCH v2 1/1] configs: qemu-riscvXX_spl_defconfig enable CMD_SBI
On Sat, Sep 4, 2021 at 5:53 PM Heinrich Schuchardt wrote: > > Both for 64bit and 32bit at least on one board we should compile the sbi > command. Enabling it on QEMU will allow to write a test for it. > > Signed-off-by: Heinrich Schuchardt > --- > v2: > additionally enable 32bit > --- > configs/qemu-riscv32_spl_defconfig | 1 + > configs/qemu-riscv64_spl_defconfig | 1 + > 2 files changed, 2 insertions(+) > Reviewed-by: Bin Meng
Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined
On 8/30/21 2:01 PM, Tom Rini wrote: [...] We shouldn't need this at all. LMB and LMB_USE_MAX_REGIONS are both in Kconfig and have the dependencies expressed that way. However, CONFIG_LMB_MEMORY_REGIONS and CONFIG_LMB_RESERVED_REGIONS may be undefined if CONFIG_LMB and !CONFIG_LMB_USE_MAX_REGIONS . They are four different symbols. I'm still not seeing it, sorry. Is there some case where we're trying to access a struct lmb without CONFIG_LMB enabled? See build failure https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331 Ah, progress. Drop from since we already have a forward declaration of struct lmb? But it's not failing without this series too, so what's changing? See 01/14 in this series. Ah, so drop 1/14 then. Why ? That patch is correct. It's not quite right, 1/14 and then 2/14 are papering over the fact that lmb.h, and it's including headers / files, need to be cleaned up so that we don't need to have redundant tests in the header. 1/14 disables LMB and CMD_BDI for tools build, we do not need those, so 1/14 is correct. We don't need to build u-boot at all for tools-only, only the tools-only build target. It's just annoying to exclude the tools-only_defconfig from "sandbox" in CI. So, what exactly is the problem with that 01/14 ? Please elaborate, I believe the patch is correct. You disable LMB in a target that's only building "all" in CI because wasn't ever worth adding ",sandbox" to the all other arches job until perhaps now. Disabling LMB in tools-only_defconfig then exposes that can only be included safely when CONFIG_LMB is set. Adding / extending an #if test in code for something that's already checked for in Kconfig is bad. We spent so much time already removing and shrinking #if tests in the code. So, the patch is correct, the headers need further clean up. No, it's not. The first patch is wrong because disabling CONFIG_LMB is invalid. Please explain why the patch disabling LMB support for tools-only build is invalid. I disagree with this statement, LMB support in tools-only build is useless, because LMB protects parts of running U-Boot from being overwritten. The second patch is conceptually wrong because if we're enforcing a check in C for a dependency that's enforced in Kconfig, we have another problem to investigate. Which I did, LMB is non-optional. Please explain why is LMB non-optional ? I disagree. LMB for tools-only build is useless, hence it should not be enabled. What kind of cleanup of lmb.h do you have in mind ? Remove it from include/image.h and fix any fall-out from that of files that got indirectly when they needed it directly instead. Uh ... that is likely for a separate series, and a big one. Honestly, checking again, I'm not sure LMB=n is valid, ever. Why wouldn't it be ? For tools, LMB=n is perfectly valid. Because it's never valid to disable LMB, LMB is what protects the running U-Boot. We are talking about tools-only build here, not running U-Boot. It's nonsense to build u-boot on tools-only_defconfig but we don't have a way currently to remove "u-boot" from the all target. Maybe once a few more of the hard/tricky CONFIG symbols get migrated to Kconfig we can then set tools-only_defconfig to NOT build u-boot at all. That's how we keep our running U-Boot from being trivially overwritten and a huge number of security issues from being re-opened. Tools are not running U-Boot. At this point, I think you should rework things to stop making CONFIG_LMB be optional, it should be a def_bool y. I disagree, see above. The only reason "tools-only_defconfig" builds a useless u-boot binary today is in CI where it would be more work than it's worth to make CI exclude that from the build list. But if you want to just do that instead, I'll also accept adding -x tools-only to the azure/gitlab jobs that build all other architectures, as tools-only is tested in its own build job, for it's only valid build target. The tools-only build is also used elsewhere, to build just that, tools. [...]
Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined
On Sat, Sep 04, 2021 at 04:03:25PM +0200, Marek Vasut wrote: > On 8/30/21 2:01 PM, Tom Rini wrote: > > [...] > > > > > > > > > > > > > > > > > We shouldn't need this at all. LMB and > > > > > > > > > > > > > > > > LMB_USE_MAX_REGIONS are both in > > > > > > > > > > > > > > > > Kconfig and have the dependencies expressed > > > > > > > > > > > > > > > > that way. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > However, CONFIG_LMB_MEMORY_REGIONS and > > > > > > > > > > > > > > > CONFIG_LMB_RESERVED_REGIONS may be > > > > > > > > > > > > > > > undefined if CONFIG_LMB and > > > > > > > > > > > > > > > !CONFIG_LMB_USE_MAX_REGIONS . They are four > > > > > > > > > > > > > > > different symbols. > > > > > > > > > > > > > > > > > > > > > > > > > > > > I'm still not seeing it, sorry. Is there some case > > > > > > > > > > > > > > where we're trying > > > > > > > > > > > > > > to access a struct lmb without CONFIG_LMB enabled? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > See build failure > > > > > > > > > > > > > https://source.denx.de/u-boot/custodians/u-boot-sh/-/jobs/315331 > > > > > > > > > > > > > > > > > > > > > > > > Ah, progress. Drop from since we > > > > > > > > > > > > already have a > > > > > > > > > > > > forward declaration of struct lmb? But it's not > > > > > > > > > > > > failing without this > > > > > > > > > > > > series too, so what's changing? > > > > > > > > > > > > > > > > > > > > > > See 01/14 in this series. > > > > > > > > > > > > > > > > > > > > Ah, so drop 1/14 then. > > > > > > > > > > > > > > > > > > Why ? That patch is correct. > > > > > > > > > > > > > > > > It's not quite right, 1/14 and then 2/14 are papering over the > > > > > > > > fact that > > > > > > > > lmb.h, and it's including headers / files, need to be cleaned > > > > > > > > up so that > > > > > > > > we don't need to have redundant tests in the header. > > > > > > > > > > > > > > 1/14 disables LMB and CMD_BDI for tools build, we do not need > > > > > > > those, so 1/14 > > > > > > > is correct. > > > > > > > > > > > > We don't need to build u-boot at all for tools-only, only the > > > > > > tools-only > > > > > > build target. It's just annoying to exclude the > > > > > > tools-only_defconfig from > > > > > > "sandbox" in CI. > > > > > > > > > > So, what exactly is the problem with that 01/14 ? Please elaborate, I > > > > > believe the patch is correct. > > > > > > > > You disable LMB in a target that's only building "all" in CI because > > > > wasn't ever worth adding ",sandbox" to the all other arches job until > > > > perhaps now. > > > > > > > > Disabling LMB in tools-only_defconfig then exposes that can only > > > > be included safely when CONFIG_LMB is set. > > > > > > > > Adding / extending an #if test in code for something that's already > > > > checked for in Kconfig is bad. We spent so much time already removing > > > > and shrinking #if tests in the code. > > > > > > So, the patch is correct, the headers need further clean up. > > > > No, it's not. The first patch is wrong because disabling CONFIG_LMB is > > invalid. > > Please explain why the patch disabling LMB support for tools-only build is > invalid. I disagree with this statement, LMB support in tools-only build is > useless, because LMB protects parts of running U-Boot from being > overwritten. > > > The second patch is conceptually wrong because if we're > > enforcing a check in C for a dependency that's enforced in Kconfig, we > > have another problem to investigate. Which I did, LMB is non-optional. > > Please explain why is LMB non-optional ? I disagree. LMB for tools-only > build is useless, hence it should not be enabled. > > > > > > > > What kind of cleanup of lmb.h do you have in mind ? > > > > > > > > > > > > Remove it from include/image.h and fix any fall-out from that of > > > > > > files > > > > > > that got indirectly when they needed it directly instead. > > > > > > > > > > Uh ... that is likely for a separate series, and a big one. > > > > > > > > Honestly, checking again, I'm not sure LMB=n is valid, ever. > > > > > > Why wouldn't it be ? For tools, LMB=n is perfectly valid. > > > > Because it's never valid to disable LMB, LMB is what protects the > > running U-Boot. > > We are talking about tools-only build here, not running U-Boot. > > > It's nonsense to build u-boot on tools-only_defconfig but we don't have > > a way currently to remove "u-boot" from the all target. Maybe once a > > few more of the hard/tricky CONFIG symbols get migrated to Kconfig we > > can then set tools-only_defconfig to NOT build u-boot at all. > > > > > > That's how > > > > we keep our running U-Boot from being trivially overwritten and a huge > > > > number of security issues from being re-opened. > > > > > > Tools are not running U-Boot. > > > > > > > At this point, I think you should rework things to stop making > > > > CONFIG_LMB be optional, it should b
Re: Pull request for efi-2021-10-rc4
On Sat, Sep 04, 2021 at 03:08:38PM +0200, Heinrich Schuchardt wrote: > > > Am 4. September 2021 15:01:11 MESZ schrieb Tom Rini : > >On Sat, Sep 04, 2021 at 11:56:47AM +0200, Heinrich Schuchardt wrote: > > > >> Dear Tom, > >> > >> The following changes since commit > >> 94509b79b13e69c209199af0757afbde8d2ebd6d: > >> > >> btrfs: Use default subvolume as filesystem root (2021-09-01 10:11:24 > >> -0400) > >> > >> are available in the Git repository at: > >> > >> https://source.denx.de/u-boot/custodians/u-boot-efi.git > >> tags/efi-2021-10-rc4 > >> > >> for you to fetch changes up to 1dfa494610c5469cc28cf1f8538abf4be6c00324: > >> > >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check > >> (2021-09-04 09:15:09 +0200) > >> > >> > >> Pull request for efi-2021-10-rc4 > >> > >> Documentation: > >> > >> Remove invalid reference to configuration variable in UEFI doc > >> > >> UEFI: > >> > >> Parameter checks for the EFI_TCG2_PROTOCOL > >> Improve support of preseeding UEFI variables. > >> Correct the calculation of the size of loaded images. > >> Allow for UEFI images with zero VirtualSize > >> > >> > >> Heinrich Schuchardt (5): > >> efi_loader: sections with zero VirtualSize > >> efi_loader: rounding of image size > >> efi_loader: don't load signature database from file > >> efi_loader: efi_auth_var_type for AuditMode, DeployedMode > >> efi_loader: correct determination of secure boot state > >> > >> Masahisa Kojima (3): > >> efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api > >> efi_loader: fix boot_service_capability_min calculation > >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check > > > >And I don't see Simon's revert in here either. And he asked you about > >that yesterday: > >https://lore.kernel.org/r/capnjgz3erdjf0jb9s-cjk6y+feuyrywf0hnkf2trib4dr4u...@mail.gmail.com/ > > > >So at this point, are you asserting there is nothing to revert? > > Never. Simons "revert" is breaking functionality. The concept for suporting > blobs in devicetrees supplied by a prior bootstage has not been defined yet. And to be clearer, reverting something that was introduced in one rc in a later rc isn't breaking functionality. U-Boot releases (well, the non-rc ones for sure) are on a very regular schedule. External projects may not depend on some feature introduced at -rcN unless they're willing to accept that some changes could happen before release. -- Tom signature.asc Description: PGP signature
Re: [PATCH v2 1/1] riscv: show code leading to exception
On Sat, Sep 4, 2021 at 4:37 PM Heinrich Schuchardt wrote: > > To make analyzing exceptions easier output the code that leads to it. > We already do the same on the ARM platform. > > Here is an example: > > => exception ebreak > Unhandled exception: Breakpoint > EPC: 8ff5d50e RA: 8ff5d62c TVAL: > EPC: 8020b50e RA: 8020b62c reloc adjusted > > Code: 2785 0693 07a0 dce3 fef6 47a5 d563 00e7 (9002) > > To disassemble the code we can use the decodecode script: > > $ echo 'Code: 2785 0693 07a0 dce3 fef6 47a5 d563 00e7 (9002)' | \ > CROSS_COMPILE=riscv64-linux-gnu- scripts/decodecode > > Code: 2785 0693 07a0 dce3 fef6 47a5 d563 00e7 (9002) > All code > >0: 2785addiw a5,a5,1 >2: 07a00693li a3,122 >6: fef6dce3bge a3,a5,0xfffe >a: 47a5li a5,9 >c: 00e7d563bge a5,a4,0x16 > 10:* 9002ebreak <-- trapping instruction > ... > > Code starting with the faulting instruction > === >0: 9002ebreak > ... > > As it is not always clear if the first 16 bits are at the start or in the > middle of a 32bit instruction it may become necessary to strip the first > u16 from the output before calling decodecode to get the correct > disassembled code. > > Signed-off-by: Heinrich Schuchardt > --- > v2: > remove support for instructions longer than 32 bit as these are > not yet specified > --- > arch/riscv/lib/interrupts.c | 33 + > 1 file changed, 33 insertions(+) > Reviewed-by: Bin Meng
Re: [PATCH v2 1/1] riscv: show code leading to exception
On 9/4/21 4:36 AM, Heinrich Schuchardt wrote:
To make analyzing exceptions easier output the code that leads to it.
We already do the same on the ARM platform.
Here is an example:
=> exception ebreak
Unhandled exception: Breakpoint
EPC: 8ff5d50e RA: 8ff5d62c TVAL:
EPC: 8020b50e RA: 8020b62c reloc adjusted
Code: 2785 0693 07a0 dce3 fef6 47a5 d563 00e7 (9002)
To disassemble the code we can use the decodecode script:
$ echo 'Code: 2785 0693 07a0 dce3 fef6 47a5 d563 00e7 (9002)' | \
CROSS_COMPILE=riscv64-linux-gnu- scripts/decodecode
Code: 2785 0693 07a0 dce3 fef6 47a5 d563 00e7 (9002)
All code
0: 2785addiw a5,a5,1
2: 07a00693li a3,122
6: fef6dce3bge a3,a5,0xfffe
a: 47a5li a5,9
c: 00e7d563bge a5,a4,0x16
10:* 9002ebreak <-- trapping instruction
...
Code starting with the faulting instruction
===
0: 9002ebreak
...
As it is not always clear if the first 16 bits are at the start or in the
middle of a 32bit instruction it may become necessary to strip the first
u16 from the output before calling decodecode to get the correct
disassembled code.
Signed-off-by: Heinrich Schuchardt
---
v2:
remove support for instructions longer than 32 bit as these are
not yet specified
---
arch/riscv/lib/interrupts.c | 33 +
1 file changed, 33 insertions(+)
diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c
index 7525c152b8..100be2e966 100644
--- a/arch/riscv/lib/interrupts.c
+++ b/arch/riscv/lib/interrupts.c
@@ -51,6 +51,38 @@ static void show_regs(struct pt_regs *regs)
#endif
}
+/**
+ * instr_len() - get instruction length
+ *
+ * @i: low 16 bits of the instruction
+ * Return: number of u16 in instruction
+ */
+static int instr_len(u16 i)
+{
+ if ((i & 0x03) != 0x03)
+ return 1;
+ /* Instructions with more than 32 bits are not yet specified */
+ return 2;
+}
+
+/**
+ * show_code() - display code leading to exception
+ *
+ * @epc: program counter
+ */
+static void show_code(ulong epc)
+{
+ u16 *pos = (u16 *)(epc & ~1UL);
+ int i, len = instr_len(*pos);
+
+ printf("\nCode: ");
+ for (i = -8; i; ++i)
+ printf("%04x ", pos[i]);
+ printf("(");
+ for (i = 0; i < len; ++i)
+ printf("%04x%s", pos[i], i + 1 == len ? ")\n" : " ");
+}
+
static void _exit_trap(ulong code, ulong epc, ulong tval, struct pt_regs
*regs)
{
static const char * const exception_code[] = {
@@ -85,6 +117,7 @@ static void _exit_trap(ulong code, ulong epc, ulong tval,
struct pt_regs *regs)
epc - gd->reloc_off, regs->ra - gd->reloc_off);
show_regs(regs);
+ show_code(epc);
show_efi_loaded_images(epc);
panic("\n");
}
--
2.30.2
Reviewed-by: Sean Anderson
Re: [PATCH 19/23] i2c: Convert CONFIG_POWER_I2C et al to Kconfig
On Sun, Aug 08, 2021 at 12:20:27PM -0600, Simon Glass wrote: > This converts the following to Kconfig: >CONFIG_POWER_I2C >CONFIG_POWER_LEGACY > > They are handled at the same time due to a dependency between them. > Update the Makefile rule to use legacy power only in U-Boot proper. > > Unfortunately a separate rule is needed in SPL to be able to build legacy > power. All of this can be cleaned up when boards are migrated or removed. > > Signed-off-by: Simon Glass > Reviewed-by: Jaehoon Chung I'm taking care of this now, but JFYI, this is actually a tricky conversion where the "hack print" tool I've posted before is required, along with running it with and without CONFIG_SPL_BUILD defined. At issue here is that a number of platforms play games with only setting CONFIG_POWER in SPL, really but moveconfig.py doesn't catch it. You can see this with the buildman size/bloat functions and that main U-Boot nows includes drivers/power/power_core.c and its non-DM non-configurable pmic cmd. -- Tom signature.asc Description: PGP signature
Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined
On 9/4/21 4:10 PM, Tom Rini wrote: [...] At this point, I think you should rework things to stop making CONFIG_LMB be optional, it should be a def_bool y. I disagree, see above. The only reason "tools-only_defconfig" builds a useless u-boot binary today is in CI where it would be more work than it's worth to make CI exclude that from the build list. But if you want to just do that instead, I'll also accept adding -x tools-only to the azure/gitlab jobs that build all other architectures, as tools-only is tested in its own build job, for it's only valid build target. The tools-only build is also used elsewhere, to build just that, tools. I've repeatedly explained myself and what I'm looking for in v2 of this series. I will summarize one last time. The "tools-only_defconfig" is for tools, only. Building anything other than the "tools-only" target isn't useful. In U-Boot itself, LMB is required as that is how we prevent a number of CVEs from being trivial to exploit. v2 of this series needs to drop patches 1 and 2 of v1 of this series. It can further do any of: 1. Nothing else. 2. Add tools-only to the exclude list in the "build everything else" CI job. 3. Make CONFIG_LMB be def_bool y. If tools-only is for tools, only, then why should it enable LMB ? The tools are userspace tools, they do not need LMB, and so LMB can be disabled. This is the part which is unclear to me.
Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined
On Sat, Sep 04, 2021 at 05:15:45PM +0200, Marek Vasut wrote: > On 9/4/21 4:10 PM, Tom Rini wrote: > [...] > > > > > > > At this point, I think you should rework things to stop making > > > > > > CONFIG_LMB be optional, it should be a def_bool y. > > > > > > > > > > I disagree, see above. > > > > > > > > The only reason "tools-only_defconfig" builds a useless u-boot binary > > > > today is in CI where it would be more work than it's worth to make CI > > > > exclude that from the build list. But if you want to just do that > > > > instead, I'll also accept adding -x tools-only to the azure/gitlab jobs > > > > that build all other architectures, as tools-only is tested in its own > > > > build job, for it's only valid build target. > > > > > > The tools-only build is also used elsewhere, to build just that, tools. > > > > I've repeatedly explained myself and what I'm looking for in v2 of this > > series. I will summarize one last time. The "tools-only_defconfig" is > > for tools, only. Building anything other than the "tools-only" target > > isn't useful. In U-Boot itself, LMB is required as that is how we > > prevent a number of CVEs from being trivial to exploit. v2 of this > > series needs to drop patches 1 and 2 of v1 of this series. It can > > further do any of: > > 1. Nothing else. > > 2. Add tools-only to the exclude list in the "build everything else" CI > > job. > > 3. Make CONFIG_LMB be def_bool y. > > If tools-only is for tools, only, then why should it enable LMB ? > The tools are userspace tools, they do not need LMB, and so LMB can be > disabled. > > This is the part which is unclear to me. I don't know why it's unclear to you at this point, sorry. -- Tom signature.asc Description: PGP signature
Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined
On 9/4/21 5:17 PM, Tom Rini wrote: On Sat, Sep 04, 2021 at 05:15:45PM +0200, Marek Vasut wrote: On 9/4/21 4:10 PM, Tom Rini wrote: [...] At this point, I think you should rework things to stop making CONFIG_LMB be optional, it should be a def_bool y. I disagree, see above. The only reason "tools-only_defconfig" builds a useless u-boot binary today is in CI where it would be more work than it's worth to make CI exclude that from the build list. But if you want to just do that instead, I'll also accept adding -x tools-only to the azure/gitlab jobs that build all other architectures, as tools-only is tested in its own build job, for it's only valid build target. The tools-only build is also used elsewhere, to build just that, tools. I've repeatedly explained myself and what I'm looking for in v2 of this series. I will summarize one last time. The "tools-only_defconfig" is for tools, only. Building anything other than the "tools-only" target isn't useful. In U-Boot itself, LMB is required as that is how we prevent a number of CVEs from being trivial to exploit. v2 of this series needs to drop patches 1 and 2 of v1 of this series. It can further do any of: 1. Nothing else. 2. Add tools-only to the exclude list in the "build everything else" CI job. 3. Make CONFIG_LMB be def_bool y. If tools-only is for tools, only, then why should it enable LMB ? The tools are userspace tools, they do not need LMB, and so LMB can be disabled. This is the part which is unclear to me. I don't know why it's unclear to you at this point, sorry. Well why exactly does a userspace program require LMB enabled ? What does LMB protect in there ? obviously not U-Boot.
Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined
On Sat, Sep 04, 2021 at 06:05:50PM +0200, Marek Vasut wrote: > On 9/4/21 5:17 PM, Tom Rini wrote: > > On Sat, Sep 04, 2021 at 05:15:45PM +0200, Marek Vasut wrote: > > > On 9/4/21 4:10 PM, Tom Rini wrote: > > > [...] > > > > > > > > > > > At this point, I think you should rework things to stop making > > > > > > > > CONFIG_LMB be optional, it should be a def_bool y. > > > > > > > > > > > > > > I disagree, see above. > > > > > > > > > > > > The only reason "tools-only_defconfig" builds a useless u-boot > > > > > > binary > > > > > > today is in CI where it would be more work than it's worth to make > > > > > > CI > > > > > > exclude that from the build list. But if you want to just do that > > > > > > instead, I'll also accept adding -x tools-only to the azure/gitlab > > > > > > jobs > > > > > > that build all other architectures, as tools-only is tested in its > > > > > > own > > > > > > build job, for it's only valid build target. > > > > > > > > > > The tools-only build is also used elsewhere, to build just that, > > > > > tools. > > > > > > > > I've repeatedly explained myself and what I'm looking for in v2 of this > > > > series. I will summarize one last time. The "tools-only_defconfig" is > > > > for tools, only. Building anything other than the "tools-only" target > > > > isn't useful. In U-Boot itself, LMB is required as that is how we > > > > prevent a number of CVEs from being trivial to exploit. v2 of this > > > > series needs to drop patches 1 and 2 of v1 of this series. It can > > > > further do any of: > > > > 1. Nothing else. > > > > 2. Add tools-only to the exclude list in the "build everything else" CI > > > > job. > > > > 3. Make CONFIG_LMB be def_bool y. > > > > > > If tools-only is for tools, only, then why should it enable LMB ? > > > The tools are userspace tools, they do not need LMB, and so LMB can be > > > disabled. > > > > > > This is the part which is unclear to me. > > > > I don't know why it's unclear to you at this point, sorry. > > Well why exactly does a userspace program require LMB enabled ? > What does LMB protect in there ? obviously not U-Boot. I feel like you've lost the thread. -- Tom signature.asc Description: PGP signature
Re: [PATCH 3/5] arm: Disable ATAGs support
Hi Tom,
On Mon, Aug 30, 2021 at 09:16:30AM -0400, Tom Rini wrote:
> With the exception of nokia_rx51 and icnova-a20-swac, disable ATAG
> support. A large number of platforms had enabled support but never
> supported a kernel so old as to require it. Further, some platforms are
> old enough to support both, but are well supported by devicetree
> booting, and have been for a number of years. This is because some of
> the ATAGs related functions have been re-used to provide the same kind
> of information, but for devicetree or just generally to inform the user.
> When needed still, rename these functions to get_board_revision()
> instead, to avoid conflicts. In other cases, these functions were
> simply unused, so drop them.
>
> Cc: Andre Przywara
> Cc: Jagan Teki
> Cc: Phil Sutter
> Cc: Stefan Bosch
> Signed-off-by: Tom Rini
> [...]
> ---
> include/configs/stemmy.h | 3 --
> [...]
> +++ b/include/configs/stemmy.h
> @@ -23,7 +23,4 @@
> #define CONFIG_SYS_L2_PL310
> #define CONFIG_SYS_PL310_BASE0xa0412000
>
> -/* Generate initrd atag for downstream kernel (others are copied in
> stemmy.c) */
> -#define CONFIG_INITRD_TAG
> -
> #endif
Please keep this option for the stemmy board. It's still used
occasionally to boot the original downstream/vendor kernel which is
currently still necessary to charge the battery on these devices.
See commit e2f82f93 ("board: stemmy: Copy atags for booting
downstream/vendor kernel") [1] for details.
Thanks!
Stephan
[1]:
https://source.denx.de/u-boot/u-boot/-/commit/e2f82f93f885dd3dd3ad74e98182add8ccd863e7.patch
Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined
On 9/4/21 6:09 PM, Tom Rini wrote: On Sat, Sep 04, 2021 at 06:05:50PM +0200, Marek Vasut wrote: On 9/4/21 5:17 PM, Tom Rini wrote: On Sat, Sep 04, 2021 at 05:15:45PM +0200, Marek Vasut wrote: On 9/4/21 4:10 PM, Tom Rini wrote: [...] At this point, I think you should rework things to stop making CONFIG_LMB be optional, it should be a def_bool y. I disagree, see above. The only reason "tools-only_defconfig" builds a useless u-boot binary today is in CI where it would be more work than it's worth to make CI exclude that from the build list. But if you want to just do that instead, I'll also accept adding -x tools-only to the azure/gitlab jobs that build all other architectures, as tools-only is tested in its own build job, for it's only valid build target. The tools-only build is also used elsewhere, to build just that, tools. I've repeatedly explained myself and what I'm looking for in v2 of this series. I will summarize one last time. The "tools-only_defconfig" is for tools, only. Building anything other than the "tools-only" target isn't useful. In U-Boot itself, LMB is required as that is how we prevent a number of CVEs from being trivial to exploit. v2 of this series needs to drop patches 1 and 2 of v1 of this series. It can further do any of: 1. Nothing else. 2. Add tools-only to the exclude list in the "build everything else" CI job. 3. Make CONFIG_LMB be def_bool y. If tools-only is for tools, only, then why should it enable LMB ? The tools are userspace tools, they do not need LMB, and so LMB can be disabled. This is the part which is unclear to me. I don't know why it's unclear to you at this point, sorry. Well why exactly does a userspace program require LMB enabled ? What does LMB protect in there ? obviously not U-Boot. I feel like you've lost the thread. Can you please answer my questions above ? This is exactly what patch 1/14 is about -- disable LMB for tools build, because it is useless for tools build.
Re: [PATCH 3/5] arm: Disable ATAGs support
On Sat, Sep 04, 2021 at 06:29:50PM +0200, Stephan Gerhold wrote:
> Hi Tom,
>
> On Mon, Aug 30, 2021 at 09:16:30AM -0400, Tom Rini wrote:
> > With the exception of nokia_rx51 and icnova-a20-swac, disable ATAG
> > support. A large number of platforms had enabled support but never
> > supported a kernel so old as to require it. Further, some platforms are
> > old enough to support both, but are well supported by devicetree
> > booting, and have been for a number of years. This is because some of
> > the ATAGs related functions have been re-used to provide the same kind
> > of information, but for devicetree or just generally to inform the user.
> > When needed still, rename these functions to get_board_revision()
> > instead, to avoid conflicts. In other cases, these functions were
> > simply unused, so drop them.
> >
> > Cc: Andre Przywara
> > Cc: Jagan Teki
> > Cc: Phil Sutter
> > Cc: Stefan Bosch
> > Signed-off-by: Tom Rini
> > [...]
> > ---
> > include/configs/stemmy.h | 3 --
> > [...]
> > +++ b/include/configs/stemmy.h
> > @@ -23,7 +23,4 @@
> > #define CONFIG_SYS_L2_PL310
> > #define CONFIG_SYS_PL310_BASE 0xa0412000
> >
> > -/* Generate initrd atag for downstream kernel (others are copied in
> > stemmy.c) */
> > -#define CONFIG_INITRD_TAG
> > -
> > #endif
>
> Please keep this option for the stemmy board. It's still used
> occasionally to boot the original downstream/vendor kernel which is
> currently still necessary to charge the battery on these devices.
>
> See commit e2f82f93 ("board: stemmy: Copy atags for booting
> downstream/vendor kernel") [1] for details.
OK, I'll re-work this to also keep ATAGS support on stemmy when merging,
thanks!
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined
[trimming the CC list] On Sat, Sep 04, 2021 at 06:49:03PM +0200, Marek Vasut wrote: > On 9/4/21 6:09 PM, Tom Rini wrote: > > On Sat, Sep 04, 2021 at 06:05:50PM +0200, Marek Vasut wrote: > > > On 9/4/21 5:17 PM, Tom Rini wrote: > > > > On Sat, Sep 04, 2021 at 05:15:45PM +0200, Marek Vasut wrote: > > > > > On 9/4/21 4:10 PM, Tom Rini wrote: > > > > > [...] > > > > > > > > > > > > > > > At this point, I think you should rework things to stop > > > > > > > > > > making > > > > > > > > > > CONFIG_LMB be optional, it should be a def_bool y. > > > > > > > > > > > > > > > > > > I disagree, see above. > > > > > > > > > > > > > > > > The only reason "tools-only_defconfig" builds a useless u-boot > > > > > > > > binary > > > > > > > > today is in CI where it would be more work than it's worth to > > > > > > > > make CI > > > > > > > > exclude that from the build list. But if you want to just do > > > > > > > > that > > > > > > > > instead, I'll also accept adding -x tools-only to the > > > > > > > > azure/gitlab jobs > > > > > > > > that build all other architectures, as tools-only is tested in > > > > > > > > its own > > > > > > > > build job, for it's only valid build target. > > > > > > > > > > > > > > The tools-only build is also used elsewhere, to build just that, > > > > > > > tools. > > > > > > > > > > > > I've repeatedly explained myself and what I'm looking for in v2 of > > > > > > this > > > > > > series. I will summarize one last time. The > > > > > > "tools-only_defconfig" is > > > > > > for tools, only. Building anything other than the "tools-only" > > > > > > target > > > > > > isn't useful. In U-Boot itself, LMB is required as that is how we > > > > > > prevent a number of CVEs from being trivial to exploit. v2 of this > > > > > > series needs to drop patches 1 and 2 of v1 of this series. It can > > > > > > further do any of: > > > > > > 1. Nothing else. > > > > > > 2. Add tools-only to the exclude list in the "build everything > > > > > > else" CI > > > > > > job. > > > > > > 3. Make CONFIG_LMB be def_bool y. > > > > > > > > > > If tools-only is for tools, only, then why should it enable LMB ? > > > > > The tools are userspace tools, they do not need LMB, and so LMB can be > > > > > disabled. > > > > > > > > > > This is the part which is unclear to me. > > > > > > > > I don't know why it's unclear to you at this point, sorry. > > > > > > Well why exactly does a userspace program require LMB enabled ? > > > What does LMB protect in there ? obviously not U-Boot. > > > > I feel like you've lost the thread. > > Can you please answer my questions above ? I have. -- Tom signature.asc Description: PGP signature
Re: Pull request for efi-2021-10-rc4
Am 4. September 2021 16:37:22 MESZ schrieb Tom Rini : >On Sat, Sep 04, 2021 at 03:08:38PM +0200, Heinrich Schuchardt wrote: >> >> >> Am 4. September 2021 15:01:11 MESZ schrieb Tom Rini : >> >On Sat, Sep 04, 2021 at 11:56:47AM +0200, Heinrich Schuchardt wrote: >> > >> >> Dear Tom, >> >> >> >> The following changes since commit >> >> 94509b79b13e69c209199af0757afbde8d2ebd6d: >> >> >> >> btrfs: Use default subvolume as filesystem root (2021-09-01 10:11:24 >> >> -0400) >> >> >> >> are available in the Git repository at: >> >> >> >> https://source.denx.de/u-boot/custodians/u-boot-efi.git >> >> tags/efi-2021-10-rc4 >> >> >> >> for you to fetch changes up to 1dfa494610c5469cc28cf1f8538abf4be6c00324: >> >> >> >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check >> >> (2021-09-04 09:15:09 +0200) >> >> >> >> >> >> Pull request for efi-2021-10-rc4 >> >> >> >> Documentation: >> >> >> >> Remove invalid reference to configuration variable in UEFI doc >> >> >> >> UEFI: >> >> >> >> Parameter checks for the EFI_TCG2_PROTOCOL >> >> Improve support of preseeding UEFI variables. >> >> Correct the calculation of the size of loaded images. >> >> Allow for UEFI images with zero VirtualSize >> >> >> >> >> >> Heinrich Schuchardt (5): >> >> efi_loader: sections with zero VirtualSize >> >> efi_loader: rounding of image size >> >> efi_loader: don't load signature database from file >> >> efi_loader: efi_auth_var_type for AuditMode, DeployedMode >> >> efi_loader: correct determination of secure boot state >> >> >> >> Masahisa Kojima (3): >> >> efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api >> >> efi_loader: fix boot_service_capability_min calculation >> >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check >> > >> >And I don't see Simon's revert in here either. And he asked you about >> >that yesterday: >> >https://lore.kernel.org/r/capnjgz3erdjf0jb9s-cjk6y+feuyrywf0hnkf2trib4dr4u...@mail.gmail.com/ >> > >> >So at this point, are you asserting there is nothing to revert? >> >> Never. Simons "revert" is breaking functionality. The concept for suporting >> blobs in devicetrees supplied by a prior bootstage has not been defined yet. > >And to be clearer, reverting something that was introduced in one rc in >a later rc isn't breaking functionality. U-Boot releases (well, the >non-rc ones for sure) are on a very regular schedule. External projects >may not depend on some feature introduced at -rcN unless they're willing >to accept that some changes could happen before release. > There is no value delivered by Simon's series. Neither does the image get smaller nor does it fix anything. If he wants to enforce a design, it must work for all use cases. But this requires some conceptual work. We already have other blobs that are not in decicetrees and Simon never complained. So why picking on this blob? Best regards Heinrich
Re: Pull request for efi-2021-10-rc4
On Sat, Sep 04, 2021 at 07:03:48PM +0200, Heinrich Schuchardt wrote: > > > Am 4. September 2021 16:37:22 MESZ schrieb Tom Rini : > >On Sat, Sep 04, 2021 at 03:08:38PM +0200, Heinrich Schuchardt wrote: > >> > >> > >> Am 4. September 2021 15:01:11 MESZ schrieb Tom Rini : > >> >On Sat, Sep 04, 2021 at 11:56:47AM +0200, Heinrich Schuchardt wrote: > >> > > >> >> Dear Tom, > >> >> > >> >> The following changes since commit > >> >> 94509b79b13e69c209199af0757afbde8d2ebd6d: > >> >> > >> >> btrfs: Use default subvolume as filesystem root (2021-09-01 10:11:24 > >> >> -0400) > >> >> > >> >> are available in the Git repository at: > >> >> > >> >> https://source.denx.de/u-boot/custodians/u-boot-efi.git > >> >> tags/efi-2021-10-rc4 > >> >> > >> >> for you to fetch changes up to 1dfa494610c5469cc28cf1f8538abf4be6c00324: > >> >> > >> >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check > >> >> (2021-09-04 09:15:09 +0200) > >> >> > >> >> > >> >> Pull request for efi-2021-10-rc4 > >> >> > >> >> Documentation: > >> >> > >> >> Remove invalid reference to configuration variable in UEFI doc > >> >> > >> >> UEFI: > >> >> > >> >> Parameter checks for the EFI_TCG2_PROTOCOL > >> >> Improve support of preseeding UEFI variables. > >> >> Correct the calculation of the size of loaded images. > >> >> Allow for UEFI images with zero VirtualSize > >> >> > >> >> > >> >> Heinrich Schuchardt (5): > >> >> efi_loader: sections with zero VirtualSize > >> >> efi_loader: rounding of image size > >> >> efi_loader: don't load signature database from file > >> >> efi_loader: efi_auth_var_type for AuditMode, DeployedMode > >> >> efi_loader: correct determination of secure boot state > >> >> > >> >> Masahisa Kojima (3): > >> >> efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api > >> >> efi_loader: fix boot_service_capability_min calculation > >> >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check > >> > > >> >And I don't see Simon's revert in here either. And he asked you about > >> >that yesterday: > >> >https://lore.kernel.org/r/capnjgz3erdjf0jb9s-cjk6y+feuyrywf0hnkf2trib4dr4u...@mail.gmail.com/ > >> > > >> >So at this point, are you asserting there is nothing to revert? > >> > >> Never. Simons "revert" is breaking functionality. The concept for > >> suporting blobs in devicetrees supplied by a prior bootstage has not been > >> defined yet. > > > >And to be clearer, reverting something that was introduced in one rc in > >a later rc isn't breaking functionality. U-Boot releases (well, the > >non-rc ones for sure) are on a very regular schedule. External projects > >may not depend on some feature introduced at -rcN unless they're willing > >to accept that some changes could happen before release. > > > > There is no value delivered by Simon's series. Neither does the image get > smaller nor does it fix anything. If he wants to enforce a design, it must > work for all use cases. But this requires some conceptual work. Yes, and what's the rush to not do the conceptual work? If I recall part of the thread correctly, yes, Simon didn't get his objections in before the patches were merged, but it was early enough in the release cycle that taking a step back and reverting was a reasonable request. What he had said wouldn't have changed if he had gotten the email out a few days earlier. So yes, please merge Simon's revert, or post and merge new more minimal revert that brings things to the same functional end. There are objections to this implementation, and thus far Simon has been responding all of the requests to better clarify all of the related code and concepts that have been asked of him, so that in the end an implementation that fulfills all of the technical requirements can be created, that hopefully leaves all parties satisfied. -- Tom signature.asc Description: PGP signature
Re: Pull request for efi-2021-10-rc4
Am 4. September 2021 19:39:49 MESZ schrieb Tom Rini : >On Sat, Sep 04, 2021 at 07:03:48PM +0200, Heinrich Schuchardt wrote: >> >> >> Am 4. September 2021 16:37:22 MESZ schrieb Tom Rini : >> >On Sat, Sep 04, 2021 at 03:08:38PM +0200, Heinrich Schuchardt wrote: >> >> >> >> >> >> Am 4. September 2021 15:01:11 MESZ schrieb Tom Rini : >> >> >On Sat, Sep 04, 2021 at 11:56:47AM +0200, Heinrich Schuchardt wrote: >> >> > >> >> >> Dear Tom, >> >> >> >> >> >> The following changes since commit >> >> >> 94509b79b13e69c209199af0757afbde8d2ebd6d: >> >> >> >> >> >> btrfs: Use default subvolume as filesystem root (2021-09-01 10:11:24 >> >> >> -0400) >> >> >> >> >> >> are available in the Git repository at: >> >> >> >> >> >> https://source.denx.de/u-boot/custodians/u-boot-efi.git >> >> >> tags/efi-2021-10-rc4 >> >> >> >> >> >> for you to fetch changes up to >> >> >> 1dfa494610c5469cc28cf1f8538abf4be6c00324: >> >> >> >> >> >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check >> >> >> (2021-09-04 09:15:09 +0200) >> >> >> >> >> >> >> >> >> Pull request for efi-2021-10-rc4 >> >> >> >> >> >> Documentation: >> >> >> >> >> >> Remove invalid reference to configuration variable in UEFI doc >> >> >> >> >> >> UEFI: >> >> >> >> >> >> Parameter checks for the EFI_TCG2_PROTOCOL >> >> >> Improve support of preseeding UEFI variables. >> >> >> Correct the calculation of the size of loaded images. >> >> >> Allow for UEFI images with zero VirtualSize >> >> >> >> >> >> >> >> >> Heinrich Schuchardt (5): >> >> >> efi_loader: sections with zero VirtualSize >> >> >> efi_loader: rounding of image size >> >> >> efi_loader: don't load signature database from file >> >> >> efi_loader: efi_auth_var_type for AuditMode, DeployedMode >> >> >> efi_loader: correct determination of secure boot state >> >> >> >> >> >> Masahisa Kojima (3): >> >> >> efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL api >> >> >> efi_loader: fix boot_service_capability_min calculation >> >> >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check >> >> > >> >> >And I don't see Simon's revert in here either. And he asked you about >> >> >that yesterday: >> >> >https://lore.kernel.org/r/capnjgz3erdjf0jb9s-cjk6y+feuyrywf0hnkf2trib4dr4u...@mail.gmail.com/ >> >> > >> >> >So at this point, are you asserting there is nothing to revert? >> >> >> >> Never. Simons "revert" is breaking functionality. The concept for >> >> suporting blobs in devicetrees supplied by a prior bootstage has not been >> >> defined yet. >> > >> >And to be clearer, reverting something that was introduced in one rc in >> >a later rc isn't breaking functionality. U-Boot releases (well, the >> >non-rc ones for sure) are on a very regular schedule. External projects >> >may not depend on some feature introduced at -rcN unless they're willing >> >to accept that some changes could happen before release. >> > >> >> There is no value delivered by Simon's series. Neither does the image get >> smaller nor does it fix anything. If he wants to enforce a design, it must >> work for all use cases. But this requires some conceptual work. > >Yes, and what's the rush to not do the conceptual work? If I recall >part of the thread correctly, yes, Simon didn't get his objections in >before the patches were merged, but it was early enough in the release >cycle that taking a step back and reverting was a reasonable request. >What he had said wouldn't have changed if he had gotten the email out a >few days earlier. > >So yes, please merge Simon's revert, or post and merge new more minimal >revert that brings things to the same functional end. There are >objections to this implementation, and thus far Simon has been >responding all of the requests to better clarify all of the related code >and concepts that have been asked of him, so that in the end an >implementation that fulfills all of the technical requirements can be >created, that hopefully leaves all parties satisfied. > There is nothing wrong with the current code. It is Simon's concept of blobs in devicetrees that is borked in that it ignores QEMU and any board that gets the DT from a prior boot stage. Simon's patches have no functional end. So what do you mean by "same functional end"? Best regards Heinrich
Re: Pull request for efi-2021-10-rc4
On Sat, Sep 04, 2021 at 08:02:49PM +0200, Heinrich Schuchardt wrote: > > > Am 4. September 2021 19:39:49 MESZ schrieb Tom Rini : > >On Sat, Sep 04, 2021 at 07:03:48PM +0200, Heinrich Schuchardt wrote: > >> > >> > >> Am 4. September 2021 16:37:22 MESZ schrieb Tom Rini : > >> >On Sat, Sep 04, 2021 at 03:08:38PM +0200, Heinrich Schuchardt wrote: > >> >> > >> >> > >> >> Am 4. September 2021 15:01:11 MESZ schrieb Tom Rini > >> >> : > >> >> >On Sat, Sep 04, 2021 at 11:56:47AM +0200, Heinrich Schuchardt wrote: > >> >> > > >> >> >> Dear Tom, > >> >> >> > >> >> >> The following changes since commit > >> >> >> 94509b79b13e69c209199af0757afbde8d2ebd6d: > >> >> >> > >> >> >> btrfs: Use default subvolume as filesystem root (2021-09-01 > >> >> >> 10:11:24 > >> >> >> -0400) > >> >> >> > >> >> >> are available in the Git repository at: > >> >> >> > >> >> >> https://source.denx.de/u-boot/custodians/u-boot-efi.git > >> >> >> tags/efi-2021-10-rc4 > >> >> >> > >> >> >> for you to fetch changes up to > >> >> >> 1dfa494610c5469cc28cf1f8538abf4be6c00324: > >> >> >> > >> >> >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check > >> >> >> (2021-09-04 09:15:09 +0200) > >> >> >> > >> >> >> > >> >> >> Pull request for efi-2021-10-rc4 > >> >> >> > >> >> >> Documentation: > >> >> >> > >> >> >> Remove invalid reference to configuration variable in UEFI doc > >> >> >> > >> >> >> UEFI: > >> >> >> > >> >> >> Parameter checks for the EFI_TCG2_PROTOCOL > >> >> >> Improve support of preseeding UEFI variables. > >> >> >> Correct the calculation of the size of loaded images. > >> >> >> Allow for UEFI images with zero VirtualSize > >> >> >> > >> >> >> > >> >> >> Heinrich Schuchardt (5): > >> >> >> efi_loader: sections with zero VirtualSize > >> >> >> efi_loader: rounding of image size > >> >> >> efi_loader: don't load signature database from file > >> >> >> efi_loader: efi_auth_var_type for AuditMode, DeployedMode > >> >> >> efi_loader: correct determination of secure boot state > >> >> >> > >> >> >> Masahisa Kojima (3): > >> >> >> efi_loader: add missing parameter check for EFI_TCG2_PROTOCOL > >> >> >> api > >> >> >> efi_loader: fix boot_service_capability_min calculation > >> >> >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter > >> >> >> check > >> >> > > >> >> >And I don't see Simon's revert in here either. And he asked you about > >> >> >that yesterday: > >> >> >https://lore.kernel.org/r/capnjgz3erdjf0jb9s-cjk6y+feuyrywf0hnkf2trib4dr4u...@mail.gmail.com/ > >> >> > > >> >> >So at this point, are you asserting there is nothing to revert? > >> >> > >> >> Never. Simons "revert" is breaking functionality. The concept for > >> >> suporting blobs in devicetrees supplied by a prior bootstage has not > >> >> been defined yet. > >> > > >> >And to be clearer, reverting something that was introduced in one rc in > >> >a later rc isn't breaking functionality. U-Boot releases (well, the > >> >non-rc ones for sure) are on a very regular schedule. External projects > >> >may not depend on some feature introduced at -rcN unless they're willing > >> >to accept that some changes could happen before release. > >> > > >> > >> There is no value delivered by Simon's series. Neither does the image get > >> smaller nor does it fix anything. If he wants to enforce a design, it must > >> work for all use cases. But this requires some conceptual work. > > > >Yes, and what's the rush to not do the conceptual work? If I recall > >part of the thread correctly, yes, Simon didn't get his objections in > >before the patches were merged, but it was early enough in the release > >cycle that taking a step back and reverting was a reasonable request. > >What he had said wouldn't have changed if he had gotten the email out a > >few days earlier. > > > >So yes, please merge Simon's revert, or post and merge new more minimal > >revert that brings things to the same functional end. There are > >objections to this implementation, and thus far Simon has been > >responding all of the requests to better clarify all of the related code > >and concepts that have been asked of him, so that in the end an > >implementation that fulfills all of the technical requirements can be > >created, that hopefully leaves all parties satisfied. > > There is nothing wrong with the current code. > > It is Simon's concept of blobs in devicetrees that is borked in that > it ignores QEMU and any board that gets the DT from a prior boot > stage. Then it should be pretty easy to get Simon to withdraw his objections, if there's such a fundamental "this is the only possible way, no changes" path forward. > Simon's patches have no functional end. So what do you mean by "same > functional end"? I mean the state of the EFI subsystem, prior to the
Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined
On 9/4/21 7:01 PM, Tom Rini wrote: [trimming the CC list] On Sat, Sep 04, 2021 at 06:49:03PM +0200, Marek Vasut wrote: On 9/4/21 6:09 PM, Tom Rini wrote: On Sat, Sep 04, 2021 at 06:05:50PM +0200, Marek Vasut wrote: On 9/4/21 5:17 PM, Tom Rini wrote: On Sat, Sep 04, 2021 at 05:15:45PM +0200, Marek Vasut wrote: On 9/4/21 4:10 PM, Tom Rini wrote: [...] At this point, I think you should rework things to stop making CONFIG_LMB be optional, it should be a def_bool y. I disagree, see above. The only reason "tools-only_defconfig" builds a useless u-boot binary today is in CI where it would be more work than it's worth to make CI exclude that from the build list. But if you want to just do that instead, I'll also accept adding -x tools-only to the azure/gitlab jobs that build all other architectures, as tools-only is tested in its own build job, for it's only valid build target. The tools-only build is also used elsewhere, to build just that, tools. I've repeatedly explained myself and what I'm looking for in v2 of this series. I will summarize one last time. The "tools-only_defconfig" is for tools, only. Building anything other than the "tools-only" target isn't useful. In U-Boot itself, LMB is required as that is how we prevent a number of CVEs from being trivial to exploit. v2 of this series needs to drop patches 1 and 2 of v1 of this series. It can further do any of: 1. Nothing else. 2. Add tools-only to the exclude list in the "build everything else" CI job. 3. Make CONFIG_LMB be def_bool y. If tools-only is for tools, only, then why should it enable LMB ? The tools are userspace tools, they do not need LMB, and so LMB can be disabled. This is the part which is unclear to me. I don't know why it's unclear to you at this point, sorry. Well why exactly does a userspace program require LMB enabled ? What does LMB protect in there ? obviously not U-Boot. I feel like you've lost the thread. Can you please answer my questions above ? I have. This attitude is not helpful. Please answer my questions, if necessary please reiterate, otherwise this discussion cannot be resolved and will only lead to frustration.
Re: [PATCH 02/14] lmb: Use CONFIG_LMB_*_REGIONS only if they are defined
On Sat, Sep 04, 2021 at 09:37:39PM +0200, Marek Vasut wrote: > On 9/4/21 7:01 PM, Tom Rini wrote: > > [trimming the CC list] > > > > On Sat, Sep 04, 2021 at 06:49:03PM +0200, Marek Vasut wrote: > > > On 9/4/21 6:09 PM, Tom Rini wrote: > > > > On Sat, Sep 04, 2021 at 06:05:50PM +0200, Marek Vasut wrote: > > > > > On 9/4/21 5:17 PM, Tom Rini wrote: > > > > > > On Sat, Sep 04, 2021 at 05:15:45PM +0200, Marek Vasut wrote: > > > > > > > On 9/4/21 4:10 PM, Tom Rini wrote: > > > > > > > [...] > > > > > > > > > > > > > > > > > > > At this point, I think you should rework things to stop > > > > > > > > > > > > making > > > > > > > > > > > > CONFIG_LMB be optional, it should be a def_bool y. > > > > > > > > > > > > > > > > > > > > > > I disagree, see above. > > > > > > > > > > > > > > > > > > > > The only reason "tools-only_defconfig" builds a useless > > > > > > > > > > u-boot binary > > > > > > > > > > today is in CI where it would be more work than it's worth > > > > > > > > > > to make CI > > > > > > > > > > exclude that from the build list. But if you want to just > > > > > > > > > > do that > > > > > > > > > > instead, I'll also accept adding -x tools-only to the > > > > > > > > > > azure/gitlab jobs > > > > > > > > > > that build all other architectures, as tools-only is tested > > > > > > > > > > in its own > > > > > > > > > > build job, for it's only valid build target. > > > > > > > > > > > > > > > > > > The tools-only build is also used elsewhere, to build just > > > > > > > > > that, tools. > > > > > > > > > > > > > > > > I've repeatedly explained myself and what I'm looking for in v2 > > > > > > > > of this > > > > > > > > series. I will summarize one last time. The > > > > > > > > "tools-only_defconfig" is > > > > > > > > for tools, only. Building anything other than the "tools-only" > > > > > > > > target > > > > > > > > isn't useful. In U-Boot itself, LMB is required as that is how > > > > > > > > we > > > > > > > > prevent a number of CVEs from being trivial to exploit. v2 of > > > > > > > > this > > > > > > > > series needs to drop patches 1 and 2 of v1 of this series. It > > > > > > > > can > > > > > > > > further do any of: > > > > > > > > 1. Nothing else. > > > > > > > > 2. Add tools-only to the exclude list in the "build everything > > > > > > > > else" CI > > > > > > > >job. > > > > > > > > 3. Make CONFIG_LMB be def_bool y. > > > > > > > > > > > > > > If tools-only is for tools, only, then why should it enable LMB ? > > > > > > > The tools are userspace tools, they do not need LMB, and so LMB > > > > > > > can be > > > > > > > disabled. > > > > > > > > > > > > > > This is the part which is unclear to me. > > > > > > > > > > > > I don't know why it's unclear to you at this point, sorry. > > > > > > > > > > Well why exactly does a userspace program require LMB enabled ? > > > > > What does LMB protect in there ? obviously not U-Boot. > > > > > > > > I feel like you've lost the thread. > > > > > > Can you please answer my questions above ? > > > > I have. > > This attitude is not helpful. Please answer my questions, if necessary > please reiterate, otherwise this discussion cannot be resolved and will only > lead to frustration. One last time then. The only reason tools-only_defconfig is ever built for a target other than "tools-only" is because CI does not exclude it from the world build stage. You can fix this by doing option #2 still quoted above. The only CONFIG options that are at all valid for "tools-only" and so the host tools related, are LOCALVERSION (which is why there's a tools-only defconfig at all) and now TOOLS_LIBCRYPTO. Nothing else at all should matter as the tools should always be the same. So your point about "what does userspace need LMB for" is irrelevant. The host tools should need NO option be enabled/disabled. Further, "disabling FOO breaks the build" means we need to investigate what the correct resolution is. In this case, LMB needs to be def_bool y. This is option #3 above. Why does u-boot-as-sandbox need LMB? Because that's how we ensure that the tests that check for overlap fail as expected. Finally, you can just drop the first two patches and call me too stubborn. This is option #1 above. -- Tom signature.asc Description: PGP signature
Re: [PATCH 01/23] mmc: Rename MMC_SUPPORT to MMC
On Sun, Aug 08, 2021 at 12:20:09PM -0600, Simon Glass wrote: > Rename these options so that CONFIG_IS_ENABLED can be used with them. > > Signed-off-by: Simon Glass > Reviewed-by: Jaehoon Chung For the series, and with a few minor changes (see git log a48f5ff4f523..21b86803ebb1), applied to u-boot/master, thanks! -- Tom signature.asc Description: PGP signature
Re: Pull request for efi-2021-10-rc4
Hi Tom, On Sat, 4 Sept 2021 at 21:08, Tom Rini wrote: > > On Sat, Sep 04, 2021 at 08:02:49PM +0200, Heinrich Schuchardt wrote: > > > > > > Am 4. September 2021 19:39:49 MESZ schrieb Tom Rini : > > >On Sat, Sep 04, 2021 at 07:03:48PM +0200, Heinrich Schuchardt wrote: > > >> > > >> > > >> Am 4. September 2021 16:37:22 MESZ schrieb Tom Rini : > > >> >On Sat, Sep 04, 2021 at 03:08:38PM +0200, Heinrich Schuchardt wrote: > > >> >> > > >> >> > > >> >> Am 4. September 2021 15:01:11 MESZ schrieb Tom Rini > > >> >> : > > >> >> >On Sat, Sep 04, 2021 at 11:56:47AM +0200, Heinrich Schuchardt wrote: > > >> >> > > > >> >> >> Dear Tom, > > >> >> >> > > >> >> >> The following changes since commit > > >> >> >> 94509b79b13e69c209199af0757afbde8d2ebd6d: > > >> >> >> > > >> >> >> btrfs: Use default subvolume as filesystem root (2021-09-01 > > >> >> >> 10:11:24 > > >> >> >> -0400) > > >> >> >> > > >> >> >> are available in the Git repository at: > > >> >> >> > > >> >> >> https://source.denx.de/u-boot/custodians/u-boot-efi.git > > >> >> >> tags/efi-2021-10-rc4 > > >> >> >> > > >> >> >> for you to fetch changes up to > > >> >> >> 1dfa494610c5469cc28cf1f8538abf4be6c00324: > > >> >> >> > > >> >> >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter check > > >> >> >> (2021-09-04 09:15:09 +0200) > > >> >> >> > > >> >> >> > > >> >> >> Pull request for efi-2021-10-rc4 > > >> >> >> > > >> >> >> Documentation: > > >> >> >> > > >> >> >> Remove invalid reference to configuration variable in UEFI doc > > >> >> >> > > >> >> >> UEFI: > > >> >> >> > > >> >> >> Parameter checks for the EFI_TCG2_PROTOCOL > > >> >> >> Improve support of preseeding UEFI variables. > > >> >> >> Correct the calculation of the size of loaded images. > > >> >> >> Allow for UEFI images with zero VirtualSize > > >> >> >> > > >> >> >> > > >> >> >> Heinrich Schuchardt (5): > > >> >> >> efi_loader: sections with zero VirtualSize > > >> >> >> efi_loader: rounding of image size > > >> >> >> efi_loader: don't load signature database from file > > >> >> >> efi_loader: efi_auth_var_type for AuditMode, DeployedMode > > >> >> >> efi_loader: correct determination of secure boot state > > >> >> >> > > >> >> >> Masahisa Kojima (3): > > >> >> >> efi_loader: add missing parameter check for > > >> >> >> EFI_TCG2_PROTOCOL api > > >> >> >> efi_loader: fix boot_service_capability_min calculation > > >> >> >> efi_loader: fix efi_tcg2_hash_log_extend_event() parameter > > >> >> >> check > > >> >> > > > >> >> >And I don't see Simon's revert in here either. And he asked you > > >> >> >about > > >> >> >that yesterday: > > >> >> >https://lore.kernel.org/r/capnjgz3erdjf0jb9s-cjk6y+feuyrywf0hnkf2trib4dr4u...@mail.gmail.com/ > > >> >> > > > >> >> >So at this point, are you asserting there is nothing to revert? > > >> >> > > >> >> Never. Simons "revert" is breaking functionality. The concept for > > >> >> suporting blobs in devicetrees supplied by a prior bootstage has not > > >> >> been defined yet. > > >> > > > >> >And to be clearer, reverting something that was introduced in one rc in > > >> >a later rc isn't breaking functionality. U-Boot releases (well, the > > >> >non-rc ones for sure) are on a very regular schedule. External projects > > >> >may not depend on some feature introduced at -rcN unless they're willing > > >> >to accept that some changes could happen before release. > > >> > > > >> > > >> There is no value delivered by Simon's series. Neither does the image > > >> get smaller nor does it fix anything. If he wants to enforce a design, > > >> it must work for all use cases. But this requires some conceptual work. > > > > > >Yes, and what's the rush to not do the conceptual work? If I recall > > >part of the thread correctly, yes, Simon didn't get his objections in > > >before the patches were merged, but it was early enough in the release > > >cycle that taking a step back and reverting was a reasonable request. > > >What he had said wouldn't have changed if he had gotten the email out a > > >few days earlier. > > > > > >So yes, please merge Simon's revert, or post and merge new more minimal > > >revert that brings things to the same functional end. There are > > >objections to this implementation, and thus far Simon has been > > >responding all of the requests to better clarify all of the related code > > >and concepts that have been asked of him, so that in the end an > > >implementation that fulfills all of the technical requirements can be > > >created, that hopefully leaves all parties satisfied. > > > > There is nothing wrong with the current code. > > > > It is Simon's concept of blobs in devicetrees that is borked in that > > it ignores QEMU and any board that gets the DT from a prior boot > > stage. > > Then it should be pretty easy to get Simon t
