[PATCH] hw/sd/sdcard: Fix handling of disabled boot partitions

[PATCH] hw/sd/sdcard: Fix handling of disabled boot partitions

[Jan Luebbe]

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(-)

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) {
-- 
2.39.2

Re: [PATCH] hw/sd/sdcard: Fix handling of disabled boot partitions

[Peter Maydell]

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

Re: [PATCH] hw/sd/sdcard: Fix handling of disabled boot partitions

[Jan Lübbe]

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
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] hw/sd/sdcard: Fix handling of disabled boot partitions

[Peter Maydell]

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

Re: [PATCH] hw/sd/sdcard: Fix handling of disabled boot partitions

[Philippe Mathieu-Daudé]

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!