| Message ID | 20200109184715.21574-1-semen.protsenko@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | fastboot: Re-implement erase according to spec | expand |
Hi, On 10/01/20 12:17 am, Sam Protsenko wrote: > Fastboot specification [1] requires MMC to be filled with 0xFF's on > "fastboot erase" command: > > erase:%s Erase the indicated partition (clear to 0xFFs) > > Current "fastboot erase" implementation uses actual MMC erase operation > blk_derase(), which can fill MMC either with 0x00 or 0xFF, depending on > used MMC controller; from [2]: > > The content of an explicitly erased memory range shall be '0' or '1' > depending on different memory technology. > > For example, on BeagleBoard X15 it fills memory with '0'. > > Furthermore, the minimal amount of memory blk_derase() can erase is > "erase group size", and it's usually 512 KiB or more. So if we try to > erase some partition which is smaller than 512 KiB, "fastboot erase" > won't work at all, due to alignment. It's good practice to align all > partitions to "erase group size" boundary, but it's not a strict > requirement, so we just can't use blk_derase() due to this restriction. > > In order to provide the consistent way to erase partitions, adhere to > the fastboot spec, and fix erasing of small partitions, let's use > regular MMC write operation and fill the partition with 0xFF. > > [1] https://android.googlesource.com/platform/system/core/+/refs/tags/android-10.0.0_r25/fastboot/README.md#command-reference > [2] https://www.jedec.org/system/files/docs/JESD84-A441.pdf > > Signed-off-by: Sam Protsenko <semen.protsenko at linaro.org> > ---Reviewed-by: Faiz Abbas <faiz_abbas at ti.com> Thanks, Faiz
Hi Sam, On Thu, Jan 09, 2020 at 08:47:15PM +0200, Sam Protsenko wrote: > Fastboot specification [1] requires MMC to be filled with 0xFF's on > "fastboot erase" command: > > erase:%s Erase the indicated partition (clear to 0xFFs) > > Current "fastboot erase" implementation uses actual MMC erase operation > blk_derase(), which can fill MMC either with 0x00 or 0xFF, depending on > used MMC controller; from [2]: > > The content of an explicitly erased memory range shall be '0' or '1' > depending on different memory technology. > > For example, on BeagleBoard X15 it fills memory with '0'. > > Furthermore, the minimal amount of memory blk_derase() can erase is > "erase group size", and it's usually 512 KiB or more. So if we try to > erase some partition which is smaller than 512 KiB, "fastboot erase" > won't work at all, due to alignment. It's good practice to align all > partitions to "erase group size" boundary, but it's not a strict > requirement, so we just can't use blk_derase() due to this restriction. > > In order to provide the consistent way to erase partitions, adhere to > the fastboot spec, and fix erasing of small partitions, let's use > regular MMC write operation and fill the partition with 0xFF. > > [1] https://android.googlesource.com/platform/system/core/+/refs/tags/android-10.0.0_r25/fastboot/README.md#command-reference > [2] https://www.jedec.org/system/files/docs/JESD84-A441.pdf > > Signed-off-by: Sam Protsenko <semen.protsenko at linaro.org> > --- > drivers/fastboot/fb_mmc.c | 90 +++++++++++++++++++++++++-------------- > 1 file changed, 59 insertions(+), 31 deletions(-) Roman (CC-ed) reported below [0-1] measurements results with and w/o the patch when erasing the eMMC 'system' partition on R-Car H3ULCB. As can be seen, 'fastboot erase' now needs roughly one order of magnitude more time to perform its job. That's a big performance hit and it is not easy to live with. I tried to track down the original commit adding the "0xff" mention to fastboot docs, but couldn't find any discussion attached to it: erase:%s Erase the indicated partition (clear to 0xFFs) I think that interpreting the above statement as a requirement is a matter of perspective. The statement could have also been written as a pure empirical observation on specific HW. It could also have been a reflection of author's experience, who might only have dealt with 0xFF-filled erased memory technologies. In any case and regardless of the above, filling memory with 0xFF on erase for ALL platforms seems rather expensive. Any ideas/thoughts? Could userspace stay agnostic whether memory is 0x0 or 0xFF on erase and just concentrate on the useful contents/payload? [0] Before applying the patch: $ time fastboot erase system ******** Did you mean to fastboot format this ext4 partition? erasing 'system_a'... OKAY [ 6.521s] finished. total time: 6.521s real 0m6,588s user 0m0,005s sys 0m0,000s [1] After applying the patch: $ time fastboot erase system ******** Did you mean to fastboot format this ext4 partition? erasing 'system_a'... OKAY [ 51.256s] finished. total time: 51.256s real 0m51,478s user 0m0,004s sys 0m0,000s
Hi Sam, [Relaying the same message to your gmail address] [Cc-ing Peng, as U-Boot MMC maintainer] [Cc-ing Faiz, as the one who already reviewed the patch] On Thu, Jan 09, 2020 at 08:47:15PM +0200, Sam Protsenko wrote: > Fastboot specification [1] requires MMC to be filled with 0xFF's on > "fastboot erase" command: > > erase:%s Erase the indicated partition (clear to 0xFFs) > > Current "fastboot erase" implementation uses actual MMC erase operation > blk_derase(), which can fill MMC either with 0x00 or 0xFF, depending on > used MMC controller; from [2]: > > The content of an explicitly erased memory range shall be '0' or '1' > depending on different memory technology. > > For example, on BeagleBoard X15 it fills memory with '0'. > > Furthermore, the minimal amount of memory blk_derase() can erase is > "erase group size", and it's usually 512 KiB or more. So if we try to > erase some partition which is smaller than 512 KiB, "fastboot erase" > won't work at all, due to alignment. It's good practice to align all > partitions to "erase group size" boundary, but it's not a strict > requirement, so we just can't use blk_derase() due to this restriction. > > In order to provide the consistent way to erase partitions, adhere to > the fastboot spec, and fix erasing of small partitions, let's use > regular MMC write operation and fill the partition with 0xFF. > > [1] https://android.googlesource.com/platform/system/core/+/refs/tags/android-10.0.0_r25/fastboot/README.md#command-reference > [2] https://www.jedec.org/system/files/docs/JESD84-A441.pdf > > Signed-off-by: Sam Protsenko <semen.protsenko at linaro.org> > --- > drivers/fastboot/fb_mmc.c | 90 +++++++++++++++++++++++++-------------- > 1 file changed, 59 insertions(+), 31 deletions(-) Roman (CC-ed) reported below [0-1] measurements results with and w/o the patch when erasing the eMMC 'system' partition on R-Car H3ULCB. As can be seen, 'fastboot erase' now needs roughly one order of magnitude more time to perform its job. That's a big performance hit and it is not easy to live with. I tried to track down the original commit adding the "0xff" mention to fastboot docs, but couldn't find any discussion attached to it: erase:%s Erase the indicated partition (clear to 0xFFs) I think that interpreting the above statement as a requirement is a matter of perspective. The statement could have also been written as a pure empirical observation on specific HW. It could also have been a reflection of author's experience, who might only have dealt with 0xFF-filled-on-erase MMC technologies. In any case and regardless of the above, filling memory with 0xFF on erase for ALL platforms seems rather expensive. Any ideas/thoughts? Could userspace stay agnostic whether memory is 0x0 or 0xFF on erase and just concentrate on the useful contents/payload? [0] Before applying the patch: $ time fastboot erase system ******** Did you mean to fastboot format this ext4 partition? erasing 'system_a'... OKAY [ 6.521s] finished. total time: 6.521s real 0m6,588s user 0m0,005s sys 0m0,000s [1] After applying the patch: $ time fastboot erase system ******** Did you mean to fastboot format this ext4 partition? erasing 'system_a'... OKAY [ 51.256s] finished. total time: 51.256s real 0m51,478s user 0m0,004s sys 0m0,000s
Hi Eugeniu On Tue, Jan 14, 2020 at 8:30 PM Eugeniu Rosca <erosca at de.adit-jv.com> wrote: > > Hi Sam, > > [Relaying the same message to your gmail address] > [Cc-ing Peng, as U-Boot MMC maintainer] > [Cc-ing Faiz, as the one who already reviewed the patch] > > On Thu, Jan 09, 2020 at 08:47:15PM +0200, Sam Protsenko wrote: > > Fastboot specification [1] requires MMC to be filled with 0xFF's on > > "fastboot erase" command: > > > > erase:%s Erase the indicated partition (clear to 0xFFs) > > > > Current "fastboot erase" implementation uses actual MMC erase operation > > blk_derase(), which can fill MMC either with 0x00 or 0xFF, depending on > > used MMC controller; from [2]: > > > > The content of an explicitly erased memory range shall be '0' or '1' > > depending on different memory technology. > > > > For example, on BeagleBoard X15 it fills memory with '0'. > > > > Furthermore, the minimal amount of memory blk_derase() can erase is > > "erase group size", and it's usually 512 KiB or more. So if we try to > > erase some partition which is smaller than 512 KiB, "fastboot erase" > > won't work at all, due to alignment. It's good practice to align all > > partitions to "erase group size" boundary, but it's not a strict > > requirement, so we just can't use blk_derase() due to this restriction. > > > > In order to provide the consistent way to erase partitions, adhere to > > the fastboot spec, and fix erasing of small partitions, let's use > > regular MMC write operation and fill the partition with 0xFF. > > > > [1] https://android.googlesource.com/platform/system/core/+/refs/tags/android-10.0.0_r25/fastboot/README.md#command-reference > > [2] https://www.jedec.org/system/files/docs/JESD84-A441.pdf > > > > Signed-off-by: Sam Protsenko <semen.protsenko at linaro.org> > > --- > > drivers/fastboot/fb_mmc.c | 90 +++++++++++++++++++++++++-------------- > > 1 file changed, 59 insertions(+), 31 deletions(-) > > Roman (CC-ed) reported below [0-1] measurements results with and w/o > the patch when erasing the eMMC 'system' partition on R-Car H3ULCB. > > As can be seen, 'fastboot erase' now needs roughly one order of > magnitude more time to perform its job. That's a big performance hit and > it is not easy to live with. > > I tried to track down the original commit adding the "0xff" mention to > fastboot docs, but couldn't find any discussion attached to it: > > erase:%s Erase the indicated partition (clear to 0xFFs) > > I think that interpreting the above statement as a requirement is a > matter of perspective. The statement could have also been written as > a pure empirical observation on specific HW. It could also have been > a reflection of author's experience, who might only have dealt with > 0xFF-filled-on-erase MMC technologies. > > In any case and regardless of the above, filling memory with 0xFF on > erase for ALL platforms seems rather expensive. Any ideas/thoughts? > > Could userspace stay agnostic whether memory is 0x0 or 0xFF on erase and > just concentrate on the useful contents/payload? > The main point of my patch is to fix the erasing of partitions smaller than group erase size (512 KiB on BeagleBoard X15). For example, it's impossible to erase 'misc' partition (128 KiB), as such an operation would erase 512 KiB, so alignment code leads to no-op. Also, I wouldn't jump into conclusions about fastboot spec. Relying on 0xFF can be useful e.g. for testing purposes. All that said, I'm positive that this issue should be fixed. The only way I see to avoid performance degradation is to: - use actual MMC erase operation when erasing partition >= group erase size - use MMC write operation when erasing partition < group erase size But that leaves unhandled the case when partition is bigger than group erase size, but not a multiple o group erase size (so the remainder won't be erased). Of course we can do MMC write op for the not aligned remainder, but how would we know if MMC controller fills with 0x00 of with 0xFF on MMC erase op? So all those options considered, I decided to go with just MMC write operation for "fastboot erase". I will try to come up with v2 soon, but I don't see any ideal/elegant solution here. So if you have any ideas, please share. > [0] Before applying the patch: > $ time fastboot erase system > ******** Did you mean to fastboot format this ext4 partition? > erasing 'system_a'... > OKAY [ 6.521s] > finished. total time: 6.521s > > real 0m6,588s > user 0m0,005s > sys 0m0,000s > > [1] After applying the patch: > $ time fastboot erase system > ******** Did you mean to fastboot format this ext4 partition? > erasing 'system_a'... > OKAY [ 51.256s] > finished. total time: 51.256s > > real 0m51,478s > user 0m0,004s > sys 0m0,000s > > -- > Best Regards, > Eugeniu
Hi Sam, On Wed, Jan 15, 2020 at 04:59:13PM +0200, Sam Protsenko wrote: > The main point of my patch is to fix the erasing of partitions smaller > than group erase size (512 KiB on BeagleBoard X15). For example, it's > impossible to erase 'misc' partition (128 KiB), as such an operation > would erase 512 KiB, so alignment code leads to no-op. At this point the discussion is no longer specific to fastboot, as your points also apply to erasing MMC partitions in general. I think clarifying below might be helpful (some questions might have obvious answers): - Is it legitimate to have partitions smaller than erase group size? - Is it legitimate to have partitions not aligned to erase group size? - How does U-Boot 'mmc' command and Linux behave when erasing a partition smaller or not aligned to erase group size? > Also, I > wouldn't jump into conclusions about fastboot spec. Relying on 0xFF > can be useful e.g. for testing purposes. > > All that said, I'm positive that this issue should be fixed. The only > way I see to avoid performance degradation is to: > - use actual MMC erase operation when erasing partition >= group erase size > - use MMC write operation when erasing partition < group erase size Fair enough. > > But that leaves unhandled the case when partition is bigger than group > erase size, but not a multiple o group erase size (so the remainder > won't be erased). Of course we can do MMC write op for the not aligned > remainder, but how would we know if MMC controller fills with 0x00 of > with 0xFF on MMC erase op? So all those options considered, I decided > to go with just MMC write operation for "fastboot erase". I think we should check how Linux behaves in similar use-cases and possibly inspire from there (ideally by backporting the necessary patches). > > I will try to come up with v2 soon, but I don't see any ideal/elegant > solution here. So if you have any ideas, please share. Just shared my thoughts. Looking forward to seeing the v2.
diff --git a/drivers/fastboot/fb_mmc.c b/drivers/fastboot/fb_mmc.c index b0b19c5762..a965ebb5af 100644 --- a/drivers/fastboot/fb_mmc.c +++ b/drivers/fastboot/fb_mmc.c @@ -16,6 +16,7 @@ #include <div64.h> #include <linux/compat.h> #include <android_image.h> +#include <mapmem.h> #define FASTBOOT_MAX_BLK_WRITE 16384 @@ -48,12 +49,12 @@ static int part_get_info_by_name_or_alias(struct blk_desc *dev_desc, } /** - * fb_mmc_blk_write() - Write/erase MMC in chunks of FASTBOOT_MAX_BLK_WRITE + * fb_mmc_blk_write() - Write MMC in chunks of FASTBOOT_MAX_BLK_WRITE. * * @block_dev: Pointer to block device - * @start: First block to write/erase + * @start: First block to write * @blkcnt: Count of blocks - * @buffer: Pointer to data buffer for write or NULL for erase + * @buffer: Pointer to data buffer for write */ static lbaint_t fb_mmc_blk_write(struct blk_desc *block_dev, lbaint_t start, lbaint_t blkcnt, const void *buffer) @@ -66,19 +67,59 @@ static lbaint_t fb_mmc_blk_write(struct blk_desc *block_dev, lbaint_t start, for (i = 0; i < blkcnt; i += FASTBOOT_MAX_BLK_WRITE) { cur_blkcnt = min((int)blkcnt - i, FASTBOOT_MAX_BLK_WRITE); - if (buffer) { - if (fastboot_progress_callback) - fastboot_progress_callback("writing"); - blks_written = blk_dwrite(block_dev, blk, cur_blkcnt, - buffer + (i * block_dev->blksz)); - } else { - if (fastboot_progress_callback) - fastboot_progress_callback("erasing"); - blks_written = blk_derase(block_dev, blk, cur_blkcnt); - } + if (fastboot_progress_callback) + fastboot_progress_callback("writing"); + blks_written = blk_dwrite(block_dev, blk, cur_blkcnt, + buffer + (i * block_dev->blksz)); + blk += blks_written; + blks += blks_written; + } + return blks; +} + +/** + * fb_mmc_blk_erase() - Erase MMC by filling it with 0xFF's. + * + * @block_dev: Pointer to block device + * @start: First block to erase + * @blkcnt: Count of blocks + * + * Fastboot protocol requires erase operation to fill the MMC with 0xFF bytes. + * So we can't use blk_derase(), because it can fill MMC with 0x00. Also, + * blk_derase() operates on "erase group size" (usually 512 KiB or more), which + * can lead to inability to erase small partitions (due to alignment). + * + * Use fastboot buffer (filled with 0xFF) to write MMC with 0xFF's. The write + * will be performed in chunks of FASTBOOT_MAX_BLK_WRITE (if possible). + */ +static lbaint_t fb_mmc_blk_erase(struct blk_desc *block_dev, lbaint_t start, + lbaint_t blkcnt) +{ + lbaint_t blk = start; + lbaint_t blks_written; + lbaint_t cur_blkcnt; + lbaint_t blks = 0; + void *buf; + size_t buf_sz, buf_blks, i; + + buf_sz = min((size_t)(FASTBOOT_MAX_BLK_WRITE * block_dev->blksz), + (size_t)CONFIG_FASTBOOT_BUF_SIZE); + buf_sz = ALIGN_DOWN(buf_sz, block_dev->blksz); + buf_blks = buf_sz / block_dev->blksz; + assert(buf_blks > 0); + buf = map_sysmem(CONFIG_FASTBOOT_BUF_ADDR, buf_sz); + memset(buf, 0xff, buf_sz); + + for (i = 0; i < blkcnt; i += buf_blks) { + cur_blkcnt = min((int)blkcnt - i, buf_blks); + if (fastboot_progress_callback) + fastboot_progress_callback("erasing"); + blks_written = blk_dwrite(block_dev, blk, cur_blkcnt, buf); blk += blks_written; blks += blks_written; } + + unmap_sysmem(buf); return blks; } @@ -441,7 +482,7 @@ void fastboot_mmc_erase(const char *cmd, char *response) int ret; struct blk_desc *dev_desc; disk_partition_t info; - lbaint_t blks, blks_start, blks_size, grp_size; + lbaint_t blks; struct mmc *mmc = find_mmc_device(CONFIG_FASTBOOT_FLASH_MMC_DEV); if (mmc == NULL) { @@ -464,27 +505,14 @@ void fastboot_mmc_erase(const char *cmd, char *response) return; } - /* Align blocks to erase group size to avoid erasing other partitions */ - grp_size = mmc->erase_grp_size; - blks_start = (info.start + grp_size - 1) & ~(grp_size - 1); - if (info.size >= grp_size) - blks_size = (info.size - (blks_start - info.start)) & - (~(grp_size - 1)); - else - blks_size = 0; - - printf("Erasing blocks " LBAFU " to " LBAFU " due to alignment\n", - blks_start, blks_start + blks_size); - - blks = fb_mmc_blk_write(dev_desc, blks_start, blks_size, NULL); - - if (blks != blks_size) { + blks = fb_mmc_blk_erase(dev_desc, info.start, info.size); + if (blks != info.size) { pr_err("failed erasing from device %d\n", dev_desc->devnum); fastboot_fail("failed erasing from device", response); return; } - printf("........ erased " LBAFU " bytes from '%s'\n", - blks_size * info.blksz, cmd); + printf("........ erased " LBAFU " bytes from '%s'\n", blks * info.blksz, + cmd); fastboot_okay(NULL, response); }
Fastboot specification [1] requires MMC to be filled with 0xFF's on "fastboot erase" command: erase:%s Erase the indicated partition (clear to 0xFFs) Current "fastboot erase" implementation uses actual MMC erase operation blk_derase(), which can fill MMC either with 0x00 or 0xFF, depending on used MMC controller; from [2]: The content of an explicitly erased memory range shall be '0' or '1' depending on different memory technology. For example, on BeagleBoard X15 it fills memory with '0'. Furthermore, the minimal amount of memory blk_derase() can erase is "erase group size", and it's usually 512 KiB or more. So if we try to erase some partition which is smaller than 512 KiB, "fastboot erase" won't work at all, due to alignment. It's good practice to align all partitions to "erase group size" boundary, but it's not a strict requirement, so we just can't use blk_derase() due to this restriction. In order to provide the consistent way to erase partitions, adhere to the fastboot spec, and fix erasing of small partitions, let's use regular MMC write operation and fill the partition with 0xFF. [1] https://android.googlesource.com/platform/system/core/+/refs/tags/android-10.0.0_r25/fastboot/README.md#command-reference [2] https://www.jedec.org/system/files/docs/JESD84-A441.pdf Signed-off-by: Sam Protsenko <semen.protsenko at linaro.org> --- drivers/fastboot/fb_mmc.c | 90 +++++++++++++++++++++++++-------------- 1 file changed, 59 insertions(+), 31 deletions(-)