| 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: From these tests it looks to me that the change is working as expected. I'm not an expert on SD internals so perhaps Philippe would like to give more detailed review comments on the code itself. So for the Orange Pi PC machine & acceptance tests: Tested-by: Niek Linnenbank <nieklinnenbank@gmail.com> Regards, Niek > > 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; > + } > + 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; > -- > 2.25.1 > >
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;