From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id EFB6AC352A1 for ; Tue, 6 Dec 2022 10:24:24 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 9D7828534C; Tue, 6 Dec 2022 11:24:22 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=foss.st.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=foss.st.com header.i=@foss.st.com header.b="DlNp1ACc"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 12E2385323; Tue, 6 Dec 2022 11:24:20 +0100 (CET) Received: from mx07-00178001.pphosted.com (mx08-00178001.pphosted.com [91.207.212.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id D044A85323 for ; Tue, 6 Dec 2022 11:24:16 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=foss.st.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=prvs=8339b4db11=patrick.delaunay@foss.st.com Received: from pps.filterd (m0046660.ppops.net [127.0.0.1]) by mx07-00178001.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 2B69XkEf006436; Tue, 6 Dec 2022 11:24:13 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=foss.st.com; h=message-id : date : mime-version : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding; s=selector1; bh=dWWqPgRtnMbYed+R4FaC7dTJ65dj3V/eKVD/Z+kWFBM=; b=DlNp1ACcg1ekxWQ5AHLsDzbUpEXY8yp87COxSi4ycPPNsb1ZP4DTQJCC00Q6A1MXz2Lf S4iHr3oP5WfdDcdyT9BwmMM7g/HP/s97616Uf3nAqt9ie1CoD3CRqF4yTkJKN3JwUcAc NU35gHS4LOTa643etw1DT4/+siezVMGkapd5YQAZ63hsLqxCpaKAEB8E8M8NiNKQZjeG fqQxWUYLKR3fgFb9GS4sC0jWkR86aAaEQUF8zLCCnkCyv35S97lz8Z+SmPab4Bwtgoxs 8SxM7ZXGCFqRuqXZH5Hc4WGu0HK5zNFfk0i+qTlq+NIYVWz7wEgNONOGTDdu7h/xtavw VA== Received: from beta.dmz-eu.st.com (beta.dmz-eu.st.com [164.129.1.35]) by mx07-00178001.pphosted.com (PPS) with ESMTPS id 3m7vferhke-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 06 Dec 2022 11:24:07 +0100 Received: from euls16034.sgp.st.com (euls16034.sgp.st.com [10.75.44.20]) by beta.dmz-eu.st.com (STMicroelectronics) with ESMTP id EC692100034; Tue, 6 Dec 2022 11:24:00 +0100 (CET) Received: from Webmail-eu.st.com (shfdag1node3.st.com [10.75.129.71]) by euls16034.sgp.st.com (STMicroelectronics) with ESMTP id DC39F2194FB; Tue, 6 Dec 2022 11:24:00 +0100 (CET) Received: from [10.48.0.157] (10.48.0.157) by SHFDAG1NODE3.st.com (10.75.129.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.13; Tue, 6 Dec 2022 11:23:57 +0100 Message-ID: Date: Tue, 6 Dec 2022 11:23:57 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [PATCH v2] fastboot: Add OEM run command Content-Language: en-US To: Sean Anderson , Marek Vasut , CC: Heiko Schocher , Lukasz Majewski References: <20221202210323.1086933-1-sean.anderson@seco.com> <82ccbf26-5f5a-b6f8-69cb-a9b0e29b2d77@foss.st.com> <72aac90c-cdf4-5206-f7a4-e8dacc524c1c@seco.com> From: Patrick DELAUNAY In-Reply-To: <72aac90c-cdf4-5206-f7a4-e8dacc524c1c@seco.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.48.0.157] X-ClientProxiedBy: SHFCAS1NODE2.st.com (10.75.129.73) To SHFDAG1NODE3.st.com (10.75.129.71) X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1 definitions=2022-12-06_05,2022-12-06_01,2022-06-22_01 X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean Hi, On 12/5/22 20:15, Sean Anderson wrote: > On 12/5/22 14:04, Patrick DELAUNAY wrote: >> Hi, >> >> On 12/2/22 22:03, Sean Anderson wrote: >>> This adds the UUU UCmd functionality as an OEM command. While the >>> fastboot tool allows sending arbitrary commands as long as they are >>> prefixed with "oem". This allows running generic U-Boot commands over >>> fastboot without UUU, which is especially useful when not using USB. >>> This is really the route we should have gone in the first place when >>> adding these commands. >>> >>> While we're here, clean up the Kconfig a bit. >>> >>> Signed-off-by: Sean Anderson >>> --- >>> >>> Changes in v2: >>> - Document usage >>> - Keep enum in order >>> >>>   doc/android/fastboot.rst      | 15 +++++++++++++++ >>>   drivers/fastboot/Kconfig      | 10 +++++----- >>>   drivers/fastboot/fb_command.c |  4 ++++ >>>   include/fastboot.h            |  1 + >>>   4 files changed, 25 insertions(+), 5 deletions(-) >>> >>> diff --git a/doc/android/fastboot.rst b/doc/android/fastboot.rst >>> index 7611f07038..7f343d5dfc 100644 >>> --- a/doc/android/fastboot.rst >>> +++ b/doc/android/fastboot.rst >>> @@ -28,6 +28,7 @@ The following OEM commands are supported (if enabled): >>>   - ``oem partconf`` - this executes ``mmc partconf %x 0`` to configure eMMC >>>     with = boot_ack boot_partition >>>   - ``oem bootbus``  - this executes ``mmc bootbus %x %s`` to configure eMMC >>> +- ``oem run`` - this executes an arbitrary U-Boot command >>>     Support for both eMMC and NAND devices is included. >>>   @@ -127,6 +128,19 @@ Boot command >>>   When executing the fastboot ``boot`` command, if ``fastboot_bootcmd`` is set >>>   then that will be executed in place of ``bootm ``. >>>   +Running Shell Commands >>> +---------------------- >>> + >>> +Normally, arbitrary U-Boot command execution is not enabled. This is so >>> +fastboot can be used to update systems using verified boot. However, such >>> +functionality can be useful for production or when verified boot is not in use. >>> +Enable ``CONFIG_FASTBOOT_UUU_SUPPORT`` to use this functionality. This will >>> +enable the ``UCmd`` and ``ACmd`` commands for use with UUU [3]_. It also >>> +enables the ``oem run`` command, which can be used with the fastboot client. >>> +For example, to print "Hello world", run:: >>> + >>> +    $ fastboot oem run:echo Hello world >>> + >>>   Partition Names >>>   --------------- >>>   @@ -232,3 +246,4 @@ References >>>     .. [1] :doc:`fastboot-protocol` >>>   .. [2] https://developer.android.com/studio/releases/platform-tools >>> +.. [3] https://github.com/NXPmicro/mfgtools >>> diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig >>> index b97c67bf60..8f2d52cb8a 100644 >>> --- a/drivers/fastboot/Kconfig >>> +++ b/drivers/fastboot/Kconfig >>> @@ -80,12 +80,12 @@ config FASTBOOT_FLASH >>>         this to enable the "fastboot flash" command. >>>     config FASTBOOT_UUU_SUPPORT >>> -    bool "Enable FASTBOOT i.MX UUU special command" >>> +    bool "Enable running arbitrary commands from FASTBOOT" >>>       help >>> -      The fastboot protocol includes "UCmd" and "ACmd" command. >>> -      Be aware that you provide full access to any U-Boot command, >>> -      including working with memory and may open a huge backdoor, >>> -      when enabling this option. >>> +      This extends the fastboot protocol with the "UCmd" and "ACmd" >>> +      commands, as well as the "oem run" command.  These commands provide >>> +      full access to any U-Boot command, including working with memory. >>> +      This may open a huge backdoor if you are using verified boot. >>> >> why the support of "oem run" generic support is include in the IMX config CONFIG_FASTBOOT_UUU_SUPPORT >> > Because they are effectively the same command, but named differently. > oem run is just an alias for UCmd which can be used by the fastboot > Linux command without modification. > >> "UCmd" and "Acmd" are imx additions in fastboot standard for UUU support >> >> https://android.googlesource.com/platform/system/core/+/2ec418a4c98f6e8f95395456e1ad4c2956cac007/fastboot/fastboot_protocol.txt > These commands are absent from that standard (and thus are non-standard). Yes, they are 'non-standard', but the command prefixed by 'oem' are allowed by the fastboot specification. So all the "oem" commands can be used with the official Android tool... $> fastboot --help ....   oem ...         Executes oem specific command. .... => they are not reserved to IMX platforms the added command UCmd and Acmd can be only used with UUU imx tools. > >> but "oem run" can be see as generic U-Boot 'oem' command to execute U-boot command. > All of these commands are effectively non-standard, although "oem" is a > common prefix. > >> for me I see 2 separate configs to handle other platform than IMX ones (with UUU support) >> >> >> config FASTBOOT_CMD_OEM_RUN >>     bool "Enable fastboot oem run command" >> >> config FASTBOOT_UUU_SUPPORT >>     bool "Enable FASTBOOT i.MX UUU special command" >>     select FASTBOOT_CMD_OEM_RUN >> >> >> PS: FASTBOOT_UUU_SUPPORT could depends on CONFIG_ARCH_IMX* > Maybe we can have something like > > config FASTBOOT_ARBITRARY_EXECUTION > > which UUU_SUPPORT could depend on. whatever the Kconfig logic, the "fastboot oem run" should be enabled independently of imx/UUU support. > >>>   choice >>>       prompt "Flash provider for FASTBOOT" >>> diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c >>> index 98eccc3455..1732406c18 100644 >>> --- a/drivers/fastboot/fb_command.c >>> +++ b/drivers/fastboot/fb_command.c >>> @@ -123,6 +123,10 @@ static const struct { >>>       }, >>>   #endif >>>   #if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) >>> +    [FASTBOOT_COMMAND_OEM_RUN] = { >>> +        .command = "oem run", >>> +        .dispatch = run_ucmd, >>> +    }, >> >>  #if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_RUN) >>     [FASTBOOT_COMMAND_OEM_RUN] = { >>         .command = "oem run", >>         .dispatch = run_ucmd, >>     }, >> >> #endif >> >> #if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) >> >>     [FASTBOOT_COMMAND_UCMD] = { >>          .command = "UCmd", >>          .dispatch = run_ucmd, >> >> >> That allows other platform to enable  FASTBOOT_CMD_OEM_RUN but not FASTBOOT_UUU_SUPPORT. >> >> >>>       [FASTBOOT_COMMAND_UCMD] = { >>>           .command = "UCmd", >>>           .dispatch = run_ucmd, >>> diff --git a/include/fastboot.h b/include/fastboot.h >>> index 57daaf1298..8b6b4b934a 100644 >>> --- a/include/fastboot.h >>> +++ b/include/fastboot.h >>> @@ -45,6 +45,7 @@ enum { >>>       FASTBOOT_COMMAND_OEM_BOOTBUS, >>>   #endif >>>   #if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) >>> +    FASTBOOT_COMMAND_OEM_RUN, >>>       FASTBOOT_COMMAND_ACMD, >>>       FASTBOOT_COMMAND_UCMD, >>>   #endif >> +#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_RUN) >> >> +    FASTBOOT_COMMAND_OEM_RUN, >> +#endif >> >> >> and also some stuff to compile the fucntion run_ucmd() >> >> >> PS: we have many #if CONFIG is the fastboot source code >> >>       they can be replaced by CONFIG_IS_ENABLED macro and __maybe_unused in .h and .c  ? >> >>       and that avoids the compilation issue => let the linker remove the used functions >> >> for example >> >> enum { >> >>     FASTBOOT_COMMAND_GETVAR = 0, >>     FASTBOOT_COMMAND_DOWNLOAD, >>     CONFIG_IS_ENABLED(FASTBOOT_FLASH, (FASTBOOT_COMMAND_FLASH,)) >>     CONFIG_IS_ENABLED(FASTBOOT_FLASH, (FASTBOOT_COMMAND_ERASE,)) >>     FASTBOOT_COMMAND_BOOT, >>     FASTBOOT_COMMAND_CONTINUE, >>     FASTBOOT_COMMAND_REBOOT, >>     FASTBOOT_COMMAND_REBOOT_BOOTLOADER, >>     FASTBOOT_COMMAND_REBOOT_FASTBOOTD, >>     FASTBOOT_COMMAND_REBOOT_RECOVERY, >>     FASTBOOT_COMMAND_SET_ACTIVE, >>     CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_FORMAT, (FASTBOOT_COMMAND_OEM_FORMAT,)) >>     CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_PARTCONF, (FASTBOOT_COMMAND_OEM_PARTCONF,)) >>     CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_BOOTBUS, (FASTBOOT_COMMAND_OEM_BOOTBUS,)) >>     CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT, (FASTBOOT_COMMAND_ACMD,)) >>     CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT, (FASTBOOT_COMMAND_UCMD,)) >>     FASTBOOT_COMMAND_COUNT >> }; >> >> ... >> >> ommands[FASTBOOT_COMMAND_COUNT] = { >> [FASTBOOT_COMMAND_GETVAR] = { >> .command = "getvar", >> .dispatch = getvar >> }, >> [FASTBOOT_COMMAND_DOWNLOAD] = { >> .command = "download", >> .dispatch = download >> }, >> CONFIG_IS_ENABLED(FASTBOOT_FLASH, ( >> [FASTBOOT_COMMAND_FLASH] = { >> .command = "flash", >> .dispatch = flash >> }, >> [FASTBOOT_COMMAND_ERASE] = { >> .command = "erase", >> .dispatch = erase >> }, >> )) >> .... >> staticvoid__maybe_unused erase(char*cmd_parameter, char*response) >> { >> if(CONFIG_IS_ENABLED(FASTBOOT_FLASH_MMC)) >> fastboot_mmc_erase(cmd_parameter, response); >> if(CONFIG_IS_ENABLED(FASTBOOT_FLASH_NAND)) >> fastboot_nand_erase(cmd_parameter, response); >> } >> > Yes, that would probably be a good cleanup idea. However, I'd like to > keep this patch's scope focused. Ok, I will try to propose something in parallel: https://source.denx.de/u-boot/custodians/u-boot-stm/-/tree/fastboot the compilation in CI is started... > > --Sean Patrick