Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace flash partitioning terminology #440

Merged
merged 1 commit into from
Mar 13, 2019
Merged

Replace flash partitioning terminology #440

merged 1 commit into from
Mar 13, 2019

Conversation

d3zd3z
Copy link
Member

@d3zd3z d3zd3z commented Mar 12, 2019

This change replaces the slot 0/1 terminology with primary/secondary
slot and replaces FLASH_AREA_IMAGE_0/1 with
FLASH_AREA_IMAGE_PRIMARY/SECONDARY. This naming convention may be more
understandable, fits better to MCUs with multiple images and it is an
architecture agnostic alternative as well.

Change-Id: I655a585f6ae023852c671ee6635399efe25209c9
Signed-off-by: David Vincze david.vincze@arm.com
Signed-off-by: David Brown david.brown@linaro.org

@d3zd3z d3zd3z requested review from utzig and carlescufi March 12, 2019 19:14
utzig
utzig reviewed Mar 13, 2019
View reviewed changes
boot/bootutil/src/bootutil_priv.h
#define BOOT_STATUS_SOURCE_PRIMARY_SLOT 2

#define BOOT_FLAG_IMAGE_OK 0
#define BOOT_FLAG_COPY_DONE 1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To pass Mynewt CI, please add the following at this line:

#if defined(MCUBOOT_MYNEWT)                                                     
    #define FLASH_AREA_IMAGE_PRIMARY   FLASH_AREA_IMAGE_0                           
    #define FLASH_AREA_IMAGE_SECONDARY FLASH_AREA_IMAGE_1                           
#endif

newt auto-generates the FLASH_AREA_IMAGE_[0|1] symbols from bsp.yml and this adds a compatibility definition.

@nvlsianpu nvlsianpu self-requested a review March 13, 2019 11:04
nvlsianpu
nvlsianpu reviewed Mar 13, 2019
View reviewed changes
boot/bootutil/src/encrypted.c
@@ -305,7 +305,7 @@ boot_enc_load(const struct image_header *hdr, const struct flash_area *fap,
int
boot_enc_valid(const struct flash_area *fap)
{
return enc_state[fap->fa_id - FLASH_AREA_IMAGE_0].valid;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR makes me aware of that this mathematics might be not compatible with zephyr flash_area numeration.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be much better to pass the slot number instead of the flash area.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the values are not sequential for Zephyr, maybe it would be better to change it to something like:

int
boot_enc_valid(const struct flash_area *fap)
{
    switch (fap->fa_id) {                                                       
    case FLASH_AREA_IMAGE_0:                                                    
        return enc_state[0].valid;                                              
    case FLASH_AREA_IMAGE_1:                                                    
        return enc_state[1].valid;                                              
    default:                                                                    
        assert(0);
        return 0;                                                               
    }
}

But on another PR please!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also checking that other enc_state accesses do not use the same math.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will prepare the patch

utzig
utzig reviewed Mar 13, 2019
View reviewed changes
boot/bootutil/src/loader.c Outdated
}

#ifdef MCUBOOT_VALIDATE_SLOT0
rc = boot_validate_slot(0, NULL);
#ifdef mcuboot_validate_primary_slot
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MCUBOOT_VALIDATE_PRIMARY_SLOT

@utzig
Copy link
Member

utzig commented Mar 13, 2019

@d3zd3z Fix the two issues I mentioned above and it should be OK, tested here, looks good.

nvlsianpu
nvlsianpu requested changes Mar 13, 2019
View reviewed changes
boot/zephyr/Kconfig
@@ -72,37 +72,37 @@ config MBEDTLS_CFG_FILE
default "mcuboot-mbedtls-cfg.h"

config BOOT_VALIDATE_SLOT0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change this keyword as well. It produces #define CONFIG_BOOT_VALIDATE_SLOT0 1 currently.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mcuboot_config.h is checking for this define as is here to avoid breaking compatibility with previous custom configurations, so I don't think it needs to be changed for now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Previous custom configuration" - do you mena proj.conf file in this repository or end-user configuration?

Copy link
Member

@utzig utzig Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone made CONFIG_BOOT_VALIDATE_SLOT0=n in a custom config

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok - can live with this

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe we can keep backward compatibility:

Suggested change
config BOOT_VALIDATE_SLOT0
config BOOT_VALIDATE_SLOT0
# Hidden selector for backward compatibility
# deprecated
bool
select BOOT_VALIDATE_PRIMARY_SLOT
config BOOT_VALIDATE_PRIMARY_SLOT

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we happy with it as it is now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's a nice suggestion from @nvlsianpu...

@utzig utzig mentioned this pull request Mar 13, 2019
Merged
@davidvincze @d3zd3z
Replace flash partitioning terminology
3ea9058 
This change replaces the slot 0/1 terminology with primary/secondary
slot and replaces FLASH_AREA_IMAGE_0/1 with
FLASH_AREA_IMAGE_PRIMARY/SECONDARY. This naming convention may be more
understandable, fits better to MCUs with multiple images and it is an
architecture agnostic alternative as well.

Change-Id: I655a585f6ae023852c671ee6635399efe25209c9
Signed-off-by: David Vincze <david.vincze@arm.com>
Signed-off-by: David Brown <david.brown@linaro.org>
@d3zd3z d3zd3z merged commit 2d736ad into mcu-tools:master Mar 13, 2019
@d3zd3z d3zd3z deleted the terminology branch March 15, 2019 19:19
@nvlsianpu nvlsianpu mentioned this pull request Apr 12, 2019
Merged
nvlsianpu added a commit to nvlsianpu/fw-nrfconnect-zephyr that referenced this pull request Apr 12, 2019
@nvlsianpu
[nrf fromlist] dfu/boot: Replace flash partitioning terminology
9b59550 
Changed image terminology to the same which is used by
mcuboot after mcu-tools/mcuboot#440

Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>
nvlsianpu added a commit to nrfconnect/sdk-zephyr that referenced this pull request Apr 12, 2019
@nvlsianpu
[nrf fromlist] dfu/boot: Replace flash partitioning terminology
6030b19 
Changed image terminology to the same which is used by
mcuboot after mcu-tools/mcuboot#440

Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>
nvlsianpu added a commit to nvlsianpu/zephyr that referenced this pull request Apr 12, 2019
@nvlsianpu
dfu/boot: Replace flash partitioning terminology
4fbadaa 
Changed image terminology to the same which is used by
mcuboot after mcu-tools/mcuboot#440

Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>
nashif pushed a commit to zephyrproject-rtos/zephyr that referenced this pull request Apr 18, 2019
@nvlsianpu @nashif
dfu/boot: Replace flash partitioning terminology
a819d16 
Changed image terminology to the same which is used by
mcuboot after mcu-tools/mcuboot#440

Signed-off-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>
@nvlsianpu nvlsianpu added this to the 1.3.1 milestone Jul 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@utzig utzig utzig approved these changes

@nvlsianpu nvlsianpu nvlsianpu requested changes

@davidvincze davidvincze davidvincze left review comments

@carlescufi carlescufi Awaiting requested review from carlescufi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.3.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@d3zd3z @utzig @nvlsianpu @davidvincze