| Message ID | 1464694565-16784-1-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
On 05/31/2016 05:36 AM, Cédric Le Goater wrote: > commit 243e6f69c129 ("m25p80: Switch to byte-based block access") > replaced blk_read() calls with blk_pread() but return values are > different. Shoot, I completely missed that when I made the conversions. Now I need to re-audit that entire series to see if the same problem happened anywhere else. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/block/m25p80.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Eric Blake <eblake@redhat.com> > > Index: qemu-ast2400-mainline.git/hw/block/m25p80.c > =================================================================== > --- qemu-ast2400-mainline.git.orig/hw/block/m25p80.c > +++ qemu-ast2400-mainline.git/hw/block/m25p80.c > @@ -900,7 +900,7 @@ static int m25p80_init(SSISlave *ss) > s->storage = blk_blockalign(s->blk, s->size); > > /* FIXME: Move to late init */ > - if (blk_pread(s->blk, 0, s->storage, s->size)) { > + if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) { > fprintf(stderr, "Failed to initialize SPI flash!\n"); > return 1; > } > >
On 05/31/2016 04:26 PM, Eric Blake wrote: > On 05/31/2016 05:36 AM, Cédric Le Goater wrote: >> commit 243e6f69c129 ("m25p80: Switch to byte-based block access") >> replaced blk_read() calls with blk_pread() but return values are >> different. > > Shoot, I completely missed that when I made the conversions. Now I need > to re-audit that entire series to see if the same problem happened > anywhere else. I took a quick look and most of the calls to blk_pread() test with < 0. So we should be fine and I should have mention that. C. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/block/m25p80.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > Reviewed-by: Eric Blake <eblake@redhat.com> > >> >> Index: qemu-ast2400-mainline.git/hw/block/m25p80.c >> =================================================================== >> --- qemu-ast2400-mainline.git.orig/hw/block/m25p80.c >> +++ qemu-ast2400-mainline.git/hw/block/m25p80.c >> @@ -900,7 +900,7 @@ static int m25p80_init(SSISlave *ss) >> s->storage = blk_blockalign(s->blk, s->size); >> >> /* FIXME: Move to late init */ >> - if (blk_pread(s->blk, 0, s->storage, s->size)) { >> + if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) { >> fprintf(stderr, "Failed to initialize SPI flash!\n"); >> return 1; >> } >> >> >
On 05/31/2016 08:29 AM, Cédric Le Goater wrote: > On 05/31/2016 04:26 PM, Eric Blake wrote: >> On 05/31/2016 05:36 AM, Cédric Le Goater wrote: >>> commit 243e6f69c129 ("m25p80: Switch to byte-based block access") >>> replaced blk_read() calls with blk_pread() but return values are >>> different. >> >> Shoot, I completely missed that when I made the conversions. Now I need >> to re-audit that entire series to see if the same problem happened >> anywhere else. > > I took a quick look and most of the calls to blk_pread() test with < 0. > So we should be fine and I should have mention that. > > C. > >>> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >>> hw/block/m25p80.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> >>> >>> Index: qemu-ast2400-mainline.git/hw/block/m25p80.c >>> =================================================================== >>> --- qemu-ast2400-mainline.git.orig/hw/block/m25p80.c >>> +++ qemu-ast2400-mainline.git/hw/block/m25p80.c >>> @@ -900,7 +900,7 @@ static int m25p80_init(SSISlave *ss) >>> s->storage = blk_blockalign(s->blk, s->size); >>> >>> /* FIXME: Move to late init */ >>> - if (blk_pread(s->blk, 0, s->storage, s->size)) { >>> + if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) { As it is, blk_pread() (and blk_pwrite()) _always_ returns negative or the input count value; it appears that it is incapable of reporting a short read amount. I guess that's intentional, but a bit odd, when compared to the standard read()/write().
Hello Eric, On 05/31/2016 04:36 PM, Eric Blake wrote: > On 05/31/2016 08:29 AM, Cédric Le Goater wrote: >> On 05/31/2016 04:26 PM, Eric Blake wrote: >>> On 05/31/2016 05:36 AM, Cédric Le Goater wrote: >>>> commit 243e6f69c129 ("m25p80: Switch to byte-based block access") >>>> replaced blk_read() calls with blk_pread() but return values are >>>> different. >>> >>> Shoot, I completely missed that when I made the conversions. Now I need >>> to re-audit that entire series to see if the same problem happened >>> anywhere else. >> >> I took a quick look and most of the calls to blk_pread() test with < 0. >> So we should be fine and I should have mention that. >> >> C. >> >>>> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>> --- >>>> hw/block/m25p80.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> >>>> >>>> Index: qemu-ast2400-mainline.git/hw/block/m25p80.c >>>> =================================================================== >>>> --- qemu-ast2400-mainline.git.orig/hw/block/m25p80.c >>>> +++ qemu-ast2400-mainline.git/hw/block/m25p80.c >>>> @@ -900,7 +900,7 @@ static int m25p80_init(SSISlave *ss) >>>> s->storage = blk_blockalign(s->blk, s->size); >>>> >>>> /* FIXME: Move to late init */ >>>> - if (blk_pread(s->blk, 0, s->storage, s->size)) { >>>> + if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) { > > As it is, blk_pread() (and blk_pwrite()) _always_ returns negative or > the input count value; it appears that it is incapable of reporting a > short read amount. I guess that's intentional, but a bit odd, when > compared to the standard read()/write(). It seems that commit 243e6f69c129 ("m25p80: Switch to byte-based block access") is bringing another issue : qemu-system-arm: /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1252: bdrv_aligned_pwritev: Assertion `!qiov || bytes == qiov->size' failed. Aborted (core dumped) The flash page size is 256. How should we handle this ? Thanks, C.
On 06/13/2016 10:25 AM, Cédric Le Goater wrote: > > It seems that commit 243e6f69c129 ("m25p80: Switch to byte-based block access") > is bringing another issue : > > qemu-system-arm: /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1252: bdrv_aligned_pwritev: Assertion `!qiov || bytes == qiov->size' failed. > Aborted (core dumped) Can you provide a more complete stack dump, and/or a recipe on how to repeat the assertion? > > The flash page size is 256. > > How should we handle this ? Sounds like a bug to be fixed.
On 06/13/2016 06:47 PM, Eric Blake wrote: > On 06/13/2016 10:25 AM, Cédric Le Goater wrote: > >> >> It seems that commit 243e6f69c129 ("m25p80: Switch to byte-based block access") >> is bringing another issue : >> >> qemu-system-arm: /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1252: bdrv_aligned_pwritev: Assertion `!qiov || bytes == qiov->size' failed. >> Aborted (core dumped) > > Can you provide a more complete stack dump, yes, see below. > and/or a recipe on how to repeat the assertion? That's more difficult right now. The patchset I am working on is not mainline. It adds the SPI controller to the ast2400 soc and it uses m25p80 flash modules with -mtdblock. I am trying to rebase on qemu's head to send it and I am hitting this issue. So I need to find a simpler way to reproduce, with code only in mainline of course. Until then, here is a gdb backtrace. Sorry about that. Thanks, C. $ gdb /home/legoater/work/qemu/qemu-ast2400-mainline.git/install/bin/qemu-system-arm core GNU gdb (Debian 7.7.1+dfsg-5) 7.7.1 Copyright (C) 2014 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-linux-gnu". Type "show configuration" for configuration details. For bug reporting instructions, please see: <http://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from /home/legoater/work/qemu/qemu-ast2400-mainline.git/install/bin/qemu-system-arm...done. [New LWP 662] [New LWP 1120] [New LWP 674] [New LWP 663] [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". Core was generated by `/home/legoater/work/qemu/qemu-ast2400-mainline.git/install/bin/qemu-system-arm'. Program terminated with signal SIGABRT, Aborted. #0 0x00007fa818e98067 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 56 ../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory. (gdb) bt #0 0x00007fa818e98067 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56 #1 0x00007fa818e99448 in __GI_abort () at abort.c:89 #2 0x00007fa818e91266 in __assert_fail_base (fmt=0x7fa818fca238 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x7fa81c7bffc4 "!qiov || bytes == qiov->size", file=file@entry=0x7fa81c7bfaa0 "/home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c", line=line@entry=1243, function=function@entry=0x7fa81c7c0170 <__PRETTY_FUNCTION__.34512> "bdrv_aligned_pwritev") at assert.c:92 #3 0x00007fa818e91312 in __GI___assert_fail ( assertion=assertion@entry=0x7fa81c7bffc4 "!qiov || bytes == qiov->size", file=file@entry=0x7fa81c7bfaa0 "/home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c", line=line@entry=1243, function=function@entry=0x7fa81c7c0170 <__PRETTY_FUNCTION__.34512> "bdrv_aligned_pwritev") at assert.c:101 #4 0x00007fa81c6694ac in bdrv_aligned_pwritev (bs=0x7fa81d4dd050, req=<optimized out>, offset=30878208, bytes=512, qiov=0x7fa7f47fee60, flags=0) at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1243 #5 0x00007fa81c669ecb in bdrv_co_pwritev (bs=0x7fa81d4dd050, offset=8, bytes=512, qiov=0x7fa80d5191c0, flags=(BDRV_REQ_COPY_ON_READ | BDRV_REQ_ZERO_WRITE | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_SERIALISING | BDRV_REQ_FUA | unknown: 4278124256), flags@entry=(unknown: 0)) at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1492 #6 0x00007fa81c65e367 in blk_co_pwritev (blk=0x7fa81d4c5b60, offset=30878208, bytes=256, qiov=0x7fa80d5191c0, flags=(unknown: 0)) at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/block-backend.c:788 #7 0x00007fa81c65e49b in blk_aio_write_entry (opaque=0x7fa7e849aca0) at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/block-backend.c:977 #8 0x00007fa81c6c823a in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>) at /home/legoater/work/qemu/qemu-ast2400-mainline.git/util/coroutine-ucontext.c:78 #9 0x00007fa818ea8f00 in ?? () from /lib/x86_64-linux-gnu/libc.so.6 #10 0x00007fa80d5189d0 in ?? () #11 0x0000000000000000 in ?? () (gdb) up 4 #4 0x00007fa81c6694ac in bdrv_aligned_pwritev (bs=0x7fa81d4dd050, req=<optimized out>, offset=30878208, bytes=512, qiov=0x7fa7f47fee60, flags=0) at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1243 1243 assert(!qiov || bytes == qiov->size); (gdb) p *qiov $1 = {iov = 0x7fa81da671d0, niov = 1, nalloc = 1, size = 256}
On 06/13/2016 11:43 AM, Cédric Le Goater wrote: > On 06/13/2016 06:47 PM, Eric Blake wrote: >> On 06/13/2016 10:25 AM, Cédric Le Goater wrote: >> >>> >>> It seems that commit 243e6f69c129 ("m25p80: Switch to byte-based block access") >>> is bringing another issue : >>> >>> qemu-system-arm: /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1252: bdrv_aligned_pwritev: Assertion `!qiov || bytes == qiov->size' failed. >>> Aborted (core dumped) >> >> Can you provide a more complete stack dump, > > yes, see below. > >> and/or a recipe on how to repeat the assertion? > > That's more difficult right now. The patchset I am working on is not > mainline. It adds the SPI controller to the ast2400 soc and it uses > m25p80 flash modules with -mtdblock. > > I am trying to rebase on qemu's head to send it and I am hitting this > issue. So I need to find a simpler way to reproduce, with code only in > mainline of course. > > Until then, here is a gdb backtrace. Sorry about that. > > #4 0x00007fa81c6694ac in bdrv_aligned_pwritev (bs=0x7fa81d4dd050, req=<optimized out>, offset=30878208, > bytes=512, qiov=0x7fa7f47fee60, flags=0) > at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1243 > #5 0x00007fa81c669ecb in bdrv_co_pwritev (bs=0x7fa81d4dd050, offset=8, bytes=512, qiov=0x7fa80d5191c0, > flags=(BDRV_REQ_COPY_ON_READ | BDRV_REQ_ZERO_WRITE | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_SERIALISING | BDRV_REQ_FUA | unknown: 4278124256), flags@entry=(unknown: 0)) > at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1492 That 'flags' value looks bogus... > #6 0x00007fa81c65e367 in blk_co_pwritev (blk=0x7fa81d4c5b60, offset=30878208, bytes=256, qiov=0x7fa80d5191c0, > flags=(unknown: 0)) at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/block-backend.c:788 > #7 0x00007fa81c65e49b in blk_aio_write_entry (opaque=0x7fa7e849aca0) > at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/block-backend.c:977 > #8 0x00007fa81c6c823a in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>) > at /home/legoater/work/qemu/qemu-ast2400-mainline.git/util/coroutine-ucontext.c:78 > #9 0x00007fa818ea8f00 in ?? () from /lib/x86_64-linux-gnu/libc.so.6 and we don't get anything further in the backtrace beyond coroutines, to see who's sending the bad parameters. I recently debugged a bogus flags in bdrv_aio_preadv, by hoisting an assert to occur before coroutines are used in blk_aio_prwv(): https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg02948.html I've just posted v2 of that patch (now a 2/2 series), but in v2 no longer kept the assert at that point. But maybe the correct fix, and/or the hack for catching the bug prior to coroutines, will help you debug where the bad arguments are coming from. > #10 0x00007fa80d5189d0 in ?? () > #11 0x0000000000000000 in ?? () > (gdb) up 4 > #4 0x00007fa81c6694ac in bdrv_aligned_pwritev (bs=0x7fa81d4dd050, req=<optimized out>, offset=30878208, > bytes=512, qiov=0x7fa7f47fee60, flags=0) > at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1243 > 1243 assert(!qiov || bytes == qiov->size); > (gdb) p *qiov > $1 = {iov = 0x7fa81da671d0, niov = 1, nalloc = 1, size = 256} > >
Am 31.05.2016 um 16:26 hat Eric Blake geschrieben: > On 05/31/2016 05:36 AM, Cédric Le Goater wrote: > > commit 243e6f69c129 ("m25p80: Switch to byte-based block access") > > replaced blk_read() calls with blk_pread() but return values are > > different. > > Shoot, I completely missed that when I made the conversions. Now I need > to re-audit that entire series to see if the same problem happened > anywhere else. > > > > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > > --- > > hw/block/m25p80.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks, applied to the block branch. Kevin
Hello Eric, On 06/13/2016 06:47 PM, Eric Blake wrote: > On 06/13/2016 10:25 AM, Cédric Le Goater wrote: > >> >> It seems that commit 243e6f69c129 ("m25p80: Switch to byte-based block access") >> is bringing another issue : >> >> qemu-system-arm: /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1252: bdrv_aligned_pwritev: Assertion `!qiov || bytes == qiov->size' failed. >> Aborted (core dumped) > > Can you provide a more complete stack dump, and/or a recipe on how to > repeat the assertion? Here you are, it is a bit long ... You need to get a few files : * an OpenBMC kernel : wget https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/images/palmetto/cuImage * an OpenBMC rootfs : wget https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/images/palmetto/obmc-phosphor-image-palmetto.cpio.gz * an OpenBMC flash image containing the above (with which we should boot someday) : wget https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/images/palmetto/flash-palmetto * an OpenPower flash image for the host : wget https://openpower.xyz/job/openpower-op-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/images/palmetto.pnor Clone this qemu branch which adds to the ast24000 SOC its SPI/SMC controllers: https://github.com/legoater/qemu/commits/aspeed-ssi The extra commits bring : ast2400: add SMC controllers (FMC and SPI) ast2400: add SPI flash slave object ast2400: create SPI flash slaves hw/arm/ast2400.c | 31 +++ hw/arm/palmetto-bmc.c | 3 + hw/ssi/Makefile.objs | 1 + hw/ssi/aspeed_smc.c | 451 ++++++++++++++++++++++++++++++++++++++++++++ include/hw/arm/ast2400.h | 3 + include/hw/ssi/aspeed_smc.h | 105 +++++++++++ 6 files changed, 594 insertions(+) create mode 100644 hw/ssi/aspeed_smc.c create mode 100644 include/hw/ssi/aspeed_smc.h and these, but we don't really care : m25p80: fix test on blk_pread() return value ssi: change ssi_slave_init to be a realize ops Compile with arm-softmmu, should be enough, and run with : qemu-system-arm -m 256 -M palmetto-bmc -kernel ./cuImage -initrd ./obmc-phosphor-image-palmetto.cpio.gz -mtdblock ./flash-palmetto -mtdblock ./palmetto.pnor -snapshot -nographic -nodefaults -monitor stdio -serial pty -S When booted, log with root/OpenBmc and run : dd if=/dev/zero of=/dev/mtd0 you should get : (qemu) qemu-system-arm: .../block/io.c:1243: bdrv_aligned_pwritev: Assertion `!qiov || bytes == qiov->size' failed. Aborted (core dumped) If there are some messages like : qemu-system-arm: aspeed_smc_flash_read: flash not in usermode This is because a userspace tool is trying to access the host flash image (./palmetto.pnor) but the support for linear addressing mode is not in the branch yet. you can ignore them. If you have read up to here, that probably means you might try the above :) I wish I had a simpler way, but no ... we need to work on some unit test I guess. Thanks, C.
Index: qemu-ast2400-mainline.git/hw/block/m25p80.c =================================================================== --- qemu-ast2400-mainline.git.orig/hw/block/m25p80.c +++ qemu-ast2400-mainline.git/hw/block/m25p80.c @@ -900,7 +900,7 @@ static int m25p80_init(SSISlave *ss) s->storage = blk_blockalign(s->blk, s->size); /* FIXME: Move to late init */ - if (blk_pread(s->blk, 0, s->storage, s->size)) { + if (blk_pread(s->blk, 0, s->storage, s->size) != s->size) { fprintf(stderr, "Failed to initialize SPI flash!\n"); return 1; }
commit 243e6f69c129 ("m25p80: Switch to byte-based block access") replaced blk_read() calls with blk_pread() but return values are different. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/block/m25p80.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)