| Message ID | 20201025152357.11865-1-bmeng.cn@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/sd: Fix 2 GiB card CSD register values | expand |
On 10/25/20 4:23 PM, Bin Meng wrote: > From: Bin Meng <bin.meng@windriver.com> > > Per the SD spec, to indicate a 2 GiB card, BLOCK_LEN shall be 1024 > bytes, hence the READ_BL_LEN field in the CSD register shall be 10 > instead of 9. > > This fixes the acceptance test error for the NetBSD 9.0 test of the > Orange Pi PC that has an expanded SD card image of 2 GiB size. > > Fixes: 6d2d4069c47e ("hw/sd: Correct the maximum size of a Standard Capacity SD Memory Card") > Reported-by: Niek Linnenbank <nieklinnenbank@gmail.com> > Signed-off-by: Bin Meng <bin.meng@windriver.com> > --- > > hw/sd/sd.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index bd10ec8fc4..732fcb5ff0 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -386,10 +386,17 @@ static const uint8_t sd_csd_rw_mask[16] = { > > static void sd_set_csd(SDState *sd, uint64_t size) > { > - uint32_t csize = (size >> (CMULT_SHIFT + HWBLOCK_SHIFT)) - 1; > + int hwblock_shift = HWBLOCK_SHIFT; > + uint32_t csize; > uint32_t sectsize = (1 << (SECTOR_SHIFT + 1)) - 1; > uint32_t wpsize = (1 << (WPGROUP_SHIFT + 1)) - 1; > > + /* To indicate 2 GiB card, BLOCK_LEN shall be 1024 bytes */ > + if (size == SDSC_MAX_CAPACITY) { > + hwblock_shift += 1; This is going in the good direction, however now we have an huge security hole, as SDState::data[] is 512 bytes, and you announce the guest it can use 1024 bytes. See sd_blk_read() and sd_blk_write(). Now SDState::data[] is migrated, so this isn't an easy field to modify without breaking compatibility again :( I've been working on a more robust approach today, doing some cleanup first. I'll send it during the next days hopefully. > + } > + csize = (size >> (CMULT_SHIFT + hwblock_shift)) - 1; > + > if (size <= SDSC_MAX_CAPACITY) { /* Standard Capacity SD */ > sd->csd[0] = 0x00; /* CSD structure */ > sd->csd[1] = 0x26; /* Data read access-time-1 */ > @@ -397,7 +404,7 @@ static void sd_set_csd(SDState *sd, uint64_t size) > sd->csd[3] = 0x32; /* Max. data transfer rate: 25 MHz */ > sd->csd[4] = 0x5f; /* Card Command Classes */ > sd->csd[5] = 0x50 | /* Max. read data block length */ > - HWBLOCK_SHIFT; > + hwblock_shift; > sd->csd[6] = 0xe0 | /* Partial block for read allowed */ > ((csize >> 10) & 0x03); > sd->csd[7] = 0x00 | /* Device size */ > @@ -411,9 +418,9 @@ static void sd_set_csd(SDState *sd, uint64_t size) > sd->csd[11] = 0x00 | /* Write protect group size */ > ((sectsize << 7) & 0x80) | wpsize; > sd->csd[12] = 0x90 | /* Write speed factor */ > - (HWBLOCK_SHIFT >> 2); > + (hwblock_shift >> 2); > sd->csd[13] = 0x20 | /* Max. write data block length */ > - ((HWBLOCK_SHIFT << 6) & 0xc0); > + ((hwblock_shift << 6) & 0xc0); > sd->csd[14] = 0x00; /* File format group */ > } else { /* SDHC */ > size /= 512 * KiB; >
On 10/25/20 7:56 PM, Philippe Mathieu-Daudé wrote: > On 10/25/20 4:23 PM, Bin Meng wrote: >> From: Bin Meng <bin.meng@windriver.com> >> >> Per the SD spec, to indicate a 2 GiB card, BLOCK_LEN shall be 1024 >> bytes, hence the READ_BL_LEN field in the CSD register shall be 10 >> instead of 9. >> >> This fixes the acceptance test error for the NetBSD 9.0 test of the >> Orange Pi PC that has an expanded SD card image of 2 GiB size. >> >> Fixes: 6d2d4069c47e ("hw/sd: Correct the maximum size of a Standard >> Capacity SD Memory Card") >> Reported-by: Niek Linnenbank <nieklinnenbank@gmail.com> >> Signed-off-by: Bin Meng <bin.meng@windriver.com> >> --- >> >> hw/sd/sd.c | 15 +++++++++++---- >> 1 file changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/hw/sd/sd.c b/hw/sd/sd.c >> index bd10ec8fc4..732fcb5ff0 100644 >> --- a/hw/sd/sd.c >> +++ b/hw/sd/sd.c >> @@ -386,10 +386,17 @@ static const uint8_t sd_csd_rw_mask[16] = { >> static void sd_set_csd(SDState *sd, uint64_t size) >> { >> - uint32_t csize = (size >> (CMULT_SHIFT + HWBLOCK_SHIFT)) - 1; >> + int hwblock_shift = HWBLOCK_SHIFT; >> + uint32_t csize; >> uint32_t sectsize = (1 << (SECTOR_SHIFT + 1)) - 1; >> uint32_t wpsize = (1 << (WPGROUP_SHIFT + 1)) - 1; >> + /* To indicate 2 GiB card, BLOCK_LEN shall be 1024 bytes */ >> + if (size == SDSC_MAX_CAPACITY) { >> + hwblock_shift += 1; > > This is going in the good direction, however now we have an huge > security hole, as SDState::data[] is 512 bytes, and you announce the > guest it can use 1024 bytes. See sd_blk_read() and sd_blk_write(). > > Now SDState::data[] is migrated, so this isn't an easy field to > modify without breaking compatibility again :( OMG see commit 12c125cba9c ("sd: Switch to byte-based block access")... We have 512 bytes available just after the 512 data[] in the migration stream!!! How can we be that lucky? =) > > I've been working on a more robust approach today, doing some cleanup > first. I'll send it during the next days hopefully. > >> + } >> + csize = (size >> (CMULT_SHIFT + hwblock_shift)) - 1; >> + >> if (size <= SDSC_MAX_CAPACITY) { /* Standard Capacity SD */ >> sd->csd[0] = 0x00; /* CSD structure */ >> sd->csd[1] = 0x26; /* Data read access-time-1 */ >> @@ -397,7 +404,7 @@ static void sd_set_csd(SDState *sd, uint64_t size) >> sd->csd[3] = 0x32; /* Max. data transfer rate: 25 MHz */ >> sd->csd[4] = 0x5f; /* Card Command Classes */ >> sd->csd[5] = 0x50 | /* Max. read data block length */ >> - HWBLOCK_SHIFT; >> + hwblock_shift; >> sd->csd[6] = 0xe0 | /* Partial block for read allowed */ >> ((csize >> 10) & 0x03); >> sd->csd[7] = 0x00 | /* Device size */ >> @@ -411,9 +418,9 @@ static void sd_set_csd(SDState *sd, uint64_t size) >> sd->csd[11] = 0x00 | /* Write protect group size */ >> ((sectsize << 7) & 0x80) | wpsize; >> sd->csd[12] = 0x90 | /* Write speed factor */ >> - (HWBLOCK_SHIFT >> 2); >> + (hwblock_shift >> 2); >> sd->csd[13] = 0x20 | /* Max. write data block length */ >> - ((HWBLOCK_SHIFT << 6) & 0xc0); >> + ((hwblock_shift << 6) & 0xc0); >> sd->csd[14] = 0x00; /* File format group */ >> } else { /* SDHC */ >> size /= 512 * KiB; >> >
Hello Bin, On Sun, Oct 25, 2020 at 4:25 PM Bin Meng <bmeng.cn@gmail.com> wrote: > From: Bin Meng <bin.meng@windriver.com> > > Per the SD spec, to indicate a 2 GiB card, BLOCK_LEN shall be 1024 > bytes, hence the READ_BL_LEN field in the CSD register shall be 10 > instead of 9. > > This fixes the acceptance test error for the NetBSD 9.0 test of the > Orange Pi PC that has an expanded SD card image of 2 GiB size. > Thanks for your quick response on this. I re-ran all the avocado tests again with this patch added to philips series and now all tests are passing: (4/6) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic_19_11: -console: U-Boot SPL 2019.04-armbian (Nov 18 2019 - 23:08:35 +0100) console: DRAM: 1024 MiB console: Failed to set core voltage! Can't set CPU frequency console: Trying to boot from MMC1 console: U-Boot 2019.04-armbian (Nov 18 2019 - 23:08:35 +0100) Allwinner Technology console: CPU: Allwinner H3 (SUN8I 0000) ... console: [ OK ] Listening on Journal Socket. console: systemd[1]: Starting Create list of required static device nodes for the current kernel... console: Starting Create list of required st…ce nodes for the current kernel... console: systemd[1]: Starting Load Kernel Modules... PASS (51.09 s) (5/6) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic_20_08: \console: U-Boot SPL 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200) |console: DRAM: 1024 MiB console: Failed to set core voltage! Can't set CPU frequency console: Trying to boot from MMC1 console: U-Boot 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200) Allwinner Technology ... console: [ OK ] Listening on /dev/initctl Compatibility Named Pipe. console: [ OK ] Listening on Journal Audit Socket. console: Starting Load Kernel Modules... PASS (61.05 s) (6/6) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9: \console: U-Boot SPL 2020.01+dfsg-1 (Jan 08 2020 - 08:19:44 +0000) console: DRAM: 1024 MiB console: Failed to set core voltage! Can't set CPU frequency console: Trying to boot from MMC1 console: U-Boot 2020.01+dfsg-1 (Jan 08 2020 - 08:19:44 +0000) Allwinner Technology ... console: [ 3.6296962] root on ld0a dumps on ld0b console: [ 3.7250834] root file system type: ffs console: [ 3.7878476] kern.module.path=/stand/evbarm/9.0/modules -console: Sun Oct 25 23:08:43 UTC 2020 |console: Starting root file system check: PASS (21.12 s) RESULTS : PASS 6 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0 JOB TIME : 183.41 s I've also booted the Armbian 20.08 image manually and it starts up fine till the login/setup prompt: $ qemu-system-arm -M orangepi-pc -nographic -nic user -sd Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img ... WARNING: Image format was not specified for 'Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img' and probing guessed raw. Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted. Specify the 'raw' format explicitly to remove the restrictions. U-Boot SPL 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200) DRAM: 1024 MiB Failed to set core voltage! Can't set CPU frequency Trying to boot from MMC1 U-Boot 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200) Allwinner Technology CPU: Allwinner H3 (SUN8I 0000) Model: Xunlong Orange Pi PC ... Autoboot in 1 seconds, press <Space> to stop => setenv extraargs 'console=ttyS0,115200' => boot ... Uncompressing Linux... done, booting the kernel. Loading, please wait... starting version 237 Begin: Loading essential drivers ... done. Begin: Running /scripts/init-premount ... done. Begin: Mounting root file system ... Begin: Running /scripts/local-top ... done. Begin: Running /scripts/local-premount ... Scanning for Btrfs filesystems done. Begin: Will now check root file system ... fsck from util-linux 2.31.1 [/sbin/fsck.ext4 (1) -- /dev/mmcblk0p1] fsck.ext4 -a -C0 /dev/mmcblk0p1 /dev/mmcblk0p1: recovering journal /dev/mmcblk0p1: clean, 38118/117504 files, 230861/497048 blocks done. done. Begin: Running /scripts/local-bottom ... done. Begin: Running /scripts/init-bottom ... done. Welcome to Armbian 20.08.1 Bionic! ... [ OK ] Created slice system-getty.slice. [ OK ] Started Getty on tty1. [ OK ] Reached target Login Prompts. [ OK ] Started Network Manager Script Dispatcher Service. Armbian 20.08.1 Bionic ttyS0 orangepipc login:
Hi Philippe, On Mon, Oct 26, 2020 at 2:56 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 10/25/20 4:23 PM, Bin Meng wrote: > > From: Bin Meng <bin.meng@windriver.com> > > > > Per the SD spec, to indicate a 2 GiB card, BLOCK_LEN shall be 1024 > > bytes, hence the READ_BL_LEN field in the CSD register shall be 10 > > instead of 9. > > > > This fixes the acceptance test error for the NetBSD 9.0 test of the > > Orange Pi PC that has an expanded SD card image of 2 GiB size. > > > > Fixes: 6d2d4069c47e ("hw/sd: Correct the maximum size of a Standard Capacity SD Memory Card") > > Reported-by: Niek Linnenbank <nieklinnenbank@gmail.com> > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > > --- > > > > hw/sd/sd.c | 15 +++++++++++---- > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > > index bd10ec8fc4..732fcb5ff0 100644 > > --- a/hw/sd/sd.c > > +++ b/hw/sd/sd.c > > @@ -386,10 +386,17 @@ static const uint8_t sd_csd_rw_mask[16] = { > > > > static void sd_set_csd(SDState *sd, uint64_t size) > > { > > - uint32_t csize = (size >> (CMULT_SHIFT + HWBLOCK_SHIFT)) - 1; > > + int hwblock_shift = HWBLOCK_SHIFT; > > + uint32_t csize; > > uint32_t sectsize = (1 << (SECTOR_SHIFT + 1)) - 1; > > uint32_t wpsize = (1 << (WPGROUP_SHIFT + 1)) - 1; > > > > + /* To indicate 2 GiB card, BLOCK_LEN shall be 1024 bytes */ > > + if (size == SDSC_MAX_CAPACITY) { > > + hwblock_shift += 1; > > This is going in the good direction, however now we have an huge > security hole, as SDState::data[] is 512 bytes, and you announce the > guest it can use 1024 bytes. See sd_blk_read() and sd_blk_write(). Currently sd_normal_command() ensures that the maximum block length is 512 bytes as the response to cmd 16. The spec also says in chapter4.3.2 2 GByte Card: "However, the Block Length, set by CMD16, shall be up to 512 bytes to keep consistency with 512 bytes Maximum Block Length cards (Less than or equal 2GBytes cards). I don't see any issue here. Am I missing anything? > > Now SDState::data[] is migrated, so this isn't an easy field to > modify without breaking compatibility again :( > > I've been working on a more robust approach today, doing some cleanup > first. I'll send it during the next days hopefully. Regards, Bin
On 10/26/20 2:40 AM, Bin Meng wrote: > Hi Philippe, > > On Mon, Oct 26, 2020 at 2:56 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> On 10/25/20 4:23 PM, Bin Meng wrote: >>> From: Bin Meng <bin.meng@windriver.com> >>> >>> Per the SD spec, to indicate a 2 GiB card, BLOCK_LEN shall be 1024 >>> bytes, hence the READ_BL_LEN field in the CSD register shall be 10 >>> instead of 9. >>> >>> This fixes the acceptance test error for the NetBSD 9.0 test of the >>> Orange Pi PC that has an expanded SD card image of 2 GiB size. >>> >>> Fixes: 6d2d4069c47e ("hw/sd: Correct the maximum size of a Standard Capacity SD Memory Card") >>> Reported-by: Niek Linnenbank <nieklinnenbank@gmail.com> >>> Signed-off-by: Bin Meng <bin.meng@windriver.com> >>> --- >>> >>> hw/sd/sd.c | 15 +++++++++++---- >>> 1 file changed, 11 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c >>> index bd10ec8fc4..732fcb5ff0 100644 >>> --- a/hw/sd/sd.c >>> +++ b/hw/sd/sd.c >>> @@ -386,10 +386,17 @@ static const uint8_t sd_csd_rw_mask[16] = { >>> >>> static void sd_set_csd(SDState *sd, uint64_t size) >>> { >>> - uint32_t csize = (size >> (CMULT_SHIFT + HWBLOCK_SHIFT)) - 1; >>> + int hwblock_shift = HWBLOCK_SHIFT; >>> + uint32_t csize; >>> uint32_t sectsize = (1 << (SECTOR_SHIFT + 1)) - 1; >>> uint32_t wpsize = (1 << (WPGROUP_SHIFT + 1)) - 1; >>> >>> + /* To indicate 2 GiB card, BLOCK_LEN shall be 1024 bytes */ >>> + if (size == SDSC_MAX_CAPACITY) { >>> + hwblock_shift += 1; >> >> This is going in the good direction, however now we have an huge >> security hole, as SDState::data[] is 512 bytes, and you announce the >> guest it can use 1024 bytes. See sd_blk_read() and sd_blk_write(). > > Currently sd_normal_command() ensures that the maximum block length is > 512 bytes as the response to cmd 16. > > The spec also says in chapter4.3.2 2 GByte Card: > > "However, the Block Length, set by CMD16, shall be up to 512 bytes to > keep consistency with 512 bytes Maximum Block Length cards (Less than > or equal 2GBytes cards). > > I don't see any issue here. Am I missing anything? You are not missing anything, I was confused by the spec. Patch applied. Regards, Phil.
diff --git a/hw/sd/sd.c b/hw/sd/sd.c index bd10ec8fc4..732fcb5ff0 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -386,10 +386,17 @@ static const uint8_t sd_csd_rw_mask[16] = { static void sd_set_csd(SDState *sd, uint64_t size) { - uint32_t csize = (size >> (CMULT_SHIFT + HWBLOCK_SHIFT)) - 1; + int hwblock_shift = HWBLOCK_SHIFT; + uint32_t csize; uint32_t sectsize = (1 << (SECTOR_SHIFT + 1)) - 1; uint32_t wpsize = (1 << (WPGROUP_SHIFT + 1)) - 1; + /* To indicate 2 GiB card, BLOCK_LEN shall be 1024 bytes */ + if (size == SDSC_MAX_CAPACITY) { + hwblock_shift += 1; + } + csize = (size >> (CMULT_SHIFT + hwblock_shift)) - 1; + if (size <= SDSC_MAX_CAPACITY) { /* Standard Capacity SD */ sd->csd[0] = 0x00; /* CSD structure */ sd->csd[1] = 0x26; /* Data read access-time-1 */ @@ -397,7 +404,7 @@ static void sd_set_csd(SDState *sd, uint64_t size) sd->csd[3] = 0x32; /* Max. data transfer rate: 25 MHz */ sd->csd[4] = 0x5f; /* Card Command Classes */ sd->csd[5] = 0x50 | /* Max. read data block length */ - HWBLOCK_SHIFT; + hwblock_shift; sd->csd[6] = 0xe0 | /* Partial block for read allowed */ ((csize >> 10) & 0x03); sd->csd[7] = 0x00 | /* Device size */ @@ -411,9 +418,9 @@ static void sd_set_csd(SDState *sd, uint64_t size) sd->csd[11] = 0x00 | /* Write protect group size */ ((sectsize << 7) & 0x80) | wpsize; sd->csd[12] = 0x90 | /* Write speed factor */ - (HWBLOCK_SHIFT >> 2); + (hwblock_shift >> 2); sd->csd[13] = 0x20 | /* Max. write data block length */ - ((HWBLOCK_SHIFT << 6) & 0xc0); + ((hwblock_shift << 6) & 0xc0); sd->csd[14] = 0x00; /* File format group */ } else { /* SDHC */ size /= 512 * KiB;