[PATCH v2 0/8] sd: Add RPMB emulation to eMMC model

[PATCH v2 0/8] sd: Add RPMB emulation to eMMC model

[Jan Kiszka]

Changes in v2 [1]:
 - handle write counter expiry
 - assert() availability of QCRYPTO_HASH_ALGO_SHA256
 - add missing SPDX-License-Identifier

This closes an old gap in system integration testing for the very
complex ARM firmware stacks by adding fairly advanced Replay Protected
Memory Block (RPMB) emulation to the eMMC device model. Key programming
and message authentication are working, so is the write counter. Known
users are happy with the result. What is missing, but not only for RPMB-
related registers, is state persistence across QEMU restarts. This is OK
at this stage for most test scenarios, though, and could still be added
later on.

What can already be done with it is demonstrated in the WIP branch of
isar-cip-core at [2]: TF-A + OP-TEE + StandaloneMM TA + fTPM TA, used by
U-Boot and Linux for UEFI variable storage and TPM scenarios. If you
want to try: build qemu-arm64 target for trixie with 6.12-cip *head*
kernel, enable secure boot and disk encryption, then run

$ QEMU_PATH=/path/to/qemu-build/ ./start-qemu.sh

Deploy snakeoil keys into PK, KEK and db after first boot to enable
secure booting:

root@demo:~# cert-to-efi-sig-list PkKek-1-snakeoil.pem PK.esl
root@demo:~# sign-efi-sig-list -k PkKek-1-snakeoil.key -c PkKek-1-snakeoil.pem PK PK.esl PK.auth
root@demo:~# efi-updatevar -f PK.auth db
root@demo:~# efi-updatevar -f PK.auth KEK
root@demo:~# efi-updatevar -f PK.auth PK

Note that emulation is a bit slow in general, and specifically the
partition encryption on first boot is taking 20 min. - we should
probably reduce its size or understand if there is still something to
optimize.

Jan

[1] https://github.com/siemens/qemu/commits/queues/emmc/
[2] https://gitlab.com/cip-project/cip-core/isar-cip-core/-/commits/wip/qemu-rpmb

Cc: "Daniel P. Berrangé" <berrange@redhat.com>

Jan Kiszka (8):
  hw/sd/sdcard: Fix size check for backing block image
  hw/sd/sdcard: Add validation for boot-partition-size
  hw/sd/sdcard: Allow user-instantiated eMMC
  hw/sd/sdcard: Refactor sd_bootpart_offset
  hw/sd/sdcard: Add basic support for RPMB partition
  crypto/hmac: Allow to build hmac over multiple
    qcrypto_gnutls_hmac_bytes[v] calls
  hw/sd/sdcard: Handle RPMB MAC field
  scripts: Add helper script to generate eMMC block device images

 crypto/hmac-gcrypt.c   |   4 +-
 crypto/hmac-glib.c     |   4 +-
 crypto/hmac-gnutls.c   |   4 +-
 crypto/hmac-nettle.c   |   4 +-
 hw/sd/sd.c             | 314 ++++++++++++++++++++++++++++++++++++++---
 hw/sd/sdmmc-internal.h |  24 +++-
 hw/sd/trace-events     |   2 +
 include/crypto/hmac.h  |  12 ++
 scripts/mkemmc.sh      | 186 ++++++++++++++++++++++++
 9 files changed, 532 insertions(+), 22 deletions(-)
 create mode 100755 scripts/mkemmc.sh

-- 
2.43.0

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

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

The power-of-2 rule applies to the user data area, not the complete
block image. The latter can be concatenation of boot partition images
and the user data.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 8c290595f0..16aee210b4 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
             return;
         }
 
-        blk_size = blk_getlength(sd->blk);
+        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
         if (blk_size > 0 && !is_power_of_2(blk_size)) {
             int64_t blk_size_aligned = pow2ceil(blk_size);
             char *blk_size_str;
-- 
2.43.0

[PATCH v2 2/8] hw/sd/sdcard: Add validation for boot-partition-size

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Make sure we are not silently rounding down or even wrapping around,
causing inconsistencies with the provided image.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/sd/sd.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 16aee210b4..834392b0a8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2818,6 +2818,16 @@ static void sd_realize(DeviceState *dev, Error **errp)
         }
         blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
     }
+    if (sd->boot_part_size % (128 * KiB) ||
+        sd->boot_part_size > 255 * 128 * KiB) {
+        char *size_str = size_to_str(sd->boot_part_size);
+
+        error_setg(errp, "Invalid boot partition size: %s", size_str);
+        g_free(size_str);
+        error_append_hint(errp,
+                          "The boot partition size must be multiples of 128K"
+                          "and not larger than 32640K.\n");
+    }
 }
 
 static void emmc_realize(DeviceState *dev, Error **errp)
-- 
2.43.0

[PATCH v2 3/8] hw/sd/sdcard: Allow user-instantiated eMMC

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Enable user-instantiation so that PCI-attached eMMCs can be created for
virt machines, for QA purposes for the eMMC model itself and for complex
firmware/OS integrations using the upcoming RPMB partition support.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/sd/sd.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 834392b0a8..8a4f58295b 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2912,8 +2912,6 @@ static void emmc_class_init(ObjectClass *klass, const void *data)
     dc->desc = "eMMC";
     dc->realize = emmc_realize;
     device_class_set_props(dc, emmc_properties);
-    /* Reason: Soldered on board */
-    dc->user_creatable = false;
 
     sc->proto = &sd_proto_emmc;
 
-- 
2.43.0

[PATCH v2 4/8] hw/sd/sdcard: Refactor sd_bootpart_offset

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This function provides the offset for any partition in the block image,
not only the boot partitions, therefore rename it. Align the constant
names with the numbering scheme in the standard and use constants for
both boot partitions for consistency reasons. There is also no reason to
return early if boot_part_size is zero because the existing code will
provide the right value in that case as well.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c             | 16 ++++++++--------
 hw/sd/sdmmc-internal.h |  3 ++-
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 8a4f58295b..b727a37d06 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -838,14 +838,14 @@ static uint32_t sd_blk_len(SDState *sd)
 
 /*
  * This requires a disk image that has two boot partitions inserted at the
- * beginning of it. The size of the boot partitions is the "boot-size"
- * property.
+ * beginning of it, followed by an RPMB partition. The size of the boot
+ * partitions is the "boot-partition-size" property.
  */
-static uint32_t sd_bootpart_offset(SDState *sd)
+static uint32_t sd_part_offset(SDState *sd)
 {
     unsigned partition_access;
 
-    if (!sd->boot_part_size || !sd_is_emmc(sd)) {
+    if (!sd_is_emmc(sd)) {
         return 0;
     }
 
@@ -854,9 +854,9 @@ static uint32_t sd_bootpart_offset(SDState *sd)
     switch (partition_access) {
     case EXT_CSD_PART_CONFIG_ACC_DEFAULT:
         return sd->boot_part_size * 2;
-    case EXT_CSD_PART_CONFIG_ACC_BOOT0:
+    case EXT_CSD_PART_CONFIG_ACC_BOOT1:
         return 0;
-    case EXT_CSD_PART_CONFIG_ACC_BOOT0 + 1:
+    case EXT_CSD_PART_CONFIG_ACC_BOOT2:
         return sd->boot_part_size * 1;
     default:
          g_assert_not_reached();
@@ -1057,7 +1057,7 @@ static const VMStateDescription sd_vmstate = {
 static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
 {
     trace_sdcard_read_block(addr, len);
-    addr += sd_bootpart_offset(sd);
+    addr += sd_part_offset(sd);
     if (!sd->blk || blk_pread(sd->blk, addr, len, sd->data, 0) < 0) {
         fprintf(stderr, "sd_blk_read: read error on host side\n");
     }
@@ -1066,7 +1066,7 @@ static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
 static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
 {
     trace_sdcard_write_block(addr, len);
-    addr += sd_bootpart_offset(sd);
+    addr += sd_part_offset(sd);
     if (!sd->blk || blk_pwrite(sd->blk, addr, len, sd->data, 0) < 0) {
         fprintf(stderr, "sd_blk_write: write error on host side\n");
     }
diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
index 91eb5b6b2f..ce6bc4e6ec 100644
--- a/hw/sd/sdmmc-internal.h
+++ b/hw/sd/sdmmc-internal.h
@@ -116,7 +116,8 @@ DECLARE_OBJ_CHECKERS(SDState, SDCardClass, SDMMC_COMMON, TYPE_SDMMC_COMMON)
 
 #define EXT_CSD_PART_CONFIG_ACC_MASK            (0x7)
 #define EXT_CSD_PART_CONFIG_ACC_DEFAULT         (0x0)
-#define EXT_CSD_PART_CONFIG_ACC_BOOT0           (0x1)
+#define EXT_CSD_PART_CONFIG_ACC_BOOT1           (0x1)
+#define EXT_CSD_PART_CONFIG_ACC_BOOT2           (0x2)
 
 #define EXT_CSD_PART_CONFIG_EN_MASK             (0x7 << 3)
 #define EXT_CSD_PART_CONFIG_EN_BOOT0            (0x1 << 3)
-- 
2.43.0

[PATCH v2 5/8] hw/sd/sdcard: Add basic support for RPMB partition

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

The Replay Protected Memory Block (RPMB) is available since eMMC 4.4
which has been obsoleted by 4.41. Therefore lift the provided
EXT_CSD_REV to 5 (4.41) and provide the basic logic to implement basic
support for it. This allows to set the authentication key, read the
write counter and authenticated perform data read and write requests.
Those aren't actually authenticated yet, support for that will be added
later.

The RPMB image needs to be added to backing block images after potential
boot partitions and before the user data. It's size is controlled by
the rpmb-partition-size property.

Also missing in this version (and actually not only for RPMB bits) is
persistence of registers that are supposed to survive power cycles. Most
prominent are the write counters or the authentication key. This feature
can be added later, e.g. by append a state structure to the backing
block image.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/sd/sd.c             | 207 +++++++++++++++++++++++++++++++++++++++--
 hw/sd/sdmmc-internal.h |  21 +++++
 hw/sd/trace-events     |   2 +
 3 files changed, 222 insertions(+), 8 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b727a37d06..7b4a48f822 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -117,6 +117,20 @@ typedef struct SDProto {
     } cmd[SDMMC_CMD_MAX], acmd[SDMMC_CMD_MAX];
 } SDProto;
 
+#define RPMB_KEY_MAC_LEN    32
+
+typedef struct {
+    uint8_t stuff_bytes[196];
+    uint8_t key_mac[RPMB_KEY_MAC_LEN];
+    uint8_t data[256];
+    uint8_t nonce[16];
+    uint32_t write_counter;
+    uint16_t address;
+    uint16_t block_count;
+    uint16_t result;
+    uint16_t req_resp;
+} RPMBDataFrame;
+
 struct SDState {
     DeviceState parent_obj;
 
@@ -140,6 +154,7 @@ struct SDState {
 
     uint8_t spec_version;
     uint64_t boot_part_size;
+    uint64_t rpmb_part_size;
     BlockBackend *blk;
     uint8_t boot_config;
 
@@ -172,6 +187,10 @@ struct SDState {
     uint32_t data_offset;
     size_t data_size;
     uint8_t data[512];
+    RPMBDataFrame rpmb_result;
+    uint32_t rpmb_write_counter;
+    uint8_t rpmb_key[32];
+    uint8_t rpmb_key_set;
     QEMUTimer *ocr_power_timer;
     uint8_t dat_lines;
     bool cmd_line;
@@ -511,7 +530,9 @@ static void emmc_set_ext_csd(SDState *sd, uint64_t size)
     sd->ext_csd[205] = 0x46; /* Min read perf for 4bit@26Mhz */
     sd->ext_csd[EXT_CSD_CARD_TYPE] = 0b11;
     sd->ext_csd[EXT_CSD_STRUCTURE] = 2;
-    sd->ext_csd[EXT_CSD_REV] = 3;
+    sd->ext_csd[EXT_CSD_REV] = 5;
+    sd->ext_csd[EXT_CSD_RPMB_MULT] = sd->rpmb_part_size / (128 * KiB);
+    sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0b111;
 
     /* Mode segment (RW) */
     sd->ext_csd[EXT_CSD_PART_CONFIG] = sd->boot_config;
@@ -839,7 +860,8 @@ static uint32_t sd_blk_len(SDState *sd)
 /*
  * This requires a disk image that has two boot partitions inserted at the
  * beginning of it, followed by an RPMB partition. The size of the boot
- * partitions is the "boot-partition-size" property.
+ * partitions is the "boot-partition-size" property, the one of the RPMB
+ * partition is 'rpmb-partition-size'.
  */
 static uint32_t sd_part_offset(SDState *sd)
 {
@@ -853,11 +875,13 @@ static uint32_t sd_part_offset(SDState *sd)
                                  & EXT_CSD_PART_CONFIG_ACC_MASK;
     switch (partition_access) {
     case EXT_CSD_PART_CONFIG_ACC_DEFAULT:
-        return sd->boot_part_size * 2;
+        return sd->boot_part_size * 2 + sd->rpmb_part_size;
     case EXT_CSD_PART_CONFIG_ACC_BOOT1:
         return 0;
     case EXT_CSD_PART_CONFIG_ACC_BOOT2:
         return sd->boot_part_size * 1;
+    case EXT_CSD_PART_CONFIG_ACC_RPMB:
+        return sd->boot_part_size * 2;
     default:
          g_assert_not_reached();
     }
@@ -896,7 +920,7 @@ static void sd_reset(DeviceState *dev)
     }
     size = sect << HWBLOCK_SHIFT;
     if (sd_is_emmc(sd)) {
-        size -= sd->boot_part_size * 2;
+        size -= sd->boot_part_size * 2 + sd->rpmb_part_size;
     }
 
     sect = sd_addr_to_wpnum(size) + 1;
@@ -984,6 +1008,34 @@ static const VMStateDescription sd_ocr_vmstate = {
     },
 };
 
+static bool vmstate_needed_for_rpmb(void *opaque)
+{
+    SDState *sd = opaque;
+
+    return sd->rpmb_part_size > 0;
+}
+
+static const VMStateDescription emmc_rpmb_vmstate = {
+    .name = "sd-card/ext_csd_modes-state",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = vmstate_needed_for_rpmb,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT8_ARRAY(rpmb_result.key_mac, SDState, RPMB_KEY_MAC_LEN),
+        VMSTATE_UINT8_ARRAY(rpmb_result.data, SDState, 256),
+        VMSTATE_UINT8_ARRAY(rpmb_result.nonce, SDState, 16),
+        VMSTATE_UINT32(rpmb_result.write_counter, SDState),
+        VMSTATE_UINT16(rpmb_result.address, SDState),
+        VMSTATE_UINT16(rpmb_result.block_count, SDState),
+        VMSTATE_UINT16(rpmb_result.result, SDState),
+        VMSTATE_UINT16(rpmb_result.req_resp, SDState),
+        VMSTATE_UINT32(rpmb_write_counter, SDState),
+        VMSTATE_UINT8_ARRAY(rpmb_key, SDState, 32),
+        VMSTATE_UINT8(rpmb_key_set, SDState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static bool vmstate_needed_for_emmc(void *opaque)
 {
     SDState *sd = opaque;
@@ -1050,6 +1102,7 @@ static const VMStateDescription sd_vmstate = {
     .subsections = (const VMStateDescription * const []) {
         &sd_ocr_vmstate,
         &emmc_extcsd_vmstate,
+        &emmc_rpmb_vmstate,
         NULL
     },
 };
@@ -1072,6 +1125,105 @@ static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
     }
 }
 
+static void emmc_rpmb_blk_read(SDState *sd, uint64_t addr, uint32_t len)
+{
+    uint16_t resp = be16_to_cpu(sd->rpmb_result.req_resp);
+    uint16_t result = be16_to_cpu(sd->rpmb_result.result);
+    unsigned int curr_block = 0;
+
+    if ((result & ~RPMB_RESULT_COUTER_EXPIRED) == RPMB_RESULT_OK &&
+        resp == RPMB_RESP(RPMB_REQ_AUTH_DATA_READ)) {
+        curr_block = be16_to_cpu(sd->rpmb_result.address);
+        if (sd->rpmb_result.block_count == 0) {
+            sd->rpmb_result.block_count = cpu_to_be16(sd->multi_blk_cnt);
+        } else {
+            curr_block += be16_to_cpu(sd->rpmb_result.block_count) -
+                sd->multi_blk_cnt;
+        }
+        addr = curr_block * 256 + sd_part_offset(sd);
+        if (blk_pread(sd->blk, addr, 256, sd->rpmb_result.data, 0) < 0) {
+            fprintf(stderr, "sd_blk_read: read error on host side\n");
+            memset(sd->rpmb_result.data, 0, sizeof(sd->rpmb_result.data));
+            sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_READ_FAILURE |
+                (result & RPMB_RESULT_COUTER_EXPIRED));
+        }
+    }
+    memcpy(sd->data, &sd->rpmb_result, sizeof(sd->rpmb_result));
+
+    trace_sdcard_rpmb_read_block(resp, curr_block,
+                                 be16_to_cpu(sd->rpmb_result.result));
+}
+
+static void emmc_rpmb_blk_write(SDState *sd, uint64_t addr, uint32_t len)
+{
+    RPMBDataFrame *frame = (RPMBDataFrame *)sd->data;
+    uint16_t req = be16_to_cpu(frame->req_resp);
+
+    if (req == RPMB_REQ_READ_RESULT) {
+        /* just return the current result register */
+        goto exit;
+    }
+    memset(&sd->rpmb_result, 0, sizeof(sd->rpmb_result));
+    memcpy(sd->rpmb_result.nonce, frame->nonce, sizeof(sd->rpmb_result.nonce));
+    sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_OK);
+    sd->rpmb_result.req_resp = cpu_to_be16(RPMB_RESP(req));
+
+    if (!sd->rpmb_key_set && req != RPMB_REQ_PROGRAM_AUTH_KEY) {
+        sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_NO_AUTH_KEY);
+        goto exit;
+    }
+
+    switch (req) {
+    case RPMB_REQ_PROGRAM_AUTH_KEY:
+        if (sd->rpmb_key_set) {
+            sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_WRITE_FAILURE);
+            break;
+        }
+        memcpy(sd->rpmb_key, frame->key_mac, sizeof(sd->rpmb_key));
+        sd->rpmb_key_set = 1;
+        break;
+    case RPMB_REQ_READ_WRITE_COUNTER:
+        sd->rpmb_result.write_counter = cpu_to_be32(sd->rpmb_write_counter);
+        break;
+    case RPMB_REQ_AUTH_DATA_WRITE:
+        /* We only support single-block writes so far */
+        if (sd->multi_blk_cnt != 1) {
+            sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_GENERAL_FAILURE);
+            break;
+        }
+        if (sd->rpmb_write_counter == 0xffffffff) {
+            sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_WRITE_FAILURE);
+            break;
+        }
+        if (be32_to_cpu(frame->write_counter) != sd->rpmb_write_counter) {
+            sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_COUNTER_FAILURE);
+            break;
+        }
+        sd->rpmb_result.address = frame->address;
+        addr = be16_to_cpu(frame->address) * 256 + sd_part_offset(sd);
+        if (blk_pwrite(sd->blk, addr, 256, frame->data, 0) < 0) {
+            fprintf(stderr, "sd_blk_write: write error on host side\n");
+            sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_WRITE_FAILURE);
+        } else {
+            sd->rpmb_write_counter++;
+        }
+        sd->rpmb_result.write_counter = cpu_to_be32(sd->rpmb_write_counter);
+        break;
+    case RPMB_REQ_AUTH_DATA_READ:
+        sd->rpmb_result.address = frame->address;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "RPMB request %d not implemented\n", req);
+        sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_GENERAL_FAILURE);
+        break;
+    }
+exit:
+    if (sd->rpmb_write_counter == 0xffffffff) {
+        sd->rpmb_result.result |= cpu_to_be16(RPMB_RESULT_COUTER_EXPIRED);
+    }
+    trace_sdcard_rpmb_write_block(req, be16_to_cpu(sd->rpmb_result.result));
+}
+
 static void sd_erase(SDState *sd)
 {
     uint64_t erase_start = sd->erase_start;
@@ -1185,6 +1337,19 @@ static void emmc_function_switch(SDState *sd, uint32_t arg)
         break;
     }
 
+    if (index == EXT_CSD_PART_CONFIG) {
+        uint8_t part = b & EXT_CSD_PART_CONFIG_ACC_MASK;
+
+        if (((part == EXT_CSD_PART_CONFIG_ACC_BOOT1 ||
+              part == EXT_CSD_PART_CONFIG_ACC_BOOT2) && !sd->boot_part_size) ||
+            (part == EXT_CSD_PART_CONFIG_ACC_RPMB && !sd->rpmb_part_size)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "MMC switching to illegal partition\n");
+            sd->card_status |= R_CSR_SWITCH_ERROR_MASK;
+            return;
+        }
+    }
+
     trace_sdcard_ext_csd_update(index, sd->ext_csd[index], b);
     sd->ext_csd[index] = b;
 }
@@ -2386,6 +2551,7 @@ static bool sd_generic_read_byte(SDState *sd, uint8_t *value)
 
 static void sd_write_byte(SDState *sd, uint8_t value)
 {
+    unsigned int partition_access;
     int i;
 
     if (!sd->blk || !blk_is_inserted(sd->blk)) {
@@ -2435,7 +2601,13 @@ static void sd_write_byte(SDState *sd, uint8_t value)
         if (sd->data_offset >= sd->blk_len) {
             /* TODO: Check CRC before committing */
             sd->state = sd_programming_state;
-            sd_blk_write(sd, sd->data_start, sd->data_offset);
+            partition_access = sd->ext_csd[EXT_CSD_PART_CONFIG]
+                    & EXT_CSD_PART_CONFIG_ACC_MASK;
+            if (partition_access == EXT_CSD_PART_CONFIG_ACC_RPMB) {
+                emmc_rpmb_blk_write(sd, sd->data_start, sd->data_offset);
+            } else {
+                sd_blk_write(sd, sd->data_start, sd->data_offset);
+            }
             sd->blk_written++;
             sd->data_start += sd->blk_len;
             sd->data_offset = 0;
@@ -2518,6 +2690,7 @@ static uint8_t sd_read_byte(SDState *sd)
 {
     /* TODO: Append CRCs */
     const uint8_t dummy_byte = 0x00;
+    unsigned int partition_access;
     uint8_t ret;
     uint32_t io_len;
 
@@ -2561,7 +2734,13 @@ static uint8_t sd_read_byte(SDState *sd)
                                   sd->data_start, io_len)) {
                 return dummy_byte;
             }
-            sd_blk_read(sd, sd->data_start, io_len);
+            partition_access = sd->ext_csd[EXT_CSD_PART_CONFIG]
+                    & EXT_CSD_PART_CONFIG_ACC_MASK;
+            if (partition_access == EXT_CSD_PART_CONFIG_ACC_RPMB) {
+                emmc_rpmb_blk_read(sd, sd->data_start, io_len);
+            } else {
+                sd_blk_read(sd, sd->data_start, io_len);
+            }
         }
         ret = sd->data[sd->data_offset ++];
 
@@ -2789,7 +2968,8 @@ static void sd_realize(DeviceState *dev, Error **errp)
             return;
         }
 
-        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
+        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2 -
+            sd->rpmb_part_size;
         if (blk_size > 0 && !is_power_of_2(blk_size)) {
             int64_t blk_size_aligned = pow2ceil(blk_size);
             char *blk_size_str;
@@ -2828,13 +3008,23 @@ static void sd_realize(DeviceState *dev, Error **errp)
                           "The boot partition size must be multiples of 128K"
                           "and not larger than 32640K.\n");
     }
+    if (sd->rpmb_part_size % (128 * KiB) ||
+        sd->rpmb_part_size > 128 * 128 * KiB) {
+        char *size_str = size_to_str(sd->boot_part_size);
+
+        error_setg(errp, "Invalid boot partition size: %s", size_str);
+        g_free(size_str);
+        error_append_hint(errp,
+                          "The RPMB partition size must be multiples of 128K"
+                          "and not larger than 16384K.\n");
+    }
 }
 
 static void emmc_realize(DeviceState *dev, Error **errp)
 {
     SDState *sd = SDMMC_COMMON(dev);
 
-    sd->spec_version = SD_PHY_SPECv3_01_VERS; /* Actually v4.3 */
+    sd->spec_version = SD_PHY_SPECv3_01_VERS; /* Actually v4.5 */
 
     sd_realize(dev, errp);
 }
@@ -2851,6 +3041,7 @@ static const Property sd_properties[] = {
 static const Property emmc_properties[] = {
     DEFINE_PROP_UINT64("boot-partition-size", SDState, boot_part_size, 0),
     DEFINE_PROP_UINT8("boot-config", SDState, boot_config, 0x0),
+    DEFINE_PROP_UINT64("rpmb-partition-size", SDState, rpmb_part_size, 0),
 };
 
 static void sdmmc_common_class_init(ObjectClass *klass, const void *data)
diff --git a/hw/sd/sdmmc-internal.h b/hw/sd/sdmmc-internal.h
index ce6bc4e6ec..c4a9aa8edf 100644
--- a/hw/sd/sdmmc-internal.h
+++ b/hw/sd/sdmmc-internal.h
@@ -118,9 +118,30 @@ DECLARE_OBJ_CHECKERS(SDState, SDCardClass, SDMMC_COMMON, TYPE_SDMMC_COMMON)
 #define EXT_CSD_PART_CONFIG_ACC_DEFAULT         (0x0)
 #define EXT_CSD_PART_CONFIG_ACC_BOOT1           (0x1)
 #define EXT_CSD_PART_CONFIG_ACC_BOOT2           (0x2)
+#define EXT_CSD_PART_CONFIG_ACC_RPMB            (0x3)
 
 #define EXT_CSD_PART_CONFIG_EN_MASK             (0x7 << 3)
 #define EXT_CSD_PART_CONFIG_EN_BOOT0            (0x1 << 3)
 #define EXT_CSD_PART_CONFIG_EN_USER             (0x7 << 3)
 
+#define RPMB_REQ_PROGRAM_AUTH_KEY       (1)
+#define RPMB_REQ_READ_WRITE_COUNTER     (2)
+#define RPMB_REQ_AUTH_DATA_WRITE        (3)
+#define RPMB_REQ_AUTH_DATA_READ         (4)
+#define RPMB_REQ_READ_RESULT            (5)
+#define RPMB_REQ_AUTH_CONFIG_WRITE      (6)
+#define RPMB_REQ_AUTH_CONFIG_READ       (7)
+
+#define RPMB_RESP(__req__)              ((__req__) << 8)
+
+#define RPMB_RESULT_OK                  (0)
+#define RPMB_RESULT_GENERAL_FAILURE     (1)
+#define RPMB_RESULT_AUTH_FAILURE        (2)
+#define RPMB_RESULT_COUNTER_FAILURE     (3)
+#define RPMB_RESULT_ADDRESS_FAILURE     (4)
+#define RPMB_RESULT_WRITE_FAILURE       (5)
+#define RPMB_RESULT_READ_FAILURE        (6)
+#define RPMB_RESULT_NO_AUTH_KEY         (7)
+#define RPMB_RESULT_COUTER_EXPIRED      (0x80)
+
 #endif
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 8d49840917..d30daa2143 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -59,6 +59,8 @@ sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint32_t
 sdcard_set_voltage(uint16_t millivolts) "%u mV"
 sdcard_ext_csd_update(unsigned index, uint8_t oval, uint8_t nval) "index %u: 0x%02x -> 0x%02x"
 sdcard_switch(unsigned access, unsigned index, unsigned value, unsigned set) "SWITCH acc:%u idx:%u val:%u set:%u"
+sdcard_rpmb_read_block(uint16_t resp, uint16_t read_addr, uint16_t result) "resp 0x%x read_addr 0x%x result 0x%x"
+sdcard_rpmb_write_block(uint16_t req, uint16_t result) "req 0x%x result 0x%x"
 
 # pl181.c
 pl181_command_send(uint8_t cmd, uint32_t arg) "sending CMD%02d arg 0x%08" PRIx32
-- 
2.43.0

[PATCH v2 6/8] crypto/hmac: Allow to build hmac over multiple qcrypto_gnutls_hmac_bytes[v] calls

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

If the buffers that should be considered for building the hmac are not
available at the same time, the current API is unsuitable. Extend it so
that passing a NULL pointer as result_len is used as indicator that
further buffers will be passed in succeeding calls to
qcrypto_gnutls_hmac_bytes[v].

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
Cc: "Daniel P. Berrangé" <berrange@redhat.com>
---
 crypto/hmac-gcrypt.c  |  4 +++-
 crypto/hmac-glib.c    |  4 +++-
 crypto/hmac-gnutls.c  |  4 +++-
 crypto/hmac-nettle.c  |  4 +++-
 include/crypto/hmac.h | 12 ++++++++++++
 5 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/crypto/hmac-gcrypt.c b/crypto/hmac-gcrypt.c
index 5273086eb9..e428d17479 100644
--- a/crypto/hmac-gcrypt.c
+++ b/crypto/hmac-gcrypt.c
@@ -121,7 +121,9 @@ qcrypto_gcrypt_hmac_bytesv(QCryptoHmac *hmac,
         return -1;
     }
 
-    if (*resultlen == 0) {
+    if (resultlen == NULL) {
+        return 0;
+    } else if (*resultlen == 0) {
         *resultlen = ret;
         *result = g_new0(uint8_t, *resultlen);
     } else if (*resultlen != ret) {
diff --git a/crypto/hmac-glib.c b/crypto/hmac-glib.c
index ea80c8d1b2..b845133a05 100644
--- a/crypto/hmac-glib.c
+++ b/crypto/hmac-glib.c
@@ -104,7 +104,9 @@ qcrypto_glib_hmac_bytesv(QCryptoHmac *hmac,
         return -1;
     }
 
-    if (*resultlen == 0) {
+    if (resultlen == NULL) {
+        return 0;
+    } else if (*resultlen == 0) {
         *resultlen = ret;
         *result = g_new0(uint8_t, *resultlen);
     } else if (*resultlen != ret) {
diff --git a/crypto/hmac-gnutls.c b/crypto/hmac-gnutls.c
index 822995505c..3c5bcbe80b 100644
--- a/crypto/hmac-gnutls.c
+++ b/crypto/hmac-gnutls.c
@@ -119,7 +119,9 @@ qcrypto_gnutls_hmac_bytesv(QCryptoHmac *hmac,
         return -1;
     }
 
-    if (*resultlen == 0) {
+    if (resultlen == NULL) {
+        return 0;
+    } else if (*resultlen == 0) {
         *resultlen = ret;
         *result = g_new0(uint8_t, *resultlen);
     } else if (*resultlen != ret) {
diff --git a/crypto/hmac-nettle.c b/crypto/hmac-nettle.c
index dd5b2ab7a1..2cff7931e1 100644
--- a/crypto/hmac-nettle.c
+++ b/crypto/hmac-nettle.c
@@ -164,7 +164,9 @@ qcrypto_nettle_hmac_bytesv(QCryptoHmac *hmac,
         }
     }
 
-    if (*resultlen == 0) {
+    if (resultlen == NULL) {
+        return 0;
+    } else if (*resultlen == 0) {
         *resultlen = qcrypto_hmac_alg_map[hmac->alg].len;
         *result = g_new0(uint8_t, *resultlen);
     } else if (*resultlen != qcrypto_hmac_alg_map[hmac->alg].len) {
diff --git a/include/crypto/hmac.h b/include/crypto/hmac.h
index da8a1e3ceb..af3d5f8feb 100644
--- a/include/crypto/hmac.h
+++ b/include/crypto/hmac.h
@@ -90,6 +90,12 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoHmac, qcrypto_hmac_free)
  * The memory referenced in @result must be released with a call
  * to g_free() when no longer required by the caller.
  *
+ * If @result_len is set to a NULL pointer, no result will be returned, and
+ * the hmac object can be used for further invocations of qcrypto_hmac_bytes()
+ * or qcrypto_hmac_bytesv() until a non-NULL pointer is provided. This allows
+ * to build the hmac across memory regions that are not available at the same
+ * time.
+ *
  * Returns:
  *  0 on success, -1 on error
  */
@@ -123,6 +129,12 @@ int qcrypto_hmac_bytesv(QCryptoHmac *hmac,
  * The memory referenced in @result must be released with a call
  * to g_free() when no longer required by the caller.
  *
+ * If @result_len is set to a NULL pointer, no result will be returned, and
+ * the hmac object can be used for further invocations of qcrypto_hmac_bytes()
+ * or qcrypto_hmac_bytesv() until a non-NULL pointer is provided. This allows
+ * to build the hmac across memory regions that are not available at the same
+ * time.
+ *
  * Returns:
  *  0 on success, -1 on error
  */
-- 
2.43.0

[PATCH v2 7/8] hw/sd/sdcard: Handle RPMB MAC field

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Implement correct setting of the MAC field when passing RPMB frames back
to the guest. Also check the MAC on authenticated write requests.

This depends on HMAC support for QCRYPTO_HASH_ALGO_SHA256 which is
always available via glib - assert this, just to be safe.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/sd/sd.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 7b4a48f822..7ac73b8afa 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -51,6 +51,7 @@
 #include "qemu/module.h"
 #include "sdmmc-internal.h"
 #include "trace.h"
+#include "crypto/hmac.h"
 
 //#define DEBUG_SD 1
 
@@ -118,6 +119,7 @@ typedef struct SDProto {
 } SDProto;
 
 #define RPMB_KEY_MAC_LEN    32
+#define RPMB_HASH_LEN       284
 
 typedef struct {
     uint8_t stuff_bytes[196];
@@ -1125,6 +1127,66 @@ static void sd_blk_write(SDState *sd, uint64_t addr, uint32_t len)
     }
 }
 
+static bool rpmb_calc_hmac(SDState *sd, RPMBDataFrame *frame,
+                           unsigned int num_blocks, uint8_t *mac)
+{
+    size_t mac_len = RPMB_KEY_MAC_LEN;
+    bool success = true;
+    Error *err = NULL;
+    QCryptoHmac *hmac;
+    uint64_t addr;
+
+    hmac = qcrypto_hmac_new(QCRYPTO_HASH_ALGO_SHA256, sd->rpmb_key,
+                            RPMB_KEY_MAC_LEN, &err);
+    if (!hmac) {
+        error_report_err(err);
+        return false;
+    }
+
+    /*
+     * This implies a read request because we only support single-block write
+     * requests so far.
+     */
+    if (num_blocks > 1) {
+        /*
+         * Unfortunately, the underlying crypto libraries do not allow us to
+         * migrate an active QCryptoHmac state. Therefore, we have to calculate
+         * the HMAC in one run. To avoid buffering a complete read sequence in
+         * SDState, reconstruct all frames except for the last one.
+         */
+        char *buf = (char *)sd->data;
+
+        memcpy(buf, frame->data, RPMB_HASH_LEN);
+        addr = be16_to_cpu(frame->address) * 256 + sd_part_offset(sd);
+        do {
+            if (blk_pread(sd->blk, addr, 256, buf, 0) < 0) {
+                fprintf(stderr, "sd_blk_read: read error on host side\n");
+                success = false;
+                break;
+            }
+            if (qcrypto_hmac_bytes(hmac, buf, RPMB_HASH_LEN, NULL, NULL,
+                                   &err) < 0) {
+                error_report_err(err);
+                success = false;
+                break;
+            }
+            addr += 256;
+        } while (--num_blocks > 1);
+    }
+
+    if (success &&
+        qcrypto_hmac_bytes(hmac, (const char*)frame->data, RPMB_HASH_LEN, &mac,
+                           &mac_len, &err) < 0) {
+        error_report_err(err);
+        success = false;
+    }
+    assert(!success || mac_len == RPMB_KEY_MAC_LEN);
+
+    qcrypto_hmac_free(hmac);
+
+    return success;
+}
+
 static void emmc_rpmb_blk_read(SDState *sd, uint64_t addr, uint32_t len)
 {
     uint16_t resp = be16_to_cpu(sd->rpmb_result.req_resp);
@@ -1147,6 +1209,17 @@ static void emmc_rpmb_blk_read(SDState *sd, uint64_t addr, uint32_t len)
             sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_READ_FAILURE |
                 (result & RPMB_RESULT_COUTER_EXPIRED));
         }
+        if (sd->multi_blk_cnt == 1 &&
+            !rpmb_calc_hmac(sd, &sd->rpmb_result,
+                            be16_to_cpu(sd->rpmb_result.block_count),
+                            sd->rpmb_result.key_mac)) {
+            memset(sd->rpmb_result.data, 0, sizeof(sd->rpmb_result.data));
+            sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_AUTH_FAILURE);
+        }
+    } else if (!rpmb_calc_hmac(sd, &sd->rpmb_result, 1,
+                               sd->rpmb_result.key_mac)) {
+        memset(sd->rpmb_result.data, 0, sizeof(sd->rpmb_result.data));
+        sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_AUTH_FAILURE);
     }
     memcpy(sd->data, &sd->rpmb_result, sizeof(sd->rpmb_result));
 
@@ -1158,6 +1231,7 @@ static void emmc_rpmb_blk_write(SDState *sd, uint64_t addr, uint32_t len)
 {
     RPMBDataFrame *frame = (RPMBDataFrame *)sd->data;
     uint16_t req = be16_to_cpu(frame->req_resp);
+    uint8_t mac[RPMB_KEY_MAC_LEN];
 
     if (req == RPMB_REQ_READ_RESULT) {
         /* just return the current result register */
@@ -1195,6 +1269,11 @@ static void emmc_rpmb_blk_write(SDState *sd, uint64_t addr, uint32_t len)
             sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_WRITE_FAILURE);
             break;
         }
+        if (!rpmb_calc_hmac(sd, frame, 1, mac) ||
+            memcmp(frame->key_mac, mac, RPMB_KEY_MAC_LEN) != 0) {
+            sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_AUTH_FAILURE);
+            break;
+        }
         if (be32_to_cpu(frame->write_counter) != sd->rpmb_write_counter) {
             sd->rpmb_result.result = cpu_to_be16(RPMB_RESULT_COUNTER_FAILURE);
             break;
@@ -3100,6 +3179,8 @@ static void emmc_class_init(ObjectClass *klass, const void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     SDCardClass *sc = SDMMC_COMMON_CLASS(klass);
 
+    assert(qcrypto_hmac_supports(QCRYPTO_HASH_ALGO_SHA256));
+
     dc->desc = "eMMC";
     dc->realize = emmc_realize;
     device_class_set_props(dc, emmc_properties);
-- 
2.43.0

[PATCH v2 8/8] scripts: Add helper script to generate eMMC block device images

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

As an eMMC block device image may consist of more than just the user
data partition, provide a helper script that can compose the image from
boot partitions, an RPMB partition and the user data image. The script
also does the required size validation and/or rounding.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 scripts/mkemmc.sh | 186 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 186 insertions(+)
 create mode 100755 scripts/mkemmc.sh

diff --git a/scripts/mkemmc.sh b/scripts/mkemmc.sh
new file mode 100755
index 0000000000..9e52cd5f32
--- /dev/null
+++ b/scripts/mkemmc.sh
@@ -0,0 +1,186 @@
+#!/bin/sh -e
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Create eMMC block device image from boot, RPMB and user data images
+#
+# Copyright (c) Siemens, 2025
+#
+# Authors:
+#  Jan Kiszka <jan.kiszka@siemens.com>
+#
+# This work is licensed under the terms of the GNU GPL version 2.
+# See the COPYING file in the top-level directory.
+#
+
+usage() {
+    echo "$0 [OPTIONS] USER_IMG[:SIZE] OUTPUT_IMG"
+    echo ""
+    echo "SIZE must be a power of 2. If no SIZE is specified, the size of USER_ING will"
+    echo "be used (rounded up)."
+    echo ""
+    echo "Supported options:"
+    echo "  -b BOOT1_IMG[:SIZE]   Add boot partitions. SIZE must be multiples of 128K. If"
+    echo "                          no SIZE is specified, the size of BOOT_IMG will be"
+    echo "                          used (rounded up). BOOT1_IMG will be stored in boot"
+    echo "                          partition 1, and a boot partition 2 of the same size"
+    echo "                          will be created as empty (all zeros) unless -B is"
+    echo "                          specified as well."
+    echo "  -B BOOT2_IMG          Fill boot partition 2 with BOOT2_IMG. Must be combined"
+    echo "                          with -b which is also defining the partition size."
+    echo "  -r RPMB_IMG[:SIZE]    Add RPMB partition. SIZE must be multiples of 128K. If"
+    echo "                          no SIZE is specified, the size of RPMB_IMG will be"
+    echo "                          used (rounded up)."
+    echo "  -h, --help            This help"
+    echo ""
+    echo "All SIZE parameters support the units K, M, G. If SIZE is smaller than the"
+    echo "associated image, it will be truncated in the output image."
+    exit "$1"
+}
+
+process_size() {
+    if [ "${4#*:}" = "$4"  ]; then
+        if ! size=$(stat -L -c %s "$2" 2>/dev/null); then
+            echo "Missing $1 image '$2'." >&2
+            exit 1
+        fi
+        if [ "$3" = 128 ]; then
+            size=$(( (size + 128 * 1024 - 1) & ~(128 * 1024 - 1) ))
+        elif [ $(( size & (size - 1) )) -gt 0 ]; then
+            n=0
+            while [ "$size" -gt 0 ]; do
+                size=$((size >> 1))
+                n=$((n + 1))
+            done
+            size=$((1 << n))
+        fi
+    else
+        value="${4#*:}"
+        if [ "${value%K}" != "$value" ]; then
+            size=${value%K}
+            multiplier=1024
+        elif [ "${value%M}" != "$value" ]; then
+            size=${value%M}
+            multiplier=$((1024 * 1024))
+        elif [ "${value%G}" != "$value" ]; then
+            size=${value%G}
+            multiplier=$((1024 * 1024 * 1024))
+        else
+            size=$value
+            multiplier=1
+        fi
+        if [ "$size" -eq "$size" ] 2>/dev/null; then
+            size=$((size * multiplier))
+        else
+            echo "Invalid value '$value' specified for $2 image size." >&2
+            exit 1
+        fi
+        if [ "$3" = 128 ]; then
+            if [ $(( size & (128 * 1024 - 1) )) -ne 0 ]; then
+                echo "The $2 image size must be multiples of 128K." >&2
+                exit 1
+            fi
+        elif [ $(( size & (size - 1) )) -gt 0 ]; then
+            echo "The %2 image size must be power of 2." >&2
+            exit 1
+        fi
+    fi
+    echo $size
+}
+
+userimg=
+outimg=
+bootimg1=
+bootimg2=/dev/zero
+bootsz=0
+rpmbimg=
+rpmbsz=0
+
+while [ $# -gt 0 ]; do
+    case "$1" in
+        -b)
+            shift
+            [ $# -ge 1 ] || usage 1
+            bootimg1=${1%%:*}
+            bootsz=$(process_size boot "$bootimg1" 128 "$1")
+            shift
+            ;;
+        -B)
+            shift
+            [ $# -ge 1 ] || usage 1
+            bootimg2=$1
+            shift
+            ;;
+        -r)
+            shift
+            [ $# -ge 1 ] || usage 1
+            rpmbimg=${1%%:*}
+            rpmbsz=$(process_size RPMB "$rpmbimg" 128 "$1")
+            shift
+            ;;
+        -h|--help)
+            usage 0
+            ;;
+        *)
+            if [ -z "$userimg" ]; then
+                userimg=${1%%:*}
+                usersz=$(process_size user "$userimg" 2 "$1")
+            elif [ -z "$outimg" ]; then
+                outimg=$1
+            else
+                usage 1
+            fi
+            shift
+            ;;
+    esac
+done
+
+[ -n "$outimg" ] || usage 1
+
+if [ "$bootsz" -gt $((32640 * 1024)) ]; then
+    echo "Boot image size is larger than 32640K." >&2
+    exit 1
+fi
+if [ "$rpmbsz" -gt $((16384 * 1024)) ]; then
+    echo "RPMB image size is larger than 16384K." >&2
+    exit 1
+fi
+
+echo "Creating eMMC image"
+
+truncate "$outimg" -s 0
+pos=0
+
+if [ "$bootsz" -gt 0 ]; then
+    echo "  Boot partition 1 and 2:   $((bootsz / 1024))K each"
+    blocks=$(( bootsz / (128 * 1024) ))
+    dd if="$bootimg1" of="$outimg" conv=sparse bs=128K count=$blocks \
+        status=none
+    dd if="$bootimg2" of="$outimg" conv=sparse bs=128K count=$blocks \
+        seek=$blocks status=none
+    pos=$((2 * bootsz))
+fi
+
+if [ "$rpmbsz" -gt 0 ]; then
+    echo "  RPMB partition:           $((rpmbsz / 1024))K"
+    blocks=$(( rpmbsz / (128 * 1024) ))
+    dd if="$rpmbimg" of="$outimg" conv=sparse bs=128K count=$blocks \
+        seek=$(( pos / (128 * 1024) )) status=none
+    pos=$((pos + rpmbsz))
+fi
+
+if [ "$usersz" -lt 1024 ]; then
+    echo "  User data:                $usersz bytes"
+elif [ "$usersz" -lt $((1024 * 1024)) ]; then
+    echo "  User data:                $(( (usersz + 1023) / 1024 ))K ($usersz)"
+elif [ "$usersz" -lt $((1024 * 1024 * 1024)) ]; then
+    echo "  User data:                $(( (usersz + 1048575) / 1048576))M ($usersz)"
+else
+    echo "  User data:                $(( (usersz + 1073741823) / 1073741824))G ($usersz)"
+fi
+dd if="$userimg" of="$outimg" conv=sparse bs=128K seek=$(( pos / (128 * 1024) )) \
+    count=$(( (usersz + 128 * 1024 - 1) / (128 * 1024) )) status=none
+pos=$((pos + usersz))
+truncate "$outimg" -s $pos
+
+echo ""
+echo "Instantiate via '-device emmc,boot-partition-size=$bootsz,rpmb-partition-size=$rpmbsz,drive=$outimg'"
-- 
2.43.0

Re: [PATCH v2 6/8] crypto/hmac: Allow to build hmac over multiple qcrypto_gnutls_hmac_bytes[v] calls

[Daniel P. Berrangé]

On Mon, Sep 01, 2025 at 07:56:26AM +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> If the buffers that should be considered for building the hmac are not
> available at the same time, the current API is unsuitable. Extend it so
> that passing a NULL pointer as result_len is used as indicator that
> further buffers will be passed in succeeding calls to
> qcrypto_gnutls_hmac_bytes[v].
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> ---
>  crypto/hmac-gcrypt.c  |  4 +++-
>  crypto/hmac-glib.c    |  4 +++-
>  crypto/hmac-gnutls.c  |  4 +++-
>  crypto/hmac-nettle.c  |  4 +++-
>  include/crypto/hmac.h | 12 ++++++++++++
>  5 files changed, 24 insertions(+), 4 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 2/8] hw/sd/sdcard: Add validation for boot-partition-size

[Alex Bennée]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> Make sure we are not silently rounding down or even wrapping around,
> causing inconsistencies with the provided image.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  hw/sd/sd.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 16aee210b4..834392b0a8 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2818,6 +2818,16 @@ static void sd_realize(DeviceState *dev, Error **errp)
>          }
>          blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
>      }
> +    if (sd->boot_part_size % (128 * KiB) ||
> +        sd->boot_part_size > 255 * 128 * KiB) {
> +        char *size_str = size_to_str(sd->boot_part_size);
> +

This could be:

  g_autofree char *size_str = size_to_str(sd->boot_part_size);

> +        error_setg(errp, "Invalid boot partition size: %s", size_str);
> +        g_free(size_str);

which drops this.

> +        error_append_hint(errp,
> +                          "The boot partition size must be multiples of 128K"
> +                          "and not larger than 32640K.\n");
> +    }
>  }
>  
>  static void emmc_realize(DeviceState *dev, Error **errp)

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 8/8] scripts: Add helper script to generate eMMC block device images

[Alex Bennée]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> As an eMMC block device image may consist of more than just the user
> data partition, provide a helper script that can compose the image from
> boot partitions, an RPMB partition and the user data image. The script
> also does the required size validation and/or rounding.
>
<snip>
> +
> +echo ""
> +echo "Instantiate via '-device emmc,boot-partition-size=$bootsz,rpmb-partition-size=$rpmbsz,drive=$outimg'"

Given there is no explicit option for emmc which we can document in
qemu-options I think we could do with a basic documentation of the now
user-creatable emmc device in the "Emulated Devices" section. You could
then give an example of the scripts usage as well.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 0/8] sd: Add RPMB emulation to eMMC model

[Philippe Mathieu-Daudé]

Hi Jan,

On 1/9/25 07:56, Jan Kiszka wrote:

> Jan Kiszka (8):
>    hw/sd/sdcard: Fix size check for backing block image
>    hw/sd/sdcard: Add validation for boot-partition-size

>    hw/sd/sdcard: Refactor sd_bootpart_offset

>    crypto/hmac: Allow to build hmac over multiple
>      qcrypto_gnutls_hmac_bytes[v] calls
I'm queuing the 4 reviewed preparatory patches to alleviate
your series (and keep the RPMB patches in my TOREVIEW folder
-- no objection so far, I just need more testing time).

Regards,

Phil.

Re: [PATCH v2 0/8] sd: Add RPMB emulation to eMMC model

[Jan Kiszka]

On 01.09.25 22:58, Philippe Mathieu-Daudé wrote:
> Hi Jan,
> 
> On 1/9/25 07:56, Jan Kiszka wrote:
> 
>> Jan Kiszka (8):
>>    hw/sd/sdcard: Fix size check for backing block image
>>    hw/sd/sdcard: Add validation for boot-partition-size
> 
>>    hw/sd/sdcard: Refactor sd_bootpart_offset
> 
>>    crypto/hmac: Allow to build hmac over multiple
>>      qcrypto_gnutls_hmac_bytes[v] calls
> I'm queuing the 4 reviewed preparatory patches to alleviate
> your series (and keep the RPMB patches in my TOREVIEW folder
> -- no objection so far, I just need more testing time).
> 

As Alex had one useful proposal for patch 2 - should I patch that on top
then?

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH v2 0/8] sd: Add RPMB emulation to eMMC model

[Philippe Mathieu-Daudé]

On 2/9/25 13:42, Jan Kiszka wrote:
> On 01.09.25 22:58, Philippe Mathieu-Daudé wrote:
>> Hi Jan,
>>
>> On 1/9/25 07:56, Jan Kiszka wrote:
>>
>>> Jan Kiszka (8):
>>>     hw/sd/sdcard: Fix size check for backing block image
>>>     hw/sd/sdcard: Add validation for boot-partition-size
>>
>>>     hw/sd/sdcard: Refactor sd_bootpart_offset
>>
>>>     crypto/hmac: Allow to build hmac over multiple
>>>       qcrypto_gnutls_hmac_bytes[v] calls
>> I'm queuing the 4 reviewed preparatory patches to alleviate
>> your series (and keep the RPMB patches in my TOREVIEW folder
>> -- no objection so far, I just need more testing time).
>>
> 
> As Alex had one useful proposal for patch 2 - should I patch that on top
> then?

Already taken care of ;)
https://lore.kernel.org/qemu-devel/20250902131016.84968-31-philmd@linaro.org/

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Philippe Mathieu-Daudé]

On 1/9/25 07:56, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> The power-of-2 rule applies to the user data area, not the complete
> block image. The latter can be concatenation of boot partition images
> and the user data.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/sd/sd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 8c290595f0..16aee210b4 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
>               return;
>           }
>   
> -        blk_size = blk_getlength(sd->blk);
> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>           if (blk_size > 0 && !is_power_of_2(blk_size)) {
>               int64_t blk_size_aligned = pow2ceil(blk_size);
>               char *blk_size_str;

This seems to break the tests/functional/arm/test_aspeed_rainier.py
test due to mmc-p10bmc-20240617.qcow2 size:

Command: /builds/qemu-project/qemu/build/qemu-system-arm -display none 
-vga none -chardev socket,id=mon,fd=5 -mon chardev=mon,mode=control 
-machine rainier-bmc -chardev socket,id=console,fd=10 -serial 
chardev:console -drive 
file=/builds/qemu-project/qemu/functional-cache/download/d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 
-net nic -net user -snapshot
Output: qemu-system-arm: Invalid SD card size: 16 GiB
SD card size has to be a power of 2, e.g. 16 GiB.

https://gitlab.com/qemu-project/qemu/-/jobs/11217561316

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Jan Kiszka]

On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
> On 1/9/25 07:56, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> The power-of-2 rule applies to the user data area, not the complete
>> block image. The latter can be concatenation of boot partition images
>> and the user data.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/sd/sd.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 8c290595f0..16aee210b4 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev, Error
>> **errp)
>>               return;
>>           }
>>   -        blk_size = blk_getlength(sd->blk);
>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>>           if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>               int64_t blk_size_aligned = pow2ceil(blk_size);
>>               char *blk_size_str;
> 
> This seems to break the tests/functional/arm/test_aspeed_rainier.py
> test due to mmc-p10bmc-20240617.qcow2 size:
> 
> Command: /builds/qemu-project/qemu/build/qemu-system-arm -display none -
> vga none -chardev socket,id=mon,fd=5 -mon chardev=mon,mode=control -
> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
> chardev:console -drive file=/builds/qemu-project/qemu/functional-cache/
> download/
> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
> Output: qemu-system-arm: Invalid SD card size: 16 GiB
> SD card size has to be a power of 2, e.g. 16 GiB.
> 
> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
> 

Hmm, then the test was always wrong as well. I suspect the aspeed is
enabling boot partitions by default, and the image was created to pass
the wrong alignment check. Where / by whom is the image maintained?

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Philippe Mathieu-Daudé]

On 2/9/25 17:34, Jan Kiszka wrote:
> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
>> On 1/9/25 07:56, Jan Kiszka wrote:
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> The power-of-2 rule applies to the user data area, not the complete
>>> block image. The latter can be concatenation of boot partition images
>>> and the user data.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    hw/sd/sd.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index 8c290595f0..16aee210b4 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev, Error
>>> **errp)
>>>                return;
>>>            }
>>>    -        blk_size = blk_getlength(sd->blk);
>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>>>            if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>                int64_t blk_size_aligned = pow2ceil(blk_size);
>>>                char *blk_size_str;
>>
>> This seems to break the tests/functional/arm/test_aspeed_rainier.py
>> test due to mmc-p10bmc-20240617.qcow2 size:
>>
>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -display none -
>> vga none -chardev socket,id=mon,fd=5 -mon chardev=mon,mode=control -
>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
>> chardev:console -drive file=/builds/qemu-project/qemu/functional-cache/
>> download/
>> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
>> SD card size has to be a power of 2, e.g. 16 GiB.
>>
>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
>>
> 
> Hmm, then the test was always wrong as well. I suspect the aspeed is
> enabling boot partitions by default, and the image was created to pass
> the wrong alignment check. Where / by whom is the image maintained?

Cédric Le Goater (Cc'ed).

The test comes from:
https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb-a85f-09964268533d@kaod.org/

Maybe also relevant to your suspicion:
https://lore.kernel.org/qemu-devel/e401d119-402e-0edd-c2bf-28950ba48ccb@kaod.org/

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Cédric Le Goater]

On 9/2/25 17:34, Jan Kiszka wrote:
> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
>> On 1/9/25 07:56, Jan Kiszka wrote:
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> The power-of-2 rule applies to the user data area, not the complete
>>> block image. The latter can be concatenation of boot partition images
>>> and the user data.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    hw/sd/sd.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index 8c290595f0..16aee210b4 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev, Error
>>> **errp)
>>>                return;
>>>            }
>>>    -        blk_size = blk_getlength(sd->blk);
>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>>>            if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>                int64_t blk_size_aligned = pow2ceil(blk_size);
>>>                char *blk_size_str;
>>
>> This seems to break the tests/functional/arm/test_aspeed_rainier.py
>> test due to mmc-p10bmc-20240617.qcow2 size:
>>
>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -display none -
>> vga none -chardev socket,id=mon,fd=5 -mon chardev=mon,mode=control -
>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
>> chardev:console -drive file=/builds/qemu-project/qemu/functional-cache/
>> download/
>> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
>> SD card size has to be a power of 2, e.g. 16 GiB.
>>
>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
>>
> 
> Hmm, then the test was always wrong as well. I suspect the aspeed is
> enabling boot partitions by default, and the image was created to pass
> the wrong alignment check. Where / by whom is the image maintained?

See this commit to know how it is constructed :

   https://gitlab.com/qemu-project/qemu/-/commit/c8cb19876d3e

Thanks,

C.

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Philippe Mathieu-Daudé]

On 2/9/25 17:43, Philippe Mathieu-Daudé wrote:
> On 2/9/25 17:34, Jan Kiszka wrote:
>> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
>>> On 1/9/25 07:56, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> The power-of-2 rule applies to the user data area, not the complete
>>>> block image. The latter can be concatenation of boot partition images
>>>> and the user data.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>    hw/sd/sd.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>> index 8c290595f0..16aee210b4 100644
>>>> --- a/hw/sd/sd.c
>>>> +++ b/hw/sd/sd.c
>>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev, Error
>>>> **errp)
>>>>                return;
>>>>            }
>>>>    -        blk_size = blk_getlength(sd->blk);
>>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>>>>            if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>                int64_t blk_size_aligned = pow2ceil(blk_size);
>>>>                char *blk_size_str;
>>>
>>> This seems to break the tests/functional/arm/test_aspeed_rainier.py
>>> test due to mmc-p10bmc-20240617.qcow2 size:
>>>
>>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -display none -
>>> vga none -chardev socket,id=mon,fd=5 -mon chardev=mon,mode=control -
>>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
>>> chardev:console -drive file=/builds/qemu-project/qemu/functional-cache/
>>> download/
>>> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
>>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
>>> SD card size has to be a power of 2, e.g. 16 GiB.
>>>
>>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
>>>
>>
>> Hmm, then the test was always wrong as well. I suspect the aspeed is
>> enabling boot partitions by default, and the image was created to pass
>> the wrong alignment check. Where / by whom is the image maintained?
> 
> Cédric Le Goater (Cc'ed).
> 
> The test comes from:
> https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb- 
> a85f-09964268533d@kaod.org/
> 
> Maybe also relevant to your suspicion:
> https://lore.kernel.org/qemu-devel/e401d119-402e-0edd- 
> c2bf-28950ba48ccb@kaod.org/

Digging further:
https://lore.kernel.org/qemu-devel/9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Philippe Mathieu-Daudé]

On 2/9/25 17:43, Cédric Le Goater wrote:
> On 9/2/25 17:34, Jan Kiszka wrote:
>> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
>>> On 1/9/25 07:56, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> The power-of-2 rule applies to the user data area, not the complete
>>>> block image. The latter can be concatenation of boot partition images
>>>> and the user data.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>    hw/sd/sd.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>> index 8c290595f0..16aee210b4 100644
>>>> --- a/hw/sd/sd.c
>>>> +++ b/hw/sd/sd.c
>>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev, Error
>>>> **errp)
>>>>                return;
>>>>            }
>>>>    -        blk_size = blk_getlength(sd->blk);
>>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>>>>            if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>                int64_t blk_size_aligned = pow2ceil(blk_size);
>>>>                char *blk_size_str;
>>>
>>> This seems to break the tests/functional/arm/test_aspeed_rainier.py
>>> test due to mmc-p10bmc-20240617.qcow2 size:
>>>
>>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -display none -
>>> vga none -chardev socket,id=mon,fd=5 -mon chardev=mon,mode=control -
>>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
>>> chardev:console -drive file=/builds/qemu-project/qemu/functional-cache/
>>> download/
>>> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
>>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
>>> SD card size has to be a power of 2, e.g. 16 GiB.
>>>
>>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
>>>
>>
>> Hmm, then the test was always wrong as well. I suspect the aspeed is
>> enabling boot partitions by default, and the image was created to pass
>> the wrong alignment check. Where / by whom is the image maintained?
> 
> See this commit to know how it is constructed :
> 
>    https://gitlab.com/qemu-project/qemu/-/commit/c8cb19876d3e

Hmm now I remember. I wasn't enthusiastic about that approach and
suggested to use block drive for each partition, but it was not
well received as too complex on the command line.

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Cédric Le Goater]

On 9/2/25 17:45, Philippe Mathieu-Daudé wrote:
> On 2/9/25 17:43, Philippe Mathieu-Daudé wrote:
>> On 2/9/25 17:34, Jan Kiszka wrote:
>>> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
>>>> On 1/9/25 07:56, Jan Kiszka wrote:
>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>> The power-of-2 rule applies to the user data area, not the complete
>>>>> block image. The latter can be concatenation of boot partition images
>>>>> and the user data.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> ---
>>>>>    hw/sd/sd.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>>> index 8c290595f0..16aee210b4 100644
>>>>> --- a/hw/sd/sd.c
>>>>> +++ b/hw/sd/sd.c
>>>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev, Error
>>>>> **errp)
>>>>>                return;
>>>>>            }
>>>>>    -        blk_size = blk_getlength(sd->blk);
>>>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>>>>>            if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>>                int64_t blk_size_aligned = pow2ceil(blk_size);
>>>>>                char *blk_size_str;
>>>>
>>>> This seems to break the tests/functional/arm/test_aspeed_rainier.py
>>>> test due to mmc-p10bmc-20240617.qcow2 size:
>>>>
>>>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -display none -
>>>> vga none -chardev socket,id=mon,fd=5 -mon chardev=mon,mode=control -
>>>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
>>>> chardev:console -drive file=/builds/qemu-project/qemu/functional-cache/
>>>> download/
>>>> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
>>>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
>>>> SD card size has to be a power of 2, e.g. 16 GiB.
>>>>
>>>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
>>>>
>>>
>>> Hmm, then the test was always wrong as well. I suspect the aspeed is
>>> enabling boot partitions by default, and the image was created to pass
>>> the wrong alignment check. Where / by whom is the image maintained?
>>
>> Cédric Le Goater (Cc'ed).
>>
>> The test comes from:
>> https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb- a85f-09964268533d@kaod.org/
>>
>> Maybe also relevant to your suspicion:
>> https://lore.kernel.org/qemu-devel/e401d119-402e-0edd- c2bf-28950ba48ccb@kaod.org/
> 
> Digging further:
> https://lore.kernel.org/qemu-devel/9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/
> 

yes commit c078298301a8 might have some impact there.

C.

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Philippe Mathieu-Daudé]

On 2/9/25 17:47, Cédric Le Goater wrote:
> On 9/2/25 17:45, Philippe Mathieu-Daudé wrote:
>> On 2/9/25 17:43, Philippe Mathieu-Daudé wrote:
>>> On 2/9/25 17:34, Jan Kiszka wrote:
>>>> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
>>>>> On 1/9/25 07:56, Jan Kiszka wrote:
>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>
>>>>>> The power-of-2 rule applies to the user data area, not the complete
>>>>>> block image. The latter can be concatenation of boot partition images
>>>>>> and the user data.
>>>>>>
>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> ---
>>>>>>    hw/sd/sd.c | 2 +-
>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>>>> index 8c290595f0..16aee210b4 100644
>>>>>> --- a/hw/sd/sd.c
>>>>>> +++ b/hw/sd/sd.c
>>>>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev, Error
>>>>>> **errp)
>>>>>>                return;
>>>>>>            }
>>>>>>    -        blk_size = blk_getlength(sd->blk);
>>>>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>>>>>>            if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>>>                int64_t blk_size_aligned = pow2ceil(blk_size);
>>>>>>                char *blk_size_str;
>>>>>
>>>>> This seems to break the tests/functional/arm/test_aspeed_rainier.py
>>>>> test due to mmc-p10bmc-20240617.qcow2 size:
>>>>>
>>>>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -display 
>>>>> none -
>>>>> vga none -chardev socket,id=mon,fd=5 -mon chardev=mon,mode=control -
>>>>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
>>>>> chardev:console -drive file=/builds/qemu-project/qemu/functional- 
>>>>> cache/
>>>>> download/
>>>>> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
>>>>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
>>>>> SD card size has to be a power of 2, e.g. 16 GiB.
>>>>>
>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
>>>>>
>>>>
>>>> Hmm, then the test was always wrong as well. I suspect the aspeed is
>>>> enabling boot partitions by default, and the image was created to pass
>>>> the wrong alignment check. Where / by whom is the image maintained?
>>>
>>> Cédric Le Goater (Cc'ed).
>>>
>>> The test comes from:
>>> https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb- 
>>> a85f-09964268533d@kaod.org/
>>>
>>> Maybe also relevant to your suspicion:
>>> https://lore.kernel.org/qemu-devel/e401d119-402e-0edd- 
>>> c2bf-28950ba48ccb@kaod.org/
>>
>> Digging further:
>> https://lore.kernel.org/qemu- 
>> devel/9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/
>>
> 
> yes commit c078298301a8 might have some impact there.

With Jan patch, your script doesn't need anymore the

   echo "Fixing size to keep qemu happy..."

kludge.

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Cédric Le Goater]

On 9/2/25 17:34, Jan Kiszka wrote:
> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
>> On 1/9/25 07:56, Jan Kiszka wrote:
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> The power-of-2 rule applies to the user data area, not the complete
>>> block image. The latter can be concatenation of boot partition images
>>> and the user data.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>    hw/sd/sd.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index 8c290595f0..16aee210b4 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev, Error
>>> **errp)
>>>                return;
>>>            }
>>>    -        blk_size = blk_getlength(sd->blk);
>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>>>            if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>                int64_t blk_size_aligned = pow2ceil(blk_size);
>>>                char *blk_size_str;
>>
>> This seems to break the tests/functional/arm/test_aspeed_rainier.py
>> test due to mmc-p10bmc-20240617.qcow2 size:
>>
>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -display none -
>> vga none -chardev socket,id=mon,fd=5 -mon chardev=mon,mode=control -
>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
>> chardev:console -drive file=/builds/qemu-project/qemu/functional-cache/
>> download/
>> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
>> SD card size has to be a power of 2, e.g. 16 GiB.
>>
>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
>>
> 
> Hmm, then the test was always wrong as well. I suspect the aspeed is
> enabling boot partitions by default, 

Yes for the rainier machine which boots from eMMC :

         if (emmc && boot_emmc) {
             qdev_prop_set_uint64(card, "boot-partition-size", 1 * MiB);
             qdev_prop_set_uint8(card, "boot-config", 0x1 << 3);
         }

In this case, the emmc image is tailored to support boot. It's no
different from real HW AFAICT. It's old (6/7y).

Thanks,

C.

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Cédric Le Goater]

On 9/2/25 17:55, Philippe Mathieu-Daudé wrote:
> On 2/9/25 17:47, Cédric Le Goater wrote:
>> On 9/2/25 17:45, Philippe Mathieu-Daudé wrote:
>>> On 2/9/25 17:43, Philippe Mathieu-Daudé wrote:
>>>> On 2/9/25 17:34, Jan Kiszka wrote:
>>>>> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
>>>>>> On 1/9/25 07:56, Jan Kiszka wrote:
>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>
>>>>>>> The power-of-2 rule applies to the user data area, not the complete
>>>>>>> block image. The latter can be concatenation of boot partition images
>>>>>>> and the user data.
>>>>>>>
>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>> ---
>>>>>>>    hw/sd/sd.c | 2 +-
>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>>>>> index 8c290595f0..16aee210b4 100644
>>>>>>> --- a/hw/sd/sd.c
>>>>>>> +++ b/hw/sd/sd.c
>>>>>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev, Error
>>>>>>> **errp)
>>>>>>>                return;
>>>>>>>            }
>>>>>>>    -        blk_size = blk_getlength(sd->blk);
>>>>>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>>>>>>>            if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>>>>                int64_t blk_size_aligned = pow2ceil(blk_size);
>>>>>>>                char *blk_size_str;
>>>>>>
>>>>>> This seems to break the tests/functional/arm/test_aspeed_rainier.py
>>>>>> test due to mmc-p10bmc-20240617.qcow2 size:
>>>>>>
>>>>>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -display none -
>>>>>> vga none -chardev socket,id=mon,fd=5 -mon chardev=mon,mode=control -
>>>>>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
>>>>>> chardev:console -drive file=/builds/qemu-project/qemu/functional- cache/
>>>>>> download/
>>>>>> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
>>>>>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
>>>>>> SD card size has to be a power of 2, e.g. 16 GiB.
>>>>>>
>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
>>>>>>
>>>>>
>>>>> Hmm, then the test was always wrong as well. I suspect the aspeed is
>>>>> enabling boot partitions by default, and the image was created to pass
>>>>> the wrong alignment check. Where / by whom is the image maintained?
>>>>
>>>> Cédric Le Goater (Cc'ed).
>>>>
>>>> The test comes from:
>>>> https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb- a85f-09964268533d@kaod.org/
>>>>
>>>> Maybe also relevant to your suspicion:
>>>> https://lore.kernel.org/qemu-devel/e401d119-402e-0edd- c2bf-28950ba48ccb@kaod.org/
>>>
>>> Digging further:
>>> https://lore.kernel.org/qemu- devel/9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/
>>>
>>
>> yes commit c078298301a8 might have some impact there.
> 
> With Jan patch, your script doesn't need anymore the
> 
>    echo "Fixing size to keep qemu happy..."
> 
> kludge.
  
which script ?

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Philippe Mathieu-Daudé]

On 2/9/25 18:00, Cédric Le Goater wrote:
> On 9/2/25 17:55, Philippe Mathieu-Daudé wrote:
>> On 2/9/25 17:47, Cédric Le Goater wrote:
>>> On 9/2/25 17:45, Philippe Mathieu-Daudé wrote:
>>>> On 2/9/25 17:43, Philippe Mathieu-Daudé wrote:
>>>>> On 2/9/25 17:34, Jan Kiszka wrote:
>>>>>> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
>>>>>>> On 1/9/25 07:56, Jan Kiszka wrote:
>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>
>>>>>>>> The power-of-2 rule applies to the user data area, not the complete
>>>>>>>> block image. The latter can be concatenation of boot partition 
>>>>>>>> images
>>>>>>>> and the user data.
>>>>>>>>
>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>> ---
>>>>>>>>    hw/sd/sd.c | 2 +-
>>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>>>>>> index 8c290595f0..16aee210b4 100644
>>>>>>>> --- a/hw/sd/sd.c
>>>>>>>> +++ b/hw/sd/sd.c
>>>>>>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev, 
>>>>>>>> Error
>>>>>>>> **errp)
>>>>>>>>                return;
>>>>>>>>            }
>>>>>>>>    -        blk_size = blk_getlength(sd->blk);
>>>>>>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size 
>>>>>>>> * 2;
>>>>>>>>            if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>>>>>                int64_t blk_size_aligned = pow2ceil(blk_size);
>>>>>>>>                char *blk_size_str;
>>>>>>>
>>>>>>> This seems to break the tests/functional/arm/test_aspeed_rainier.py
>>>>>>> test due to mmc-p10bmc-20240617.qcow2 size:
>>>>>>>
>>>>>>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -display 
>>>>>>> none -
>>>>>>> vga none -chardev socket,id=mon,fd=5 -mon chardev=mon,mode=control -
>>>>>>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
>>>>>>> chardev:console -drive file=/builds/qemu-project/qemu/functional- 
>>>>>>> cache/
>>>>>>> download/
>>>>>>> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
>>>>>>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
>>>>>>> SD card size has to be a power of 2, e.g. 16 GiB.
>>>>>>>
>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
>>>>>>>
>>>>>>
>>>>>> Hmm, then the test was always wrong as well. I suspect the aspeed is
>>>>>> enabling boot partitions by default, and the image was created to 
>>>>>> pass
>>>>>> the wrong alignment check. Where / by whom is the image maintained?
>>>>>
>>>>> Cédric Le Goater (Cc'ed).
>>>>>
>>>>> The test comes from:
>>>>> https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb- 
>>>>> a85f-09964268533d@kaod.org/
>>>>>
>>>>> Maybe also relevant to your suspicion:
>>>>> https://lore.kernel.org/qemu-devel/e401d119-402e-0edd- 
>>>>> c2bf-28950ba48ccb@kaod.org/

[*]

>>>>
>>>> Digging further:
>>>> https://lore.kernel.org/qemu- 
>>>> devel/9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/
>>>>
>>>
>>> yes commit c078298301a8 might have some impact there.
>>
>> With Jan patch, your script doesn't need anymore the
>>
>>    echo "Fixing size to keep qemu happy..."
>>
>> kludge.
> 
> which script ?

The one you pasted in [*]:

--
#!/bin/sh

URLBASE=https://jenkins.openbmc.org/view/latest/job/latest-master/label=docker-builder,target=witherspoon-tacoma/lastSuccessfulBuild/artifact/openbmc/build/tmp/deploy/images/witherspoon-tacoma/

IMAGESIZE=128
OUTFILE=mmc.img

FILES="u-boot.bin u-boot-spl.bin 
obmc-phosphor-image-witherspoon-tacoma.wic.xz"

for file in ${FILES}; do

	if test -f ${file}; then
		echo "${file}: Already downloaded"
	else
		echo "${file}: Downloading"
		wget -nv ${URLBASE}/${file}
	fi
done

echo

echo "Creating empty image..."
dd status=none if=/dev/zero of=${OUTFILE} bs=1M count=${IMAGESIZE}
echo "Adding SPL..."
dd status=none if=u-boot-spl.bin of=${OUTFILE} conv=notrunc
echo "Adding u-boot..."
dd status=none if=u-boot.bin of=${OUTFILE} conv=notrunc bs=1K seek=64
echo "Adding userdata..."
unxz -c obmc-phosphor-image-witherspoon-tacoma.wic.xz | dd 
status=progress of=${OUTFILE} conv=notrunc bs=1M seek=2
echo "Fixing size to keep qemu happy..."
truncate --size 16G ${OUTFILE}

echo "Done!"
echo
echo " qemu-system-arm -M tacoma-bmc -nographic -drive 
file=mmc.img,if=sd,index=2,format=raw"
---

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Cédric Le Goater]

On 9/2/25 18:14, Philippe Mathieu-Daudé wrote:
> On 2/9/25 18:00, Cédric Le Goater wrote:
>> On 9/2/25 17:55, Philippe Mathieu-Daudé wrote:
>>> On 2/9/25 17:47, Cédric Le Goater wrote:
>>>> On 9/2/25 17:45, Philippe Mathieu-Daudé wrote:
>>>>> On 2/9/25 17:43, Philippe Mathieu-Daudé wrote:
>>>>>> On 2/9/25 17:34, Jan Kiszka wrote:
>>>>>>> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
>>>>>>>> On 1/9/25 07:56, Jan Kiszka wrote:
>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>
>>>>>>>>> The power-of-2 rule applies to the user data area, not the complete
>>>>>>>>> block image. The latter can be concatenation of boot partition images
>>>>>>>>> and the user data.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>> ---
>>>>>>>>>    hw/sd/sd.c | 2 +-
>>>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>>>>>>> index 8c290595f0..16aee210b4 100644
>>>>>>>>> --- a/hw/sd/sd.c
>>>>>>>>> +++ b/hw/sd/sd.c
>>>>>>>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev, Error
>>>>>>>>> **errp)
>>>>>>>>>                return;
>>>>>>>>>            }
>>>>>>>>>    -        blk_size = blk_getlength(sd->blk);
>>>>>>>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size * 2;
>>>>>>>>>            if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>>>>>>                int64_t blk_size_aligned = pow2ceil(blk_size);
>>>>>>>>>                char *blk_size_str;
>>>>>>>>
>>>>>>>> This seems to break the tests/functional/arm/test_aspeed_rainier.py
>>>>>>>> test due to mmc-p10bmc-20240617.qcow2 size:
>>>>>>>>
>>>>>>>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -display none -
>>>>>>>> vga none -chardev socket,id=mon,fd=5 -mon chardev=mon,mode=control -
>>>>>>>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
>>>>>>>> chardev:console -drive file=/builds/qemu-project/qemu/functional- cache/
>>>>>>>> download/
>>>>>>>> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
>>>>>>>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
>>>>>>>> SD card size has to be a power of 2, e.g. 16 GiB.
>>>>>>>>
>>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
>>>>>>>>
>>>>>>>
>>>>>>> Hmm, then the test was always wrong as well. I suspect the aspeed is
>>>>>>> enabling boot partitions by default, and the image was created to pass
>>>>>>> the wrong alignment check. Where / by whom is the image maintained?
>>>>>>
>>>>>> Cédric Le Goater (Cc'ed).
>>>>>>
>>>>>> The test comes from:
>>>>>> https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb- a85f-09964268533d@kaod.org/
>>>>>>
>>>>>> Maybe also relevant to your suspicion:
>>>>>> https://lore.kernel.org/qemu-devel/e401d119-402e-0edd- c2bf-28950ba48ccb@kaod.org/
> 
> [*]
> 
>>>>>
>>>>> Digging further:
>>>>> https://lore.kernel.org/qemu- devel/9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/
>>>>>
>>>>
>>>> yes commit c078298301a8 might have some impact there.
>>>
>>> With Jan patch, your script doesn't need anymore the
>>>
>>>    echo "Fixing size to keep qemu happy..."
>>>
>>> kludge.
>>
>> which script ?
> 
> The one you pasted in [*]:

ah. That's Joel's from years ago. The witherspoon-tacoma is a Frankenstein
lab board for BMC bringup. I think all the needed info is in the commit log.

Thanks,

C.




> 
> -- 
> #!/bin/sh
> 
> URLBASE=https://jenkins.openbmc.org/view/latest/job/latest-master/label=docker-builder,target=witherspoon-tacoma/lastSuccessfulBuild/artifact/openbmc/build/tmp/deploy/images/witherspoon-tacoma/
> 
> IMAGESIZE=128
> OUTFILE=mmc.img
> 
> FILES="u-boot.bin u-boot-spl.bin obmc-phosphor-image-witherspoon-tacoma.wic.xz"
> 
> for file in ${FILES}; do
> 
>      if test -f ${file}; then
>          echo "${file}: Already downloaded"
>      else
>          echo "${file}: Downloading"
>          wget -nv ${URLBASE}/${file}
>      fi
> done
> 
> echo
> 
> echo "Creating empty image..."
> dd status=none if=/dev/zero of=${OUTFILE} bs=1M count=${IMAGESIZE}
> echo "Adding SPL..."
> dd status=none if=u-boot-spl.bin of=${OUTFILE} conv=notrunc
> echo "Adding u-boot..."
> dd status=none if=u-boot.bin of=${OUTFILE} conv=notrunc bs=1K seek=64
> echo "Adding userdata..."
> unxz -c obmc-phosphor-image-witherspoon-tacoma.wic.xz | dd status=progress of=${OUTFILE} conv=notrunc bs=1M seek=2
> echo "Fixing size to keep qemu happy..."
> truncate --size 16G ${OUTFILE}
> 
> echo "Done!"
> echo
> echo " qemu-system-arm -M tacoma-bmc -nographic -drive file=mmc.img,if=sd,index=2,format=raw"
> ---

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Philippe Mathieu-Daudé]

On 2/9/25 18:14, Philippe Mathieu-Daudé wrote:
> On 2/9/25 18:00, Cédric Le Goater wrote:
>> On 9/2/25 17:55, Philippe Mathieu-Daudé wrote:
>>> On 2/9/25 17:47, Cédric Le Goater wrote:
>>>> On 9/2/25 17:45, Philippe Mathieu-Daudé wrote:
>>>>> On 2/9/25 17:43, Philippe Mathieu-Daudé wrote:
>>>>>> On 2/9/25 17:34, Jan Kiszka wrote:
>>>>>>> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
>>>>>>>> On 1/9/25 07:56, Jan Kiszka wrote:
>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>
>>>>>>>>> The power-of-2 rule applies to the user data area, not the 
>>>>>>>>> complete
>>>>>>>>> block image. The latter can be concatenation of boot partition 
>>>>>>>>> images
>>>>>>>>> and the user data.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>> ---
>>>>>>>>>    hw/sd/sd.c | 2 +-
>>>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>>>>>>> index 8c290595f0..16aee210b4 100644
>>>>>>>>> --- a/hw/sd/sd.c
>>>>>>>>> +++ b/hw/sd/sd.c
>>>>>>>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev, 
>>>>>>>>> Error
>>>>>>>>> **errp)
>>>>>>>>>                return;
>>>>>>>>>            }
>>>>>>>>>    -        blk_size = blk_getlength(sd->blk);
>>>>>>>>> +        blk_size = blk_getlength(sd->blk) - sd->boot_part_size 
>>>>>>>>> * 2;
>>>>>>>>>            if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>>>>>>                int64_t blk_size_aligned = pow2ceil(blk_size);
>>>>>>>>>                char *blk_size_str;
>>>>>>>>
>>>>>>>> This seems to break the tests/functional/arm/test_aspeed_rainier.py
>>>>>>>> test due to mmc-p10bmc-20240617.qcow2 size:
>>>>>>>>
>>>>>>>> Command: /builds/qemu-project/qemu/build/qemu-system-arm - 
>>>>>>>> display none -
>>>>>>>> vga none -chardev socket,id=mon,fd=5 -mon 
>>>>>>>> chardev=mon,mode=control -
>>>>>>>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
>>>>>>>> chardev:console -drive file=/builds/qemu-project/qemu/ 
>>>>>>>> functional- cache/
>>>>>>>> download/
>>>>>>>> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
>>>>>>>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
>>>>>>>> SD card size has to be a power of 2, e.g. 16 GiB.
>>>>>>>>
>>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
>>>>>>>>
>>>>>>>
>>>>>>> Hmm, then the test was always wrong as well. I suspect the aspeed is
>>>>>>> enabling boot partitions by default, and the image was created to 
>>>>>>> pass
>>>>>>> the wrong alignment check. Where / by whom is the image maintained?
>>>>>>
>>>>>> Cédric Le Goater (Cc'ed).
>>>>>>
>>>>>> The test comes from:
>>>>>> https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb- 
>>>>>> a85f-09964268533d@kaod.org/
>>>>>>
>>>>>> Maybe also relevant to your suspicion:
>>>>>> https://lore.kernel.org/qemu-devel/e401d119-402e-0edd- 
>>>>>> c2bf-28950ba48ccb@kaod.org/
> 
> [*]
> 
>>>>>
>>>>> Digging further:
>>>>> https://lore.kernel.org/qemu- 
>>>>> devel/9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/
>>>>>
>>>>
>>>> yes commit c078298301a8 might have some impact there.
>>>
>>> With Jan patch, your script doesn't need anymore the
>>>
>>>    echo "Fixing size to keep qemu happy..."
>>>
>>> kludge.
>>
>> which script ?
> 
> The one you pasted in [*]:
> 
> -- 
> #!/bin/sh
> 
> URLBASE=https://jenkins.openbmc.org/view/latest/job/latest-master/ 
> label=docker-builder,target=witherspoon-tacoma/lastSuccessfulBuild/ 
> artifact/openbmc/build/tmp/deploy/images/witherspoon-tacoma/
> 
> IMAGESIZE=128
> OUTFILE=mmc.img
> 
> FILES="u-boot.bin u-boot-spl.bin obmc-phosphor-image-witherspoon- 
> tacoma.wic.xz"
> 
> for file in ${FILES}; do
> 
>      if test -f ${file}; then
>          echo "${file}: Already downloaded"
>      else
>          echo "${file}: Downloading"
>          wget -nv ${URLBASE}/${file}
>      fi
> done
> 
> echo
> 
> echo "Creating empty image..."
> dd status=none if=/dev/zero of=${OUTFILE} bs=1M count=${IMAGESIZE}
> echo "Adding SPL..."
> dd status=none if=u-boot-spl.bin of=${OUTFILE} conv=notrunc
> echo "Adding u-boot..."
> dd status=none if=u-boot.bin of=${OUTFILE} conv=notrunc bs=1K seek=64
> echo "Adding userdata..."
> unxz -c obmc-phosphor-image-witherspoon-tacoma.wic.xz | dd 
> status=progress of=${OUTFILE} conv=notrunc bs=1M seek=2
> echo "Fixing size to keep qemu happy..."
> truncate --size 16G ${OUTFILE}
> 
> echo "Done!"
> echo
> echo " qemu-system-arm -M tacoma-bmc -nographic -drive 
> file=mmc.img,if=sd,index=2,format=raw"
> ---

FTR the alignment check was added to shut up fuzzed CVEs in commit
a9bcedd15a5 ("hw/sd/sdcard: Do not allow invalid SD card sizes"):

     QEMU allows to create SD card with unrealistic sizes. This could
     work, but some guests (at least Linux) consider sizes that are not
     a power of 2 as a firmware bug and fix the card size to the next
     power of 2.

     While the possibility to use small SD card images has been seen as
     a feature, it became a bug with CVE-2020-13253, where the guest is
     able to do OOB read/write accesses past the image size end.

     In a pair of commits we will fix CVE-2020-13253 as:

         Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
         occurred and no data transfer is performed.

         Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
         occurred and no data transfer is performed.

         WP_VIOLATION errors are not modified: the error bit is set, we
         stay in receive-data state, wait for a stop command. All further
         data transfer is ignored. See the check on sd->card_status at
         the beginning of sd_read_data() and sd_write_data().

     While this is the correct behavior, in case QEMU create smaller SD
     cards, guests still try to access past the image size end, and QEMU
     considers this is an invalid address, thus "all further data
     transfer is ignored". This is wrong and make the guest looping until
     eventually timeouts.

     Fix by not allowing invalid SD card sizes (suggesting the expected
     size as a hint):

       $ qemu-system-arm -M orangepi-pc -drive 
file=rootfs.ext2,if=sd,format=raw
       qemu-system-arm: Invalid SD card size: 60 MiB
       SD card size has to be a power of 2, e.g. 64 MiB.
       You can resize disk images with 'qemu-img resize <imagefile> 
<new-size>'
       (note that this will lose data if you make the image smaller than 
it currently is).


I expect us to be safe and able to deal with non-pow2 regions if we use
QEMUSGList from the "system/dma.h" API. But this is a rework nobody had
time to do so far.

Regards,

Phil.

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Jan Kiszka]

On 02.09.25 18:20, Philippe Mathieu-Daudé wrote:
> On 2/9/25 18:14, Philippe Mathieu-Daudé wrote:
>> On 2/9/25 18:00, Cédric Le Goater wrote:
>>> On 9/2/25 17:55, Philippe Mathieu-Daudé wrote:
>>>> On 2/9/25 17:47, Cédric Le Goater wrote:
>>>>> On 9/2/25 17:45, Philippe Mathieu-Daudé wrote:
>>>>>> On 2/9/25 17:43, Philippe Mathieu-Daudé wrote:
>>>>>>> On 2/9/25 17:34, Jan Kiszka wrote:
>>>>>>>> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
>>>>>>>>> On 1/9/25 07:56, Jan Kiszka wrote:
>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>>
>>>>>>>>>> The power-of-2 rule applies to the user data area, not the
>>>>>>>>>> complete
>>>>>>>>>> block image. The latter can be concatenation of boot partition
>>>>>>>>>> images
>>>>>>>>>> and the user data.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>>> ---
>>>>>>>>>>    hw/sd/sd.c | 2 +-
>>>>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>>>>>>>> index 8c290595f0..16aee210b4 100644
>>>>>>>>>> --- a/hw/sd/sd.c
>>>>>>>>>> +++ b/hw/sd/sd.c
>>>>>>>>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev,
>>>>>>>>>> Error
>>>>>>>>>> **errp)
>>>>>>>>>>                return;
>>>>>>>>>>            }
>>>>>>>>>>    -        blk_size = blk_getlength(sd->blk);
>>>>>>>>>> +        blk_size = blk_getlength(sd->blk) - sd-
>>>>>>>>>> >boot_part_size * 2;
>>>>>>>>>>            if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>>>>>>>                int64_t blk_size_aligned = pow2ceil(blk_size);
>>>>>>>>>>                char *blk_size_str;
>>>>>>>>>
>>>>>>>>> This seems to break the tests/functional/arm/
>>>>>>>>> test_aspeed_rainier.py
>>>>>>>>> test due to mmc-p10bmc-20240617.qcow2 size:
>>>>>>>>>
>>>>>>>>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -
>>>>>>>>> display none -
>>>>>>>>> vga none -chardev socket,id=mon,fd=5 -mon
>>>>>>>>> chardev=mon,mode=control -
>>>>>>>>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
>>>>>>>>> chardev:console -drive file=/builds/qemu-project/qemu/
>>>>>>>>> functional- cache/
>>>>>>>>> download/
>>>>>>>>> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
>>>>>>>>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
>>>>>>>>> SD card size has to be a power of 2, e.g. 16 GiB.
>>>>>>>>>
>>>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hmm, then the test was always wrong as well. I suspect the
>>>>>>>> aspeed is
>>>>>>>> enabling boot partitions by default, and the image was created
>>>>>>>> to pass
>>>>>>>> the wrong alignment check. Where / by whom is the image maintained?
>>>>>>>
>>>>>>> Cédric Le Goater (Cc'ed).
>>>>>>>
>>>>>>> The test comes from:
>>>>>>> https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb-
>>>>>>> a85f-09964268533d@kaod.org/
>>>>>>>
>>>>>>> Maybe also relevant to your suspicion:
>>>>>>> https://lore.kernel.org/qemu-devel/e401d119-402e-0edd-
>>>>>>> c2bf-28950ba48ccb@kaod.org/
>>
>> [*]
>>
>>>>>>
>>>>>> Digging further:
>>>>>> https://lore.kernel.org/qemu-
>>>>>> devel/9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/
>>>>>>
>>>>>
>>>>> yes commit c078298301a8 might have some impact there.
>>>>
>>>> With Jan patch, your script doesn't need anymore the
>>>>
>>>>    echo "Fixing size to keep qemu happy..."
>>>>
>>>> kludge.
>>>
>>> which script ?
>>
>> The one you pasted in [*]:
>>
>> -- 
>> #!/bin/sh
>>
>> URLBASE=https://jenkins.openbmc.org/view/latest/job/latest-master/
>> label=docker-builder,target=witherspoon-tacoma/lastSuccessfulBuild/
>> artifact/openbmc/build/tmp/deploy/images/witherspoon-tacoma/
>>
>> IMAGESIZE=128
>> OUTFILE=mmc.img
>>
>> FILES="u-boot.bin u-boot-spl.bin obmc-phosphor-image-witherspoon-
>> tacoma.wic.xz"
>>
>> for file in ${FILES}; do
>>
>>      if test -f ${file}; then
>>          echo "${file}: Already downloaded"
>>      else
>>          echo "${file}: Downloading"
>>          wget -nv ${URLBASE}/${file}
>>      fi
>> done
>>
>> echo
>>
>> echo "Creating empty image..."
>> dd status=none if=/dev/zero of=${OUTFILE} bs=1M count=${IMAGESIZE}
>> echo "Adding SPL..."
>> dd status=none if=u-boot-spl.bin of=${OUTFILE} conv=notrunc
>> echo "Adding u-boot..."
>> dd status=none if=u-boot.bin of=${OUTFILE} conv=notrunc bs=1K seek=64
>> echo "Adding userdata..."
>> unxz -c obmc-phosphor-image-witherspoon-tacoma.wic.xz | dd
>> status=progress of=${OUTFILE} conv=notrunc bs=1M seek=2
>> echo "Fixing size to keep qemu happy..."
>> truncate --size 16G ${OUTFILE}
>>
>> echo "Done!"
>> echo
>> echo " qemu-system-arm -M tacoma-bmc -nographic -drive
>> file=mmc.img,if=sd,index=2,format=raw"
>> ---
> 
> FTR the alignment check was added to shut up fuzzed CVEs in commit
> a9bcedd15a5 ("hw/sd/sdcard: Do not allow invalid SD card sizes"):
> 
>     QEMU allows to create SD card with unrealistic sizes. This could
>     work, but some guests (at least Linux) consider sizes that are not
>     a power of 2 as a firmware bug and fix the card size to the next
>     power of 2.
> 
>     While the possibility to use small SD card images has been seen as
>     a feature, it became a bug with CVE-2020-13253, where the guest is
>     able to do OOB read/write accesses past the image size end.
> 
>     In a pair of commits we will fix CVE-2020-13253 as:
> 
>         Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>         occurred and no data transfer is performed.
> 
>         Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>         occurred and no data transfer is performed.
> 
>         WP_VIOLATION errors are not modified: the error bit is set, we
>         stay in receive-data state, wait for a stop command. All further
>         data transfer is ignored. See the check on sd->card_status at
>         the beginning of sd_read_data() and sd_write_data().
> 
>     While this is the correct behavior, in case QEMU create smaller SD
>     cards, guests still try to access past the image size end, and QEMU
>     considers this is an invalid address, thus "all further data
>     transfer is ignored". This is wrong and make the guest looping until
>     eventually timeouts.
> 
>     Fix by not allowing invalid SD card sizes (suggesting the expected
>     size as a hint):
> 
>       $ qemu-system-arm -M orangepi-pc -drive
> file=rootfs.ext2,if=sd,format=raw
>       qemu-system-arm: Invalid SD card size: 60 MiB
>       SD card size has to be a power of 2, e.g. 64 MiB.
>       You can resize disk images with 'qemu-img resize <imagefile> <new-
> size>'
>       (note that this will lose data if you make the image smaller than
> it currently is).
> 
> 
> I expect us to be safe and able to deal with non-pow2 regions if we use
> QEMUSGList from the "system/dma.h" API. But this is a rework nobody had
> time to do so far.

We have to tell two things apart: partitions sizes on the one side and
backing storage sizes. The partitions sizes are (to my reading) clearly
defined in the spec, and the user partition (alone!) has to be power of
2. The boot and RPMB partitions are multiples of 128K. The sum of them
all is nowhere limited to power of 2 or even only multiples of 128K.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Jan Kiszka]

On 02.09.25 18:24, Jan Kiszka wrote:
> On 02.09.25 18:20, Philippe Mathieu-Daudé wrote:
>> On 2/9/25 18:14, Philippe Mathieu-Daudé wrote:
>>> On 2/9/25 18:00, Cédric Le Goater wrote:
>>>> On 9/2/25 17:55, Philippe Mathieu-Daudé wrote:
>>>>> On 2/9/25 17:47, Cédric Le Goater wrote:
>>>>>> On 9/2/25 17:45, Philippe Mathieu-Daudé wrote:
>>>>>>> On 2/9/25 17:43, Philippe Mathieu-Daudé wrote:
>>>>>>>> On 2/9/25 17:34, Jan Kiszka wrote:
>>>>>>>>> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
>>>>>>>>>> On 1/9/25 07:56, Jan Kiszka wrote:
>>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>>>
>>>>>>>>>>> The power-of-2 rule applies to the user data area, not the
>>>>>>>>>>> complete
>>>>>>>>>>> block image. The latter can be concatenation of boot partition
>>>>>>>>>>> images
>>>>>>>>>>> and the user data.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>>>> ---
>>>>>>>>>>>    hw/sd/sd.c | 2 +-
>>>>>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>>>>>>>>>> index 8c290595f0..16aee210b4 100644
>>>>>>>>>>> --- a/hw/sd/sd.c
>>>>>>>>>>> +++ b/hw/sd/sd.c
>>>>>>>>>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev,
>>>>>>>>>>> Error
>>>>>>>>>>> **errp)
>>>>>>>>>>>                return;
>>>>>>>>>>>            }
>>>>>>>>>>>    -        blk_size = blk_getlength(sd->blk);
>>>>>>>>>>> +        blk_size = blk_getlength(sd->blk) - sd-
>>>>>>>>>>>> boot_part_size * 2;
>>>>>>>>>>>            if (blk_size > 0 && !is_power_of_2(blk_size)) {
>>>>>>>>>>>                int64_t blk_size_aligned = pow2ceil(blk_size);
>>>>>>>>>>>                char *blk_size_str;
>>>>>>>>>>
>>>>>>>>>> This seems to break the tests/functional/arm/
>>>>>>>>>> test_aspeed_rainier.py
>>>>>>>>>> test due to mmc-p10bmc-20240617.qcow2 size:
>>>>>>>>>>
>>>>>>>>>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -
>>>>>>>>>> display none -
>>>>>>>>>> vga none -chardev socket,id=mon,fd=5 -mon
>>>>>>>>>> chardev=mon,mode=control -
>>>>>>>>>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
>>>>>>>>>> chardev:console -drive file=/builds/qemu-project/qemu/
>>>>>>>>>> functional- cache/
>>>>>>>>>> download/
>>>>>>>>>> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
>>>>>>>>>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
>>>>>>>>>> SD card size has to be a power of 2, e.g. 16 GiB.
>>>>>>>>>>
>>>>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hmm, then the test was always wrong as well. I suspect the
>>>>>>>>> aspeed is
>>>>>>>>> enabling boot partitions by default, and the image was created
>>>>>>>>> to pass
>>>>>>>>> the wrong alignment check. Where / by whom is the image maintained?
>>>>>>>>
>>>>>>>> Cédric Le Goater (Cc'ed).
>>>>>>>>
>>>>>>>> The test comes from:
>>>>>>>> https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb-
>>>>>>>> a85f-09964268533d@kaod.org/
>>>>>>>>
>>>>>>>> Maybe also relevant to your suspicion:
>>>>>>>> https://lore.kernel.org/qemu-devel/e401d119-402e-0edd-
>>>>>>>> c2bf-28950ba48ccb@kaod.org/
>>>
>>> [*]
>>>
>>>>>>>
>>>>>>> Digging further:
>>>>>>> https://lore.kernel.org/qemu-
>>>>>>> devel/9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/
>>>>>>>
>>>>>>
>>>>>> yes commit c078298301a8 might have some impact there.
>>>>>
>>>>> With Jan patch, your script doesn't need anymore the
>>>>>
>>>>>    echo "Fixing size to keep qemu happy..."
>>>>>
>>>>> kludge.
>>>>
>>>> which script ?
>>>
>>> The one you pasted in [*]:
>>>
>>> -- 
>>> #!/bin/sh
>>>
>>> URLBASE=https://jenkins.openbmc.org/view/latest/job/latest-master/
>>> label=docker-builder,target=witherspoon-tacoma/lastSuccessfulBuild/
>>> artifact/openbmc/build/tmp/deploy/images/witherspoon-tacoma/
>>>
>>> IMAGESIZE=128
>>> OUTFILE=mmc.img
>>>
>>> FILES="u-boot.bin u-boot-spl.bin obmc-phosphor-image-witherspoon-
>>> tacoma.wic.xz"
>>>
>>> for file in ${FILES}; do
>>>
>>>      if test -f ${file}; then
>>>          echo "${file}: Already downloaded"
>>>      else
>>>          echo "${file}: Downloading"
>>>          wget -nv ${URLBASE}/${file}
>>>      fi
>>> done
>>>
>>> echo
>>>
>>> echo "Creating empty image..."
>>> dd status=none if=/dev/zero of=${OUTFILE} bs=1M count=${IMAGESIZE}
>>> echo "Adding SPL..."
>>> dd status=none if=u-boot-spl.bin of=${OUTFILE} conv=notrunc
>>> echo "Adding u-boot..."
>>> dd status=none if=u-boot.bin of=${OUTFILE} conv=notrunc bs=1K seek=64
>>> echo "Adding userdata..."
>>> unxz -c obmc-phosphor-image-witherspoon-tacoma.wic.xz | dd
>>> status=progress of=${OUTFILE} conv=notrunc bs=1M seek=2
>>> echo "Fixing size to keep qemu happy..."
>>> truncate --size 16G ${OUTFILE}
>>>
>>> echo "Done!"
>>> echo
>>> echo " qemu-system-arm -M tacoma-bmc -nographic -drive
>>> file=mmc.img,if=sd,index=2,format=raw"
>>> ---
>>
>> FTR the alignment check was added to shut up fuzzed CVEs in commit
>> a9bcedd15a5 ("hw/sd/sdcard: Do not allow invalid SD card sizes"):
>>
>>     QEMU allows to create SD card with unrealistic sizes. This could
>>     work, but some guests (at least Linux) consider sizes that are not
>>     a power of 2 as a firmware bug and fix the card size to the next
>>     power of 2.
>>
>>     While the possibility to use small SD card images has been seen as
>>     a feature, it became a bug with CVE-2020-13253, where the guest is
>>     able to do OOB read/write accesses past the image size end.
>>
>>     In a pair of commits we will fix CVE-2020-13253 as:
>>
>>         Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>>         occurred and no data transfer is performed.
>>
>>         Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>>         occurred and no data transfer is performed.
>>
>>         WP_VIOLATION errors are not modified: the error bit is set, we
>>         stay in receive-data state, wait for a stop command. All further
>>         data transfer is ignored. See the check on sd->card_status at
>>         the beginning of sd_read_data() and sd_write_data().
>>
>>     While this is the correct behavior, in case QEMU create smaller SD
>>     cards, guests still try to access past the image size end, and QEMU
>>     considers this is an invalid address, thus "all further data
>>     transfer is ignored". This is wrong and make the guest looping until
>>     eventually timeouts.
>>
>>     Fix by not allowing invalid SD card sizes (suggesting the expected
>>     size as a hint):
>>
>>       $ qemu-system-arm -M orangepi-pc -drive
>> file=rootfs.ext2,if=sd,format=raw
>>       qemu-system-arm: Invalid SD card size: 60 MiB
>>       SD card size has to be a power of 2, e.g. 64 MiB.
>>       You can resize disk images with 'qemu-img resize <imagefile> <new-
>> size>'
>>       (note that this will lose data if you make the image smaller than
>> it currently is).
>>
>>
>> I expect us to be safe and able to deal with non-pow2 regions if we use
>> QEMUSGList from the "system/dma.h" API. But this is a rework nobody had
>> time to do so far.
> 
> We have to tell two things apart: partitions sizes on the one side and
> backing storage sizes. The partitions sizes are (to my reading) clearly
> defined in the spec, and the user partition (alone!) has to be power of
> 2. The boot and RPMB partitions are multiples of 128K. The sum of them
> all is nowhere limited to power of 2 or even only multiples of 128K.
> 

Re-reading the part of the device capacity, the rules are more complex:
 - power of two up to 2 GB
 - multiple of 512 bytes beyond that

So that power-of-two enforcement was and still is likely too strict.

But I still see no indication, neither in the existing eMMC code of QEMU
nor the spec, that the boot and RPMB partition sizes are included in that.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Jan Lübbe]

On Tue, 2025-09-02 at 18:39 +0200, Jan Kiszka wrote:
> > > I expect us to be safe and able to deal with non-pow2 regions if we use
> > > QEMUSGList from the "system/dma.h" API. But this is a rework nobody had
> > > time to do so far.
> > 
> > We have to tell two things apart: partitions sizes on the one side and
> > backing storage sizes. The partitions sizes are (to my reading) clearly
> > defined in the spec, and the user partition (alone!) has to be power of
> > 2. The boot and RPMB partitions are multiples of 128K. The sum of them
> > all is nowhere limited to power of 2 or even only multiples of 128K.
> > 
> 
> Re-reading the part of the device capacity, the rules are more complex:
>  - power of two up to 2 GB
>  - multiple of 512 bytes beyond that
> 
> So that power-of-two enforcement was and still is likely too strict.
>
> But I still see no indication, neither in the existing eMMC code of QEMU
> nor the spec, that the boot and RPMB partition sizes are included in that.

Correct. Non-power-of-two sizes are very common for real eMMCs. Taking a random
one from our lab:
[    1.220588] mmcblk1: mmc1:0001 S0J56X 14.8 GiB
[    1.228055]  mmcblk1: p1 p2 p3 p4
[    1.230375] mmcblk1boot0: mmc1:0001 S0J56X 31.5 MiB
[    1.233651] mmcblk1boot1: mmc1:0001 S0J56X 31.5 MiB
[    1.236682] mmcblk1rpmb: mmc1:0001 S0J56X 4.00 MiB, chardev (244:0)

For eMMCs using MLC NAND, you can also configure part of the user data area to
be pSLC (pseudo single level cell), which changes the available capacity (after
a required power cycle).

Regards,
Jan


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Jan Kiszka]

On 02.09.25 18:47, Jan Lübbe wrote:
> On Tue, 2025-09-02 at 18:39 +0200, Jan Kiszka wrote:
>>>> I expect us to be safe and able to deal with non-pow2 regions if we use
>>>> QEMUSGList from the "system/dma.h" API. But this is a rework nobody had
>>>> time to do so far.
>>>
>>> We have to tell two things apart: partitions sizes on the one side and
>>> backing storage sizes. The partitions sizes are (to my reading) clearly
>>> defined in the spec, and the user partition (alone!) has to be power of
>>> 2. The boot and RPMB partitions are multiples of 128K. The sum of them
>>> all is nowhere limited to power of 2 or even only multiples of 128K.
>>>
>>
>> Re-reading the part of the device capacity, the rules are more complex:
>>  - power of two up to 2 GB
>>  - multiple of 512 bytes beyond that
>>
>> So that power-of-two enforcement was and still is likely too strict.
>>
>> But I still see no indication, neither in the existing eMMC code of QEMU
>> nor the spec, that the boot and RPMB partition sizes are included in that.
> 
> Correct. Non-power-of-two sizes are very common for real eMMCs. Taking a random
> one from our lab:
> [    1.220588] mmcblk1: mmc1:0001 S0J56X 14.8 GiB
> [    1.228055]  mmcblk1: p1 p2 p3 p4
> [    1.230375] mmcblk1boot0: mmc1:0001 S0J56X 31.5 MiB
> [    1.233651] mmcblk1boot1: mmc1:0001 S0J56X 31.5 MiB
> [    1.236682] mmcblk1rpmb: mmc1:0001 S0J56X 4.00 MiB, chardev (244:0)
> 
> For eMMCs using MLC NAND, you can also configure part of the user data area to
> be pSLC (pseudo single level cell), which changes the available capacity (after
> a required power cycle).
> 

Then let's fix the entry check - the code setting up registers to report
sizes was reflecting that already.

Patch on top, right? Staging is not rebased for updated patches, is it?

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 2730 bytes --]

On Tue, Sep 2, 2025 at 10:49 AM Jan Lübbe <jlu@pengutronix.de> wrote:

> On Tue, 2025-09-02 at 18:39 +0200, Jan Kiszka wrote:
> > > > I expect us to be safe and able to deal with non-pow2 regions if we
> use
> > > > QEMUSGList from the "system/dma.h" API. But this is a rework nobody
> had
> > > > time to do so far.
> > >
> > > We have to tell two things apart: partitions sizes on the one side and
> > > backing storage sizes. The partitions sizes are (to my reading) clearly
> > > defined in the spec, and the user partition (alone!) has to be power of
> > > 2. The boot and RPMB partitions are multiples of 128K. The sum of them
> > > all is nowhere limited to power of 2 or even only multiples of 128K.
> > >
> >
> > Re-reading the part of the device capacity, the rules are more complex:
> >  - power of two up to 2 GB
> >  - multiple of 512 bytes beyond that
> >
> > So that power-of-two enforcement was and still is likely too strict.
>

It is. Version 0 (and MMC) cards had the capacity encoded like so:
                m = mmc_get_bits(raw_csd, 128, 62, 12);
                e = mmc_get_bits(raw_csd, 128, 47, 3);
                csd->capacity = ((1 + m) << (e + 2)) * csd->read_bl_len;
so any card less than 2GB (well, technically 4GB, but 4GB version 0 cards
were
rare and broke some stacks... I have one and I love it on my embedded ARM
board
that can't do version 1 cards). Version 1 cards encoded it like:
                csd->capacity = ((uint64_t)mmc_get_bits(raw_csd, 128, 48,
22) +
                    1) * 512 * 1024;
So it's a multiple of 512k. These are also called 'high capacity' cards.

Version 4 introduces an extended CSD, which had a pure sector count in the
EXT CSD. I think this
is only for MMC cards. And also the partition information.


> > But I still see no indication, neither in the existing eMMC code of QEMU
> > nor the spec, that the boot and RPMB partition sizes are included in
> that.
>
> Correct. Non-power-of-two sizes are very common for real eMMCs. Taking a
> random
> one from our lab:
> [    1.220588] mmcblk1: mmc1:0001 S0J56X 14.8 GiB
> [    1.228055]  mmcblk1: p1 p2 p3 p4
> [    1.230375] mmcblk1boot0: mmc1:0001 S0J56X 31.5 MiB
> [    1.233651] mmcblk1boot1: mmc1:0001 S0J56X 31.5 MiB
> [    1.236682] mmcblk1rpmb: mmc1:0001 S0J56X 4.00 MiB, chardev (244:0)
>
> For eMMCs using MLC NAND, you can also configure part of the user data
> area to
> be pSLC (pseudo single level cell), which changes the available capacity
> (after
> a required power cycle).
>

Yes. Extended partitions are a feature of version 4 cards, so don't have
power-of-2 limits since they are a pure sector count in the ext_csd.

Warner

[-- Attachment #2: Type: text/html, Size: 3645 bytes --]

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Jan Kiszka]

On 02.09.25 19:07, Warner Losh wrote:
> 
> 
> On Tue, Sep 2, 2025 at 10:49 AM Jan Lübbe <jlu@pengutronix.de
> <mailto:jlu@pengutronix.de>> wrote:
> 
>     On Tue, 2025-09-02 at 18:39 +0200, Jan Kiszka wrote:
>     > > > I expect us to be safe and able to deal with non-pow2 regions
>     if we use
>     > > > QEMUSGList from the "system/dma.h" API. But this is a rework
>     nobody had
>     > > > time to do so far.
>     > >
>     > > We have to tell two things apart: partitions sizes on the one
>     side and
>     > > backing storage sizes. The partitions sizes are (to my reading)
>     clearly
>     > > defined in the spec, and the user partition (alone!) has to be
>     power of
>     > > 2. The boot and RPMB partitions are multiples of 128K. The sum
>     of them
>     > > all is nowhere limited to power of 2 or even only multiples of 128K.
>     > >
>     >
>     > Re-reading the part of the device capacity, the rules are more
>     complex:
>     >  - power of two up to 2 GB
>     >  - multiple of 512 bytes beyond that
>     >
>     > So that power-of-two enforcement was and still is likely too strict.
> 
> 
> It is. Version 0 (and MMC) cards had the capacity encoded like so:
>                 m = mmc_get_bits(raw_csd, 128, 62, 12);
>                 e = mmc_get_bits(raw_csd, 128, 47, 3);
>                 csd->capacity = ((1 + m) << (e + 2)) * csd->read_bl_len;
> so any card less than 2GB (well, technically 4GB, but 4GB version 0
> cards were
> rare and broke some stacks... I have one and I love it on my embedded
> ARM board
> that can't do version 1 cards). Version 1 cards encoded it like:
>                 csd->capacity = ((uint64_t)mmc_get_bits(raw_csd, 128,
> 48, 22) +
>                     1) * 512 * 1024;
> So it's a multiple of 512k. These are also called 'high capacity' cards.
> 
> Version 4 introduces an extended CSD, which had a pure sector count in
> the EXT CSD. I think this
> is only for MMC cards. And also the partition information.
>  
> 
>     > But I still see no indication, neither in the existing eMMC code
>     of QEMU
>     > nor the spec, that the boot and RPMB partition sizes are included
>     in that.
> 
>     Correct. Non-power-of-two sizes are very common for real eMMCs.
>     Taking a random
>     one from our lab:
>     [    1.220588] mmcblk1: mmc1:0001 S0J56X 14.8 GiB
>     [    1.228055]  mmcblk1: p1 p2 p3 p4
>     [    1.230375] mmcblk1boot0: mmc1:0001 S0J56X 31.5 MiB
>     [    1.233651] mmcblk1boot1: mmc1:0001 S0J56X 31.5 MiB
>     [    1.236682] mmcblk1rpmb: mmc1:0001 S0J56X 4.00 MiB, chardev (244:0)
> 
>     For eMMCs using MLC NAND, you can also configure part of the user
>     data area to
>     be pSLC (pseudo single level cell), which changes the available
>     capacity (after
>     a required power cycle).
> 
> 
> Yes. Extended partitions are a feature of version 4 cards, so don't have
> power-of-2 limits since they are a pure sector count in the ext_csd.
> 

JESD84-B51A (eMMC 5.1A):

"The C_SIZE parameter is used to compute the device capacity for devices
up to 2 GB of density. See 7.4.52, SEC_COUNT [215:212] , for details on
calculating densities greater than 2 GB."

So I would now continue to enforce power-of-2 for 2G (including) cards,
and relax to multiples of 512 for larger ones.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 9975 bytes --]

On Tue, Sep 2, 2025 at 10:40 AM Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 02.09.25 18:24, Jan Kiszka wrote:
> > On 02.09.25 18:20, Philippe Mathieu-Daudé wrote:
> >> On 2/9/25 18:14, Philippe Mathieu-Daudé wrote:
> >>> On 2/9/25 18:00, Cédric Le Goater wrote:
> >>>> On 9/2/25 17:55, Philippe Mathieu-Daudé wrote:
> >>>>> On 2/9/25 17:47, Cédric Le Goater wrote:
> >>>>>> On 9/2/25 17:45, Philippe Mathieu-Daudé wrote:
> >>>>>>> On 2/9/25 17:43, Philippe Mathieu-Daudé wrote:
> >>>>>>>> On 2/9/25 17:34, Jan Kiszka wrote:
> >>>>>>>>> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
> >>>>>>>>>> On 1/9/25 07:56, Jan Kiszka wrote:
> >>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>>>>>>
> >>>>>>>>>>> The power-of-2 rule applies to the user data area, not the
> >>>>>>>>>>> complete
> >>>>>>>>>>> block image. The latter can be concatenation of boot partition
> >>>>>>>>>>> images
> >>>>>>>>>>> and the user data.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>>>>>>>>>> ---
> >>>>>>>>>>>    hw/sd/sd.c | 2 +-
> >>>>>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> >>>>>>>>>>> index 8c290595f0..16aee210b4 100644
> >>>>>>>>>>> --- a/hw/sd/sd.c
> >>>>>>>>>>> +++ b/hw/sd/sd.c
> >>>>>>>>>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState *dev,
> >>>>>>>>>>> Error
> >>>>>>>>>>> **errp)
> >>>>>>>>>>>                return;
> >>>>>>>>>>>            }
> >>>>>>>>>>>    -        blk_size = blk_getlength(sd->blk);
> >>>>>>>>>>> +        blk_size = blk_getlength(sd->blk) - sd-
> >>>>>>>>>>>> boot_part_size * 2;
> >>>>>>>>>>>            if (blk_size > 0 && !is_power_of_2(blk_size)) {
> >>>>>>>>>>>                int64_t blk_size_aligned = pow2ceil(blk_size);
> >>>>>>>>>>>                char *blk_size_str;
> >>>>>>>>>>
> >>>>>>>>>> This seems to break the tests/functional/arm/
> >>>>>>>>>> test_aspeed_rainier.py
> >>>>>>>>>> test due to mmc-p10bmc-20240617.qcow2 size:
> >>>>>>>>>>
> >>>>>>>>>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -
> >>>>>>>>>> display none -
> >>>>>>>>>> vga none -chardev socket,id=mon,fd=5 -mon
> >>>>>>>>>> chardev=mon,mode=control -
> >>>>>>>>>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
> >>>>>>>>>> chardev:console -drive file=/builds/qemu-project/qemu/
> >>>>>>>>>> functional- cache/
> >>>>>>>>>> download/
> >>>>>>>>>>
> d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2
> -net nic -net user -snapshot
> >>>>>>>>>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
> >>>>>>>>>> SD card size has to be a power of 2, e.g. 16 GiB.
> >>>>>>>>>>
> >>>>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Hmm, then the test was always wrong as well. I suspect the
> >>>>>>>>> aspeed is
> >>>>>>>>> enabling boot partitions by default, and the image was created
> >>>>>>>>> to pass
> >>>>>>>>> the wrong alignment check. Where / by whom is the image
> maintained?
> >>>>>>>>
> >>>>>>>> Cédric Le Goater (Cc'ed).
> >>>>>>>>
> >>>>>>>> The test comes from:
> >>>>>>>> https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb-
> >>>>>>>> a85f-09964268533d@kaod.org/
> >>>>>>>>
> >>>>>>>> Maybe also relevant to your suspicion:
> >>>>>>>> https://lore.kernel.org/qemu-devel/e401d119-402e-0edd-
> >>>>>>>> c2bf-28950ba48ccb@kaod.org/
> >>>
> >>> [*]
> >>>
> >>>>>>>
> >>>>>>> Digging further:
> >>>>>>> https://lore.kernel.org/qemu-
> >>>>>>> devel/
> 9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/
> >>>>>>>
> >>>>>>
> >>>>>> yes commit c078298301a8 might have some impact there.
> >>>>>
> >>>>> With Jan patch, your script doesn't need anymore the
> >>>>>
> >>>>>    echo "Fixing size to keep qemu happy..."
> >>>>>
> >>>>> kludge.
> >>>>
> >>>> which script ?
> >>>
> >>> The one you pasted in [*]:
> >>>
> >>> --
> >>> #!/bin/sh
> >>>
> >>> URLBASE=https://jenkins.openbmc.org/view/latest/job/latest-master/
> >>> label=docker-builder,target=witherspoon-tacoma/lastSuccessfulBuild/
> >>> artifact/openbmc/build/tmp/deploy/images/witherspoon-tacoma/
> >>>
> >>> IMAGESIZE=128
> >>> OUTFILE=mmc.img
> >>>
> >>> FILES="u-boot.bin u-boot-spl.bin obmc-phosphor-image-witherspoon-
> >>> tacoma.wic.xz"
> >>>
> >>> for file in ${FILES}; do
> >>>
> >>>      if test -f ${file}; then
> >>>          echo "${file}: Already downloaded"
> >>>      else
> >>>          echo "${file}: Downloading"
> >>>          wget -nv ${URLBASE}/${file}
> >>>      fi
> >>> done
> >>>
> >>> echo
> >>>
> >>> echo "Creating empty image..."
> >>> dd status=none if=/dev/zero of=${OUTFILE} bs=1M count=${IMAGESIZE}
> >>> echo "Adding SPL..."
> >>> dd status=none if=u-boot-spl.bin of=${OUTFILE} conv=notrunc
> >>> echo "Adding u-boot..."
> >>> dd status=none if=u-boot.bin of=${OUTFILE} conv=notrunc bs=1K seek=64
> >>> echo "Adding userdata..."
> >>> unxz -c obmc-phosphor-image-witherspoon-tacoma.wic.xz | dd
> >>> status=progress of=${OUTFILE} conv=notrunc bs=1M seek=2
> >>> echo "Fixing size to keep qemu happy..."
> >>> truncate --size 16G ${OUTFILE}
> >>>
> >>> echo "Done!"
> >>> echo
> >>> echo " qemu-system-arm -M tacoma-bmc -nographic -drive
> >>> file=mmc.img,if=sd,index=2,format=raw"
> >>> ---
> >>
> >> FTR the alignment check was added to shut up fuzzed CVEs in commit
> >> a9bcedd15a5 ("hw/sd/sdcard: Do not allow invalid SD card sizes"):
> >>
> >>     QEMU allows to create SD card with unrealistic sizes. This could
> >>     work, but some guests (at least Linux) consider sizes that are not
> >>     a power of 2 as a firmware bug and fix the card size to the next
> >>     power of 2.
> >>
> >>     While the possibility to use small SD card images has been seen as
> >>     a feature, it became a bug with CVE-2020-13253, where the guest is
> >>     able to do OOB read/write accesses past the image size end.
> >>
> >>     In a pair of commits we will fix CVE-2020-13253 as:
> >>
> >>         Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
> >>         occurred and no data transfer is performed.
> >>
> >>         Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
> >>         occurred and no data transfer is performed.
> >>
> >>         WP_VIOLATION errors are not modified: the error bit is set, we
> >>         stay in receive-data state, wait for a stop command. All further
> >>         data transfer is ignored. See the check on sd->card_status at
> >>         the beginning of sd_read_data() and sd_write_data().
> >>
> >>     While this is the correct behavior, in case QEMU create smaller SD
> >>     cards, guests still try to access past the image size end, and QEMU
> >>     considers this is an invalid address, thus "all further data
> >>     transfer is ignored". This is wrong and make the guest looping until
> >>     eventually timeouts.
> >>
> >>     Fix by not allowing invalid SD card sizes (suggesting the expected
> >>     size as a hint):
> >>
> >>       $ qemu-system-arm -M orangepi-pc -drive
> >> file=rootfs.ext2,if=sd,format=raw
> >>       qemu-system-arm: Invalid SD card size: 60 MiB
> >>       SD card size has to be a power of 2, e.g. 64 MiB.
> >>       You can resize disk images with 'qemu-img resize <imagefile> <new-
> >> size>'
> >>       (note that this will lose data if you make the image smaller than
> >> it currently is).
> >>
> >>
> >> I expect us to be safe and able to deal with non-pow2 regions if we use
> >> QEMUSGList from the "system/dma.h" API. But this is a rework nobody had
> >> time to do so far.
> >
> > We have to tell two things apart: partitions sizes on the one side and
> > backing storage sizes. The partitions sizes are (to my reading) clearly
> > defined in the spec, and the user partition (alone!) has to be power of
> > 2. The boot and RPMB partitions are multiples of 128K. The sum of them
> > all is nowhere limited to power of 2 or even only multiples of 128K.
> >
>
> Re-reading the part of the device capacity, the rules are more complex:
>  - power of two up to 2 GB
>  - multiple of 512 bytes beyond that
>

Kinda. It is power of 2 up to 2GiB, but there were a number of 4GiB cards
that were not high capacity cards that encoded their size and were otherwise
low capacity cards. Qemu doesn't need to support that. Its existing capacity
check should be enough.

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index da5bdd134a..18b3f93965 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2151,7 +2151,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
         }

         blk_size = blk_getlength(sd->blk);
-        if (blk_size > 0 && !is_power_of_2(blk_size)) {
+        if (blk_size > 0 && (blksize < SDSC_MAX_CAPACITY &&
!is_power_of_2(blk_size)) {
             int64_t blk_size_aligned = pow2ceil(blk_size);
             char *blk_size_str;

is what I'm running with, but it should have a second check for 512k size
if not an ext_csd situation.

High capacity cards, though have a limitation where it's the number of 1024
sectors (which are 512 bytes), so the limit is 512k. It encodes the CSD
differently than normal capacity cards. Thankfully, we have this in our
code already.

And really high capacity cards have an extended structure the size of the
card is reported in, and that appears to be in sectors.


> So that power-of-two enforcement was and still is likely too strict.
>

Agreed.

Warner


> But I still see no indication, neither in the existing eMMC code of QEMU
> nor the spec, that the boot and RPMB partition sizes are included in that.
>
> Jan
>
> --
> Siemens AG, Foundational Technologies
> Linux Expert Center
>
>

[-- Attachment #2: Type: text/html, Size: 15768 bytes --]

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 3732 bytes --]

On Tue, Sep 2, 2025 at 11:18 AM Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 02.09.25 19:07, Warner Losh wrote:
> >
> >
> > On Tue, Sep 2, 2025 at 10:49 AM Jan Lübbe <jlu@pengutronix.de
> > <mailto:jlu@pengutronix.de>> wrote:
> >
> >     On Tue, 2025-09-02 at 18:39 +0200, Jan Kiszka wrote:
> >     > > > I expect us to be safe and able to deal with non-pow2 regions
> >     if we use
> >     > > > QEMUSGList from the "system/dma.h" API. But this is a rework
> >     nobody had
> >     > > > time to do so far.
> >     > >
> >     > > We have to tell two things apart: partitions sizes on the one
> >     side and
> >     > > backing storage sizes. The partitions sizes are (to my reading)
> >     clearly
> >     > > defined in the spec, and the user partition (alone!) has to be
> >     power of
> >     > > 2. The boot and RPMB partitions are multiples of 128K. The sum
> >     of them
> >     > > all is nowhere limited to power of 2 or even only multiples of
> 128K.
> >     > >
> >     >
> >     > Re-reading the part of the device capacity, the rules are more
> >     complex:
> >     >  - power of two up to 2 GB
> >     >  - multiple of 512 bytes beyond that
> >     >
> >     > So that power-of-two enforcement was and still is likely too
> strict.
> >
> >
> > It is. Version 0 (and MMC) cards had the capacity encoded like so:
> >                 m = mmc_get_bits(raw_csd, 128, 62, 12);
> >                 e = mmc_get_bits(raw_csd, 128, 47, 3);
> >                 csd->capacity = ((1 + m) << (e + 2)) * csd->read_bl_len;
> > so any card less than 2GB (well, technically 4GB, but 4GB version 0
> > cards were
> > rare and broke some stacks... I have one and I love it on my embedded
> > ARM board
> > that can't do version 1 cards). Version 1 cards encoded it like:
> >                 csd->capacity = ((uint64_t)mmc_get_bits(raw_csd, 128,
> > 48, 22) +
> >                     1) * 512 * 1024;
> > So it's a multiple of 512k. These are also called 'high capacity' cards.
> >
> > Version 4 introduces an extended CSD, which had a pure sector count in
> > the EXT CSD. I think this
> > is only for MMC cards. And also the partition information.
> >
> >
> >     > But I still see no indication, neither in the existing eMMC code
> >     of QEMU
> >     > nor the spec, that the boot and RPMB partition sizes are included
> >     in that.
> >
> >     Correct. Non-power-of-two sizes are very common for real eMMCs.
> >     Taking a random
> >     one from our lab:
> >     [    1.220588] mmcblk1: mmc1:0001 S0J56X 14.8 GiB
> >     [    1.228055]  mmcblk1: p1 p2 p3 p4
> >     [    1.230375] mmcblk1boot0: mmc1:0001 S0J56X 31.5 MiB
> >     [    1.233651] mmcblk1boot1: mmc1:0001 S0J56X 31.5 MiB
> >     [    1.236682] mmcblk1rpmb: mmc1:0001 S0J56X 4.00 MiB, chardev
> (244:0)
> >
> >     For eMMCs using MLC NAND, you can also configure part of the user
> >     data area to
> >     be pSLC (pseudo single level cell), which changes the available
> >     capacity (after
> >     a required power cycle).
> >
> >
> > Yes. Extended partitions are a feature of version 4 cards, so don't have
> > power-of-2 limits since they are a pure sector count in the ext_csd.
> >
>
> JESD84-B51A (eMMC 5.1A):
>
> "The C_SIZE parameter is used to compute the device capacity for devices
> up to 2 GB of density. See 7.4.52, SEC_COUNT [215:212] , for details on
> calculating densities greater than 2 GB."
>
> So I would now continue to enforce power-of-2 for 2G (including) cards,
> and relax to multiples of 512 for larger ones.
>

It's a multiple of 512k unless the card has a ext_csd, in which case it's a
multiple of 512.

Warner

[-- Attachment #2: Type: text/html, Size: 4960 bytes --]

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 4047 bytes --]

On Tue, Sep 2, 2025 at 11:22 AM Warner Losh <imp@bsdimp.com> wrote:

>
>
> On Tue, Sep 2, 2025 at 11:18 AM Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
>> On 02.09.25 19:07, Warner Losh wrote:
>> >
>> >
>> > On Tue, Sep 2, 2025 at 10:49 AM Jan Lübbe <jlu@pengutronix.de
>> > <mailto:jlu@pengutronix.de>> wrote:
>> >
>> >     On Tue, 2025-09-02 at 18:39 +0200, Jan Kiszka wrote:
>> >     > > > I expect us to be safe and able to deal with non-pow2 regions
>> >     if we use
>> >     > > > QEMUSGList from the "system/dma.h" API. But this is a rework
>> >     nobody had
>> >     > > > time to do so far.
>> >     > >
>> >     > > We have to tell two things apart: partitions sizes on the one
>> >     side and
>> >     > > backing storage sizes. The partitions sizes are (to my reading)
>> >     clearly
>> >     > > defined in the spec, and the user partition (alone!) has to be
>> >     power of
>> >     > > 2. The boot and RPMB partitions are multiples of 128K. The sum
>> >     of them
>> >     > > all is nowhere limited to power of 2 or even only multiples of
>> 128K.
>> >     > >
>> >     >
>> >     > Re-reading the part of the device capacity, the rules are more
>> >     complex:
>> >     >  - power of two up to 2 GB
>> >     >  - multiple of 512 bytes beyond that
>> >     >
>> >     > So that power-of-two enforcement was and still is likely too
>> strict.
>> >
>> >
>> > It is. Version 0 (and MMC) cards had the capacity encoded like so:
>> >                 m = mmc_get_bits(raw_csd, 128, 62, 12);
>> >                 e = mmc_get_bits(raw_csd, 128, 47, 3);
>> >                 csd->capacity = ((1 + m) << (e + 2)) * csd->read_bl_len;
>> > so any card less than 2GB (well, technically 4GB, but 4GB version 0
>> > cards were
>> > rare and broke some stacks... I have one and I love it on my embedded
>> > ARM board
>> > that can't do version 1 cards). Version 1 cards encoded it like:
>> >                 csd->capacity = ((uint64_t)mmc_get_bits(raw_csd, 128,
>> > 48, 22) +
>> >                     1) * 512 * 1024;
>> > So it's a multiple of 512k. These are also called 'high capacity' cards.
>> >
>> > Version 4 introduces an extended CSD, which had a pure sector count in
>> > the EXT CSD. I think this
>> > is only for MMC cards. And also the partition information.
>> >
>> >
>> >     > But I still see no indication, neither in the existing eMMC code
>> >     of QEMU
>> >     > nor the spec, that the boot and RPMB partition sizes are included
>> >     in that.
>> >
>> >     Correct. Non-power-of-two sizes are very common for real eMMCs.
>> >     Taking a random
>> >     one from our lab:
>> >     [    1.220588] mmcblk1: mmc1:0001 S0J56X 14.8 GiB
>> >     [    1.228055]  mmcblk1: p1 p2 p3 p4
>> >     [    1.230375] mmcblk1boot0: mmc1:0001 S0J56X 31.5 MiB
>> >     [    1.233651] mmcblk1boot1: mmc1:0001 S0J56X 31.5 MiB
>> >     [    1.236682] mmcblk1rpmb: mmc1:0001 S0J56X 4.00 MiB, chardev
>> (244:0)
>> >
>> >     For eMMCs using MLC NAND, you can also configure part of the user
>> >     data area to
>> >     be pSLC (pseudo single level cell), which changes the available
>> >     capacity (after
>> >     a required power cycle).
>> >
>> >
>> > Yes. Extended partitions are a feature of version 4 cards, so don't have
>> > power-of-2 limits since they are a pure sector count in the ext_csd.
>> >
>>
>> JESD84-B51A (eMMC 5.1A):
>>
>> "The C_SIZE parameter is used to compute the device capacity for devices
>> up to 2 GB of density. See 7.4.52, SEC_COUNT [215:212] , for details on
>> calculating densities greater than 2 GB."
>>
>> So I would now continue to enforce power-of-2 for 2G (including) cards,
>> and relax to multiples of 512 for larger ones.
>>
>
> It's a multiple of 512k unless the card has a ext_csd, in which case it's
> a multiple of 512.
>

More completely, this is from MMC 4.0 and newer. Extended Capacity SD cards
report this in units of 512k bytes for all cards > 2GiB.

Warner

[-- Attachment #2: Type: text/html, Size: 5507 bytes --]

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Jan Kiszka]

On 02.09.25 19:30, Warner Losh wrote:
> 
> 
> On Tue, Sep 2, 2025 at 11:22 AM Warner Losh <imp@bsdimp.com
> <mailto:imp@bsdimp.com>> wrote:
> 
> 
> 
>     On Tue, Sep 2, 2025 at 11:18 AM Jan Kiszka <jan.kiszka@siemens.com
>     <mailto:jan.kiszka@siemens.com>> wrote:
> 
>         On 02.09.25 19:07, Warner Losh wrote:
>         >
>         >
>         > On Tue, Sep 2, 2025 at 10:49 AM Jan Lübbe <jlu@pengutronix.de
>         <mailto:jlu@pengutronix.de>
>         > <mailto:jlu@pengutronix.de <mailto:jlu@pengutronix.de>>> wrote:
>         >
>         >     On Tue, 2025-09-02 at 18:39 +0200, Jan Kiszka wrote:
>         >     > > > I expect us to be safe and able to deal with non-
>         pow2 regions
>         >     if we use
>         >     > > > QEMUSGList from the "system/dma.h" API. But this is
>         a rework
>         >     nobody had
>         >     > > > time to do so far.
>         >     > >
>         >     > > We have to tell two things apart: partitions sizes on
>         the one
>         >     side and
>         >     > > backing storage sizes. The partitions sizes are (to my
>         reading)
>         >     clearly
>         >     > > defined in the spec, and the user partition (alone!)
>         has to be
>         >     power of
>         >     > > 2. The boot and RPMB partitions are multiples of 128K.
>         The sum
>         >     of them
>         >     > > all is nowhere limited to power of 2 or even only
>         multiples of 128K.
>         >     > >
>         >     >
>         >     > Re-reading the part of the device capacity, the rules
>         are more
>         >     complex:
>         >     >  - power of two up to 2 GB
>         >     >  - multiple of 512 bytes beyond that
>         >     >
>         >     > So that power-of-two enforcement was and still is likely
>         too strict.
>         >
>         >
>         > It is. Version 0 (and MMC) cards had the capacity encoded like so:
>         >                 m = mmc_get_bits(raw_csd, 128, 62, 12);
>         >                 e = mmc_get_bits(raw_csd, 128, 47, 3);
>         >                 csd->capacity = ((1 + m) << (e + 2)) * csd-
>         >read_bl_len;
>         > so any card less than 2GB (well, technically 4GB, but 4GB
>         version 0
>         > cards were
>         > rare and broke some stacks... I have one and I love it on my
>         embedded
>         > ARM board
>         > that can't do version 1 cards). Version 1 cards encoded it like:
>         >                 csd->capacity =
>         ((uint64_t)mmc_get_bits(raw_csd, 128,
>         > 48, 22) +
>         >                     1) * 512 * 1024;
>         > So it's a multiple of 512k. These are also called 'high
>         capacity' cards.
>         >
>         > Version 4 introduces an extended CSD, which had a pure sector
>         count in
>         > the EXT CSD. I think this
>         > is only for MMC cards. And also the partition information.
>         >  
>         >
>         >     > But I still see no indication, neither in the existing
>         eMMC code
>         >     of QEMU
>         >     > nor the spec, that the boot and RPMB partition sizes are
>         included
>         >     in that.
>         >
>         >     Correct. Non-power-of-two sizes are very common for real
>         eMMCs.
>         >     Taking a random
>         >     one from our lab:
>         >     [    1.220588] mmcblk1: mmc1:0001 S0J56X 14.8 GiB
>         >     [    1.228055]  mmcblk1: p1 p2 p3 p4
>         >     [    1.230375] mmcblk1boot0: mmc1:0001 S0J56X 31.5 MiB
>         >     [    1.233651] mmcblk1boot1: mmc1:0001 S0J56X 31.5 MiB
>         >     [    1.236682] mmcblk1rpmb: mmc1:0001 S0J56X 4.00 MiB,
>         chardev (244:0)
>         >
>         >     For eMMCs using MLC NAND, you can also configure part of
>         the user
>         >     data area to
>         >     be pSLC (pseudo single level cell), which changes the
>         available
>         >     capacity (after
>         >     a required power cycle).
>         >
>         >
>         > Yes. Extended partitions are a feature of version 4 cards, so
>         don't have
>         > power-of-2 limits since they are a pure sector count in the
>         ext_csd.
>         >
> 
>         JESD84-B51A (eMMC 5.1A):
> 
>         "The C_SIZE parameter is used to compute the device capacity for
>         devices
>         up to 2 GB of density. See 7.4.52, SEC_COUNT [215:212] , for
>         details on
>         calculating densities greater than 2 GB."
> 
>         So I would now continue to enforce power-of-2 for 2G (including)
>         cards,
>         and relax to multiples of 512 for larger ones.
> 
> 
>     It's a multiple of 512k unless the card has a ext_csd, in which case
>     it's a multiple of 512.
> 
> 
> More completely, this is from MMC 4.0 and newer. Extended Capacity SD
> cards report this in units of 512k bytes for all cards > 2GiB.
> 

I'm not sure which spec version you are referring to, but JESD84-A441
and JESD84-B51A mention nothing about 512K, rather "Device density =
SEC_COUNT x 512B". And these are the specs we very likely need to follow
here.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Jan Kiszka]

On 02.09.25 19:20, Warner Losh wrote:
> 
> 
> On Tue, Sep 2, 2025 at 10:40 AM Jan Kiszka <jan.kiszka@siemens.com
> <mailto:jan.kiszka@siemens.com>> wrote:
> 
>     On 02.09.25 18:24, Jan Kiszka wrote:
>     > On 02.09.25 18:20, Philippe Mathieu-Daudé wrote:
>     >> On 2/9/25 18:14, Philippe Mathieu-Daudé wrote:
>     >>> On 2/9/25 18:00, Cédric Le Goater wrote:
>     >>>> On 9/2/25 17:55, Philippe Mathieu-Daudé wrote:
>     >>>>> On 2/9/25 17:47, Cédric Le Goater wrote:
>     >>>>>> On 9/2/25 17:45, Philippe Mathieu-Daudé wrote:
>     >>>>>>> On 2/9/25 17:43, Philippe Mathieu-Daudé wrote:
>     >>>>>>>> On 2/9/25 17:34, Jan Kiszka wrote:
>     >>>>>>>>> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
>     >>>>>>>>>> On 1/9/25 07:56, Jan Kiszka wrote:
>     >>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com
>     <mailto:jan.kiszka@siemens.com>>
>     >>>>>>>>>>>
>     >>>>>>>>>>> The power-of-2 rule applies to the user data area, not the
>     >>>>>>>>>>> complete
>     >>>>>>>>>>> block image. The latter can be concatenation of boot
>     partition
>     >>>>>>>>>>> images
>     >>>>>>>>>>> and the user data.
>     >>>>>>>>>>>
>     >>>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com
>     <mailto:jan.kiszka@siemens.com>>
>     >>>>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org
>     <mailto:philmd@linaro.org>>
>     >>>>>>>>>>> ---
>     >>>>>>>>>>>    hw/sd/sd.c | 2 +-
>     >>>>>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>     >>>>>>>>>>>
>     >>>>>>>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>     >>>>>>>>>>> index 8c290595f0..16aee210b4 100644
>     >>>>>>>>>>> --- a/hw/sd/sd.c
>     >>>>>>>>>>> +++ b/hw/sd/sd.c
>     >>>>>>>>>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState
>     *dev,
>     >>>>>>>>>>> Error
>     >>>>>>>>>>> **errp)
>     >>>>>>>>>>>                return;
>     >>>>>>>>>>>            }
>     >>>>>>>>>>>    -        blk_size = blk_getlength(sd->blk);
>     >>>>>>>>>>> +        blk_size = blk_getlength(sd->blk) - sd-
>     >>>>>>>>>>>> boot_part_size * 2;
>     >>>>>>>>>>>            if (blk_size > 0 && !is_power_of_2(blk_size)) {
>     >>>>>>>>>>>                int64_t blk_size_aligned =
>     pow2ceil(blk_size);
>     >>>>>>>>>>>                char *blk_size_str;
>     >>>>>>>>>>
>     >>>>>>>>>> This seems to break the tests/functional/arm/
>     >>>>>>>>>> test_aspeed_rainier.py
>     >>>>>>>>>> test due to mmc-p10bmc-20240617.qcow2 size:
>     >>>>>>>>>>
>     >>>>>>>>>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -
>     >>>>>>>>>> display none -
>     >>>>>>>>>> vga none -chardev socket,id=mon,fd=5 -mon
>     >>>>>>>>>> chardev=mon,mode=control -
>     >>>>>>>>>> machine rainier-bmc -chardev socket,id=console,fd=10 -serial
>     >>>>>>>>>> chardev:console -drive file=/builds/qemu-project/qemu/
>     >>>>>>>>>> functional- cache/
>     >>>>>>>>>> download/
>     >>>>>>>>>>
>     d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2 -net nic -net user -snapshot
>     >>>>>>>>>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
>     >>>>>>>>>> SD card size has to be a power of 2, e.g. 16 GiB.
>     >>>>>>>>>>
>     >>>>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
>     <https://gitlab.com/qemu-project/qemu/-/jobs/11217561316>
>     >>>>>>>>>>
>     >>>>>>>>>
>     >>>>>>>>> Hmm, then the test was always wrong as well. I suspect the
>     >>>>>>>>> aspeed is
>     >>>>>>>>> enabling boot partitions by default, and the image was created
>     >>>>>>>>> to pass
>     >>>>>>>>> the wrong alignment check. Where / by whom is the image
>     maintained?
>     >>>>>>>>
>     >>>>>>>> Cédric Le Goater (Cc'ed).
>     >>>>>>>>
>     >>>>>>>> The test comes from:
>     >>>>>>>> https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb-
>     <https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb->
>     >>>>>>>> a85f-09964268533d@kaod.org/ <http://
>     a85f-09964268533d@kaod.org/>
>     >>>>>>>>
>     >>>>>>>> Maybe also relevant to your suspicion:
>     >>>>>>>> https://lore.kernel.org/qemu-devel/e401d119-402e-0edd-
>     <https://lore.kernel.org/qemu-devel/e401d119-402e-0edd->
>     >>>>>>>> c2bf-28950ba48ccb@kaod.org/ <http://
>     c2bf-28950ba48ccb@kaod.org/>
>     >>>
>     >>> [*]
>     >>>
>     >>>>>>>
>     >>>>>>> Digging further:
>     >>>>>>> https://lore.kernel.org/qemu- <https://lore.kernel.org/qemu->
>     >>>>>>>
>     devel/9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/
>     <http://9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/>
>     >>>>>>>
>     >>>>>>
>     >>>>>> yes commit c078298301a8 might have some impact there.
>     >>>>>
>     >>>>> With Jan patch, your script doesn't need anymore the
>     >>>>>
>     >>>>>    echo "Fixing size to keep qemu happy..."
>     >>>>>
>     >>>>> kludge.
>     >>>>
>     >>>> which script ?
>     >>>
>     >>> The one you pasted in [*]:
>     >>>
>     >>> -- 
>     >>> #!/bin/sh
>     >>>
>     >>> URLBASE=https://jenkins.openbmc.org/view/latest/job/latest-
>     master/ <https://jenkins.openbmc.org/view/latest/job/latest-master/>
>     >>> label=docker-builder,target=witherspoon-tacoma/lastSuccessfulBuild/
>     >>> artifact/openbmc/build/tmp/deploy/images/witherspoon-tacoma/
>     >>>
>     >>> IMAGESIZE=128
>     >>> OUTFILE=mmc.img
>     >>>
>     >>> FILES="u-boot.bin u-boot-spl.bin obmc-phosphor-image-witherspoon-
>     >>> tacoma.wic.xz"
>     >>>
>     >>> for file in ${FILES}; do
>     >>>
>     >>>      if test -f ${file}; then
>     >>>          echo "${file}: Already downloaded"
>     >>>      else
>     >>>          echo "${file}: Downloading"
>     >>>          wget -nv ${URLBASE}/${file}
>     >>>      fi
>     >>> done
>     >>>
>     >>> echo
>     >>>
>     >>> echo "Creating empty image..."
>     >>> dd status=none if=/dev/zero of=${OUTFILE} bs=1M count=${IMAGESIZE}
>     >>> echo "Adding SPL..."
>     >>> dd status=none if=u-boot-spl.bin of=${OUTFILE} conv=notrunc
>     >>> echo "Adding u-boot..."
>     >>> dd status=none if=u-boot.bin of=${OUTFILE} conv=notrunc bs=1K
>     seek=64
>     >>> echo "Adding userdata..."
>     >>> unxz -c obmc-phosphor-image-witherspoon-tacoma.wic.xz | dd
>     >>> status=progress of=${OUTFILE} conv=notrunc bs=1M seek=2
>     >>> echo "Fixing size to keep qemu happy..."
>     >>> truncate --size 16G ${OUTFILE}
>     >>>
>     >>> echo "Done!"
>     >>> echo
>     >>> echo " qemu-system-arm -M tacoma-bmc -nographic -drive
>     >>> file=mmc.img,if=sd,index=2,format=raw"
>     >>> ---
>     >>
>     >> FTR the alignment check was added to shut up fuzzed CVEs in commit
>     >> a9bcedd15a5 ("hw/sd/sdcard: Do not allow invalid SD card sizes"):
>     >>
>     >>     QEMU allows to create SD card with unrealistic sizes. This could
>     >>     work, but some guests (at least Linux) consider sizes that
>     are not
>     >>     a power of 2 as a firmware bug and fix the card size to the next
>     >>     power of 2.
>     >>
>     >>     While the possibility to use small SD card images has been
>     seen as
>     >>     a feature, it became a bug with CVE-2020-13253, where the
>     guest is
>     >>     able to do OOB read/write accesses past the image size end.
>     >>
>     >>     In a pair of commits we will fix CVE-2020-13253 as:
>     >>
>     >>         Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>     >>         occurred and no data transfer is performed.
>     >>
>     >>         Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>     >>         occurred and no data transfer is performed.
>     >>
>     >>         WP_VIOLATION errors are not modified: the error bit is
>     set, we
>     >>         stay in receive-data state, wait for a stop command. All
>     further
>     >>         data transfer is ignored. See the check on sd->card_status at
>     >>         the beginning of sd_read_data() and sd_write_data().
>     >>
>     >>     While this is the correct behavior, in case QEMU create
>     smaller SD
>     >>     cards, guests still try to access past the image size end,
>     and QEMU
>     >>     considers this is an invalid address, thus "all further data
>     >>     transfer is ignored". This is wrong and make the guest
>     looping until
>     >>     eventually timeouts.
>     >>
>     >>     Fix by not allowing invalid SD card sizes (suggesting the
>     expected
>     >>     size as a hint):
>     >>
>     >>       $ qemu-system-arm -M orangepi-pc -drive
>     >> file=rootfs.ext2,if=sd,format=raw
>     >>       qemu-system-arm: Invalid SD card size: 60 MiB
>     >>       SD card size has to be a power of 2, e.g. 64 MiB.
>     >>       You can resize disk images with 'qemu-img resize
>     <imagefile> <new-
>     >> size>'
>     >>       (note that this will lose data if you make the image
>     smaller than
>     >> it currently is).
>     >>
>     >>
>     >> I expect us to be safe and able to deal with non-pow2 regions if
>     we use
>     >> QEMUSGList from the "system/dma.h" API. But this is a rework
>     nobody had
>     >> time to do so far.
>     >
>     > We have to tell two things apart: partitions sizes on the one side and
>     > backing storage sizes. The partitions sizes are (to my reading)
>     clearly
>     > defined in the spec, and the user partition (alone!) has to be
>     power of
>     > 2. The boot and RPMB partitions are multiples of 128K. The sum of them
>     > all is nowhere limited to power of 2 or even only multiples of 128K.
>     >
> 
>     Re-reading the part of the device capacity, the rules are more complex:
>      - power of two up to 2 GB
>      - multiple of 512 bytes beyond that
> 
> 
> Kinda. It is power of 2 up to 2GiB, but there were a number of 4GiB cards
> that were not high capacity cards that encoded their size and were otherwise
> low capacity cards. Qemu doesn't need to support that. Its existing capacity
> check should be enough.
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index da5bdd134a..18b3f93965 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2151,7 +2151,7 @@ static void sd_realize(DeviceState *dev, Error **errp)
>          }
> 
>          blk_size = blk_getlength(sd->blk);
> -        if (blk_size > 0 && !is_power_of_2(blk_size)) {
> +        if (blk_size > 0 && (blksize < SDSC_MAX_CAPACITY && !
> is_power_of_2(blk_size)) {

Close but not yet correct: <=

I have a patch under test for staging that also enforces the 512-byte rule.

Jan

>              int64_t blk_size_aligned = pow2ceil(blk_size);
>              char *blk_size_str;
> 
> is what I'm running with, but it should have a second check for 512k
> size if not an ext_csd situation.
> 
> High capacity cards, though have a limitation where it's the number of
> 1024 sectors (which are 512 bytes), so the limit is 512k. It encodes the
> CSD differently than normal capacity cards. Thankfully, we have this in
> our code already.
> 
> And really high capacity cards have an extended structure the size of
> the card is reported in, and that appears to be in sectors.
>  
> 
>     So that power-of-two enforcement was and still is likely too strict.
> 
> 
> Agreed.
> 
> Warner
>  
> 
>     But I still see no indication, neither in the existing eMMC code of QEMU
>     nor the spec, that the boot and RPMB partition sizes are included in
>     that.
> 
>     Jan
> 
>     -- 
>     Siemens AG, Foundational Technologies
>     Linux Expert Center
> 

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 6801 bytes --]

On Tue, Sep 2, 2025 at 11:37 AM Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 02.09.25 19:30, Warner Losh wrote:
> >
> >
> > On Tue, Sep 2, 2025 at 11:22 AM Warner Losh <imp@bsdimp.com
> > <mailto:imp@bsdimp.com>> wrote:
> >
> >
> >
> >     On Tue, Sep 2, 2025 at 11:18 AM Jan Kiszka <jan.kiszka@siemens.com
> >     <mailto:jan.kiszka@siemens.com>> wrote:
> >
> >         On 02.09.25 19:07, Warner Losh wrote:
> >         >
> >         >
> >         > On Tue, Sep 2, 2025 at 10:49 AM Jan Lübbe <jlu@pengutronix.de
> >         <mailto:jlu@pengutronix.de>
> >         > <mailto:jlu@pengutronix.de <mailto:jlu@pengutronix.de>>>
> wrote:
> >         >
> >         >     On Tue, 2025-09-02 at 18:39 +0200, Jan Kiszka wrote:
> >         >     > > > I expect us to be safe and able to deal with non-
> >         pow2 regions
> >         >     if we use
> >         >     > > > QEMUSGList from the "system/dma.h" API. But this is
> >         a rework
> >         >     nobody had
> >         >     > > > time to do so far.
> >         >     > >
> >         >     > > We have to tell two things apart: partitions sizes on
> >         the one
> >         >     side and
> >         >     > > backing storage sizes. The partitions sizes are (to my
> >         reading)
> >         >     clearly
> >         >     > > defined in the spec, and the user partition (alone!)
> >         has to be
> >         >     power of
> >         >     > > 2. The boot and RPMB partitions are multiples of 128K.
> >         The sum
> >         >     of them
> >         >     > > all is nowhere limited to power of 2 or even only
> >         multiples of 128K.
> >         >     > >
> >         >     >
> >         >     > Re-reading the part of the device capacity, the rules
> >         are more
> >         >     complex:
> >         >     >  - power of two up to 2 GB
> >         >     >  - multiple of 512 bytes beyond that
> >         >     >
> >         >     > So that power-of-two enforcement was and still is likely
> >         too strict.
> >         >
> >         >
> >         > It is. Version 0 (and MMC) cards had the capacity encoded like
> so:
> >         >                 m = mmc_get_bits(raw_csd, 128, 62, 12);
> >         >                 e = mmc_get_bits(raw_csd, 128, 47, 3);
> >         >                 csd->capacity = ((1 + m) << (e + 2)) * csd-
> >         >read_bl_len;
> >         > so any card less than 2GB (well, technically 4GB, but 4GB
> >         version 0
> >         > cards were
> >         > rare and broke some stacks... I have one and I love it on my
> >         embedded
> >         > ARM board
> >         > that can't do version 1 cards). Version 1 cards encoded it
> like:
> >         >                 csd->capacity =
> >         ((uint64_t)mmc_get_bits(raw_csd, 128,
> >         > 48, 22) +
> >         >                     1) * 512 * 1024;
> >         > So it's a multiple of 512k. These are also called 'high
> >         capacity' cards.
> >         >
> >         > Version 4 introduces an extended CSD, which had a pure sector
> >         count in
> >         > the EXT CSD. I think this
> >         > is only for MMC cards. And also the partition information.
> >         >
> >         >
> >         >     > But I still see no indication, neither in the existing
> >         eMMC code
> >         >     of QEMU
> >         >     > nor the spec, that the boot and RPMB partition sizes are
> >         included
> >         >     in that.
> >         >
> >         >     Correct. Non-power-of-two sizes are very common for real
> >         eMMCs.
> >         >     Taking a random
> >         >     one from our lab:
> >         >     [    1.220588] mmcblk1: mmc1:0001 S0J56X 14.8 GiB
> >         >     [    1.228055]  mmcblk1: p1 p2 p3 p4
> >         >     [    1.230375] mmcblk1boot0: mmc1:0001 S0J56X 31.5 MiB
> >         >     [    1.233651] mmcblk1boot1: mmc1:0001 S0J56X 31.5 MiB
> >         >     [    1.236682] mmcblk1rpmb: mmc1:0001 S0J56X 4.00 MiB,
> >         chardev (244:0)
> >         >
> >         >     For eMMCs using MLC NAND, you can also configure part of
> >         the user
> >         >     data area to
> >         >     be pSLC (pseudo single level cell), which changes the
> >         available
> >         >     capacity (after
> >         >     a required power cycle).
> >         >
> >         >
> >         > Yes. Extended partitions are a feature of version 4 cards, so
> >         don't have
> >         > power-of-2 limits since they are a pure sector count in the
> >         ext_csd.
> >         >
> >
> >         JESD84-B51A (eMMC 5.1A):
> >
> >         "The C_SIZE parameter is used to compute the device capacity for
> >         devices
> >         up to 2 GB of density. See 7.4.52, SEC_COUNT [215:212] , for
> >         details on
> >         calculating densities greater than 2 GB."
> >
> >         So I would now continue to enforce power-of-2 for 2G (including)
> >         cards,
> >         and relax to multiples of 512 for larger ones.
> >
> >
> >     It's a multiple of 512k unless the card has a ext_csd, in which case
> >     it's a multiple of 512.
> >
> >
> > More completely, this is from MMC 4.0 and newer. Extended Capacity SD
> > cards report this in units of 512k bytes for all cards > 2GiB.
> >
>
> I'm not sure which spec version you are referring to, but JESD84-A441
> and JESD84-B51A mention nothing about 512K, rather "Device density =
> SEC_COUNT x 512B". And these are the specs we very likely need to follow
> here.
>

You are right that this is in the MMC spec. However, the SD spec is
controlling for SD cards.

SD Specifications Part 1 Physical Layer Simplified Specification Version
9.10
December 1, 2023

Section 5.3 describes the CSD. Version 1.0 (which I'd called version 0 in
an earlier email because of its encoding) is the 2GB rule. Version 2.0 and
3.0 encode it as 512k count (from 5.3.3):

C_SIZE
This field is expanded to 28 bits and can indicate up to 128 TBytes.

This parameter is used to calculate the user data area capacity in the SD
memory card (note that size of the protected area is zero for SDUC card).
The user data area capacity is calculated from C_SIZE as follows:

memory capacity = (C_SIZE+1) * 512KByte

The Minimum user area size of SDUC Card is 4,294,968,320 sectors
(2TB+0.5MB).
The Minimum value of C_SIZE for SDUC in CSD Version 3.0 is 0400000h
(4194304). The Maximum user area size of SDUC Card is 274,877,906,944
sectors (128TB).
The Maximum value of C_SIZE for SDUC in CSD Version 3.0 is FFFFFFFh
(268435455).

So SD cards are yet again gratuitously different than MMC cards.

Warner

[-- Attachment #2: Type: text/html, Size: 9570 bytes --]

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Jan Kiszka]

On 02.09.25 19:48, Warner Losh wrote:
> 
> 
> On Tue, Sep 2, 2025 at 11:37 AM Jan Kiszka <jan.kiszka@siemens.com
> <mailto:jan.kiszka@siemens.com>> wrote:
> 
>     On 02.09.25 19:30, Warner Losh wrote:
>     >
>     >
>     > On Tue, Sep 2, 2025 at 11:22 AM Warner Losh <imp@bsdimp.com
>     <mailto:imp@bsdimp.com>
>     > <mailto:imp@bsdimp.com <mailto:imp@bsdimp.com>>> wrote:
>     >
>     >
>     >
>     >     On Tue, Sep 2, 2025 at 11:18 AM Jan Kiszka
>     <jan.kiszka@siemens.com <mailto:jan.kiszka@siemens.com>
>     >     <mailto:jan.kiszka@siemens.com
>     <mailto:jan.kiszka@siemens.com>>> wrote:
>     >
>     >         On 02.09.25 19:07, Warner Losh wrote:
>     >         >
>     >         >
>     >         > On Tue, Sep 2, 2025 at 10:49 AM Jan Lübbe
>     <jlu@pengutronix.de <mailto:jlu@pengutronix.de>
>     >         <mailto:jlu@pengutronix.de <mailto:jlu@pengutronix.de>>
>     >         > <mailto:jlu@pengutronix.de <mailto:jlu@pengutronix.de>
>     <mailto:jlu@pengutronix.de <mailto:jlu@pengutronix.de>>>> wrote:
>     >         >
>     >         >     On Tue, 2025-09-02 at 18:39 +0200, Jan Kiszka wrote:
>     >         >     > > > I expect us to be safe and able to deal with non-
>     >         pow2 regions
>     >         >     if we use
>     >         >     > > > QEMUSGList from the "system/dma.h" API. But
>     this is
>     >         a rework
>     >         >     nobody had
>     >         >     > > > time to do so far.
>     >         >     > >
>     >         >     > > We have to tell two things apart: partitions
>     sizes on
>     >         the one
>     >         >     side and
>     >         >     > > backing storage sizes. The partitions sizes are
>     (to my
>     >         reading)
>     >         >     clearly
>     >         >     > > defined in the spec, and the user partition (alone!)
>     >         has to be
>     >         >     power of
>     >         >     > > 2. The boot and RPMB partitions are multiples of
>     128K.
>     >         The sum
>     >         >     of them
>     >         >     > > all is nowhere limited to power of 2 or even only
>     >         multiples of 128K.
>     >         >     > >
>     >         >     >
>     >         >     > Re-reading the part of the device capacity, the rules
>     >         are more
>     >         >     complex:
>     >         >     >  - power of two up to 2 GB
>     >         >     >  - multiple of 512 bytes beyond that
>     >         >     >
>     >         >     > So that power-of-two enforcement was and still is
>     likely
>     >         too strict.
>     >         >
>     >         >
>     >         > It is. Version 0 (and MMC) cards had the capacity
>     encoded like so:
>     >         >                 m = mmc_get_bits(raw_csd, 128, 62, 12);
>     >         >                 e = mmc_get_bits(raw_csd, 128, 47, 3);
>     >         >                 csd->capacity = ((1 + m) << (e + 2)) * csd-
>     >         >read_bl_len;
>     >         > so any card less than 2GB (well, technically 4GB, but 4GB
>     >         version 0
>     >         > cards were
>     >         > rare and broke some stacks... I have one and I love it on my
>     >         embedded
>     >         > ARM board
>     >         > that can't do version 1 cards). Version 1 cards encoded
>     it like:
>     >         >                 csd->capacity =
>     >         ((uint64_t)mmc_get_bits(raw_csd, 128,
>     >         > 48, 22) +
>     >         >                     1) * 512 * 1024;
>     >         > So it's a multiple of 512k. These are also called 'high
>     >         capacity' cards.
>     >         >
>     >         > Version 4 introduces an extended CSD, which had a pure
>     sector
>     >         count in
>     >         > the EXT CSD. I think this
>     >         > is only for MMC cards. And also the partition information.
>     >         >  
>     >         >
>     >         >     > But I still see no indication, neither in the existing
>     >         eMMC code
>     >         >     of QEMU
>     >         >     > nor the spec, that the boot and RPMB partition
>     sizes are
>     >         included
>     >         >     in that.
>     >         >
>     >         >     Correct. Non-power-of-two sizes are very common for real
>     >         eMMCs.
>     >         >     Taking a random
>     >         >     one from our lab:
>     >         >     [    1.220588] mmcblk1: mmc1:0001 S0J56X 14.8 GiB
>     >         >     [    1.228055]  mmcblk1: p1 p2 p3 p4
>     >         >     [    1.230375] mmcblk1boot0: mmc1:0001 S0J56X 31.5 MiB
>     >         >     [    1.233651] mmcblk1boot1: mmc1:0001 S0J56X 31.5 MiB
>     >         >     [    1.236682] mmcblk1rpmb: mmc1:0001 S0J56X 4.00 MiB,
>     >         chardev (244:0)
>     >         >
>     >         >     For eMMCs using MLC NAND, you can also configure part of
>     >         the user
>     >         >     data area to
>     >         >     be pSLC (pseudo single level cell), which changes the
>     >         available
>     >         >     capacity (after
>     >         >     a required power cycle).
>     >         >
>     >         >
>     >         > Yes. Extended partitions are a feature of version 4
>     cards, so
>     >         don't have
>     >         > power-of-2 limits since they are a pure sector count in the
>     >         ext_csd.
>     >         >
>     >
>     >         JESD84-B51A (eMMC 5.1A):
>     >
>     >         "The C_SIZE parameter is used to compute the device
>     capacity for
>     >         devices
>     >         up to 2 GB of density. See 7.4.52, SEC_COUNT [215:212] , for
>     >         details on
>     >         calculating densities greater than 2 GB."
>     >
>     >         So I would now continue to enforce power-of-2 for 2G
>     (including)
>     >         cards,
>     >         and relax to multiples of 512 for larger ones.
>     >
>     >
>     >     It's a multiple of 512k unless the card has a ext_csd, in
>     which case
>     >     it's a multiple of 512.
>     >
>     >
>     > More completely, this is from MMC 4.0 and newer. Extended Capacity SD
>     > cards report this in units of 512k bytes for all cards > 2GiB.
>     >
> 
>     I'm not sure which spec version you are referring to, but JESD84-A441
>     and JESD84-B51A mention nothing about 512K, rather "Device density =
>     SEC_COUNT x 512B". And these are the specs we very likely need to follow
>     here.
> 
> 
> You are right that this is in the MMC spec. However, the SD spec is
> controlling for SD cards.
> 
> SD Specifications Part 1 Physical Layer Simplified Specification Version
> 9.10
> December 1, 2023
> 
> Section 5.3 describes the CSD. Version 1.0 (which I'd called version 0
> in an earlier email because of its encoding) is the 2GB rule. Version

< 2G or <= 2G? For eMMC, it is <=.

> 2.0 and 3.0 encode it as 512k count (from 5.3.3):
> 
> C_SIZE
> This field is expanded to 28 bits and can indicate up to 128 TBytes.
> 
> This parameter is used to calculate the user data area capacity in the
> SD memory card (note that size of the protected area is zero for SDUC
> card). The user data area capacity is calculated from C_SIZE as follows:
> 
> memory capacity = (C_SIZE+1) * 512KByte
> 
> The Minimum user area size of SDUC Card is 4,294,968,320 sectors
> (2TB+0.5MB).
> The Minimum value of C_SIZE for SDUC in CSD Version 3.0 is 0400000h
> (4194304). The Maximum user area size of SDUC Card is 274,877,906,944
> sectors (128TB).
> The Maximum value of C_SIZE for SDUC in CSD Version 3.0 is FFFFFFFh
> (268435455).
> 
> So SD cards are yet again gratuitously different than MMC cards.
> 

Argh, then we need to take the card type into account as well. Need to
rework my patch...

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 12899 bytes --]

On Tue, Sep 2, 2025 at 11:39 AM Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 02.09.25 19:20, Warner Losh wrote:
> >
> >
> > On Tue, Sep 2, 2025 at 10:40 AM Jan Kiszka <jan.kiszka@siemens.com
> > <mailto:jan.kiszka@siemens.com>> wrote:
> >
> >     On 02.09.25 18:24, Jan Kiszka wrote:
> >     > On 02.09.25 18:20, Philippe Mathieu-Daudé wrote:
> >     >> On 2/9/25 18:14, Philippe Mathieu-Daudé wrote:
> >     >>> On 2/9/25 18:00, Cédric Le Goater wrote:
> >     >>>> On 9/2/25 17:55, Philippe Mathieu-Daudé wrote:
> >     >>>>> On 2/9/25 17:47, Cédric Le Goater wrote:
> >     >>>>>> On 9/2/25 17:45, Philippe Mathieu-Daudé wrote:
> >     >>>>>>> On 2/9/25 17:43, Philippe Mathieu-Daudé wrote:
> >     >>>>>>>> On 2/9/25 17:34, Jan Kiszka wrote:
> >     >>>>>>>>> On 02.09.25 17:06, Philippe Mathieu-Daudé wrote:
> >     >>>>>>>>>> On 1/9/25 07:56, Jan Kiszka wrote:
> >     >>>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com
> >     <mailto:jan.kiszka@siemens.com>>
> >     >>>>>>>>>>>
> >     >>>>>>>>>>> The power-of-2 rule applies to the user data area, not
> the
> >     >>>>>>>>>>> complete
> >     >>>>>>>>>>> block image. The latter can be concatenation of boot
> >     partition
> >     >>>>>>>>>>> images
> >     >>>>>>>>>>> and the user data.
> >     >>>>>>>>>>>
> >     >>>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com
> >     <mailto:jan.kiszka@siemens.com>>
> >     >>>>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org
> >     <mailto:philmd@linaro.org>>
> >     >>>>>>>>>>> ---
> >     >>>>>>>>>>>    hw/sd/sd.c | 2 +-
> >     >>>>>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >     >>>>>>>>>>>
> >     >>>>>>>>>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> >     >>>>>>>>>>> index 8c290595f0..16aee210b4 100644
> >     >>>>>>>>>>> --- a/hw/sd/sd.c
> >     >>>>>>>>>>> +++ b/hw/sd/sd.c
> >     >>>>>>>>>>> @@ -2789,7 +2789,7 @@ static void sd_realize(DeviceState
> >     *dev,
> >     >>>>>>>>>>> Error
> >     >>>>>>>>>>> **errp)
> >     >>>>>>>>>>>                return;
> >     >>>>>>>>>>>            }
> >     >>>>>>>>>>>    -        blk_size = blk_getlength(sd->blk);
> >     >>>>>>>>>>> +        blk_size = blk_getlength(sd->blk) - sd-
> >     >>>>>>>>>>>> boot_part_size * 2;
> >     >>>>>>>>>>>            if (blk_size > 0 && !is_power_of_2(blk_size))
> {
> >     >>>>>>>>>>>                int64_t blk_size_aligned =
> >     pow2ceil(blk_size);
> >     >>>>>>>>>>>                char *blk_size_str;
> >     >>>>>>>>>>
> >     >>>>>>>>>> This seems to break the tests/functional/arm/
> >     >>>>>>>>>> test_aspeed_rainier.py
> >     >>>>>>>>>> test due to mmc-p10bmc-20240617.qcow2 size:
> >     >>>>>>>>>>
> >     >>>>>>>>>> Command: /builds/qemu-project/qemu/build/qemu-system-arm -
> >     >>>>>>>>>> display none -
> >     >>>>>>>>>> vga none -chardev socket,id=mon,fd=5 -mon
> >     >>>>>>>>>> chardev=mon,mode=control -
> >     >>>>>>>>>> machine rainier-bmc -chardev socket,id=console,fd=10
> -serial
> >     >>>>>>>>>> chardev:console -drive file=/builds/qemu-project/qemu/
> >     >>>>>>>>>> functional- cache/
> >     >>>>>>>>>> download/
> >     >>>>>>>>>>
> >
>  d523fb478d2b84d5adc5658d08502bc64b1486955683814f89c6137518acd90b,if=sd,id=sd2,index=2
> -net nic -net user -snapshot
> >     >>>>>>>>>> Output: qemu-system-arm: Invalid SD card size: 16 GiB
> >     >>>>>>>>>> SD card size has to be a power of 2, e.g. 16 GiB.
> >     >>>>>>>>>>
> >     >>>>>>>>>> https://gitlab.com/qemu-project/qemu/-/jobs/11217561316
> >     <https://gitlab.com/qemu-project/qemu/-/jobs/11217561316>
> >     >>>>>>>>>>
> >     >>>>>>>>>
> >     >>>>>>>>> Hmm, then the test was always wrong as well. I suspect the
> >     >>>>>>>>> aspeed is
> >     >>>>>>>>> enabling boot partitions by default, and the image was
> created
> >     >>>>>>>>> to pass
> >     >>>>>>>>> the wrong alignment check. Where / by whom is the image
> >     maintained?
> >     >>>>>>>>
> >     >>>>>>>> Cédric Le Goater (Cc'ed).
> >     >>>>>>>>
> >     >>>>>>>> The test comes from:
> >     >>>>>>>> https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb-
> >     <https://lore.kernel.org/qemu-devel/4d1777d6-0195-4ecb->
> >     >>>>>>>> a85f-09964268533d@kaod.org/ <http://
> >     a85f-09964268533d@kaod.org/>
> >     >>>>>>>>
> >     >>>>>>>> Maybe also relevant to your suspicion:
> >     >>>>>>>> https://lore.kernel.org/qemu-devel/e401d119-402e-0edd-
> >     <https://lore.kernel.org/qemu-devel/e401d119-402e-0edd->
> >     >>>>>>>> c2bf-28950ba48ccb@kaod.org/ <http://
> >     c2bf-28950ba48ccb@kaod.org/>
> >     >>>
> >     >>> [*]
> >     >>>
> >     >>>>>>>
> >     >>>>>>> Digging further:
> >     >>>>>>> https://lore.kernel.org/qemu- <https://lore.kernel.org/qemu-
> >
> >     >>>>>>>
> >     devel/9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/
> >     <
> http://9046a4327336d4425f1e7e7a973edef9e9948e80.camel@pengutronix.de/>
> >     >>>>>>>
> >     >>>>>>
> >     >>>>>> yes commit c078298301a8 might have some impact there.
> >     >>>>>
> >     >>>>> With Jan patch, your script doesn't need anymore the
> >     >>>>>
> >     >>>>>    echo "Fixing size to keep qemu happy..."
> >     >>>>>
> >     >>>>> kludge.
> >     >>>>
> >     >>>> which script ?
> >     >>>
> >     >>> The one you pasted in [*]:
> >     >>>
> >     >>> --
> >     >>> #!/bin/sh
> >     >>>
> >     >>> URLBASE=https://jenkins.openbmc.org/view/latest/job/latest-
> >     master/ <https://jenkins.openbmc.org/view/latest/job/latest-master/>
> >     >>>
> label=docker-builder,target=witherspoon-tacoma/lastSuccessfulBuild/
> >     >>> artifact/openbmc/build/tmp/deploy/images/witherspoon-tacoma/
> >     >>>
> >     >>> IMAGESIZE=128
> >     >>> OUTFILE=mmc.img
> >     >>>
> >     >>> FILES="u-boot.bin u-boot-spl.bin obmc-phosphor-image-witherspoon-
> >     >>> tacoma.wic.xz"
> >     >>>
> >     >>> for file in ${FILES}; do
> >     >>>
> >     >>>      if test -f ${file}; then
> >     >>>          echo "${file}: Already downloaded"
> >     >>>      else
> >     >>>          echo "${file}: Downloading"
> >     >>>          wget -nv ${URLBASE}/${file}
> >     >>>      fi
> >     >>> done
> >     >>>
> >     >>> echo
> >     >>>
> >     >>> echo "Creating empty image..."
> >     >>> dd status=none if=/dev/zero of=${OUTFILE} bs=1M
> count=${IMAGESIZE}
> >     >>> echo "Adding SPL..."
> >     >>> dd status=none if=u-boot-spl.bin of=${OUTFILE} conv=notrunc
> >     >>> echo "Adding u-boot..."
> >     >>> dd status=none if=u-boot.bin of=${OUTFILE} conv=notrunc bs=1K
> >     seek=64
> >     >>> echo "Adding userdata..."
> >     >>> unxz -c obmc-phosphor-image-witherspoon-tacoma.wic.xz | dd
> >     >>> status=progress of=${OUTFILE} conv=notrunc bs=1M seek=2
> >     >>> echo "Fixing size to keep qemu happy..."
> >     >>> truncate --size 16G ${OUTFILE}
> >     >>>
> >     >>> echo "Done!"
> >     >>> echo
> >     >>> echo " qemu-system-arm -M tacoma-bmc -nographic -drive
> >     >>> file=mmc.img,if=sd,index=2,format=raw"
> >     >>> ---
> >     >>
> >     >> FTR the alignment check was added to shut up fuzzed CVEs in commit
> >     >> a9bcedd15a5 ("hw/sd/sdcard: Do not allow invalid SD card sizes"):
> >     >>
> >     >>     QEMU allows to create SD card with unrealistic sizes. This
> could
> >     >>     work, but some guests (at least Linux) consider sizes that
> >     are not
> >     >>     a power of 2 as a firmware bug and fix the card size to the
> next
> >     >>     power of 2.
> >     >>
> >     >>     While the possibility to use small SD card images has been
> >     seen as
> >     >>     a feature, it became a bug with CVE-2020-13253, where the
> >     guest is
> >     >>     able to do OOB read/write accesses past the image size end.
> >     >>
> >     >>     In a pair of commits we will fix CVE-2020-13253 as:
> >     >>
> >     >>         Read command is rejected if BLOCK_LEN_ERROR or
> ADDRESS_ERROR
> >     >>         occurred and no data transfer is performed.
> >     >>
> >     >>         Write command is rejected if BLOCK_LEN_ERROR or
> ADDRESS_ERROR
> >     >>         occurred and no data transfer is performed.
> >     >>
> >     >>         WP_VIOLATION errors are not modified: the error bit is
> >     set, we
> >     >>         stay in receive-data state, wait for a stop command. All
> >     further
> >     >>         data transfer is ignored. See the check on
> sd->card_status at
> >     >>         the beginning of sd_read_data() and sd_write_data().
> >     >>
> >     >>     While this is the correct behavior, in case QEMU create
> >     smaller SD
> >     >>     cards, guests still try to access past the image size end,
> >     and QEMU
> >     >>     considers this is an invalid address, thus "all further data
> >     >>     transfer is ignored". This is wrong and make the guest
> >     looping until
> >     >>     eventually timeouts.
> >     >>
> >     >>     Fix by not allowing invalid SD card sizes (suggesting the
> >     expected
> >     >>     size as a hint):
> >     >>
> >     >>       $ qemu-system-arm -M orangepi-pc -drive
> >     >> file=rootfs.ext2,if=sd,format=raw
> >     >>       qemu-system-arm: Invalid SD card size: 60 MiB
> >     >>       SD card size has to be a power of 2, e.g. 64 MiB.
> >     >>       You can resize disk images with 'qemu-img resize
> >     <imagefile> <new-
> >     >> size>'
> >     >>       (note that this will lose data if you make the image
> >     smaller than
> >     >> it currently is).
> >     >>
> >     >>
> >     >> I expect us to be safe and able to deal with non-pow2 regions if
> >     we use
> >     >> QEMUSGList from the "system/dma.h" API. But this is a rework
> >     nobody had
> >     >> time to do so far.
> >     >
> >     > We have to tell two things apart: partitions sizes on the one side
> and
> >     > backing storage sizes. The partitions sizes are (to my reading)
> >     clearly
> >     > defined in the spec, and the user partition (alone!) has to be
> >     power of
> >     > 2. The boot and RPMB partitions are multiples of 128K. The sum of
> them
> >     > all is nowhere limited to power of 2 or even only multiples of
> 128K.
> >     >
> >
> >     Re-reading the part of the device capacity, the rules are more
> complex:
> >      - power of two up to 2 GB
> >      - multiple of 512 bytes beyond that
> >
> >
> > Kinda. It is power of 2 up to 2GiB, but there were a number of 4GiB cards
> > that were not high capacity cards that encoded their size and were
> otherwise
> > low capacity cards. Qemu doesn't need to support that. Its existing
> capacity
> > check should be enough.
> >
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index da5bdd134a..18b3f93965 100644
> > --- a/hw/sd/sd.c
> > +++ b/hw/sd/sd.c
> > @@ -2151,7 +2151,7 @@ static void sd_realize(DeviceState *dev, Error
> **errp)
> >          }
> >
> >          blk_size = blk_getlength(sd->blk);
> > -        if (blk_size > 0 && !is_power_of_2(blk_size)) {
> > +        if (blk_size > 0 && (blksize < SDSC_MAX_CAPACITY && !
> > is_power_of_2(blk_size)) {
>
> Close but not yet correct: <=
>

Ah, right.


> I have a patch under test for staging that also enforces the 512-byte rule.
>

Yes. But  that rule only applies to MMC cards and not SD cards which have a
512k rule since they don't have the EXT_CSD that MMC cards have. The qemu
stack supports both kinds of cards. But I've not traced through app the
ACMD support to see if we record this difference or not.

Warner


> Jan
>
> >              int64_t blk_size_aligned = pow2ceil(blk_size);
> >              char *blk_size_str;
> >
> > is what I'm running with, but it should have a second check for 512k
> > size if not an ext_csd situation.
> >
> > High capacity cards, though have a limitation where it's the number of
> > 1024 sectors (which are 512 bytes), so the limit is 512k. It encodes the
> > CSD differently than normal capacity cards. Thankfully, we have this in
> > our code already.
> >
> > And really high capacity cards have an extended structure the size of
> > the card is reported in, and that appears to be in sectors.
> >
> >
> >     So that power-of-two enforcement was and still is likely too strict.
> >
> >
> > Agreed.
> >
> > Warner
> >
> >
> >     But I still see no indication, neither in the existing eMMC code of
> QEMU
> >     nor the spec, that the boot and RPMB partition sizes are included in
> >     that.
> >
> >     Jan
> >
> >     --
> >     Siemens AG, Foundational Technologies
> >     Linux Expert Center
> >
>
> --
> Siemens AG, Foundational Technologies
> Linux Expert Center
>

[-- Attachment #2: Type: text/html, Size: 21410 bytes --]

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 8649 bytes --]

On Tue, Sep 2, 2025 at 11:53 AM Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 02.09.25 19:48, Warner Losh wrote:
> >
> >
> > On Tue, Sep 2, 2025 at 11:37 AM Jan Kiszka <jan.kiszka@siemens.com
> > <mailto:jan.kiszka@siemens.com>> wrote:
> >
> >     On 02.09.25 19:30, Warner Losh wrote:
> >     >
> >     >
> >     > On Tue, Sep 2, 2025 at 11:22 AM Warner Losh <imp@bsdimp.com
> >     <mailto:imp@bsdimp.com>
> >     > <mailto:imp@bsdimp.com <mailto:imp@bsdimp.com>>> wrote:
> >     >
> >     >
> >     >
> >     >     On Tue, Sep 2, 2025 at 11:18 AM Jan Kiszka
> >     <jan.kiszka@siemens.com <mailto:jan.kiszka@siemens.com>
> >     >     <mailto:jan.kiszka@siemens.com
> >     <mailto:jan.kiszka@siemens.com>>> wrote:
> >     >
> >     >         On 02.09.25 19:07, Warner Losh wrote:
> >     >         >
> >     >         >
> >     >         > On Tue, Sep 2, 2025 at 10:49 AM Jan Lübbe
> >     <jlu@pengutronix.de <mailto:jlu@pengutronix.de>
> >     >         <mailto:jlu@pengutronix.de <mailto:jlu@pengutronix.de>>
> >     >         > <mailto:jlu@pengutronix.de <mailto:jlu@pengutronix.de>
> >     <mailto:jlu@pengutronix.de <mailto:jlu@pengutronix.de>>>> wrote:
> >     >         >
> >     >         >     On Tue, 2025-09-02 at 18:39 +0200, Jan Kiszka wrote:
> >     >         >     > > > I expect us to be safe and able to deal with
> non-
> >     >         pow2 regions
> >     >         >     if we use
> >     >         >     > > > QEMUSGList from the "system/dma.h" API. But
> >     this is
> >     >         a rework
> >     >         >     nobody had
> >     >         >     > > > time to do so far.
> >     >         >     > >
> >     >         >     > > We have to tell two things apart: partitions
> >     sizes on
> >     >         the one
> >     >         >     side and
> >     >         >     > > backing storage sizes. The partitions sizes are
> >     (to my
> >     >         reading)
> >     >         >     clearly
> >     >         >     > > defined in the spec, and the user partition
> (alone!)
> >     >         has to be
> >     >         >     power of
> >     >         >     > > 2. The boot and RPMB partitions are multiples of
> >     128K.
> >     >         The sum
> >     >         >     of them
> >     >         >     > > all is nowhere limited to power of 2 or even only
> >     >         multiples of 128K.
> >     >         >     > >
> >     >         >     >
> >     >         >     > Re-reading the part of the device capacity, the
> rules
> >     >         are more
> >     >         >     complex:
> >     >         >     >  - power of two up to 2 GB
> >     >         >     >  - multiple of 512 bytes beyond that
> >     >         >     >
> >     >         >     > So that power-of-two enforcement was and still is
> >     likely
> >     >         too strict.
> >     >         >
> >     >         >
> >     >         > It is. Version 0 (and MMC) cards had the capacity
> >     encoded like so:
> >     >         >                 m = mmc_get_bits(raw_csd, 128, 62, 12);
> >     >         >                 e = mmc_get_bits(raw_csd, 128, 47, 3);
> >     >         >                 csd->capacity = ((1 + m) << (e + 2)) *
> csd-
> >     >         >read_bl_len;
> >     >         > so any card less than 2GB (well, technically 4GB, but 4GB
> >     >         version 0
> >     >         > cards were
> >     >         > rare and broke some stacks... I have one and I love it
> on my
> >     >         embedded
> >     >         > ARM board
> >     >         > that can't do version 1 cards). Version 1 cards encoded
> >     it like:
> >     >         >                 csd->capacity =
> >     >         ((uint64_t)mmc_get_bits(raw_csd, 128,
> >     >         > 48, 22) +
> >     >         >                     1) * 512 * 1024;
> >     >         > So it's a multiple of 512k. These are also called 'high
> >     >         capacity' cards.
> >     >         >
> >     >         > Version 4 introduces an extended CSD, which had a pure
> >     sector
> >     >         count in
> >     >         > the EXT CSD. I think this
> >     >         > is only for MMC cards. And also the partition
> information.
> >     >         >
> >     >         >
> >     >         >     > But I still see no indication, neither in the
> existing
> >     >         eMMC code
> >     >         >     of QEMU
> >     >         >     > nor the spec, that the boot and RPMB partition
> >     sizes are
> >     >         included
> >     >         >     in that.
> >     >         >
> >     >         >     Correct. Non-power-of-two sizes are very common for
> real
> >     >         eMMCs.
> >     >         >     Taking a random
> >     >         >     one from our lab:
> >     >         >     [    1.220588] mmcblk1: mmc1:0001 S0J56X 14.8 GiB
> >     >         >     [    1.228055]  mmcblk1: p1 p2 p3 p4
> >     >         >     [    1.230375] mmcblk1boot0: mmc1:0001 S0J56X 31.5
> MiB
> >     >         >     [    1.233651] mmcblk1boot1: mmc1:0001 S0J56X 31.5
> MiB
> >     >         >     [    1.236682] mmcblk1rpmb: mmc1:0001 S0J56X 4.00
> MiB,
> >     >         chardev (244:0)
> >     >         >
> >     >         >     For eMMCs using MLC NAND, you can also configure
> part of
> >     >         the user
> >     >         >     data area to
> >     >         >     be pSLC (pseudo single level cell), which changes the
> >     >         available
> >     >         >     capacity (after
> >     >         >     a required power cycle).
> >     >         >
> >     >         >
> >     >         > Yes. Extended partitions are a feature of version 4
> >     cards, so
> >     >         don't have
> >     >         > power-of-2 limits since they are a pure sector count in
> the
> >     >         ext_csd.
> >     >         >
> >     >
> >     >         JESD84-B51A (eMMC 5.1A):
> >     >
> >     >         "The C_SIZE parameter is used to compute the device
> >     capacity for
> >     >         devices
> >     >         up to 2 GB of density. See 7.4.52, SEC_COUNT [215:212] ,
> for
> >     >         details on
> >     >         calculating densities greater than 2 GB."
> >     >
> >     >         So I would now continue to enforce power-of-2 for 2G
> >     (including)
> >     >         cards,
> >     >         and relax to multiples of 512 for larger ones.
> >     >
> >     >
> >     >     It's a multiple of 512k unless the card has a ext_csd, in
> >     which case
> >     >     it's a multiple of 512.
> >     >
> >     >
> >     > More completely, this is from MMC 4.0 and newer. Extended Capacity
> SD
> >     > cards report this in units of 512k bytes for all cards > 2GiB.
> >     >
> >
> >     I'm not sure which spec version you are referring to, but JESD84-A441
> >     and JESD84-B51A mention nothing about 512K, rather "Device density =
> >     SEC_COUNT x 512B". And these are the specs we very likely need to
> follow
> >     here.
> >
> >
> > You are right that this is in the MMC spec. However, the SD spec is
> > controlling for SD cards.
> >
> > SD Specifications Part 1 Physical Layer Simplified Specification Version
> > 9.10
> > December 1, 2023
> >
> > Section 5.3 describes the CSD. Version 1.0 (which I'd called version 0
> > in an earlier email because of its encoding) is the 2GB rule. Version
>
> < 2G or <= 2G? For eMMC, it is <=.
>
> > 2.0 and 3.0 encode it as 512k count (from 5.3.3):
> >
> > C_SIZE
> > This field is expanded to 28 bits and can indicate up to 128 TBytes.
> >
> > This parameter is used to calculate the user data area capacity in the
> > SD memory card (note that size of the protected area is zero for SDUC
> > card). The user data area capacity is calculated from C_SIZE as follows:
> >
> > memory capacity = (C_SIZE+1) * 512KByte
> >
> > The Minimum user area size of SDUC Card is 4,294,968,320 sectors
> > (2TB+0.5MB).
> > The Minimum value of C_SIZE for SDUC in CSD Version 3.0 is 0400000h
> > (4194304). The Maximum user area size of SDUC Card is 274,877,906,944
> > sectors (128TB).
> > The Maximum value of C_SIZE for SDUC in CSD Version 3.0 is FFFFFFFh
> > (268435455).
> >
> > So SD cards are yet again gratuitously different than MMC cards.
> >
>
> Argh, then we need to take the card type into account as well. Need to
> rework my patch...
>

Sorry to be the bearer of bad news... I stubbed my toe on this when I wrote
the FreeBSD stack 15 years ago and the experience is memorable, even after
all this time.

Warner

[-- Attachment #2: Type: text/html, Size: 13076 bytes --]

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Philippe Mathieu-Daudé]

On 2/9/25 19:53, Jan Kiszka wrote:
> On 02.09.25 19:48, Warner Losh wrote:
>>
>>
>> On Tue, Sep 2, 2025 at 11:37 AM Jan Kiszka <jan.kiszka@siemens.com
>> <mailto:jan.kiszka@siemens.com>> wrote:
>>
>>      On 02.09.25 19:30, Warner Losh wrote:
>>      >
>>      >
>>      > On Tue, Sep 2, 2025 at 11:22 AM Warner Losh <imp@bsdimp.com
>>      <mailto:imp@bsdimp.com>
>>      > <mailto:imp@bsdimp.com <mailto:imp@bsdimp.com>>> wrote:
>>      >
>>      >
>>      >
>>      >     On Tue, Sep 2, 2025 at 11:18 AM Jan Kiszka
>>      <jan.kiszka@siemens.com <mailto:jan.kiszka@siemens.com>
>>      >     <mailto:jan.kiszka@siemens.com
>>      <mailto:jan.kiszka@siemens.com>>> wrote:
>>      >
>>      >         On 02.09.25 19:07, Warner Losh wrote:
>>      >         >
>>      >         >
>>      >         > On Tue, Sep 2, 2025 at 10:49 AM Jan Lübbe
>>      <jlu@pengutronix.de <mailto:jlu@pengutronix.de>
>>      >         <mailto:jlu@pengutronix.de <mailto:jlu@pengutronix.de>>
>>      >         > <mailto:jlu@pengutronix.de <mailto:jlu@pengutronix.de>
>>      <mailto:jlu@pengutronix.de <mailto:jlu@pengutronix.de>>>> wrote:
>>      >         >
>>      >         >     On Tue, 2025-09-02 at 18:39 +0200, Jan Kiszka wrote:
>>      >         >     > > > I expect us to be safe and able to deal with non-
>>      >         pow2 regions
>>      >         >     if we use
>>      >         >     > > > QEMUSGList from the "system/dma.h" API. But
>>      this is
>>      >         a rework
>>      >         >     nobody had
>>      >         >     > > > time to do so far.
>>      >         >     > >
>>      >         >     > > We have to tell two things apart: partitions
>>      sizes on
>>      >         the one
>>      >         >     side and
>>      >         >     > > backing storage sizes. The partitions sizes are
>>      (to my
>>      >         reading)
>>      >         >     clearly
>>      >         >     > > defined in the spec, and the user partition (alone!)
>>      >         has to be
>>      >         >     power of
>>      >         >     > > 2. The boot and RPMB partitions are multiples of
>>      128K.
>>      >         The sum
>>      >         >     of them
>>      >         >     > > all is nowhere limited to power of 2 or even only
>>      >         multiples of 128K.
>>      >         >     > >
>>      >         >     >
>>      >         >     > Re-reading the part of the device capacity, the rules
>>      >         are more
>>      >         >     complex:
>>      >         >     >  - power of two up to 2 GB
>>      >         >     >  - multiple of 512 bytes beyond that
>>      >         >     >
>>      >         >     > So that power-of-two enforcement was and still is
>>      likely
>>      >         too strict.
>>      >         >
>>      >         >
>>      >         > It is. Version 0 (and MMC) cards had the capacity
>>      encoded like so:
>>      >         >                 m = mmc_get_bits(raw_csd, 128, 62, 12);
>>      >         >                 e = mmc_get_bits(raw_csd, 128, 47, 3);
>>      >         >                 csd->capacity = ((1 + m) << (e + 2)) * csd-
>>      >         >read_bl_len;
>>      >         > so any card less than 2GB (well, technically 4GB, but 4GB
>>      >         version 0
>>      >         > cards were
>>      >         > rare and broke some stacks... I have one and I love it on my
>>      >         embedded
>>      >         > ARM board
>>      >         > that can't do version 1 cards). Version 1 cards encoded
>>      it like:
>>      >         >                 csd->capacity =
>>      >         ((uint64_t)mmc_get_bits(raw_csd, 128,
>>      >         > 48, 22) +
>>      >         >                     1) * 512 * 1024;
>>      >         > So it's a multiple of 512k. These are also called 'high
>>      >         capacity' cards.
>>      >         >
>>      >         > Version 4 introduces an extended CSD, which had a pure
>>      sector
>>      >         count in
>>      >         > the EXT CSD. I think this
>>      >         > is only for MMC cards. And also the partition information.
>>      >         >
>>      >         >
>>      >         >     > But I still see no indication, neither in the existing
>>      >         eMMC code
>>      >         >     of QEMU
>>      >         >     > nor the spec, that the boot and RPMB partition
>>      sizes are
>>      >         included
>>      >         >     in that.
>>      >         >
>>      >         >     Correct. Non-power-of-two sizes are very common for real
>>      >         eMMCs.
>>      >         >     Taking a random
>>      >         >     one from our lab:
>>      >         >     [    1.220588] mmcblk1: mmc1:0001 S0J56X 14.8 GiB
>>      >         >     [    1.228055]  mmcblk1: p1 p2 p3 p4
>>      >         >     [    1.230375] mmcblk1boot0: mmc1:0001 S0J56X 31.5 MiB
>>      >         >     [    1.233651] mmcblk1boot1: mmc1:0001 S0J56X 31.5 MiB
>>      >         >     [    1.236682] mmcblk1rpmb: mmc1:0001 S0J56X 4.00 MiB,
>>      >         chardev (244:0)
>>      >         >
>>      >         >     For eMMCs using MLC NAND, you can also configure part of
>>      >         the user
>>      >         >     data area to
>>      >         >     be pSLC (pseudo single level cell), which changes the
>>      >         available
>>      >         >     capacity (after
>>      >         >     a required power cycle).
>>      >         >
>>      >         >
>>      >         > Yes. Extended partitions are a feature of version 4
>>      cards, so
>>      >         don't have
>>      >         > power-of-2 limits since they are a pure sector count in the
>>      >         ext_csd.
>>      >         >
>>      >
>>      >         JESD84-B51A (eMMC 5.1A):
>>      >
>>      >         "The C_SIZE parameter is used to compute the device
>>      capacity for
>>      >         devices
>>      >         up to 2 GB of density. See 7.4.52, SEC_COUNT [215:212] , for
>>      >         details on
>>      >         calculating densities greater than 2 GB."
>>      >
>>      >         So I would now continue to enforce power-of-2 for 2G
>>      (including)
>>      >         cards,
>>      >         and relax to multiples of 512 for larger ones.
>>      >
>>      >
>>      >     It's a multiple of 512k unless the card has a ext_csd, in
>>      which case
>>      >     it's a multiple of 512.
>>      >
>>      >
>>      > More completely, this is from MMC 4.0 and newer. Extended Capacity SD
>>      > cards report this in units of 512k bytes for all cards > 2GiB.
>>      >
>>
>>      I'm not sure which spec version you are referring to, but JESD84-A441
>>      and JESD84-B51A mention nothing about 512K, rather "Device density =
>>      SEC_COUNT x 512B". And these are the specs we very likely need to follow
>>      here.
>>
>>
>> You are right that this is in the MMC spec. However, the SD spec is
>> controlling for SD cards.
>>
>> SD Specifications Part 1 Physical Layer Simplified Specification Version
>> 9.10
>> December 1, 2023
>>
>> Section 5.3 describes the CSD. Version 1.0 (which I'd called version 0
>> in an earlier email because of its encoding) is the 2GB rule. Version
> 
> < 2G or <= 2G? For eMMC, it is <=.
> 
>> 2.0 and 3.0 encode it as 512k count (from 5.3.3):
>>
>> C_SIZE
>> This field is expanded to 28 bits and can indicate up to 128 TBytes.
>>
>> This parameter is used to calculate the user data area capacity in the
>> SD memory card (note that size of the protected area is zero for SDUC
>> card). The user data area capacity is calculated from C_SIZE as follows:
>>
>> memory capacity = (C_SIZE+1) * 512KByte
>>
>> The Minimum user area size of SDUC Card is 4,294,968,320 sectors
>> (2TB+0.5MB).
>> The Minimum value of C_SIZE for SDUC in CSD Version 3.0 is 0400000h
>> (4194304). The Maximum user area size of SDUC Card is 274,877,906,944
>> sectors (128TB).
>> The Maximum value of C_SIZE for SDUC in CSD Version 3.0 is FFFFFFFh
>> (268435455).
>>
>> So SD cards are yet again gratuitously different than MMC cards.

FTR so far QEMU only models SD spec v2.00 and v3.01, and eMMC spec 4.3.

Re: [PATCH v2 1/8] hw/sd/sdcard: Fix size check for backing block image

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 9375 bytes --]

On Tue, Sep 2, 2025 at 11:59 AM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> On 2/9/25 19:53, Jan Kiszka wrote:
> > On 02.09.25 19:48, Warner Losh wrote:
> >>
> >>
> >> On Tue, Sep 2, 2025 at 11:37 AM Jan Kiszka <jan.kiszka@siemens.com
> >> <mailto:jan.kiszka@siemens.com>> wrote:
> >>
> >>      On 02.09.25 19:30, Warner Losh wrote:
> >>      >
> >>      >
> >>      > On Tue, Sep 2, 2025 at 11:22 AM Warner Losh <imp@bsdimp.com
> >>      <mailto:imp@bsdimp.com>
> >>      > <mailto:imp@bsdimp.com <mailto:imp@bsdimp.com>>> wrote:
> >>      >
> >>      >
> >>      >
> >>      >     On Tue, Sep 2, 2025 at 11:18 AM Jan Kiszka
> >>      <jan.kiszka@siemens.com <mailto:jan.kiszka@siemens.com>
> >>      >     <mailto:jan.kiszka@siemens.com
> >>      <mailto:jan.kiszka@siemens.com>>> wrote:
> >>      >
> >>      >         On 02.09.25 19:07, Warner Losh wrote:
> >>      >         >
> >>      >         >
> >>      >         > On Tue, Sep 2, 2025 at 10:49 AM Jan Lübbe
> >>      <jlu@pengutronix.de <mailto:jlu@pengutronix.de>
> >>      >         <mailto:jlu@pengutronix.de <mailto:jlu@pengutronix.de>>
> >>      >         > <mailto:jlu@pengutronix.de <mailto:jlu@pengutronix.de>
> >>      <mailto:jlu@pengutronix.de <mailto:jlu@pengutronix.de>>>> wrote:
> >>      >         >
> >>      >         >     On Tue, 2025-09-02 at 18:39 +0200, Jan Kiszka
> wrote:
> >>      >         >     > > > I expect us to be safe and able to deal with
> non-
> >>      >         pow2 regions
> >>      >         >     if we use
> >>      >         >     > > > QEMUSGList from the "system/dma.h" API. But
> >>      this is
> >>      >         a rework
> >>      >         >     nobody had
> >>      >         >     > > > time to do so far.
> >>      >         >     > >
> >>      >         >     > > We have to tell two things apart: partitions
> >>      sizes on
> >>      >         the one
> >>      >         >     side and
> >>      >         >     > > backing storage sizes. The partitions sizes are
> >>      (to my
> >>      >         reading)
> >>      >         >     clearly
> >>      >         >     > > defined in the spec, and the user partition
> (alone!)
> >>      >         has to be
> >>      >         >     power of
> >>      >         >     > > 2. The boot and RPMB partitions are multiples
> of
> >>      128K.
> >>      >         The sum
> >>      >         >     of them
> >>      >         >     > > all is nowhere limited to power of 2 or even
> only
> >>      >         multiples of 128K.
> >>      >         >     > >
> >>      >         >     >
> >>      >         >     > Re-reading the part of the device capacity, the
> rules
> >>      >         are more
> >>      >         >     complex:
> >>      >         >     >  - power of two up to 2 GB
> >>      >         >     >  - multiple of 512 bytes beyond that
> >>      >         >     >
> >>      >         >     > So that power-of-two enforcement was and still is
> >>      likely
> >>      >         too strict.
> >>      >         >
> >>      >         >
> >>      >         > It is. Version 0 (and MMC) cards had the capacity
> >>      encoded like so:
> >>      >         >                 m = mmc_get_bits(raw_csd, 128, 62, 12);
> >>      >         >                 e = mmc_get_bits(raw_csd, 128, 47, 3);
> >>      >         >                 csd->capacity = ((1 + m) << (e + 2)) *
> csd-
> >>      >         >read_bl_len;
> >>      >         > so any card less than 2GB (well, technically 4GB, but
> 4GB
> >>      >         version 0
> >>      >         > cards were
> >>      >         > rare and broke some stacks... I have one and I love it
> on my
> >>      >         embedded
> >>      >         > ARM board
> >>      >         > that can't do version 1 cards). Version 1 cards encoded
> >>      it like:
> >>      >         >                 csd->capacity =
> >>      >         ((uint64_t)mmc_get_bits(raw_csd, 128,
> >>      >         > 48, 22) +
> >>      >         >                     1) * 512 * 1024;
> >>      >         > So it's a multiple of 512k. These are also called 'high
> >>      >         capacity' cards.
> >>      >         >
> >>      >         > Version 4 introduces an extended CSD, which had a pure
> >>      sector
> >>      >         count in
> >>      >         > the EXT CSD. I think this
> >>      >         > is only for MMC cards. And also the partition
> information.
> >>      >         >
> >>      >         >
> >>      >         >     > But I still see no indication, neither in the
> existing
> >>      >         eMMC code
> >>      >         >     of QEMU
> >>      >         >     > nor the spec, that the boot and RPMB partition
> >>      sizes are
> >>      >         included
> >>      >         >     in that.
> >>      >         >
> >>      >         >     Correct. Non-power-of-two sizes are very common
> for real
> >>      >         eMMCs.
> >>      >         >     Taking a random
> >>      >         >     one from our lab:
> >>      >         >     [    1.220588] mmcblk1: mmc1:0001 S0J56X 14.8 GiB
> >>      >         >     [    1.228055]  mmcblk1: p1 p2 p3 p4
> >>      >         >     [    1.230375] mmcblk1boot0: mmc1:0001 S0J56X 31.5
> MiB
> >>      >         >     [    1.233651] mmcblk1boot1: mmc1:0001 S0J56X 31.5
> MiB
> >>      >         >     [    1.236682] mmcblk1rpmb: mmc1:0001 S0J56X 4.00
> MiB,
> >>      >         chardev (244:0)
> >>      >         >
> >>      >         >     For eMMCs using MLC NAND, you can also configure
> part of
> >>      >         the user
> >>      >         >     data area to
> >>      >         >     be pSLC (pseudo single level cell), which changes
> the
> >>      >         available
> >>      >         >     capacity (after
> >>      >         >     a required power cycle).
> >>      >         >
> >>      >         >
> >>      >         > Yes. Extended partitions are a feature of version 4
> >>      cards, so
> >>      >         don't have
> >>      >         > power-of-2 limits since they are a pure sector count
> in the
> >>      >         ext_csd.
> >>      >         >
> >>      >
> >>      >         JESD84-B51A (eMMC 5.1A):
> >>      >
> >>      >         "The C_SIZE parameter is used to compute the device
> >>      capacity for
> >>      >         devices
> >>      >         up to 2 GB of density. See 7.4.52, SEC_COUNT [215:212] ,
> for
> >>      >         details on
> >>      >         calculating densities greater than 2 GB."
> >>      >
> >>      >         So I would now continue to enforce power-of-2 for 2G
> >>      (including)
> >>      >         cards,
> >>      >         and relax to multiples of 512 for larger ones.
> >>      >
> >>      >
> >>      >     It's a multiple of 512k unless the card has a ext_csd, in
> >>      which case
> >>      >     it's a multiple of 512.
> >>      >
> >>      >
> >>      > More completely, this is from MMC 4.0 and newer. Extended
> Capacity SD
> >>      > cards report this in units of 512k bytes for all cards > 2GiB.
> >>      >
> >>
> >>      I'm not sure which spec version you are referring to, but
> JESD84-A441
> >>      and JESD84-B51A mention nothing about 512K, rather "Device density
> =
> >>      SEC_COUNT x 512B". And these are the specs we very likely need to
> follow
> >>      here.
> >>
> >>
> >> You are right that this is in the MMC spec. However, the SD spec is
> >> controlling for SD cards.
> >>
> >> SD Specifications Part 1 Physical Layer Simplified Specification Version
> >> 9.10
> >> December 1, 2023
> >>
> >> Section 5.3 describes the CSD. Version 1.0 (which I'd called version 0
> >> in an earlier email because of its encoding) is the 2GB rule. Version
> >
> > < 2G or <= 2G? For eMMC, it is <=.
> >
> >> 2.0 and 3.0 encode it as 512k count (from 5.3.3):
> >>
> >> C_SIZE
> >> This field is expanded to 28 bits and can indicate up to 128 TBytes.
> >>
> >> This parameter is used to calculate the user data area capacity in the
> >> SD memory card (note that size of the protected area is zero for SDUC
> >> card). The user data area capacity is calculated from C_SIZE as follows:
> >>
> >> memory capacity = (C_SIZE+1) * 512KByte
> >>
> >> The Minimum user area size of SDUC Card is 4,294,968,320 sectors
> >> (2TB+0.5MB).
> >> The Minimum value of C_SIZE for SDUC in CSD Version 3.0 is 0400000h
> >> (4194304). The Maximum user area size of SDUC Card is 274,877,906,944
> >> sectors (128TB).
> >> The Maximum value of C_SIZE for SDUC in CSD Version 3.0 is FFFFFFFh
> >> (268435455).
> >>
> >> So SD cards are yet again gratuitously different than MMC cards.
>
> FTR so far QEMU only models SD spec v2.00 and v3.01, and eMMC spec 4.3.
>

IIRC (I can't find the old copies of the spec I used), the SD Spec 2.0
introduced the 512k thing with its new CSD for cards larger than 2GiB. I
had to retrofit my stack, which I'd written to the SD 1.0 spec for it.
Later versions (I'm not sure which ones) expanded the field size to what I
aquoted. eMMC introduced EXT_CSD in version 4.0, which has the 512 byte
restriction and also partitions (since MMC didn't introduce a new CSD like
SD did). The successor eMMC spec is what Jan was quoting.

Warner

[-- Attachment #2: Type: text/html, Size: 14345 bytes --]