| Message ID | 20240906164834.130257-1-jlu@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | hw/sd/sdcard: Fix handling of disabled boot partitions | expand |
On Fri, 6 Sept 2024 at 17:51, Jan Luebbe <jlu@pengutronix.de> wrote: > > The enable bits in the EXT_CSD_PART_CONFIG ext_csd register do *not* > specify whether the boot partitions exist, but whether they are enabled > for booting. Existence of the boot partitions is specified by a > EXT_CSD_BOOT_MULT != 0. > > Currently, in the case of boot-partition-size=1M and boot-config=0, > Linux detects boot partitions of 1M. But as sd_bootpart_offset always > returns 0, all reads/writes are mapped to the same offset in the backing > file. > > Fix this bug by calculating the offset independent of which partition is > enabled for booting. Looking at the spec this change seems correct to me. Can you elaborate on when users might run into this bug? As far as I can see only aspeed.c sets boot-partition-size, and when it does so it also sets boot-config to 8. Or are we fixing this for the benefit of future board types? > Signed-off-by: Jan Luebbe <jlu@pengutronix.de> > --- > hw/sd/sd.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index a140a32ccd46..26d6eebe898d 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -774,19 +774,12 @@ static uint32_t sd_blk_len(SDState *sd) > */ > static uint32_t sd_bootpart_offset(SDState *sd) > { > - bool partitions_enabled; > unsigned partition_access; > > if (!sd->boot_part_size || !sd_is_emmc(sd)) { > return 0; > } > > - partitions_enabled = sd->ext_csd[EXT_CSD_PART_CONFIG] > - & EXT_CSD_PART_CONFIG_EN_MASK; > - if (!partitions_enabled) { > - return 0; > - } > - > partition_access = sd->ext_csd[EXT_CSD_PART_CONFIG] > & EXT_CSD_PART_CONFIG_ACC_MASK; > switch (partition_access) { > -- thanks -- PMM
On Mon, 2024-09-30 at 15:18 +0100, Peter Maydell wrote: > On Fri, 6 Sept 2024 at 17:51, Jan Luebbe <jlu@pengutronix.de> wrote: > > > > The enable bits in the EXT_CSD_PART_CONFIG ext_csd register do *not* > > specify whether the boot partitions exist, but whether they are enabled > > for booting. Existence of the boot partitions is specified by a > > EXT_CSD_BOOT_MULT != 0. > > > > Currently, in the case of boot-partition-size=1M and boot-config=0, > > Linux detects boot partitions of 1M. But as sd_bootpart_offset always > > returns 0, all reads/writes are mapped to the same offset in the backing > > file. > > > > Fix this bug by calculating the offset independent of which partition is > > enabled for booting. > > Looking at the spec this change seems correct to me. > > Can you elaborate on when users might run into this bug? > As far as I can see only aspeed.c sets boot-partition-size, > and when it does so it also sets boot-config to 8. Or are > we fixing this for the benefit of future board types? I stumbled across this when trying to use the eMMC emulation for the RAUC test suite (with some unrelated local hacks, which I still need to clean up for submission) [1]. Future boards would be affected as well. One other possible issue would be disabling the boot partition by using 'mmc bootpart enable 0 0 /dev/mmcblk0' (or similar) from Linux. The layout of the backing file shouldn't change in that case. Regards, Jan [1] https://github.com/rauc/rauc/tree/master/test
On Mon, 30 Sept 2024 at 21:05, Jan Lübbe <jlu@pengutronix.de> wrote: > > On Mon, 2024-09-30 at 15:18 +0100, Peter Maydell wrote: > > On Fri, 6 Sept 2024 at 17:51, Jan Luebbe <jlu@pengutronix.de> wrote: > > > > > > The enable bits in the EXT_CSD_PART_CONFIG ext_csd register do *not* > > > specify whether the boot partitions exist, but whether they are enabled > > > for booting. Existence of the boot partitions is specified by a > > > EXT_CSD_BOOT_MULT != 0. > > > > > > Currently, in the case of boot-partition-size=1M and boot-config=0, > > > Linux detects boot partitions of 1M. But as sd_bootpart_offset always > > > returns 0, all reads/writes are mapped to the same offset in the backing > > > file. > > > > > > Fix this bug by calculating the offset independent of which partition is > > > enabled for booting. > > > > Looking at the spec this change seems correct to me. > > > > Can you elaborate on when users might run into this bug? > > As far as I can see only aspeed.c sets boot-partition-size, > > and when it does so it also sets boot-config to 8. Or are > > we fixing this for the benefit of future board types? > > I stumbled across this when trying to use the eMMC emulation for the RAUC test > suite (with some unrelated local hacks, which I still need to clean up for > submission) [1]. Future boards would be affected as well. > > One other possible issue would be disabling the boot partition by using 'mmc > bootpart enable 0 0 /dev/mmcblk0' (or similar) from Linux. The layout of the > backing file shouldn't change in that case. Thanks for the clarification. I've applied this patch to target-arm.next with the following paragraph added to the commit message: This bug is unlikely to affect many users with QEMU's current set of boards, because only aspeed sets boot-partition-size, and it also sets boot-config to 8. So to run into this a user would have to manually mark the boot partition non-booting from within the guest. and I cc'd it to stable. -- PMM
On 1/10/24 15:01, Peter Maydell wrote: > On Mon, 30 Sept 2024 at 21:05, Jan Lübbe <jlu@pengutronix.de> wrote: >> >> On Mon, 2024-09-30 at 15:18 +0100, Peter Maydell wrote: >>> On Fri, 6 Sept 2024 at 17:51, Jan Luebbe <jlu@pengutronix.de> wrote: >>>> >>>> The enable bits in the EXT_CSD_PART_CONFIG ext_csd register do *not* >>>> specify whether the boot partitions exist, but whether they are enabled >>>> for booting. Existence of the boot partitions is specified by a >>>> EXT_CSD_BOOT_MULT != 0. >>>> >>>> Currently, in the case of boot-partition-size=1M and boot-config=0, >>>> Linux detects boot partitions of 1M. But as sd_bootpart_offset always >>>> returns 0, all reads/writes are mapped to the same offset in the backing >>>> file. >>>> >>>> Fix this bug by calculating the offset independent of which partition is >>>> enabled for booting. >>> >>> Looking at the spec this change seems correct to me. >>> >>> Can you elaborate on when users might run into this bug? >>> As far as I can see only aspeed.c sets boot-partition-size, >>> and when it does so it also sets boot-config to 8. Or are >>> we fixing this for the benefit of future board types? >> >> I stumbled across this when trying to use the eMMC emulation for the RAUC test >> suite (with some unrelated local hacks, which I still need to clean up for >> submission) [1]. Future boards would be affected as well. >> >> One other possible issue would be disabling the boot partition by using 'mmc >> bootpart enable 0 0 /dev/mmcblk0' (or similar) from Linux. The layout of the >> backing file shouldn't change in that case. > > Thanks for the clarification. I've applied this patch to > target-arm.next with the following paragraph added to the > commit message: > This bug is unlikely to affect many users with QEMU's current set of > boards, because only aspeed sets boot-partition-size, and it also > sets boot-config to 8. So to run into this a user would have to > manually mark the boot partition non-booting from within the guest. > > and I cc'd it to stable. Thanks Jan for the fix and Peter for merging this patch!
diff --git a/hw/sd/sd.c b/hw/sd/sd.c index a140a32ccd46..26d6eebe898d 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -774,19 +774,12 @@ static uint32_t sd_blk_len(SDState *sd) */ static uint32_t sd_bootpart_offset(SDState *sd) { - bool partitions_enabled; unsigned partition_access; if (!sd->boot_part_size || !sd_is_emmc(sd)) { return 0; } - partitions_enabled = sd->ext_csd[EXT_CSD_PART_CONFIG] - & EXT_CSD_PART_CONFIG_EN_MASK; - if (!partitions_enabled) { - return 0; - } - partition_access = sd->ext_csd[EXT_CSD_PART_CONFIG] & EXT_CSD_PART_CONFIG_ACC_MASK; switch (partition_access) {
The enable bits in the EXT_CSD_PART_CONFIG ext_csd register do *not* specify whether the boot partitions exist, but whether they are enabled for booting. Existence of the boot partitions is specified by a EXT_CSD_BOOT_MULT != 0. Currently, in the case of boot-partition-size=1M and boot-config=0, Linux detects boot partitions of 1M. But as sd_bootpart_offset always returns 0, all reads/writes are mapped to the same offset in the backing file. Fix this bug by calculating the offset independent of which partition is enabled for booting. Signed-off-by: Jan Luebbe <jlu@pengutronix.de> --- hw/sd/sd.c | 7 ------- 1 file changed, 7 deletions(-)