You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/07/26 19:03:00 UTC

[GitHub] [incubator-nuttx] gustavonihei opened a new pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

gustavonihei opened a new pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233


   ## Summary
   This PR intends to add a new interface to the NuttX OS for enabling the application to perform the final steps of the
   booting process prior to finally switching the execution to another firmware image.
   
   The motivation for this new interface is to create secure boot applications for NuttX compliant with the OS architecture.
   
   ## Impact
   New feature, should have no impact to current users.
   
   ## Testing
   `esp32-wrover-kit` with WIP support for MCUboot bootloader.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r677490236



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       > [...] I could like to see the possibility to identify the image through the name not the address since the name match the file system more natually.
   
   Just to make sure we are in sync: for **image** I am referring to a region in the Flash storage that contains a firmware image (code+data in RAW format), and not an update file.
   
   Basically the idea is that the platform is expected to have some MTD partitions (e.g. `/dev/ota0` and `/dev/ota1`) that refer to booting slots.
   Initially, that `loadaddr` refers to the base address of either `/dev/ota0` partition or `/dev/ota1` partition.
   So, we may change the `loadaddr` parameter to be a path to MTD partition, but I feel that this may seen counter-intuitive, since the kernel will probably need to once again retrieve the base address of the selected partition.
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r678297069



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       > @xiaoxiang781216 Unfortunately sending the path as `const char *` parameter won't work out-of-the-box, because currently there is no interface for retrieving the starting address of an MTD partition.
   > 
   > To make things a bit more complicated, I've realized that besides the partition starting address (i.e `loadaddr` param) the application will also need to forward the size of the image header, which will be used as an offset for pointing to the beginning of the image's "useful" part (code + data).
   
   If you want to write a general bootloader, this detail information(e.g. image header) need be handled by board specific code since the different vendor may define a different image header.
   
   > So instead of a single value, we'd need to provide a struct, something like:
   > 
   > ```c
   > struct boot_info_s
   > {
   >   uint32_t img_header_size;   /* Size of the image header in bytes */
   >   uint32_t img_addr;          /* MTD partition starting address */
   > };
   > ```
   > 
   > If we create this MTD interface for retrieving the partition starting address, `img_addr` may be easily substituted by a `const char * img_path` field.
   > What's your suggestion on this?
   
   It's general question to get the block device offset and size, we can reference how BSD or Linux define the IOCTL. At least FIOC_MMAP/MTDIOC_XIPBASE can answer some query.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887702003


   > There are several small bootloaders that I wrote for the SAMA5Dx in the tree.  But I did an odd thing, I put the entire bootloader in the board/src directory so the whole app is inside the OS.  That eliminated the need for new OS interfaces.Board-specific IOCs do not need to be shared.  They are private.  Shared IOCs are not board-specific,  There can be no discussion of sharing private things.
   
   Yes, if the booloader is for a specific board, we don't need the public interface. The bootloader and board code can use any adhoc protocol to finish the work. But, I suppose @gustavonihei want to write a general bootloader which could work for all NuttX board. The IOCTL added by Xiaomi recently also want to support A/B boot in a general way because we need support many board come from different vendor.
   
   > If these are common IOCs, then the are extensions to OS interface and must be very carefully controlled.  People cannot just extend the public OS interface for personal needs.  These must be architected, designed optimally, conform to some roadmap, and be agreed to by the community.Sent from my Galaxy
   
   Yes, I agree that the public interface need get more attention and discussion to ensure it's general enough to cover the needs from all intersted party. Github PR or issue is a good place to let people share the idea or comment, do you have better suggestion?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_BOOT_IMAGE for booting a new application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r678807796



##########
File path: boards/boardctl.c
##########
@@ -452,6 +452,31 @@ int boardctl(unsigned int cmd, uintptr_t arg)
         break;
 #endif
 
+#ifdef CONFIG_BOARDCTL_BOOT_IMAGE
+      /* CMD:           BOARDIOC_BOOT_IMAGE
+       * DESCRIPTION:   Boot a new application image.
+       *                Execute the required actions for booting a new
+       *                application image (e.g. deinitialize peripherals,

Review comment:
       change all application to system?

##########
File path: include/sys/boardctl.h
##########
@@ -392,6 +393,17 @@ struct boardioc_nxterm_ioctl_s
 };
 #endif /* CONFIG_NXTERM */
 
+#ifdef CONFIG_BOARDCTL_BOOT_IMAGE
+
+/* Structure containing the arguments to the BOARDIOC_BOOT_IMAGE command */
+
+struct boardioc_boot_info_s
+{
+  FAR const char *img_path;           /* Path to application image */
+  uint32_t        img_header_size;    /* Size of the image header in bytes */

Review comment:
       The header is defined by mcuboot or vendor?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887729633


   One (crazy?) idea I've had: instead of relying on `boardctl`, how about calling a new MTDIOC command for an `ioctl` on the file descriptor of the MTD partition that contains the application image to be loaded? 
   This new MTDIOC command would be implemented by the `ioctl` operation of the underlying MTD driver (or by `part_ioctl` from the MTD partition), which in turn would call an `up_execute_image` function.
   Each CPU architecture would then provide its own implementation of `up_execute_image`.
   
   The premise behind this idea is that an application firmware image should map to a specific MTD partition.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r678250416



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       @xiaoxiang781216 Unfortunately sending the path as `const char *` parameter won't work out-of-the-box, because currently there is no interface for retrieving the starting address of an MTD partition.
   
   To make things a bit more complicated, I've realized that besides the partition starting address (i.e `loadaddr` param) the application will also need to forward the size of the image header, which will be used as an offset for pointing to the beginning of the image's "useful" part (code + data). So instead of a single value, we'd need to provide a struct, something like:
   
   ```c
   struct boot_info_s
   {
     uint32_t img_header_size;   /* Size of the image header in bytes */
     uint32_t img_addr;               /* MTD partition starting address */
   };
   ```
   
   If we create this MTD interface for retrieving the partition starting address, `img_addr` may be easily substituted by a `const char * img_path` field.
   What's your suggestion on this?

##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       @xiaoxiang781216 Unfortunately sending the path as `const char *` parameter won't work out-of-the-box, because currently there is no interface for retrieving the starting address of an MTD partition.
   
   To make things a bit more complicated, I've realized that besides the partition starting address (i.e `loadaddr` param) the application will also need to forward the size of the image header, which will be used as an offset for pointing to the beginning of the image's "useful" part (code + data). So instead of a single value, we'd need to provide a struct, something like:
   
   ```c
   struct boot_info_s
   {
     uint32_t img_header_size;   /* Size of the image header in bytes */
     uint32_t img_addr;          /* MTD partition starting address */
   };
   ```
   
   If we create this MTD interface for retrieving the partition starting address, `img_addr` may be easily substituted by a `const char * img_path` field.
   What's your suggestion on this?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r677504311



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       > > [...] I could like to see the possibility to identify the image through the name not the address since the name match the file system more natually.
   > 
   > Just to make sure we are in sync: for **image** I am referring to a region in the Flash storage that contains a firmware image (code+data in RAW format), and not an update file.
   > 
   > Basically the idea is that the platform is expected to have some MTD partitions (e.g. `/dev/ota0` and `/dev/ota1`) that refer to booting slots.
   > Initially, that `loadaddr` refers to the base address of either `/dev/ota0` partition or `/dev/ota1` partition.
   
   Yes, it point to a MTD partition in most case. But, why the path can't point to nuttx.bin on SD card? It's very useful in some case. It may even point to nuttx.elf and then board code can load the code/data to the correct location without hardcoding the magic address number in the source file.
   
   > So, we may change the `loadaddr` parameter to be a path to MTD partition, but I feel that this may seen counter-intuitive, since the kernel will probably need to once again retrieve the base address of the selected partition.
   
   The name(string) give many possibllity here: you can even pass itoa as the name here. The board code can:
   
   1. If the string is actually number, it's an address
   2. If the string is /dev/xxx, it's a partition
   3. Otherwise, it's a file on the file system




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887729633


   One (crazy?) idea I've had: instead of relying on `boardctl`, how about calling a new MTDIOC command for an `ioctl` on the file descriptor of the MTD partition that contains the application image to be loaded? 
   This new MTDIOC command would be implemented by the `ioctl` operation of the underlying MTD driver (or by `part_ioctl` from the MTD partition), which in turn would call an `up_boot_image` function.
   Each CPU architecture would then provide its own implementation of `up_boot_image`.
   
   The premise behind this idea is that an application firmware image should map to a specific MTD partition.
   
   **Edit**: Here is a draft of this proposal: https://github.com/gustavonihei/incubator-nuttx/commit/b32ddd49c250dfbc9a3b3fc43a7791bcdbbc6e8f


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_BOOT_IMAGE for booting a new application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r679132907



##########
File path: include/sys/boardctl.h
##########
@@ -392,6 +393,17 @@ struct boardioc_nxterm_ioctl_s
 };
 #endif /* CONFIG_NXTERM */
 
+#ifdef CONFIG_BOARDCTL_BOOT_IMAGE
+
+/* Structure containing the arguments to the BOARDIOC_BOOT_IMAGE command */
+
+struct boardioc_boot_info_s
+{
+  FAR const char *img_path;           /* Path to application image */
+  uint32_t        img_header_size;    /* Size of the image header in bytes */

Review comment:
       Ok.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_BOOT_IMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r678556077



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       I've changed `board_boot_image` to receive a string path instead of the address.
   
   > It's general question to get the block device offset and size, we can reference how BSD or Linux define the IOCTL. At least FIOC_MMAP/MTDIOC_XIPBASE can answer some query.
   
   See https://github.com/apache/incubator-nuttx/pull/4248, there I've proposed a new IOCTL for addressing this requirement.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887702003


   > There are several small bootloaders that I wrote for the SAMA5Dx in the tree.  But I did an odd thing, I put the entire bootloader in the board/src directory so the whole app is inside the OS.  That eliminated the need for new OS interfaces.Board-specific IOCs do not need to be shared.  They are private.  Shared IOCs are not board-specific,  There can be no discussion of sharing private things.
   
   Yes, if the booloader is for a specific board, we don't need the public interface. The bootloader and board code can use any adhoc protocol to finish the work. But, I suppose @gustavonihei want to write a general bootloader which could work for all NuttX board. The IOCTL added by Xiaomi recently also want to support A/B boot in a general way because we need support many board come from different vendor.
   
   > If these are common IOCs, then the are extensions to OS interface and must be very carefully controlled.  People cannot just extend the public OS interface for personal needs.  These must be architected, designed optimally, conform to some roadmap, and be agreed to by the community.Sent from my Galaxy
   
   Yes, I agree that the public interface need get more attention and discussion to ensure it's general enough to cover the needs from all intersting party. Github PR or issue is a good place to let people share the idea or comment, do you have better suggestion?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887726983


   > > Board-specific IOCs do not need to be shared.  They are private.  Shared IOCs are not board-specific,  There can be no discussion of sharing private things.
   > 
   > Yes, if the booloader is for a specific board, we don't need the public interface. The bootloader and board code can use any adhoc protocol to finish the work. 
   
   @patacongo I believe now I understand the root of your concern. The motivation for defining a new `boardctl` command is due to it being, to my knowledge, one of the most consolidated means for the application to trigger chip-level actions without hurting the OS architecture.
   
   > But, I suppose @gustavonihei want to write a general bootloader which could work with all NuttX board.
   
   @xiaoxiang781216 Indeed, I am in the process of integrating support for [MCUboot](https://github.com/mcu-tools/mcuboot), which is a platform-agnostic secure bootloader already supported by several RTOSes. Besides the already merged https://github.com/apache/incubator-nuttx/pull/4166, this PR here is the only change/addition required to the OS interface.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887729633


   One (crazy?) idea I've had: instead of relying on `boardctl`, how about calling a new MTDIOC command for an `ioctl` on the file descriptor of the MTD partition that contains the application image to be loaded? 
   This new MTDIOC command would be implemented by the `ioctl` operation of the underlying MTD driver (or by `part_ioctl` from the MTD partition), which in turn would call an `up_boot_image` function.
   Each CPU architecture would then provide its own implementation of `up_boot_image`.
   
   The premise behind this idea is that an application firmware image should map to a specific MTD partition.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r677504311



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       > > [...] I could like to see the possibility to identify the image through the name not the address since the name match the file system more natually.
   > 
   > Just to make sure we are in sync: for **image** I am referring to a region in the Flash storage that contains a firmware image (code+data in RAW format), and not an update file.
   > 
   > Basically the idea is that the platform is expected to have some MTD partitions (e.g. `/dev/ota0` and `/dev/ota1`) that refer to booting slots.
   > Initially, that `loadaddr` refers to the base address of either `/dev/ota0` partition or `/dev/ota1` partition.
   
   Yes, it point to a MTD partition in most case. But, why the path can't point to nuttx.bin on SD card? It's very useful in some case. It may even point to nuttx.elf and then board code can load the code/data to the correct location without hardcoding the magic address number in the source file.
   
   > So, we may change the `loadaddr` parameter to be a path to MTD partition, but I feel that this may seen counter-intuitive, since the kernel will probably need to once again retrieve the base address of the selected partition.
   
   The name(string) give many possibllity here: you can even pass itoa as the name here. The board code can:
   
   1. If the string is actually number, it's an address
   2. If the string is /dev/xxx, it's a partition
   3. Otherwise, it's a file on the file system
   
   We lose these possibility with loadaddr. The same discussion happen internally when we design #4191. The final conclusion is that the name is more flexible than the address, especially combine with Unix design philosophy "everything is file".




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887532317


   > A few days ago, Xiomi added they one application-specific, boot-related BOARDIOC, PR #4191
   > 
   > This is really getting out of hand quickly. We need to do something to avoid the clutter. Perhaps there should be a single BOARDIOC_BOOTCMD that has sub-commands (like BOARDIOC_IOCTL) that at least encapsulates bootloader/boot commands and minimizes the uncontrolled, unmanaged, useless (to most people) clutter.
   
   Ok, you want to design the board ioctl to the different layer. It's fine to encapsulate all boot related fucntionality in to sub IOCTL.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_BOOT_IMAGE for booting a new application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r679143469



##########
File path: include/sys/boardctl.h
##########
@@ -392,6 +393,17 @@ struct boardioc_nxterm_ioctl_s
 };
 #endif /* CONFIG_NXTERM */
 
+#ifdef CONFIG_BOARDCTL_BOOT_IMAGE
+
+/* Structure containing the arguments to the BOARDIOC_BOOT_IMAGE command */
+
+struct boardioc_boot_info_s
+{
+  FAR const char *img_path;           /* Path to application image */
+  uint32_t        img_header_size;    /* Size of the image header in bytes */

Review comment:
       > Ok.
   
   Good, I'll rename every reference of **application image** to **application firmware image** then.
   
   > how about remove img_ prefix? It look like the redundant.
   
   Ok, seems reasonable. I'll do it.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] patacongo commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887640165


   There are several small bootloaders that I wrote for the SAMA5Dx in the tree.  But I did an odd thing, I put the entire bootloader in the board/src directory so the whole app is inside the OS.  That eliminated the need for new OS interfaces.Board-specific IOCs do not need to be shared.  They are private.  Shared IOCs are not board-specific,  There can be no discussion of sharing private things.If these are common IOCs, then the are extensions to OS interface and must be very carefully controlled.  People cannot just extend the public OS interface for personal needs.  These must be architected, designed optimally, conform to some roadmap, and be agreed to by the community.Sent from my Galaxy
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r678302179



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       > If you want to write a general bootloader, this detail information(e.g. image header) need be handled by board specific code since the different vendor may define a different image header.
   
   In the case of the MCUboot project, the handling itself is both done at application-level. The board-specific implementation requires the header size just as a offset.
   I agree with you that depending on that chosen secure boot solution, the header details will probably vary, but one common aspect is that they will have an image header.
   So the board needs this offset information to correctly jump to the other image.

##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       > If you want to write a general bootloader, this detail information(e.g. image header) need be handled by board specific code since the different vendor may define a different image header.
   
   In the case of the MCUboot project, the handling itself is both done at application-level. The board-specific implementation requires the header size just as an offset.
   I agree with you that depending on that chosen secure boot solution, the header details will probably vary, but one common aspect is that they will have an image header.
   So the board needs this offset information to correctly jump to the other image.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r677474836



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       > 1. LOAD_APPIMAGE to LAUNCH_IMAGE or EXECUTE_IMAGE since it not only load but also execute image, and the image isn't an applicaiton but OS self.
   
   Ok, I like the `LAUNCH_IMAGE`, the `launch` verb fits nicely. Regarding the `application` naming, it refers to the "business" point-of-view, for the distinction between a Bootloader image and an Application image.
   
   
   > 2. It's better to change "uintptr_t loadaddr" to "FAR const char *image" or "FAR void *image". The address is too related to XIP, I could like to see the possibility to identify the image through the name not the address since the name match the file system more natually.
   
   Ok, I understand your point. The `FAR const char *image` suggestion is also a good fit, because it may map to the path of a MTD partition. I'll work on this and submit an update.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r677490236



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       > [...] I could like to see the possibility to identify the image through the name not the address since the name match the file system more natually.
   
   Just to make sure we are in sync: for **image** I am referring to a region in the Flash storage that contains a firmware image (code+data in RAW format), and not an update file.
   
   Basically the idea is that the platform is expected to have some MTD partitions (e.g. `/dev/ota0` and `/dev/ota1`) that refer to booting slots.
   So, initially, that `loadaddr` refers to the base address of either `/dev/ota0` partition or `/dev/ota1` partition.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887956710


   > Another idea, now a bit more elegant than the previous one: we may create a `/proc/boot` entry, and trigger the required actions via the previously proposed `up_boot_image`.
   
   I like procfs solutions.  However, the current procfs is limited to read-only entries (that is easily fixed).  And perhaps this is overkill?  I personally have no technical issues with a BOARDIOC solution.  We do have to properly control these non-standard interfaces (and they really need to be better documented).  We can't let BOARDIOC solutions run rampant if other better solutions are available.
   
   I am supportive of either solution, I just want to make sure that we think carefully about long term consequences to the OS.  BOARDIOC solutions can be a crutch that people use to avoid thinking about proper OS interfaces.  And it is much easier to add a non-standard interface than it is to recover from a bad choice once the solution has been fielded.
   
   With regard to up_boot_image, the name problem should be board_boot_image since the function is provided by board logic.  See https://cwiki.apache.org/confluence/display/NUTTX/Naming+of+Architecture%2C+MCU%2C+and+Board+Interfaces


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #4233: board/ctrl: Add BOARDIOC_BOOT_IMAGE for booting a new application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887702003


   > There are several small bootloaders that I wrote for the SAMA5Dx in the tree.  But I did an odd thing, I put the entire bootloader in the board/src directory so the whole app is inside the OS.  That eliminated the need for new OS interfaces.Board-specific IOCs do not need to be shared.  They are private.  Shared IOCs are not board-specific,  There can be no discussion of sharing private things.
   
   Yes, if the booloader is for a specific board, we don't need the public interface. The bootloader and board code can use any adhoc protocol to finish the work. But, I suppose @gustavonihei want to write a general bootloader which could work with all NuttX board. The IOCTL added by Xiaomi recently also want to support A/B boot in a general way because we need support many board come from different vendor.
   
   > If these are common IOCs, then the are extensions to OS interface and must be very carefully controlled.  People cannot just extend the public OS interface for personal needs.  These must be architected, designed optimally, conform to some roadmap, and be agreed to by the community.Sent from my Galaxy
   
   Yes, I agree that the public interface need get more attention and discussion to ensure it's general enough to cover the needs from all intersted party. Github PR or issue is a good place to let people share the idea or comment, do you have better suggestion?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_BOOT_IMAGE for booting a new application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r678992051



##########
File path: include/sys/boardctl.h
##########
@@ -392,6 +393,17 @@ struct boardioc_nxterm_ioctl_s
 };
 #endif /* CONFIG_NXTERM */
 
+#ifdef CONFIG_BOARDCTL_BOOT_IMAGE
+
+/* Structure containing the arguments to the BOARDIOC_BOOT_IMAGE command */
+
+struct boardioc_boot_info_s
+{
+  FAR const char *img_path;           /* Path to application image */
+  uint32_t        img_header_size;    /* Size of the image header in bytes */

Review comment:
       This is from MCUboot:
   https://github.com/mcu-tools/mcuboot/blob/main/boot/bootutil/include/bootutil/image.h#L129-L138
   
   The vendor may also add a specific header, but that will be part of the image payload.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887494626


   > > How do you suggest that an application performs the loading and execution of the next application image?
   > 
   > I would suggest using BOARDIOC_IOCTL. That allows board-specific boarctl() commands. It works exactly like the other BOARDIOC commands but does not clutter the system with garbage BOARDIOC commmands. Imagine where that will go in the long run i we allow everyone to do that?
   > 
   > The only real difference is that the board-specific IOCTL commands would be defined in a different header file in the board/include or in the arch/include directory.
   > 
   > boardctl() is a nasty kludge to bypass the standard POSIX interface. It really needs to be avoid as much as possible if you want to claim to be a portable, POSIX system. But, unfortunately, there are things that need to be done that are not covered by any POSIX interfaces. So we created this backdoor. But we can only allow the backdoor to be open a crack or we will be swamped with board-/applicatoin-specific IOCTLs which would be a very bad consequence.
   > 
   > Creating another BOARDIOC command does make it it has general usability but should never be done for a one-off solution.
   
   Right, I can see pros and cons in opting for the generic `BOARDCTL_IOCTL`.
   Functionality-wise it is no different than creating another BOARDIOC command, after all it goes through the same path.
   I agree with you that if we indiscriminately keep adding more and more commands the list will someday become a mess.
   But the benefit of a creating a new command in this scenario is that it might be supported by any platform. So if we have it properly documented and listed as a `BOARDIOC` command eases the standardization across the OS.
   In this case, `BOARDIOC_LOAD_APPIMAGE` will be used by one bootloader application, but it may easily be reused by others to come.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_BOOT_IMAGE for booting a new application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r679140452



##########
File path: boards/boardctl.c
##########
@@ -452,6 +452,31 @@ int boardctl(unsigned int cmd, uintptr_t arg)
         break;
 #endif
 
+#ifdef CONFIG_BOARDCTL_BOOT_IMAGE
+      /* CMD:           BOARDIOC_BOOT_IMAGE
+       * DESCRIPTION:   Boot a new application image.
+       *                Execute the required actions for booting a new
+       *                application image (e.g. deinitialize peripherals,

Review comment:
       Ok




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887473322


   > We really need to avoid adding application-specific BOARDIOC values. That is why BOARDIOC_IOCTL was create: In order to keep this project-specific trash from accumulating in the OS proper.
   
   This might be application-specific, but specific to a type of a application, which in this case is a bootloader.
   How do you suggest that an application performs the loading and execution of the next application image?
   Please, consider that the set of actions are platform-dependent, e.g. Arm processors require that VTOR register is configured to point to the new vector table; ESP32 chips have a different set of requirements; other processor may simply need to override the PC register with the new entry point.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r678317466



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       BTW, I've updated the PR with another proposal for addressing the naming issue. I've changed *load* to simply **boot**, which is more intuitive for the intended purpose.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887528500


   If we want to write a general application which isn't to specific to some board( and then lock to a specific vendor), we need standardized IOCTL. There isn't difference between board IOCTL or serial IOCTL from this point.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] patacongo commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887511252


   A few days ago, Xiomi added they one application-specific, boot-related BOARDIOC, PR #4191 
   
   This is really getting out of hand quickly.  We need to do something to avoid the clutter.  Perhaps there should be a single BOARDIOC_BOOTCMD that has sub-commands (like BOARDIOC_IOCTL) that at least encapsulates bootloader/boot commands and minimizes the uncontrolled, unmanaged, useless (to most people) clutter.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] patacongo commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887956710


   > Another idea, now a bit more elegant than the previous one: we may create a `/proc/boot` entry, and trigger the required actions via the previously proposed `up_boot_image`.
   
   I like procfs solutions.  However, the current procfs is limited to read-only entries (that is easily fixed).  And perhaps this is overkill?  I personally have no technical issues with a BOARDIOC solution.  We do have to properly control these non-standard interfaces (and they really need to be better documented).  We can't let BOARDIOC solutions run rampant if other better solutions are available.
   
   I am supportive of either solution, I just want to make sure that we think carefully about long term consequences to the OS.
   
   With regard to up_boot_image, the name problem should be board_boot_image since the function is provided by board logic.  See https://cwiki.apache.org/confluence/display/NUTTX/Naming+of+Architecture%2C+MCU%2C+and+Board+Interfaces


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r677315552



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       Actually, this interface should make no assumption regarding whether the code will be executed in-place or from RAM.
   The `loadaddr` parameter refers to the base address of the application image on the flash storage.
   The implementation of `board_load_appimage` may decide whether to copy the image from the load address into RAM, or to execute in place by simply overriding the PC with the application image entry point.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r678302179



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       > If you want to write a general bootloader, this detail information(e.g. image header) need be handled by board specific code since the different vendor may define a different image header.
   
   In the case of the MCUboot project, the handling itself is done at application-level. The board-specific implementation requires the header size just as an offset.
   I agree with you that depending on that chosen secure boot solution the header details will probably vary, but one common aspect is that they will have an image header.
   So the board needs this offset information to correctly jump to the other image.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] patacongo commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887470207


   We really need to avoid adding application-specific BOARDIOC values.  That is why BOARDIOC_IOCTL was create:  In order to keep this project-specific trash from accumulating in the OS proper.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887729633


   One (crazy?) idea I've had: instead of relying on `boardctl`, how about creating a new MTDIOC `ioctl` on the file descriptor of the MTD partition that contains the application image to be loaded? Then, the `ioctl` implementation could call an `up_execute_image` function.
   Each architecture would then provide its own implementation of `up_execute_image`.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887473322


   > We really need to avoid adding application-specific BOARDIOC values. That is why BOARDIOC_IOCTL was create: In order to keep this project-specific trash from accumulating in the OS proper.
   
   This might be application-specific, but specific to a group of applications, which in this case is a bootloader.
   How do you suggest that an application performs the loading and execution of the next application image?
   Please, consider that the set of actions are platform-dependent, e.g. Arm processors require that VTOR register is configured to point to the new vector table; ESP32 chips have a different set of requirements; other processor may simply need to override the PC register with the new entry point.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r677080802



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       Look like this interface assume XIP(receive the loadaddr), how about the target image(from a raw partition or a file from file system) need to be loaded into RAM before running?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887532317


   > A few days ago, Xiomi added they one application-specific, boot-related BOARDIOC, PR #4191
   > 
   > This is really getting out of hand quickly. We need to do something to avoid the clutter. Perhaps there should be a single BOARDIOC_BOOTCMD that has sub-commands (like BOARDIOC_IOCTL) that at least encapsulates bootloader/boot commands and minimizes the uncontrolled, unmanaged, useless (to most people) clutter.
   
   Ok, you want to design the board ioctl to the different layer. It's fine to encapsulate all boot related fucntionality in to sub IOCTL. My point is that IOCTL need standardize across all board, but either top IOCTL or sub IOCTL is OK for me.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887482585


   > How do you suggest that an application performs the loading and execution of the next application image?
   
   I would suggest using BOARDIOC_IOCTL.  That allows board-specific boarctl() commands.  It works exactly like the other BOARDIOC commands but does not clutter the system with garbage BOARDIOC commmands.  Imagine where that will go in the long run i we allow everyone to do that?
   
   The only real difference is that the board-specific IOCTL commands would be defined in a different header file in the board/include or in the arch/include directory.
   
   boardctl() is a nasty kludge to bypass the standard POSIX interface.  It really needs to be avoid as much as possible if you want to claim to be a portable, POSIX system.  But, unfortunately, there are things that need to be done that are not covered by any POSIX interfaces.  So we created this backdoor.  But we can only allow the backdoor to be open a crack or we will be swamped with board-/applicatoin-specific IOCTLs which would be a very bad consequence.
   
   Creating another BOARDIOC command does make it it has general usability but should never be done for a one-off solution.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r677315552



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       Actually, this interface makes no assumption regarding whether the code will be executed in place or from RAM.
   The `loadaddr` parameter refers to the base address of the application image on the flash storage.
   The implementation of `board_load_appimage` may decide whether to copy the image from the load address into RAM, or to execute in place by simply overriding the PC with the application image entry point.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_BOOT_IMAGE for booting a new application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r679141265



##########
File path: include/sys/boardctl.h
##########
@@ -392,6 +393,17 @@ struct boardioc_nxterm_ioctl_s
 };
 #endif /* CONFIG_NXTERM */
 
+#ifdef CONFIG_BOARDCTL_BOOT_IMAGE
+
+/* Structure containing the arguments to the BOARDIOC_BOOT_IMAGE command */
+
+struct boardioc_boot_info_s
+{
+  FAR const char *img_path;           /* Path to application image */
+  uint32_t        img_header_size;    /* Size of the image header in bytes */

Review comment:
       how about remove img_ prefix? It look like the redundant.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887729633


   One (crazy?) idea I've had: instead of relying on `boardctl`, how about calling a new MTDIOC command for an `ioctl` on the file descriptor of the MTD partition that contains the application image to be loaded? Then, the `ioctl` implementation could call an `up_execute_image` function.
   Each architecture would then provide its own implementation of `up_execute_image`.
   
   The premise behind this idea is that an application firmware image should map to a specific MTD partition.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r677504311



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       > > [...] I could like to see the possibility to identify the image through the name not the address since the name match the file system more natually.
   > 
   > Just to make sure we are in sync: for **image** I am referring to a region in the Flash storage that contains a firmware image (code+data in RAW format), and not an update file.
   > 
   > Basically the idea is that the platform is expected to have some MTD partitions (e.g. `/dev/ota0` and `/dev/ota1`) that refer to booting slots.
   > Initially, that `loadaddr` refers to the base address of either `/dev/ota0` partition or `/dev/ota1` partition.
   
   Yes, it point to a MTD partition in most case. But, why the path can't point to nuttx.bin on SD card? It's very useful in some case. It may even point to nuttx.elf and then board code can load the code/data to the correct location without hardcoding the magic address number in the source file.
   
   > So, we may change the `loadaddr` parameter to be a path to MTD partition, but I feel that this may seen counter-intuitive, since the kernel will probably need to once again retrieve the base address of the selected partition.
   
   The name(string) give many possibllity here: you can even pass itoa as the name here. The board code can:
   
   1. If the string is actually number, it's an address
   2. If the string is /dev/xxx, it's a partition
   3. Otherwise, it's a file on the file system
   
   We lose these possibility with loadaddr. The same discussion happen internally when we design #4191. The final conclusion is that the name is more flexible than the address, especially combine with Unix design philosophy "everything is file" 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r677490236



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
        [...] I could like to see the possibility to identify the image through the name not the address since the name match the file system more natually.
   
   Just to make sure we are in sync: for *image* I am referring to a region in the Flash storage that contains a firmware image (code+data in RAW format), and not an update file.
   
   Basically the idea is that the platform is expected to have some MTD partitions (e.g. `/dev/ota0` and `/dev/ota1`) that refer to booting slots.
   So, initially, that `loadaddr` refers to the base address of either `/dev/ota0` partition or `/dev/ota1` partition.

##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       > [...] I could like to see the possibility to identify the image through the name not the address since the name match the file system more natually.
   
   Just to make sure we are in sync: for *image* I am referring to a region in the Flash storage that contains a firmware image (code+data in RAW format), and not an update file.
   
   Basically the idea is that the platform is expected to have some MTD partitions (e.g. `/dev/ota0` and `/dev/ota1`) that refer to booting slots.
   So, initially, that `loadaddr` refers to the base address of either `/dev/ota0` partition or `/dev/ota1` partition.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_BOOT_IMAGE for booting a new application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r679181131



##########
File path: include/sys/boardctl.h
##########
@@ -392,6 +393,17 @@ struct boardioc_nxterm_ioctl_s
 };
 #endif /* CONFIG_NXTERM */
 
+#ifdef CONFIG_BOARDCTL_BOOT_IMAGE
+
+/* Structure containing the arguments to the BOARDIOC_BOOT_IMAGE command */
+
+struct boardioc_boot_info_s
+{
+  FAR const char *img_path;           /* Path to application image */
+  uint32_t        img_header_size;    /* Size of the image header in bytes */

Review comment:
       @xiaoxiang781216 Done, please take another look.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887729633


   One (crazy?) idea I've had: instead of relying on `boardctl`, how about creating a new MTDIOC `ioctl` on the file descriptor of the MTD partition that contains the application image to be loaded? Then, the `ioctl` implementation could call an `up_execute_image` function.
   Each architecture would then provide its own implementation of `up_execute_image`.
   
   The premise behind this idea is that an application firmware image should map to a specific MTD partition.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887532317


   > A few days ago, Xiomi added they one application-specific, boot-related BOARDIOC, PR #4191
   > 
   > This is really getting out of hand quickly. We need to do something to avoid the clutter. Perhaps there should be a single BOARDIOC_BOOTCMD that has sub-commands (like BOARDIOC_IOCTL) that at least encapsulates bootloader/boot commands and minimizes the uncontrolled, unmanaged, useless (to most people) clutter.
   
   Ok, you want to design the board ioctl to the different layer. It's fine to encapsulate all boot related fucntionality in to sub IOCTL. My point is that IOCTL need standardize, but either top IOCTL or sub IOCTL is fine for me.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887729633


   One (crazy?) idea I've had: instead of relying on `boardctl`, how about calling a new MTDIOC command for an `ioctl` on the file descriptor of the MTD partition that contains the application image to be loaded? 
   This new MTDIOC command would be implemented by the `ioctl` operation of the underlying MTD driver (or by `part_ioctl` from the MTD partition), which in turn would call an `up_boot_image` function.
   Each CPU architecture would then provide its own implementation of `up_boot_image`.
   
   The premise behind this idea is that an application firmware image should map to a specific MTD partition.
   
   **Edit**: Here is a draft of this proposal: https://github.com/gustavonihei/incubator-nuttx/commit/e26677f8980536a8af4db21f9d7911577dfea8b8


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-888278351


   > > Another idea, now a bit more elegant than the previous one: we may create a `/proc/boot` entry, and trigger the required actions via the previously proposed `up_boot_image`.
   > 
   > I like procfs solutions. However, the current procfs is limited to read-only entries (that is easily fixed). 
   
   Although `fs/procfs README` states that it only allows read-only access, it seems that write operations have already been implemented for some years:
   https://github.com/apache/incubator-nuttx/commit/cb051a522dfbfe61de803f2937987bdee88d2501
   https://github.com/apache/incubator-nuttx/commit/cdc8fc52d12e994fd7dfbf9efd1f384726af6de4
   
   > And perhaps this is overkill?
   
   It depends, if we plan for incorporate other features for a new **boot** subsystem (we already have two), I wouldn't consider overkill. But it might be in terms of flash footprint. The firmware image for a Bootloader should the slimmest possible. But that is just speculation, since I haven't done any real measurements for a proper comparison.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887917686


   Another idea, now a bit more elegant than the previous one: we may create a `/proc/boot` entry, and trigger the required actions via the previously proposed `up_boot_image`.
   
   This seems similar to what QNX does: http://www.qnx.com/developers/docs/qnxcar2/index.jsp?topic=%2Fcom.qnx.doc.neutrino.cookbook%2Ftopic%2Fs3_procfs_proc_boot.html
   
   The solution from https://github.com/apache/incubator-nuttx/pull/4191 could also be moved to `/proc/boot`.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r678302179



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       > If you want to write a general bootloader, this detail information(e.g. image header) need be handled by board specific code since the different vendor may define a different image header.
   
   In the case of the MCUboot project, the handling itself is both done at application-level. The board-specific implementation requires the header size just as an offset.
   I agree with you that depending on that chosen secure boot solution the header details will probably vary, but one common aspect is that they will have an image header.
   So the board needs this offset information to correctly jump to the other image.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r677315552



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       Actually, this interface should make no assumption regarding whether the code will be executed in place or from RAM.
   The `loadaddr` parameter refers to the base address of the application image on the flash storage.
   The implementation of `board_load_appimage` may decide whether to copy the image from the load address into RAM, or to execute in place by simply overriding the PC with the application image entry point.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] patacongo commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887482585


   > How do you suggest that an application performs the loading and execution of the next application image?
   
   I would suggest using BOARDIOC_IOCTL.  That allows board-specific boarctl() commands.  It works exactly like the other BOARDIOC commands but does not clutter the system with garbage BOARDIOC commmands.  Imagine where that will go in the long run i we allow everyone to do that?
   
   boardctl() is a nasty kludge to bypass the standard POSIX interface.  It really needs to be avoid as much as possible if you want to claim to be a portable, POSIX system.  But, unfortunately, there are things that need to be done that are not covered by any POSIX interfaces.  So we created this backdoor.  But we can only allow the backdoor to be open a crack or we will be swamped with board-/applicatoin-specific IOCTLs which would be a very bad consequence.
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887532317


   > A few days ago, Xiomi added they one application-specific, boot-related BOARDIOC, PR #4191
   > 
   > This is really getting out of hand quickly. We need to do something to avoid the clutter. Perhaps there should be a single BOARDIOC_BOOTCMD that has sub-commands (like BOARDIOC_IOCTL) that at least encapsulates bootloader/boot commands and minimizes the uncontrolled, unmanaged, useless (to most people) clutter.
   
   Ok, you want to design the board ioctl to the different layer. It's fine to encapsulate all boot related fucntionality in to sub IOCTL. My point is that IOCTL need standardize, but either top IOCTL or sub IOCTL is OK for me.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r677504311



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       > > [...] I could like to see the possibility to identify the image through the name not the address since the name match the file system more natually.
   > 
   > Just to make sure we are in sync: for **image** I am referring to a region in the Flash storage that contains a firmware image (code+data in RAW format), and not an update file.
   > 
   > Basically the idea is that the platform is expected to have some MTD partitions (e.g. `/dev/ota0` and `/dev/ota1`) that refer to booting slots.
   > Initially, that `loadaddr` refers to the base address of either `/dev/ota0` partition or `/dev/ota1` partition.
   
   Yes, it point to a MTD partition in most case. But, why the path can't point to nuttx.bin on SD card? It's very useful in some case. It may even point to nuttx.elf and then board code can load the code/data to the correct location without hardcoding the magic address number in the source file.
   
   > So, we may change the `loadaddr` parameter to be a path to MTD partition, but I feel that this may seen counter-intuitive, since the kernel will probably need to once again retrieve the base address of the selected partition.
   
   The name(string) give many possibllity here: you can even pass itoa as the name here. The board code can:
   
   1. If the string is actually number, it's an address
   2. If the string is /dev/xxx, it's a partition
   3. Otherwise, it's a file on the file system
   
   We lose these possibility with loadaddr.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887956710


   > Another idea, now a bit more elegant than the previous one: we may create a `/proc/boot` entry, and trigger the required actions via the previously proposed `up_boot_image`.
   
   I like procfs solutions.  However, the current procfs is limited to read-only entries (that is easily fixed).  And perhaps this is overkill?  I personally have no technical issues with a BOARDIOC solution.  We do have to properly control these non-standard interfaces (and they really need to be better documented).  We can't let BOARDIOC solutions run rampant if other better solutions are available.
   
   I am supportive of either solution, I just want to make sure that we think carefully about long term consequences to the OS.  It is much easier to add a non-standard interface than it is to recover from a bad choice once the solution has been fielded.
   
   With regard to up_boot_image, the name problem should be board_boot_image since the function is provided by board logic.  See https://cwiki.apache.org/confluence/display/NUTTX/Naming+of+Architecture%2C+MCU%2C+and+Board+Interfaces


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] patacongo commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887725347


   A wiki page summarizing a bootloader roadmap would be preferable.  It is impossible to evaluate these ad hoc changes with no idea of what we are trying to accomplish.Sent from my Galaxy
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_BOOT_IMAGE for booting a new application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r679092234



##########
File path: boards/boardctl.c
##########
@@ -452,6 +452,31 @@ int boardctl(unsigned int cmd, uintptr_t arg)
         break;
 #endif
 
+#ifdef CONFIG_BOARDCTL_BOOT_IMAGE
+      /* CMD:           BOARDIOC_BOOT_IMAGE
+       * DESCRIPTION:   Boot a new application image.
+       *                Execute the required actions for booting a new
+       *                application image (e.g. deinitialize peripherals,

Review comment:
       There is an [RFC](https://datatracker.ietf.org/doc/html/rfc9019) in the works for standardizing the firmware update process for IoT.
   They are using **Application Firmware Image** to refer to the firmware image that is not the bootloader.
   https://datatracker.ietf.org/doc/html/rfc9019#section-2.1
   
   I believe we should stick to that. What do you think?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4233: board/ctrl: Add BOARDIOC_BOOT_IMAGE for booting a new application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-889868908


   @xiaoxiang781216 Could you please merge this PR?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887598705


   > > How do you suggest that an application performs the loading and execution of the next application image?
   > 
   > I would suggest using BOARDIOC_IOCTL. That allows board-specific boarctl() commands. It works exactly like the other BOARDIOC commands but does not clutter the system with garbage BOARDIOC commmands. Imagine where that will go in the long run i we allow everyone to do that?
   > 
   
   The best thing what we can do is invite all interesting party review the design and ensure it's general enough to support the range of scenario. But, it's wrong to ignore the requirement and let's vendor expose the different IOCTL for the same thing and then create the unnecessary inconsistence between the different vendor. especially, if the requirement is a general and common feature needed by many usecase or application.
   
   > The only real difference is that the board-specific IOCTL commands would be defined in a different header file in the board/include or in the arch/include directory.
   > 
   
   If IOCTL is defined in the board specific header file, my question is:
   
   1. How to ensure the different vendor define the same IOCTL for the same functionality
   2. If they have the different idea, how to write a general application which depend on these vendor specific IOCTL
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#discussion_r677446091



##########
File path: include/nuttx/board.h
##########
@@ -331,6 +331,28 @@ int board_uniquekey(FAR uint8_t *uniquekey);
 int board_switch_boot(FAR const char *system);
 #endif
 
+/****************************************************************************
+ * Name:  board_load_appimage
+ *
+ * Description:
+ *   Load an application image from a given load address and perform the
+ *   required actions for executing it (e.g. loading the Program Counter
+ *   register with the application image entry point address).
+ *
+ * Input Parameters:
+ *   loadaddr - Load address of the application image
+ *
+ * Returned Value:
+ *   If this function returns, then it was not possible to load the
+ *   application image due to some constraints. The return value in this case
+ *   is a board-specific reason for the failure.
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_BOARDCTL_LOAD_APPIMAGE
+int board_load_appimage(uintptr_t loadaddr);

Review comment:
       Ok, understand. Here is my suggestion:
   
   1. LOAD_APPIMAGE to LAUNCH_IMAGE or EXECUTE_IMAGE since it not only load but also execute image, and the image isn't an applicaiton but OS self.
   2. It's better to change "uintptr_t loadaddr" to "FAR const char *image" or "FAR void *image". The address is too related to XIP, I could like to see the possibility to identify the image through the name not the address since the name match the file system more natually.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887473322


   > We really need to avoid adding application-specific BOARDIOC values. That is why BOARDIOC_IOCTL was create: In order to keep this project-specific trash from accumulating in the OS proper.
   
   This might be application-specific, but specific to a type of a application, which in this case is a bootloader.
   How do you suggest that an application performs the loading and execution of the next application image?
   Please, consider that the set of actions are platform-dependent, e.g. Arm processors require that VTOR is configured to point to the new vector table; ESP32 chips have a different set of requirements; other processor may simply need to override the PC with the new entry point.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #4233: board/ctrl: Add BOARDIOC_LOAD_APPIMAGE for loading an application image

Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #4233:
URL: https://github.com/apache/incubator-nuttx/pull/4233#issuecomment-887729633






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org