[PATCH v4 1/6] hw/sd/sdcard: Fix size check for backing block image

Jan Kiszka posted 6 patches 4 months, 4 weeks ago
Maintainers: "Philippe Mathieu-Daudé" <philmd@linaro.org>, Bin Meng <bmeng.cn@gmail.com>
There is a newer version of this series
[PATCH v4 1/6] hw/sd/sdcard: Fix size check for backing block image
Posted by Jan Kiszka 4 months, 4 weeks ago
From: Jan Kiszka <jan.kiszka@siemens.com>

Alignment rules apply the the individual partitions (user, boot, later
on also RPMB) and depend both on the size of the image and the type of
the device. Up to and including 2GB, the power-of-2 rule applies to the
user data area. For larger images, multiples of 512 sectors must be used
for eMMC and multiples of 512K for SD-cards. Fix the check accordingly
and also detect if the image is too small to even hold the boot
partitions.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Warner Losh <imp@bsdimp.com>
---
CC: Warner Losh <imp@bsdimp.com>
CC: Cédric Le Goater <clg@kaod.org>
CC: Joel Stanley <joel@jms.id.au>
CC: Alistair Francis <alistair@alistair23.me>
CC: Alexander Bulekov <alxndr@bu.edu>
---
 hw/sd/sd.c | 61 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 19 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index d7a496d77c..b42cd01d1f 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2759,6 +2759,28 @@ static void sd_instance_finalize(Object *obj)
     timer_free(sd->ocr_power_timer);
 }
 
+static void sd_blk_size_error(SDState *sd, int64_t blk_size,
+                              int64_t blk_size_aligned, const char *rule,
+                              Error **errp)
+{
+    const char *dev_type = sd_is_emmc(sd) ? "eMMC" : "SD card";
+    char *blk_size_str;
+
+    blk_size_str = size_to_str(blk_size);
+    error_setg(errp, "Invalid %s size: %s", dev_type, blk_size_str);
+    g_free(blk_size_str);
+
+    blk_size_str = size_to_str(blk_size_aligned);
+    error_append_hint(errp,
+                      "%s size has to be %s, e.g. %s.\n"
+                      "You can resize disk images with"
+                      " 'qemu-img resize <imagefile> <new-size>'\n"
+                      "(note that this will lose data if you make the"
+                      " image smaller than it currently is).\n",
+                      dev_type, rule, blk_size_str);
+    g_free(blk_size_str);
+}
+
 static void sd_realize(DeviceState *dev, Error **errp)
 {
     SDState *sd = SDMMC_COMMON(dev);
@@ -2781,25 +2803,26 @@ static void sd_realize(DeviceState *dev, Error **errp)
             return;
         }
 
-        blk_size = blk_getlength(sd->blk);
-        if (blk_size > 0 && !is_power_of_2(blk_size)) {
-            int64_t blk_size_aligned = pow2ceil(blk_size);
-            char *blk_size_str;
-
-            blk_size_str = size_to_str(blk_size);
-            error_setg(errp, "Invalid SD card size: %s", blk_size_str);
-            g_free(blk_size_str);
-
-            blk_size_str = size_to_str(blk_size_aligned);
-            error_append_hint(errp,
-                              "SD card size has to be a power of 2, e.g. %s.\n"
-                              "You can resize disk images with"
-                              " 'qemu-img resize <imagefile> <new-size>'\n"
-                              "(note that this will lose data if you make the"
-                              " image smaller than it currently is).\n",
-                              blk_size_str);
-            g_free(blk_size_str);
-
+        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
+        if (blk_size > SDSC_MAX_CAPACITY) {
+            if (sd_is_emmc(sd) && blk_size % (1 << HWBLOCK_SHIFT) != 0) {
+                int64_t blk_size_aligned =
+                    ((blk_size >> HWBLOCK_SHIFT) + 1) << HWBLOCK_SHIFT;
+                sd_blk_size_error(sd, blk_size, blk_size_aligned,
+                                  "multiples of 512", errp);
+                return;
+            } else if (!sd_is_emmc(sd) && blk_size % (512 * KiB)) {
+                int64_t blk_size_aligned = ((blk_size >> 19) + 1) << 19;
+                sd_blk_size_error(sd, blk_size, blk_size_aligned,
+                                  "multiples of 512K", errp);
+                return;
+            }
+        } else if (blk_size > 0 && !is_power_of_2(blk_size)) {
+            sd_blk_size_error(sd, blk_size, pow2ceil(blk_size), "a power of 2",
+                              errp);
+            return;
+        } else if (blk_size < 0) {
+            error_setg(errp, "eMMC image smaller than boot partitions");
             return;
         }
 
-- 
2.51.0


Re: [PATCH v4 1/6] hw/sd/sdcard: Fix size check for backing block image
Posted by Jan Kiszka 4 months, 3 weeks ago
On 14.09.25 14:46, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> Alignment rules apply the the individual partitions (user, boot, later
> on also RPMB) and depend both on the size of the image and the type of
> the device. Up to and including 2GB, the power-of-2 rule applies to the
> user data area. For larger images, multiples of 512 sectors must be used
> for eMMC and multiples of 512K for SD-cards. Fix the check accordingly
> and also detect if the image is too small to even hold the boot
> partitions.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> Reviewed-by: Warner Losh <imp@bsdimp.com>
> ---
> CC: Warner Losh <imp@bsdimp.com>
> CC: Cédric Le Goater <clg@kaod.org>
> CC: Joel Stanley <joel@jms.id.au>
> CC: Alistair Francis <alistair@alistair23.me>
> CC: Alexander Bulekov <alxndr@bu.edu>
> ---
>  hw/sd/sd.c | 61 +++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 42 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index d7a496d77c..b42cd01d1f 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2759,6 +2759,28 @@ static void sd_instance_finalize(Object *obj)
>      timer_free(sd->ocr_power_timer);
>  }
>  
> +static void sd_blk_size_error(SDState *sd, int64_t blk_size,
> +                              int64_t blk_size_aligned, const char *rule,
> +                              Error **errp)
> +{
> +    const char *dev_type = sd_is_emmc(sd) ? "eMMC" : "SD card";
> +    char *blk_size_str;
> +
> +    blk_size_str = size_to_str(blk_size);
> +    error_setg(errp, "Invalid %s size: %s", dev_type, blk_size_str);
> +    g_free(blk_size_str);
> +
> +    blk_size_str = size_to_str(blk_size_aligned);
> +    error_append_hint(errp,
> +                      "%s size has to be %s, e.g. %s.\n"
> +                      "You can resize disk images with"
> +                      " 'qemu-img resize <imagefile> <new-size>'\n"
> +                      "(note that this will lose data if you make the"
> +                      " image smaller than it currently is).\n",
> +                      dev_type, rule, blk_size_str);
> +    g_free(blk_size_str);
> +}
> +
>  static void sd_realize(DeviceState *dev, Error **errp)
>  {
>      SDState *sd = SDMMC_COMMON(dev);
> @@ -2781,25 +2803,26 @@ static void sd_realize(DeviceState *dev, Error **errp)
>              return;
>          }
>  
> -        blk_size = blk_getlength(sd->blk);
> -        if (blk_size > 0 && !is_power_of_2(blk_size)) {
> -            int64_t blk_size_aligned = pow2ceil(blk_size);
> -            char *blk_size_str;
> -
> -            blk_size_str = size_to_str(blk_size);
> -            error_setg(errp, "Invalid SD card size: %s", blk_size_str);
> -            g_free(blk_size_str);
> -
> -            blk_size_str = size_to_str(blk_size_aligned);
> -            error_append_hint(errp,
> -                              "SD card size has to be a power of 2, e.g. %s.\n"
> -                              "You can resize disk images with"
> -                              " 'qemu-img resize <imagefile> <new-size>'\n"
> -                              "(note that this will lose data if you make the"
> -                              " image smaller than it currently is).\n",
> -                              blk_size_str);
> -            g_free(blk_size_str);
> -
> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
> +        if (blk_size > SDSC_MAX_CAPACITY) {
> +            if (sd_is_emmc(sd) && blk_size % (1 << HWBLOCK_SHIFT) != 0) {
> +                int64_t blk_size_aligned =
> +                    ((blk_size >> HWBLOCK_SHIFT) + 1) << HWBLOCK_SHIFT;
> +                sd_blk_size_error(sd, blk_size, blk_size_aligned,
> +                                  "multiples of 512", errp);
> +                return;
> +            } else if (!sd_is_emmc(sd) && blk_size % (512 * KiB)) {
> +                int64_t blk_size_aligned = ((blk_size >> 19) + 1) << 19;
> +                sd_blk_size_error(sd, blk_size, blk_size_aligned,
> +                                  "multiples of 512K", errp);
> +                return;
> +            }
> +        } else if (blk_size > 0 && !is_power_of_2(blk_size)) {
> +            sd_blk_size_error(sd, blk_size, pow2ceil(blk_size), "a power of 2",
> +                              errp);
> +            return;
> +        } else if (blk_size < 0) {
> +            error_setg(errp, "eMMC image smaller than boot partitions");

Cedric, I just played with some ast* machines and noticed that they now
trigger that error above when no eMMC disk image is specified
("qemu-system-aarch64 -M ast2700a1-evb"). Is that a valid error, i.e. we
shouldn't have tried to boot without an eMMC at all so far, or would
this be a regression?

Jan

>              return;
>          }
>  

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH v4 1/6] hw/sd/sdcard: Fix size check for backing block image
Posted by Cédric Le Goater 4 months, 3 weeks ago
+ Jamin

On 9/16/25 13:39, Jan Kiszka wrote:
> On 14.09.25 14:46, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Alignment rules apply the the individual partitions (user, boot, later
>> on also RPMB) and depend both on the size of the image and the type of
>> the device. Up to and including 2GB, the power-of-2 rule applies to the
>> user data area. For larger images, multiples of 512 sectors must be used
>> for eMMC and multiples of 512K for SD-cards. Fix the check accordingly
>> and also detect if the image is too small to even hold the boot
>> partitions.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> Reviewed-by: Warner Losh <imp@bsdimp.com>
>> ---
>> CC: Warner Losh <imp@bsdimp.com>
>> CC: Cédric Le Goater <clg@kaod.org>
>> CC: Joel Stanley <joel@jms.id.au>
>> CC: Alistair Francis <alistair@alistair23.me>
>> CC: Alexander Bulekov <alxndr@bu.edu>
>> ---
>>   hw/sd/sd.c | 61 +++++++++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 42 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index d7a496d77c..b42cd01d1f 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -2759,6 +2759,28 @@ static void sd_instance_finalize(Object *obj)
>>       timer_free(sd->ocr_power_timer);
>>   }
>>   
>> +static void sd_blk_size_error(SDState *sd, int64_t blk_size,
>> +                              int64_t blk_size_aligned, const char *rule,
>> +                              Error **errp)
>> +{
>> +    const char *dev_type = sd_is_emmc(sd) ? "eMMC" : "SD card";
>> +    char *blk_size_str;
>> +
>> +    blk_size_str = size_to_str(blk_size);
>> +    error_setg(errp, "Invalid %s size: %s", dev_type, blk_size_str);
>> +    g_free(blk_size_str);
>> +
>> +    blk_size_str = size_to_str(blk_size_aligned);
>> +    error_append_hint(errp,
>> +                      "%s size has to be %s, e.g. %s.\n"
>> +                      "You can resize disk images with"
>> +                      " 'qemu-img resize <imagefile> <new-size>'\n"
>> +                      "(note that this will lose data if you make the"
>> +                      " image smaller than it currently is).\n",
>> +                      dev_type, rule, blk_size_str);
>> +    g_free(blk_size_str);
>> +}
>> +
>>   static void sd_realize(DeviceState *dev, Error **errp)
>>   {
>>       SDState *sd = SDMMC_COMMON(dev);
>> @@ -2781,25 +2803,26 @@ static void sd_realize(DeviceState *dev, Error **errp)
>>               return;
>>           }
>>   
>> -        blk_size = blk_getlength(sd->blk);
>> -        if (blk_size > 0 && !is_power_of_2(blk_size)) {
>> -            int64_t blk_size_aligned = pow2ceil(blk_size);
>> -            char *blk_size_str;
>> -
>> -            blk_size_str = size_to_str(blk_size);
>> -            error_setg(errp, "Invalid SD card size: %s", blk_size_str);
>> -            g_free(blk_size_str);
>> -
>> -            blk_size_str = size_to_str(blk_size_aligned);
>> -            error_append_hint(errp,
>> -                              "SD card size has to be a power of 2, e.g. %s.\n"
>> -                              "You can resize disk images with"
>> -                              " 'qemu-img resize <imagefile> <new-size>'\n"
>> -                              "(note that this will lose data if you make the"
>> -                              " image smaller than it currently is).\n",
>> -                              blk_size_str);
>> -            g_free(blk_size_str);
>> -
>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>> +        if (blk_size > SDSC_MAX_CAPACITY) {
>> +            if (sd_is_emmc(sd) && blk_size % (1 << HWBLOCK_SHIFT) != 0) {
>> +                int64_t blk_size_aligned =
>> +                    ((blk_size >> HWBLOCK_SHIFT) + 1) << HWBLOCK_SHIFT;
>> +                sd_blk_size_error(sd, blk_size, blk_size_aligned,
>> +                                  "multiples of 512", errp);
>> +                return;
>> +            } else if (!sd_is_emmc(sd) && blk_size % (512 * KiB)) {
>> +                int64_t blk_size_aligned = ((blk_size >> 19) + 1) << 19;
>> +                sd_blk_size_error(sd, blk_size, blk_size_aligned,
>> +                                  "multiples of 512K", errp);
>> +                return;
>> +            }
>> +        } else if (blk_size > 0 && !is_power_of_2(blk_size)) {
>> +            sd_blk_size_error(sd, blk_size, pow2ceil(blk_size), "a power of 2",
>> +                              errp);
>> +            return;
>> +        } else if (blk_size < 0) {
>> +            error_setg(errp, "eMMC image smaller than boot partitions");
> 
> Cedric, I just played with some ast* machines and noticed that they now
> trigger that error above when no eMMC disk image is specified
> ("qemu-system-aarch64 -M ast2700a1-evb"). Is that a valid error, i.e. we
> shouldn't have tried to boot without an eMMC at all so far, or would
> this be a regression?

Only the ast2600-evb and the rainier-bmc have eMMC support.
I don't think the ast2700a1-evb has eMMC support. Jamin ?



The rainier-bmc boots by default from eMMC. Nothing really
special about the image, the first boot partition includes
the u-boot-spl.bin and u-boot.bin images at expected offset.
The machine model loads the u-boot-spl.bin contents as a ROM.

The ast2600-evb machine boots from flash. To add an eMMC drive
(needs to be the 3rd 'sd' drive), use this command line  :

     $ qemu-system-arm -M ast2600-evb -net nic,netdev=net0 -netdev user,id=net0 \
       -drive file=./v09.07/ast2600-default/image-bmc,format=raw,if=mtd -serial mon:stdio \
       -drive file=mmc-ast2600-evb-noboot.qcow2,format=qcow2,if=sd,id=sd2,index=2
   
     ....
     U-Boot 2019.04-v00.04.22 (Jun 17 2025 - 08:57:39 +0000)
     
     SOC: AST2600-A3
     eSPI Mode: SIO:Enable : SuperIO-2e
     Eth: MAC0: RGMII, MAC1: RGMII, MAC2: RGMII, MAC3: RGMII
     Model: AST2600 EVB
     DRAM:  already initialized, 1008 MiB (capacity:1024 MiB, VGA:64 MiB, ECC:off)
     RC Bridge phy@1e6ed200 : Link up
     MMC:   sdhci_slot0@100: 1, sdhci_slot1@200: 2, emmc_slot0@100: 0
     
     ....
     [    4.209117] mmc0: new high speed MMC card at address 0001
     [    4.211765] mmcblk0: mmc0:0001 QEMU!! 16.0 GiB
     [    4.233637] GPT:Primary header thinks Alt. header is not at the end of the disk.
     [    4.233995] GPT:29624393 != 33554431
     [    4.234161] GPT:Alternate GPT header not at the end of the disk.
     [    4.234399] GPT:29624393 != 33554431
     [    4.234549] GPT: Use GNU Parted to correct GPT errors.
     [    4.235223]  mmcblk0: p1 p2 p3 p4 p5 p6 p7
     

Thanks,

C.



RE: [PATCH v4 1/6] hw/sd/sdcard: Fix size check for backing block image
Posted by Jamin Lin 4 months, 3 weeks ago
> Only the ast2600-evb and the rainier-bmc have eMMC support.
> I don't think the ast2700a1-evb has eMMC support. Jamin ?
>
Yes, the AST2700 does not support booting from eMMC yet.
Jamin
> 
> 
> The rainier-bmc boots by default from eMMC. Nothing really special about the
> image, the first boot partition includes the u-boot-spl.bin and u-boot.bin
> images at expected offset.
> The machine model loads the u-boot-spl.bin contents as a ROM.
> 
> The ast2600-evb machine boots from flash. To add an eMMC drive (needs to
> be the 3rd 'sd' drive), use this command line  :
> 
>      $ qemu-system-arm -M ast2600-evb -net nic,netdev=net0 -netdev
> user,id=net0 \
>        -drive file=./v09.07/ast2600-default/image-bmc,format=raw,if=mtd
> -serial mon:stdio \
>        -drive
> file=mmc-ast2600-evb-noboot.qcow2,format=qcow2,if=sd,id=sd2,index=2
> 
>      ....
>      U-Boot 2019.04-v00.04.22 (Jun 17 2025 - 08:57:39 +0000)
> 
>      SOC: AST2600-A3
>      eSPI Mode: SIO:Enable : SuperIO-2e
>      Eth: MAC0: RGMII, MAC1: RGMII, MAC2: RGMII, MAC3: RGMII
>      Model: AST2600 EVB
>      DRAM:  already initialized, 1008 MiB (capacity:1024 MiB, VGA:64 MiB,
> ECC:off)
>      RC Bridge phy@1e6ed200 : Link up
>      MMC:   sdhci_slot0@100: 1, sdhci_slot1@200: 2, emmc_slot0@100: 0
> 
>      ....
>      [    4.209117] mmc0: new high speed MMC card at address 0001
>      [    4.211765] mmcblk0: mmc0:0001 QEMU!! 16.0 GiB
>      [    4.233637] GPT:Primary header thinks Alt. header is not at the end
> of the disk.
>      [    4.233995] GPT:29624393 != 33554431
>      [    4.234161] GPT:Alternate GPT header not at the end of the disk.
>      [    4.234399] GPT:29624393 != 33554431
>      [    4.234549] GPT: Use GNU Parted to correct GPT errors.
>      [    4.235223]  mmcblk0: p1 p2 p3 p4 p5 p6 p7
> 
> 
> Thanks,
> 
> C.
> 

Re: [PATCH v4 1/6] hw/sd/sdcard: Fix size check for backing block image
Posted by Jan Kiszka 4 months, 3 weeks ago
On 16.09.25 18:14, Cédric Le Goater wrote:
> + Jamin
> 
> On 9/16/25 13:39, Jan Kiszka wrote:
>> On 14.09.25 14:46, Jan Kiszka wrote:
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> Alignment rules apply the the individual partitions (user, boot, later
>>> on also RPMB) and depend both on the size of the image and the type of
>>> the device. Up to and including 2GB, the power-of-2 rule applies to the
>>> user data area. For larger images, multiples of 512 sectors must be used
>>> for eMMC and multiples of 512K for SD-cards. Fix the check accordingly
>>> and also detect if the image is too small to even hold the boot
>>> partitions.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> Reviewed-by: Warner Losh <imp@bsdimp.com>
>>> ---
>>> CC: Warner Losh <imp@bsdimp.com>
>>> CC: Cédric Le Goater <clg@kaod.org>
>>> CC: Joel Stanley <joel@jms.id.au>
>>> CC: Alistair Francis <alistair@alistair23.me>
>>> CC: Alexander Bulekov <alxndr@bu.edu>
>>> ---
>>>   hw/sd/sd.c | 61 +++++++++++++++++++++++++++++++++++++-----------------
>>>   1 file changed, 42 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index d7a496d77c..b42cd01d1f 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -2759,6 +2759,28 @@ static void sd_instance_finalize(Object *obj)
>>>       timer_free(sd->ocr_power_timer);
>>>   }
>>>   +static void sd_blk_size_error(SDState *sd, int64_t blk_size,
>>> +                              int64_t blk_size_aligned, const char
>>> *rule,
>>> +                              Error **errp)
>>> +{
>>> +    const char *dev_type = sd_is_emmc(sd) ? "eMMC" : "SD card";
>>> +    char *blk_size_str;
>>> +
>>> +    blk_size_str = size_to_str(blk_size);
>>> +    error_setg(errp, "Invalid %s size: %s", dev_type, blk_size_str);
>>> +    g_free(blk_size_str);
>>> +
>>> +    blk_size_str = size_to_str(blk_size_aligned);
>>> +    error_append_hint(errp,
>>> +                      "%s size has to be %s, e.g. %s.\n"
>>> +                      "You can resize disk images with"
>>> +                      " 'qemu-img resize <imagefile> <new-size>'\n"
>>> +                      "(note that this will lose data if you make the"
>>> +                      " image smaller than it currently is).\n",
>>> +                      dev_type, rule, blk_size_str);
>>> +    g_free(blk_size_str);
>>> +}
>>> +
>>>   static void sd_realize(DeviceState *dev, Error **errp)
>>>   {
>>>       SDState *sd = SDMMC_COMMON(dev);
>>> @@ -2781,25 +2803,26 @@ static void sd_realize(DeviceState *dev,
>>> Error **errp)
>>>               return;
>>>           }
>>>   -        blk_size = blk_getlength(sd->blk);
>>> -        if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>> -            int64_t blk_size_aligned = pow2ceil(blk_size);
>>> -            char *blk_size_str;
>>> -
>>> -            blk_size_str = size_to_str(blk_size);
>>> -            error_setg(errp, "Invalid SD card size: %s", blk_size_str);
>>> -            g_free(blk_size_str);
>>> -
>>> -            blk_size_str = size_to_str(blk_size_aligned);
>>> -            error_append_hint(errp,
>>> -                              "SD card size has to be a power of 2,
>>> e.g. %s.\n"
>>> -                              "You can resize disk images with"
>>> -                              " 'qemu-img resize <imagefile> <new-
>>> size>'\n"
>>> -                              "(note that this will lose data if you
>>> make the"
>>> -                              " image smaller than it currently is).
>>> \n",
>>> -                              blk_size_str);
>>> -            g_free(blk_size_str);
>>> -
>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>>> +        if (blk_size > SDSC_MAX_CAPACITY) {
>>> +            if (sd_is_emmc(sd) && blk_size % (1 << HWBLOCK_SHIFT) !=
>>> 0) {
>>> +                int64_t blk_size_aligned =
>>> +                    ((blk_size >> HWBLOCK_SHIFT) + 1) << HWBLOCK_SHIFT;
>>> +                sd_blk_size_error(sd, blk_size, blk_size_aligned,
>>> +                                  "multiples of 512", errp);
>>> +                return;
>>> +            } else if (!sd_is_emmc(sd) && blk_size % (512 * KiB)) {
>>> +                int64_t blk_size_aligned = ((blk_size >> 19) + 1) <<
>>> 19;
>>> +                sd_blk_size_error(sd, blk_size, blk_size_aligned,
>>> +                                  "multiples of 512K", errp);
>>> +                return;
>>> +            }
>>> +        } else if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>> +            sd_blk_size_error(sd, blk_size, pow2ceil(blk_size), "a
>>> power of 2",
>>> +                              errp);
>>> +            return;
>>> +        } else if (blk_size < 0) {
>>> +            error_setg(errp, "eMMC image smaller than boot
>>> partitions");
>>
>> Cedric, I just played with some ast* machines and noticed that they now
>> trigger that error above when no eMMC disk image is specified
>> ("qemu-system-aarch64 -M ast2700a1-evb"). Is that a valid error, i.e. we
>> shouldn't have tried to boot without an eMMC at all so far, or would
>> this be a regression?
> 
> Only the ast2600-evb and the rainier-bmc have eMMC support.
> I don't think the ast2700a1-evb has eMMC support. Jamin ?
> 
> 
> 
> The rainier-bmc boots by default from eMMC. Nothing really
> special about the image, the first boot partition includes
> the u-boot-spl.bin and u-boot.bin images at expected offset.
> The machine model loads the u-boot-spl.bin contents as a ROM.
> 
> The ast2600-evb machine boots from flash. To add an eMMC drive
> (needs to be the 3rd 'sd' drive), use this command line  :
> 
>     $ qemu-system-arm -M ast2600-evb -net nic,netdev=net0 -netdev
> user,id=net0 \
>       -drive file=./v09.07/ast2600-default/image-bmc,format=raw,if=mtd -
> serial mon:stdio \
>       -drive file=mmc-ast2600-evb-
> noboot.qcow2,format=qcow2,if=sd,id=sd2,index=2
>       ....
>     U-Boot 2019.04-v00.04.22 (Jun 17 2025 - 08:57:39 +0000)
>         SOC: AST2600-A3
>     eSPI Mode: SIO:Enable : SuperIO-2e
>     Eth: MAC0: RGMII, MAC1: RGMII, MAC2: RGMII, MAC3: RGMII
>     Model: AST2600 EVB
>     DRAM:  already initialized, 1008 MiB (capacity:1024 MiB, VGA:64 MiB,
> ECC:off)
>     RC Bridge phy@1e6ed200 : Link up
>     MMC:   sdhci_slot0@100: 1, sdhci_slot1@200: 2, emmc_slot0@100: 0
>         ....
>     [    4.209117] mmc0: new high speed MMC card at address 0001
>     [    4.211765] mmcblk0: mmc0:0001 QEMU!! 16.0 GiB
>     [    4.233637] GPT:Primary header thinks Alt. header is not at the
> end of the disk.
>     [    4.233995] GPT:29624393 != 33554431
>     [    4.234161] GPT:Alternate GPT header not at the end of the disk.
>     [    4.234399] GPT:29624393 != 33554431
>     [    4.234549] GPT: Use GNU Parted to correct GPT errors.
>     [    4.235223]  mmcblk0: p1 p2 p3 p4 p5 p6 p7
>    

$ ./qemu-system-arm -M ast2600-evb
qemu-system-arm: eMMC image smaller than boot partitions
$ ./qemu-system-arm -M ast2600-evb -drive file=disk.image,if=sd
<works if disk.image is large enough>

Is that ok?

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH v4 1/6] hw/sd/sdcard: Fix size check for backing block image
Posted by Cédric Le Goater 4 months, 3 weeks ago
On 9/16/25 19:17, Jan Kiszka wrote:
> On 16.09.25 18:14, Cédric Le Goater wrote:
>> + Jamin
>>
>> On 9/16/25 13:39, Jan Kiszka wrote:
>>> On 14.09.25 14:46, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> Alignment rules apply the the individual partitions (user, boot, later
>>>> on also RPMB) and depend both on the size of the image and the type of
>>>> the device. Up to and including 2GB, the power-of-2 rule applies to the
>>>> user data area. For larger images, multiples of 512 sectors must be used
>>>> for eMMC and multiples of 512K for SD-cards. Fix the check accordingly
>>>> and also detect if the image is too small to even hold the boot
>>>> partitions.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> Reviewed-by: Warner Losh <imp@bsdimp.com>
>>>> ---
>>>> CC: Warner Losh <imp@bsdimp.com>
>>>> CC: Cédric Le Goater <clg@kaod.org>
>>>> CC: Joel Stanley <joel@jms.id.au>
>>>> CC: Alistair Francis <alistair@alistair23.me>
>>>> CC: Alexander Bulekov <alxndr@bu.edu>
>>>> ---
>>>>    hw/sd/sd.c | 61 +++++++++++++++++++++++++++++++++++++-----------------
>>>>    1 file changed, 42 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>> index d7a496d77c..b42cd01d1f 100644
>>>> --- a/hw/sd/sd.c
>>>> +++ b/hw/sd/sd.c
>>>> @@ -2759,6 +2759,28 @@ static void sd_instance_finalize(Object *obj)
>>>>        timer_free(sd->ocr_power_timer);
>>>>    }
>>>>    +static void sd_blk_size_error(SDState *sd, int64_t blk_size,
>>>> +                              int64_t blk_size_aligned, const char
>>>> *rule,
>>>> +                              Error **errp)
>>>> +{
>>>> +    const char *dev_type = sd_is_emmc(sd) ? "eMMC" : "SD card";
>>>> +    char *blk_size_str;
>>>> +
>>>> +    blk_size_str = size_to_str(blk_size);
>>>> +    error_setg(errp, "Invalid %s size: %s", dev_type, blk_size_str);
>>>> +    g_free(blk_size_str);
>>>> +
>>>> +    blk_size_str = size_to_str(blk_size_aligned);
>>>> +    error_append_hint(errp,
>>>> +                      "%s size has to be %s, e.g. %s.\n"
>>>> +                      "You can resize disk images with"
>>>> +                      " 'qemu-img resize <imagefile> <new-size>'\n"
>>>> +                      "(note that this will lose data if you make the"
>>>> +                      " image smaller than it currently is).\n",
>>>> +                      dev_type, rule, blk_size_str);
>>>> +    g_free(blk_size_str);
>>>> +}
>>>> +
>>>>    static void sd_realize(DeviceState *dev, Error **errp)
>>>>    {
>>>>        SDState *sd = SDMMC_COMMON(dev);
>>>> @@ -2781,25 +2803,26 @@ static void sd_realize(DeviceState *dev,
>>>> Error **errp)
>>>>                return;
>>>>            }
>>>>    -        blk_size = blk_getlength(sd->blk);
>>>> -        if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>> -            int64_t blk_size_aligned = pow2ceil(blk_size);
>>>> -            char *blk_size_str;
>>>> -
>>>> -            blk_size_str = size_to_str(blk_size);
>>>> -            error_setg(errp, "Invalid SD card size: %s", blk_size_str);
>>>> -            g_free(blk_size_str);
>>>> -
>>>> -            blk_size_str = size_to_str(blk_size_aligned);
>>>> -            error_append_hint(errp,
>>>> -                              "SD card size has to be a power of 2,
>>>> e.g. %s.\n"
>>>> -                              "You can resize disk images with"
>>>> -                              " 'qemu-img resize <imagefile> <new-
>>>> size>'\n"
>>>> -                              "(note that this will lose data if you
>>>> make the"
>>>> -                              " image smaller than it currently is).
>>>> \n",
>>>> -                              blk_size_str);
>>>> -            g_free(blk_size_str);
>>>> -
>>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>>>> +        if (blk_size > SDSC_MAX_CAPACITY) {
>>>> +            if (sd_is_emmc(sd) && blk_size % (1 << HWBLOCK_SHIFT) !=
>>>> 0) {
>>>> +                int64_t blk_size_aligned =
>>>> +                    ((blk_size >> HWBLOCK_SHIFT) + 1) << HWBLOCK_SHIFT;
>>>> +                sd_blk_size_error(sd, blk_size, blk_size_aligned,
>>>> +                                  "multiples of 512", errp);
>>>> +                return;
>>>> +            } else if (!sd_is_emmc(sd) && blk_size % (512 * KiB)) {
>>>> +                int64_t blk_size_aligned = ((blk_size >> 19) + 1) <<
>>>> 19;
>>>> +                sd_blk_size_error(sd, blk_size, blk_size_aligned,
>>>> +                                  "multiples of 512K", errp);
>>>> +                return;
>>>> +            }
>>>> +        } else if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>> +            sd_blk_size_error(sd, blk_size, pow2ceil(blk_size), "a
>>>> power of 2",
>>>> +                              errp);
>>>> +            return;
>>>> +        } else if (blk_size < 0) {
>>>> +            error_setg(errp, "eMMC image smaller than boot
>>>> partitions");
>>>
>>> Cedric, I just played with some ast* machines and noticed that they now
>>> trigger that error above when no eMMC disk image is specified
>>> ("qemu-system-aarch64 -M ast2700a1-evb"). Is that a valid error, i.e. we
>>> shouldn't have tried to boot without an eMMC at all so far, or would
>>> this be a regression?
>>
>> Only the ast2600-evb and the rainier-bmc have eMMC support.
>> I don't think the ast2700a1-evb has eMMC support. Jamin ?
>>
>>
>>
>> The rainier-bmc boots by default from eMMC. Nothing really
>> special about the image, the first boot partition includes
>> the u-boot-spl.bin and u-boot.bin images at expected offset.
>> The machine model loads the u-boot-spl.bin contents as a ROM.
>>
>> The ast2600-evb machine boots from flash. To add an eMMC drive
>> (needs to be the 3rd 'sd' drive), use this command line  :
>>
>>      $ qemu-system-arm -M ast2600-evb -net nic,netdev=net0 -netdev
>> user,id=net0 \
>>        -drive file=./v09.07/ast2600-default/image-bmc,format=raw,if=mtd -
>> serial mon:stdio \
>>        -drive file=mmc-ast2600-evb-
>> noboot.qcow2,format=qcow2,if=sd,id=sd2,index=2
>>        ....
>>      U-Boot 2019.04-v00.04.22 (Jun 17 2025 - 08:57:39 +0000)
>>          SOC: AST2600-A3
>>      eSPI Mode: SIO:Enable : SuperIO-2e
>>      Eth: MAC0: RGMII, MAC1: RGMII, MAC2: RGMII, MAC3: RGMII
>>      Model: AST2600 EVB
>>      DRAM:  already initialized, 1008 MiB (capacity:1024 MiB, VGA:64 MiB,
>> ECC:off)
>>      RC Bridge phy@1e6ed200 : Link up
>>      MMC:   sdhci_slot0@100: 1, sdhci_slot1@200: 2, emmc_slot0@100: 0
>>          ....
>>      [    4.209117] mmc0: new high speed MMC card at address 0001
>>      [    4.211765] mmcblk0: mmc0:0001 QEMU!! 16.0 GiB
>>      [    4.233637] GPT:Primary header thinks Alt. header is not at the
>> end of the disk.
>>      [    4.233995] GPT:29624393 != 33554431
>>      [    4.234161] GPT:Alternate GPT header not at the end of the disk.
>>      [    4.234399] GPT:29624393 != 33554431
>>      [    4.234549] GPT: Use GNU Parted to correct GPT errors.
>>      [    4.235223]  mmcblk0: p1 p2 p3 p4 p5 p6 p7
>>     
> 
> $ ./qemu-system-arm -M ast2600-evb
> qemu-system-arm: eMMC image smaller than boot partitions
> $ ./qemu-system-arm -M ast2600-evb -drive file=disk.image,if=sd
> <works if disk.image is large enough>
> 
> Is that ok?

No. This is wrong.

An sd card device is auto created at init time and a 'DriveInfo *'
is always available for index 0. 'mc->auto_create_sdcard' should
be set to false IMO.

commit cdc8d7cadaac ("hw/boards: Rename no_sdcard ->
auto_create_sdcard") seems to have changed the behavior of
several machines.  See 'make check'.

C.

Re: [PATCH v4 1/6] hw/sd/sdcard: Fix size check for backing block image
Posted by Jan Kiszka 4 months, 1 week ago
On 17.09.25 07:53, Cédric Le Goater wrote:
> On 9/16/25 19:17, Jan Kiszka wrote:
>> On 16.09.25 18:14, Cédric Le Goater wrote:
>>> + Jamin
>>>
>>> On 9/16/25 13:39, Jan Kiszka wrote:
>>>> On 14.09.25 14:46, Jan Kiszka wrote:
>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> Alignment rules apply the the individual partitions (user, boot, later
>>>>> on also RPMB) and depend both on the size of the image and the type of
>>>>> the device. Up to and including 2GB, the power-of-2 rule applies to
>>>>> the
>>>>> user data area. For larger images, multiples of 512 sectors must be
>>>>> used
>>>>> for eMMC and multiples of 512K for SD-cards. Fix the check accordingly
>>>>> and also detect if the image is too small to even hold the boot
>>>>> partitions.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> Reviewed-by: Warner Losh <imp@bsdimp.com>
>>>>> ---
>>>>> CC: Warner Losh <imp@bsdimp.com>
>>>>> CC: Cédric Le Goater <clg@kaod.org>
>>>>> CC: Joel Stanley <joel@jms.id.au>
>>>>> CC: Alistair Francis <alistair@alistair23.me>
>>>>> CC: Alexander Bulekov <alxndr@bu.edu>
>>>>> ---
>>>>>    hw/sd/sd.c | 61 ++++++++++++++++++++++++++++++++++++
>>>>> +-----------------
>>>>>    1 file changed, 42 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>>> index d7a496d77c..b42cd01d1f 100644
>>>>> --- a/hw/sd/sd.c
>>>>> +++ b/hw/sd/sd.c
>>>>> @@ -2759,6 +2759,28 @@ static void sd_instance_finalize(Object *obj)
>>>>>        timer_free(sd->ocr_power_timer);
>>>>>    }
>>>>>    +static void sd_blk_size_error(SDState *sd, int64_t blk_size,
>>>>> +                              int64_t blk_size_aligned, const char
>>>>> *rule,
>>>>> +                              Error **errp)
>>>>> +{
>>>>> +    const char *dev_type = sd_is_emmc(sd) ? "eMMC" : "SD card";
>>>>> +    char *blk_size_str;
>>>>> +
>>>>> +    blk_size_str = size_to_str(blk_size);
>>>>> +    error_setg(errp, "Invalid %s size: %s", dev_type, blk_size_str);
>>>>> +    g_free(blk_size_str);
>>>>> +
>>>>> +    blk_size_str = size_to_str(blk_size_aligned);
>>>>> +    error_append_hint(errp,
>>>>> +                      "%s size has to be %s, e.g. %s.\n"
>>>>> +                      "You can resize disk images with"
>>>>> +                      " 'qemu-img resize <imagefile> <new-size>'\n"
>>>>> +                      "(note that this will lose data if you make
>>>>> the"
>>>>> +                      " image smaller than it currently is).\n",
>>>>> +                      dev_type, rule, blk_size_str);
>>>>> +    g_free(blk_size_str);
>>>>> +}
>>>>> +
>>>>>    static void sd_realize(DeviceState *dev, Error **errp)
>>>>>    {
>>>>>        SDState *sd = SDMMC_COMMON(dev);
>>>>> @@ -2781,25 +2803,26 @@ static void sd_realize(DeviceState *dev,
>>>>> Error **errp)
>>>>>                return;
>>>>>            }
>>>>>    -        blk_size = blk_getlength(sd->blk);
>>>>> -        if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>> -            int64_t blk_size_aligned = pow2ceil(blk_size);
>>>>> -            char *blk_size_str;
>>>>> -
>>>>> -            blk_size_str = size_to_str(blk_size);
>>>>> -            error_setg(errp, "Invalid SD card size: %s",
>>>>> blk_size_str);
>>>>> -            g_free(blk_size_str);
>>>>> -
>>>>> -            blk_size_str = size_to_str(blk_size_aligned);
>>>>> -            error_append_hint(errp,
>>>>> -                              "SD card size has to be a power of 2,
>>>>> e.g. %s.\n"
>>>>> -                              "You can resize disk images with"
>>>>> -                              " 'qemu-img resize <imagefile> <new-
>>>>> size>'\n"
>>>>> -                              "(note that this will lose data if you
>>>>> make the"
>>>>> -                              " image smaller than it currently is).
>>>>> \n",
>>>>> -                              blk_size_str);
>>>>> -            g_free(blk_size_str);
>>>>> -
>>>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>>>>> +        if (blk_size > SDSC_MAX_CAPACITY) {
>>>>> +            if (sd_is_emmc(sd) && blk_size % (1 << HWBLOCK_SHIFT) !=
>>>>> 0) {
>>>>> +                int64_t blk_size_aligned =
>>>>> +                    ((blk_size >> HWBLOCK_SHIFT) + 1) <<
>>>>> HWBLOCK_SHIFT;
>>>>> +                sd_blk_size_error(sd, blk_size, blk_size_aligned,
>>>>> +                                  "multiples of 512", errp);
>>>>> +                return;
>>>>> +            } else if (!sd_is_emmc(sd) && blk_size % (512 * KiB)) {
>>>>> +                int64_t blk_size_aligned = ((blk_size >> 19) + 1) <<
>>>>> 19;
>>>>> +                sd_blk_size_error(sd, blk_size, blk_size_aligned,
>>>>> +                                  "multiples of 512K", errp);
>>>>> +                return;
>>>>> +            }
>>>>> +        } else if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>> +            sd_blk_size_error(sd, blk_size, pow2ceil(blk_size), "a
>>>>> power of 2",
>>>>> +                              errp);
>>>>> +            return;
>>>>> +        } else if (blk_size < 0) {
>>>>> +            error_setg(errp, "eMMC image smaller than boot
>>>>> partitions");
>>>>
>>>> Cedric, I just played with some ast* machines and noticed that they now
>>>> trigger that error above when no eMMC disk image is specified
>>>> ("qemu-system-aarch64 -M ast2700a1-evb"). Is that a valid error,
>>>> i.e. we
>>>> shouldn't have tried to boot without an eMMC at all so far, or would
>>>> this be a regression?
>>>
>>> Only the ast2600-evb and the rainier-bmc have eMMC support.
>>> I don't think the ast2700a1-evb has eMMC support. Jamin ?
>>>
>>>
>>>
>>> The rainier-bmc boots by default from eMMC. Nothing really
>>> special about the image, the first boot partition includes
>>> the u-boot-spl.bin and u-boot.bin images at expected offset.
>>> The machine model loads the u-boot-spl.bin contents as a ROM.
>>>
>>> The ast2600-evb machine boots from flash. To add an eMMC drive
>>> (needs to be the 3rd 'sd' drive), use this command line  :
>>>
>>>      $ qemu-system-arm -M ast2600-evb -net nic,netdev=net0 -netdev
>>> user,id=net0 \
>>>        -drive file=./v09.07/ast2600-default/image-
>>> bmc,format=raw,if=mtd -
>>> serial mon:stdio \
>>>        -drive file=mmc-ast2600-evb-
>>> noboot.qcow2,format=qcow2,if=sd,id=sd2,index=2
>>>        ....
>>>      U-Boot 2019.04-v00.04.22 (Jun 17 2025 - 08:57:39 +0000)
>>>          SOC: AST2600-A3
>>>      eSPI Mode: SIO:Enable : SuperIO-2e
>>>      Eth: MAC0: RGMII, MAC1: RGMII, MAC2: RGMII, MAC3: RGMII
>>>      Model: AST2600 EVB
>>>      DRAM:  already initialized, 1008 MiB (capacity:1024 MiB, VGA:64
>>> MiB,
>>> ECC:off)
>>>      RC Bridge phy@1e6ed200 : Link up
>>>      MMC:   sdhci_slot0@100: 1, sdhci_slot1@200: 2, emmc_slot0@100: 0
>>>          ....
>>>      [    4.209117] mmc0: new high speed MMC card at address 0001
>>>      [    4.211765] mmcblk0: mmc0:0001 QEMU!! 16.0 GiB
>>>      [    4.233637] GPT:Primary header thinks Alt. header is not at the
>>> end of the disk.
>>>      [    4.233995] GPT:29624393 != 33554431
>>>      [    4.234161] GPT:Alternate GPT header not at the end of the disk.
>>>      [    4.234399] GPT:29624393 != 33554431
>>>      [    4.234549] GPT: Use GNU Parted to correct GPT errors.
>>>      [    4.235223]  mmcblk0: p1 p2 p3 p4 p5 p6 p7
>>>     
>>
>> $ ./qemu-system-arm -M ast2600-evb
>> qemu-system-arm: eMMC image smaller than boot partitions
>> $ ./qemu-system-arm -M ast2600-evb -drive file=disk.image,if=sd
>> <works if disk.image is large enough>
>>
>> Is that ok?
> 
> No. This is wrong.
> 
> An sd card device is auto created at init time and a 'DriveInfo *'
> is always available for index 0. 'mc->auto_create_sdcard' should
> be set to false IMO.
> 
> commit cdc8d7cadaac ("hw/boards: Rename no_sdcard ->
> auto_create_sdcard") seems to have changed the behavior of
> several machines.  See 'make check'.
> 

Just to avoid we are in deadlock: My understanding of this issue is that
it is not a fault of this series. Am I right? Or am I supposed to
address that as well?

Regarding the rest of the series, there was another comment on potential
improvements for the docs. I was waiting for further remarks before
creating a potential v5.

So, what is needed now to move forward with my series?

Thanks,
Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH v4 1/6] hw/sd/sdcard: Fix size check for backing block image
Posted by Cédric Le Goater 4 months, 1 week ago
On 9/30/25 11:06, Jan Kiszka wrote:
> On 17.09.25 07:53, Cédric Le Goater wrote:
>> On 9/16/25 19:17, Jan Kiszka wrote:
>>> On 16.09.25 18:14, Cédric Le Goater wrote:
>>>> + Jamin
>>>>
>>>> On 9/16/25 13:39, Jan Kiszka wrote:
>>>>> On 14.09.25 14:46, Jan Kiszka wrote:
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> Alignment rules apply the the individual partitions (user, boot, later
>>>>>> on also RPMB) and depend both on the size of the image and the type of
>>>>>> the device. Up to and including 2GB, the power-of-2 rule applies to
>>>>>> the
>>>>>> user data area. For larger images, multiples of 512 sectors must be
>>>>>> used
>>>>>> for eMMC and multiples of 512K for SD-cards. Fix the check accordingly
>>>>>> and also detect if the image is too small to even hold the boot
>>>>>> partitions.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> Reviewed-by: Warner Losh <imp@bsdimp.com>
>>>>>> ---
>>>>>> CC: Warner Losh <imp@bsdimp.com>
>>>>>> CC: Cédric Le Goater <clg@kaod.org>
>>>>>> CC: Joel Stanley <joel@jms.id.au>
>>>>>> CC: Alistair Francis <alistair@alistair23.me>
>>>>>> CC: Alexander Bulekov <alxndr@bu.edu>
>>>>>> ---
>>>>>>     hw/sd/sd.c | 61 ++++++++++++++++++++++++++++++++++++
>>>>>> +-----------------
>>>>>>     1 file changed, 42 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>>>> index d7a496d77c..b42cd01d1f 100644
>>>>>> --- a/hw/sd/sd.c
>>>>>> +++ b/hw/sd/sd.c
>>>>>> @@ -2759,6 +2759,28 @@ static void sd_instance_finalize(Object *obj)
>>>>>>         timer_free(sd->ocr_power_timer);
>>>>>>     }
>>>>>>     +static void sd_blk_size_error(SDState *sd, int64_t blk_size,
>>>>>> +                              int64_t blk_size_aligned, const char
>>>>>> *rule,
>>>>>> +                              Error **errp)
>>>>>> +{
>>>>>> +    const char *dev_type = sd_is_emmc(sd) ? "eMMC" : "SD card";
>>>>>> +    char *blk_size_str;
>>>>>> +
>>>>>> +    blk_size_str = size_to_str(blk_size);
>>>>>> +    error_setg(errp, "Invalid %s size: %s", dev_type, blk_size_str);
>>>>>> +    g_free(blk_size_str);
>>>>>> +
>>>>>> +    blk_size_str = size_to_str(blk_size_aligned);
>>>>>> +    error_append_hint(errp,
>>>>>> +                      "%s size has to be %s, e.g. %s.\n"
>>>>>> +                      "You can resize disk images with"
>>>>>> +                      " 'qemu-img resize <imagefile> <new-size>'\n"
>>>>>> +                      "(note that this will lose data if you make
>>>>>> the"
>>>>>> +                      " image smaller than it currently is).\n",
>>>>>> +                      dev_type, rule, blk_size_str);
>>>>>> +    g_free(blk_size_str);
>>>>>> +}
>>>>>> +
>>>>>>     static void sd_realize(DeviceState *dev, Error **errp)
>>>>>>     {
>>>>>>         SDState *sd = SDMMC_COMMON(dev);
>>>>>> @@ -2781,25 +2803,26 @@ static void sd_realize(DeviceState *dev,
>>>>>> Error **errp)
>>>>>>                 return;
>>>>>>             }
>>>>>>     -        blk_size = blk_getlength(sd->blk);
>>>>>> -        if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>>> -            int64_t blk_size_aligned = pow2ceil(blk_size);
>>>>>> -            char *blk_size_str;
>>>>>> -
>>>>>> -            blk_size_str = size_to_str(blk_size);
>>>>>> -            error_setg(errp, "Invalid SD card size: %s",
>>>>>> blk_size_str);
>>>>>> -            g_free(blk_size_str);
>>>>>> -
>>>>>> -            blk_size_str = size_to_str(blk_size_aligned);
>>>>>> -            error_append_hint(errp,
>>>>>> -                              "SD card size has to be a power of 2,
>>>>>> e.g. %s.\n"
>>>>>> -                              "You can resize disk images with"
>>>>>> -                              " 'qemu-img resize <imagefile> <new-
>>>>>> size>'\n"
>>>>>> -                              "(note that this will lose data if you
>>>>>> make the"
>>>>>> -                              " image smaller than it currently is).
>>>>>> \n",
>>>>>> -                              blk_size_str);
>>>>>> -            g_free(blk_size_str);
>>>>>> -
>>>>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>>>>>> +        if (blk_size > SDSC_MAX_CAPACITY) {
>>>>>> +            if (sd_is_emmc(sd) && blk_size % (1 << HWBLOCK_SHIFT) !=
>>>>>> 0) {
>>>>>> +                int64_t blk_size_aligned =
>>>>>> +                    ((blk_size >> HWBLOCK_SHIFT) + 1) <<
>>>>>> HWBLOCK_SHIFT;
>>>>>> +                sd_blk_size_error(sd, blk_size, blk_size_aligned,
>>>>>> +                                  "multiples of 512", errp);
>>>>>> +                return;
>>>>>> +            } else if (!sd_is_emmc(sd) && blk_size % (512 * KiB)) {
>>>>>> +                int64_t blk_size_aligned = ((blk_size >> 19) + 1) <<
>>>>>> 19;
>>>>>> +                sd_blk_size_error(sd, blk_size, blk_size_aligned,
>>>>>> +                                  "multiples of 512K", errp);
>>>>>> +                return;
>>>>>> +            }
>>>>>> +        } else if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>>> +            sd_blk_size_error(sd, blk_size, pow2ceil(blk_size), "a
>>>>>> power of 2",
>>>>>> +                              errp);
>>>>>> +            return;
>>>>>> +        } else if (blk_size < 0) {
>>>>>> +            error_setg(errp, "eMMC image smaller than boot
>>>>>> partitions");
>>>>>
>>>>> Cedric, I just played with some ast* machines and noticed that they now
>>>>> trigger that error above when no eMMC disk image is specified
>>>>> ("qemu-system-aarch64 -M ast2700a1-evb"). Is that a valid error,
>>>>> i.e. we
>>>>> shouldn't have tried to boot without an eMMC at all so far, or would
>>>>> this be a regression?
>>>>
>>>> Only the ast2600-evb and the rainier-bmc have eMMC support.
>>>> I don't think the ast2700a1-evb has eMMC support. Jamin ?
>>>>
>>>>
>>>>
>>>> The rainier-bmc boots by default from eMMC. Nothing really
>>>> special about the image, the first boot partition includes
>>>> the u-boot-spl.bin and u-boot.bin images at expected offset.
>>>> The machine model loads the u-boot-spl.bin contents as a ROM.
>>>>
>>>> The ast2600-evb machine boots from flash. To add an eMMC drive
>>>> (needs to be the 3rd 'sd' drive), use this command line  :
>>>>
>>>>       $ qemu-system-arm -M ast2600-evb -net nic,netdev=net0 -netdev
>>>> user,id=net0 \
>>>>         -drive file=./v09.07/ast2600-default/image-
>>>> bmc,format=raw,if=mtd -
>>>> serial mon:stdio \
>>>>         -drive file=mmc-ast2600-evb-
>>>> noboot.qcow2,format=qcow2,if=sd,id=sd2,index=2
>>>>         ....
>>>>       U-Boot 2019.04-v00.04.22 (Jun 17 2025 - 08:57:39 +0000)
>>>>           SOC: AST2600-A3
>>>>       eSPI Mode: SIO:Enable : SuperIO-2e
>>>>       Eth: MAC0: RGMII, MAC1: RGMII, MAC2: RGMII, MAC3: RGMII
>>>>       Model: AST2600 EVB
>>>>       DRAM:  already initialized, 1008 MiB (capacity:1024 MiB, VGA:64
>>>> MiB,
>>>> ECC:off)
>>>>       RC Bridge phy@1e6ed200 : Link up
>>>>       MMC:   sdhci_slot0@100: 1, sdhci_slot1@200: 2, emmc_slot0@100: 0
>>>>           ....
>>>>       [    4.209117] mmc0: new high speed MMC card at address 0001
>>>>       [    4.211765] mmcblk0: mmc0:0001 QEMU!! 16.0 GiB
>>>>       [    4.233637] GPT:Primary header thinks Alt. header is not at the
>>>> end of the disk.
>>>>       [    4.233995] GPT:29624393 != 33554431
>>>>       [    4.234161] GPT:Alternate GPT header not at the end of the disk.
>>>>       [    4.234399] GPT:29624393 != 33554431
>>>>       [    4.234549] GPT: Use GNU Parted to correct GPT errors.
>>>>       [    4.235223]  mmcblk0: p1 p2 p3 p4 p5 p6 p7
>>>>      
>>>
>>> $ ./qemu-system-arm -M ast2600-evb
>>> qemu-system-arm: eMMC image smaller than boot partitions
>>> $ ./qemu-system-arm -M ast2600-evb -drive file=disk.image,if=sd
>>> <works if disk.image is large enough>
>>>
>>> Is that ok?
>>
>> No. This is wrong.
>>
>> An sd card device is auto created at init time and a 'DriveInfo *'
>> is always available for index 0. 'mc->auto_create_sdcard' should
>> be set to false IMO.
>>
>> commit cdc8d7cadaac ("hw/boards: Rename no_sdcard ->
>> auto_create_sdcard") seems to have changed the behavior of
>> several machines.  See 'make check'.
>>
> 
> Just to avoid we are in deadlock: My understanding of this issue is that
> it is not a fault of this series. Am I right? Or am I supposed to
> address that as well?

Could you add to your series :

    https://lore.kernel.org/qemu-devel/20250930142448.1030476-1-clg@redhat.com/

and retry the aspeed machines ?

I am afraid more should be done to run 'make check' with your series.
Maybe set 'mc->auto_create_sdcard' to false for all machines ?

Thanks,

C.



> 
> Regarding the rest of the series, there was another comment on potential
> improvements for the docs. I was waiting for further remarks before
> creating a potential v5.
> 
> So, what is needed now to move forward with my series?
> 
> Thanks,
> Jan
> 


Re: [PATCH v4 1/6] hw/sd/sdcard: Fix size check for backing block image
Posted by Jan Kiszka 3 months, 4 weeks ago
On 01.10.25 08:24, Cédric Le Goater wrote:
> On 9/30/25 11:06, Jan Kiszka wrote:
>> On 17.09.25 07:53, Cédric Le Goater wrote:
>>> On 9/16/25 19:17, Jan Kiszka wrote:
>>>> On 16.09.25 18:14, Cédric Le Goater wrote:
>>>>> + Jamin
>>>>>
>>>>> On 9/16/25 13:39, Jan Kiszka wrote:
>>>>>> On 14.09.25 14:46, Jan Kiszka wrote:
>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>
>>>>>>> Alignment rules apply the the individual partitions (user, boot,
>>>>>>> later
>>>>>>> on also RPMB) and depend both on the size of the image and the
>>>>>>> type of
>>>>>>> the device. Up to and including 2GB, the power-of-2 rule applies to
>>>>>>> the
>>>>>>> user data area. For larger images, multiples of 512 sectors must be
>>>>>>> used
>>>>>>> for eMMC and multiples of 512K for SD-cards. Fix the check
>>>>>>> accordingly
>>>>>>> and also detect if the image is too small to even hold the boot
>>>>>>> partitions.
>>>>>>>
>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>> Reviewed-by: Warner Losh <imp@bsdimp.com>
>>>>>>> ---
>>>>>>> CC: Warner Losh <imp@bsdimp.com>
>>>>>>> CC: Cédric Le Goater <clg@kaod.org>
>>>>>>> CC: Joel Stanley <joel@jms.id.au>
>>>>>>> CC: Alistair Francis <alistair@alistair23.me>
>>>>>>> CC: Alexander Bulekov <alxndr@bu.edu>
>>>>>>> ---
>>>>>>>     hw/sd/sd.c | 61 ++++++++++++++++++++++++++++++++++++
>>>>>>> +-----------------
>>>>>>>     1 file changed, 42 insertions(+), 19 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>>>>> index d7a496d77c..b42cd01d1f 100644
>>>>>>> --- a/hw/sd/sd.c
>>>>>>> +++ b/hw/sd/sd.c
>>>>>>> @@ -2759,6 +2759,28 @@ static void sd_instance_finalize(Object *obj)
>>>>>>>         timer_free(sd->ocr_power_timer);
>>>>>>>     }
>>>>>>>     +static void sd_blk_size_error(SDState *sd, int64_t blk_size,
>>>>>>> +                              int64_t blk_size_aligned, const char
>>>>>>> *rule,
>>>>>>> +                              Error **errp)
>>>>>>> +{
>>>>>>> +    const char *dev_type = sd_is_emmc(sd) ? "eMMC" : "SD card";
>>>>>>> +    char *blk_size_str;
>>>>>>> +
>>>>>>> +    blk_size_str = size_to_str(blk_size);
>>>>>>> +    error_setg(errp, "Invalid %s size: %s", dev_type,
>>>>>>> blk_size_str);
>>>>>>> +    g_free(blk_size_str);
>>>>>>> +
>>>>>>> +    blk_size_str = size_to_str(blk_size_aligned);
>>>>>>> +    error_append_hint(errp,
>>>>>>> +                      "%s size has to be %s, e.g. %s.\n"
>>>>>>> +                      "You can resize disk images with"
>>>>>>> +                      " 'qemu-img resize <imagefile> <new-size>'\n"
>>>>>>> +                      "(note that this will lose data if you make
>>>>>>> the"
>>>>>>> +                      " image smaller than it currently is).\n",
>>>>>>> +                      dev_type, rule, blk_size_str);
>>>>>>> +    g_free(blk_size_str);
>>>>>>> +}
>>>>>>> +
>>>>>>>     static void sd_realize(DeviceState *dev, Error **errp)
>>>>>>>     {
>>>>>>>         SDState *sd = SDMMC_COMMON(dev);
>>>>>>> @@ -2781,25 +2803,26 @@ static void sd_realize(DeviceState *dev,
>>>>>>> Error **errp)
>>>>>>>                 return;
>>>>>>>             }
>>>>>>>     -        blk_size = blk_getlength(sd->blk);
>>>>>>> -        if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>>>> -            int64_t blk_size_aligned = pow2ceil(blk_size);
>>>>>>> -            char *blk_size_str;
>>>>>>> -
>>>>>>> -            blk_size_str = size_to_str(blk_size);
>>>>>>> -            error_setg(errp, "Invalid SD card size: %s",
>>>>>>> blk_size_str);
>>>>>>> -            g_free(blk_size_str);
>>>>>>> -
>>>>>>> -            blk_size_str = size_to_str(blk_size_aligned);
>>>>>>> -            error_append_hint(errp,
>>>>>>> -                              "SD card size has to be a power of 2,
>>>>>>> e.g. %s.\n"
>>>>>>> -                              "You can resize disk images with"
>>>>>>> -                              " 'qemu-img resize <imagefile> <new-
>>>>>>> size>'\n"
>>>>>>> -                              "(note that this will lose data if
>>>>>>> you
>>>>>>> make the"
>>>>>>> -                              " image smaller than it currently
>>>>>>> is).
>>>>>>> \n",
>>>>>>> -                              blk_size_str);
>>>>>>> -            g_free(blk_size_str);
>>>>>>> -
>>>>>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>>>>>>> +        if (blk_size > SDSC_MAX_CAPACITY) {
>>>>>>> +            if (sd_is_emmc(sd) && blk_size % (1 <<
>>>>>>> HWBLOCK_SHIFT) !=
>>>>>>> 0) {
>>>>>>> +                int64_t blk_size_aligned =
>>>>>>> +                    ((blk_size >> HWBLOCK_SHIFT) + 1) <<
>>>>>>> HWBLOCK_SHIFT;
>>>>>>> +                sd_blk_size_error(sd, blk_size, blk_size_aligned,
>>>>>>> +                                  "multiples of 512", errp);
>>>>>>> +                return;
>>>>>>> +            } else if (!sd_is_emmc(sd) && blk_size % (512 * KiB)) {
>>>>>>> +                int64_t blk_size_aligned = ((blk_size >> 19) +
>>>>>>> 1) <<
>>>>>>> 19;
>>>>>>> +                sd_blk_size_error(sd, blk_size, blk_size_aligned,
>>>>>>> +                                  "multiples of 512K", errp);
>>>>>>> +                return;
>>>>>>> +            }
>>>>>>> +        } else if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>>>> +            sd_blk_size_error(sd, blk_size, pow2ceil(blk_size), "a
>>>>>>> power of 2",
>>>>>>> +                              errp);
>>>>>>> +            return;
>>>>>>> +        } else if (blk_size < 0) {
>>>>>>> +            error_setg(errp, "eMMC image smaller than boot
>>>>>>> partitions");
>>>>>>
>>>>>> Cedric, I just played with some ast* machines and noticed that
>>>>>> they now
>>>>>> trigger that error above when no eMMC disk image is specified
>>>>>> ("qemu-system-aarch64 -M ast2700a1-evb"). Is that a valid error,
>>>>>> i.e. we
>>>>>> shouldn't have tried to boot without an eMMC at all so far, or would
>>>>>> this be a regression?
>>>>>
>>>>> Only the ast2600-evb and the rainier-bmc have eMMC support.
>>>>> I don't think the ast2700a1-evb has eMMC support. Jamin ?
>>>>>
>>>>>
>>>>>
>>>>> The rainier-bmc boots by default from eMMC. Nothing really
>>>>> special about the image, the first boot partition includes
>>>>> the u-boot-spl.bin and u-boot.bin images at expected offset.
>>>>> The machine model loads the u-boot-spl.bin contents as a ROM.
>>>>>
>>>>> The ast2600-evb machine boots from flash. To add an eMMC drive
>>>>> (needs to be the 3rd 'sd' drive), use this command line  :
>>>>>
>>>>>       $ qemu-system-arm -M ast2600-evb -net nic,netdev=net0 -netdev
>>>>> user,id=net0 \
>>>>>         -drive file=./v09.07/ast2600-default/image-
>>>>> bmc,format=raw,if=mtd -
>>>>> serial mon:stdio \
>>>>>         -drive file=mmc-ast2600-evb-
>>>>> noboot.qcow2,format=qcow2,if=sd,id=sd2,index=2
>>>>>         ....
>>>>>       U-Boot 2019.04-v00.04.22 (Jun 17 2025 - 08:57:39 +0000)
>>>>>           SOC: AST2600-A3
>>>>>       eSPI Mode: SIO:Enable : SuperIO-2e
>>>>>       Eth: MAC0: RGMII, MAC1: RGMII, MAC2: RGMII, MAC3: RGMII
>>>>>       Model: AST2600 EVB
>>>>>       DRAM:  already initialized, 1008 MiB (capacity:1024 MiB, VGA:64
>>>>> MiB,
>>>>> ECC:off)
>>>>>       RC Bridge phy@1e6ed200 : Link up
>>>>>       MMC:   sdhci_slot0@100: 1, sdhci_slot1@200: 2, emmc_slot0@100: 0
>>>>>           ....
>>>>>       [    4.209117] mmc0: new high speed MMC card at address 0001
>>>>>       [    4.211765] mmcblk0: mmc0:0001 QEMU!! 16.0 GiB
>>>>>       [    4.233637] GPT:Primary header thinks Alt. header is not
>>>>> at the
>>>>> end of the disk.
>>>>>       [    4.233995] GPT:29624393 != 33554431
>>>>>       [    4.234161] GPT:Alternate GPT header not at the end of the
>>>>> disk.
>>>>>       [    4.234399] GPT:29624393 != 33554431
>>>>>       [    4.234549] GPT: Use GNU Parted to correct GPT errors.
>>>>>       [    4.235223]  mmcblk0: p1 p2 p3 p4 p5 p6 p7
>>>>>      
>>>>
>>>> $ ./qemu-system-arm -M ast2600-evb
>>>> qemu-system-arm: eMMC image smaller than boot partitions
>>>> $ ./qemu-system-arm -M ast2600-evb -drive file=disk.image,if=sd
>>>> <works if disk.image is large enough>
>>>>
>>>> Is that ok?
>>>
>>> No. This is wrong.
>>>
>>> An sd card device is auto created at init time and a 'DriveInfo *'
>>> is always available for index 0. 'mc->auto_create_sdcard' should
>>> be set to false IMO.
>>>
>>> commit cdc8d7cadaac ("hw/boards: Rename no_sdcard ->
>>> auto_create_sdcard") seems to have changed the behavior of
>>> several machines.  See 'make check'.
>>>
>>
>> Just to avoid we are in deadlock: My understanding of this issue is that
>> it is not a fault of this series. Am I right? Or am I supposed to
>> address that as well?
> 
> Could you add to your series :
> 
>    https://lore.kernel.org/qemu-devel/20250930142448.1030476-1-
> clg@redhat.com/
> 
> and retry the aspeed machines ?

Yes, that patch resolves the issue above.

> 
> I am afraid more should be done to run 'make check' with your series.
> Maybe set 'mc->auto_create_sdcard' to false for all machines ?

Who should do that?

> 
> Thanks,
> 
> C.
> 
> 
> 
>>
>> Regarding the rest of the series, there was another comment on potential
>> improvements for the docs. I was waiting for further remarks before
>> creating a potential v5.
>>
>> So, what is needed now to move forward with my series?
>>

This question is still open for me.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH v4 1/6] hw/sd/sdcard: Fix size check for backing block image
Posted by Cédric Le Goater 3 months, 3 weeks ago
Hello Jan,

>>> Just to avoid we are in deadlock: My understanding of this issue is that
>>> it is not a fault of this series. Am I right? Or am I supposed to
>>> address that as well?
>>
>> Could you add to your series :
>>
>>     https://lore.kernel.org/qemu-devel/20250930142448.1030476-1-
>> clg@redhat.com/
>>
>> and retry the aspeed machines ?
> 
> Yes, that patch resolves the issue above.

A similar version is now merged.

>> I am afraid more should be done to run 'make check' with your series.
>> Maybe set 'mc->auto_create_sdcard' to false for all machines ?
> 
> Who should do that?

/me looks at sd maintainers. 
Thanks,

C.


Re: [PATCH v4 1/6] hw/sd/sdcard: Fix size check for backing block image
Posted by Jan Kiszka 3 months, 3 weeks ago
On 17.10.25 07:31, Cédric Le Goater wrote:
> Hello Jan,
> 
>>>> Just to avoid we are in deadlock: My understanding of this issue is
>>>> that
>>>> it is not a fault of this series. Am I right? Or am I supposed to
>>>> address that as well?
>>>
>>> Could you add to your series :
>>>
>>>     https://lore.kernel.org/qemu-devel/20250930142448.1030476-1-
>>> clg@redhat.com/
>>>
>>> and retry the aspeed machines ?
>>
>> Yes, that patch resolves the issue above.
> 
> A similar version is now merged.
> 
>>> I am afraid more should be done to run 'make check' with your series.
>>> Maybe set 'mc->auto_create_sdcard' to false for all machines ?
>>
>> Who should do that?
> 
> /me looks at sd maintainers. Thanks,
> 
> C.
> 

FWIW, v5 of this series is coming: I found an issue of this patch /wrt
unplugged SD cards. make check now passes.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center