* [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value
@ 2016-05-31 11:36 Cédric Le Goater
2016-05-31 14:26 ` Eric Blake
0 siblings, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2016-05-31 11:36 UTC (permalink / raw)
To: Eric Blake
Cc: Peter Crosthwaite, Kevin Wolf, Max Reitz, qemu-block, qemu-devel,
Cédric Le Goater
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(-)
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;
}
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value
2016-05-31 11:36 [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value Cédric Le Goater
@ 2016-05-31 14:26 ` Eric Blake
2016-05-31 14:29 ` Cédric Le Goater
2016-06-14 8:54 ` Kevin Wolf
0 siblings, 2 replies; 15+ messages in thread
From: Eric Blake @ 2016-05-31 14:26 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Peter Crosthwaite, Kevin Wolf, Max Reitz, qemu-block, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]
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;
> }
>
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value
2016-05-31 14:26 ` Eric Blake
@ 2016-05-31 14:29 ` Cédric Le Goater
2016-05-31 14:36 ` Eric Blake
2016-06-14 8:54 ` Kevin Wolf
1 sibling, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2016-05-31 14:29 UTC (permalink / raw)
To: Eric Blake
Cc: Peter Crosthwaite, Kevin Wolf, Max Reitz, qemu-block, qemu-devel
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;
>> }
>>
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value
2016-05-31 14:29 ` Cédric Le Goater
@ 2016-05-31 14:36 ` Eric Blake
2016-06-13 16:25 ` Cédric Le Goater
0 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2016-05-31 14:36 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Peter Crosthwaite, Kevin Wolf, Max Reitz, qemu-block, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1758 bytes --]
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().
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value
2016-05-31 14:36 ` Eric Blake
@ 2016-06-13 16:25 ` Cédric Le Goater
2016-06-13 16:47 ` Eric Blake
0 siblings, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2016-06-13 16:25 UTC (permalink / raw)
To: Eric Blake
Cc: Peter Crosthwaite, Kevin Wolf, Max Reitz, qemu-block, qemu-devel
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value
2016-06-13 16:25 ` Cédric Le Goater
@ 2016-06-13 16:47 ` Eric Blake
2016-06-13 17:43 ` Cédric Le Goater
2016-06-15 17:03 ` Cédric Le Goater
0 siblings, 2 replies; 15+ messages in thread
From: Eric Blake @ 2016-06-13 16:47 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Peter Crosthwaite, Kevin Wolf, Max Reitz, qemu-block, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 674 bytes --]
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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value
2016-06-13 16:47 ` Eric Blake
@ 2016-06-13 17:43 ` Cédric Le Goater
2016-06-13 18:56 ` Eric Blake
2016-06-15 17:03 ` Cédric Le Goater
1 sibling, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2016-06-13 17:43 UTC (permalink / raw)
To: Eric Blake
Cc: Peter Crosthwaite, Kevin Wolf, Max Reitz, qemu-block, qemu-devel
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}
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value
2016-06-13 17:43 ` Cédric Le Goater
@ 2016-06-13 18:56 ` Eric Blake
2016-06-14 8:02 ` Cédric Le Goater
0 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2016-06-13 18:56 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Peter Crosthwaite, Kevin Wolf, Max Reitz, qemu-block, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3450 bytes --]
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}
>
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value
2016-06-13 18:56 ` Eric Blake
@ 2016-06-14 8:02 ` Cédric Le Goater
2016-06-14 8:38 ` Kevin Wolf
0 siblings, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2016-06-14 8:02 UTC (permalink / raw)
To: Eric Blake
Cc: Peter Crosthwaite, Kevin Wolf, Max Reitz, qemu-block, qemu-devel
>> #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.
That does not fix the assert.
>> #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}
So, it seems that the issue is coming from the fact that bdrv_co_pwritev()
does not handle alignments less than BDRV_SECTOR_SIZE :
/* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
It calls bdrv_aligned_pwritev() which does the assert :
assert(!qiov || bytes == qiov->size);
This is because flash_sync_page(), in m25p80.c, now writes with a len of
0x100, which the page size in flash modules. commit 243e6f69c129
("m25p80: Switch to byte-based block access") removed the alignment on
BDRV_SECTOR_SIZE.
So I think the safest is to restore the alignment on writes. see below.
If this is ok, I will send a little serie of fixes for m25p80 with this
one included.
Thanks,
C.
From: Cédric Le Goater <clg@kaod.org>
Subject: [PATCH] m25p80: restore BDRV_SECTOR_SIZE alignment on writes
Date: Tue, 14 Jun 2016 09:32:22 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
commit 243e6f69c129 ("m25p80: Switch to byte-based block access")
removed the alignment of writes on BDRV_SECTOR_SIZE, so they are
now done on a page size (0x100) basis. This is not supported by the
bdrv routines which asserts in bdrv_aligned_pwritev() :
assert(!qiov || bytes == qiov->size);
bytes being rounded up to BDRV_SECTOR_SIZE and qiov->size == 0x100
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
hw/block/m25p80.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
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
@@ -359,21 +359,25 @@ static void blk_sync_complete(void *opaq
static void flash_sync_page(Flash *s, int page)
{
+ int blk_sector, nb_sectors;
QEMUIOVector iov;
if (!s->blk || blk_is_read_only(s->blk)) {
return;
}
+ blk_sector = (page * s->pi->page_size) / BDRV_SECTOR_SIZE;
+ nb_sectors = DIV_ROUND_UP(s->pi->page_size, BDRV_SECTOR_SIZE);
qemu_iovec_init(&iov, 1);
- qemu_iovec_add(&iov, s->storage + page * s->pi->page_size,
- s->pi->page_size);
- blk_aio_pwritev(s->blk, page * s->pi->page_size, &iov, 0,
+ qemu_iovec_add(&iov, s->storage + blk_sector * BDRV_SECTOR_SIZE,
+ nb_sectors * BDRV_SECTOR_SIZE);
+ blk_aio_pwritev(s->blk, blk_sector * BDRV_SECTOR_SIZE, &iov, 0,
blk_sync_complete, NULL);
}
static inline void flash_sync_area(Flash *s, int64_t off, int64_t len)
{
+ int64_t start, end, nb_sectors;
QEMUIOVector iov;
if (!s->blk || blk_is_read_only(s->blk)) {
@@ -381,9 +385,13 @@ static inline void flash_sync_area(Flash
}
assert(!(len % BDRV_SECTOR_SIZE));
+ start = off / BDRV_SECTOR_SIZE;
+ end = (off + len) / BDRV_SECTOR_SIZE;
+ nb_sectors = end - start;
qemu_iovec_init(&iov, 1);
- qemu_iovec_add(&iov, s->storage + off, len);
- blk_aio_pwritev(s->blk, off, &iov, 0, blk_sync_complete, NULL);
+ qemu_iovec_add(&iov, s->storage + (start * BDRV_SECTOR_SIZE),
+ nb_sectors * BDRV_SECTOR_SIZE);
+ blk_aio_pwritev(s->blk, start, &iov, 0, blk_sync_complete, NULL);
}
static void flash_erase(Flash *s, int offset, FlashCMD cmd)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value
2016-06-14 8:02 ` Cédric Le Goater
@ 2016-06-14 8:38 ` Kevin Wolf
2016-06-14 16:02 ` Cédric Le Goater
0 siblings, 1 reply; 15+ messages in thread
From: Kevin Wolf @ 2016-06-14 8:38 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Eric Blake, Peter Crosthwaite, Max Reitz, qemu-block, qemu-devel
Am 14.06.2016 um 10:02 hat Cédric Le Goater geschrieben:
> >> #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.
>
> That does not fix the assert.
>
> >> #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}
>
> So, it seems that the issue is coming from the fact that bdrv_co_pwritev()
> does not handle alignments less than BDRV_SECTOR_SIZE :
>
> /* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
> uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
>
> It calls bdrv_aligned_pwritev() which does the assert :
>
> assert(!qiov || bytes == qiov->size);
Yes, but between these two places, there is code that should actually
enforce the right alignment:
if ((offset + bytes) & (align - 1)) {
...
}
You can see in your backtrace that bdrv_aligned_pwritev() gets a
different qiov than bdrv_co_pwritev() (which is local_qiov in the latter
function).
It's just unclear to me why this code extended bytes, but didn't add the
tail_buf iovec to local_qiov.
> This is because flash_sync_page(), in m25p80.c, now writes with a len of
> 0x100, which the page size in flash modules. commit 243e6f69c129
> ("m25p80: Switch to byte-based block access") removed the alignment on
> BDRV_SECTOR_SIZE.
>
> So I think the safest is to restore the alignment on writes. see below.
> If this is ok, I will send a little serie of fixes for m25p80 with this
> one included.
No, bdrv_co_pwritev() is supposed to handle sub-sector writes correctly,
so this is what we need to fix.
Kevin
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value
2016-05-31 14:26 ` Eric Blake
2016-05-31 14:29 ` Cédric Le Goater
@ 2016-06-14 8:54 ` Kevin Wolf
1 sibling, 0 replies; 15+ messages in thread
From: Kevin Wolf @ 2016-06-14 8:54 UTC (permalink / raw)
To: Eric Blake
Cc: Cédric Le Goater, Peter Crosthwaite, Max Reitz, qemu-block,
qemu-devel
[-- Attachment #1: Type: text/plain, Size: 685 bytes --]
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
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value
2016-06-14 8:38 ` Kevin Wolf
@ 2016-06-14 16:02 ` Cédric Le Goater
2016-06-15 7:57 ` Kevin Wolf
0 siblings, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2016-06-14 16:02 UTC (permalink / raw)
To: Kevin Wolf
Cc: Eric Blake, Peter Crosthwaite, Max Reitz, qemu-block, qemu-devel
On 06/14/2016 10:38 AM, Kevin Wolf wrote:
> Am 14.06.2016 um 10:02 hat Cédric Le Goater geschrieben:
>>>> #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.
>>
>> That does not fix the assert.
>>
>>>> #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}
>>
>> So, it seems that the issue is coming from the fact that bdrv_co_pwritev()
>> does not handle alignments less than BDRV_SECTOR_SIZE :
>>
>> /* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
>> uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
>>
>> It calls bdrv_aligned_pwritev() which does the assert :
>>
>> assert(!qiov || bytes == qiov->size);
>
> Yes, but between these two places, there is code that should actually
> enforce the right alignment:
>
> if ((offset + bytes) & (align - 1)) {
> ...
> }
>
> You can see in your backtrace that bdrv_aligned_pwritev() gets a
> different qiov than bdrv_co_pwritev() (which is local_qiov in the latter
> function).
>
> It's just unclear to me why this code extended bytes, but didn't add the
> tail_buf iovec to local_qiov.
The gdb backtrace is bogus. It does not make sense. May be a gdb issue
with multithread on jessie.
In the path tracking the tail bytes, we have :
if ((offset + bytes) & (align - 1)) {
...
tail_bytes = (offset + bytes) & (align - 1);
qemu_iovec_add(&local_qiov, tail_buf + tail_bytes, align - tail_bytes);
bytes = ROUND_UP(bytes, align);
}
This is where the issue is I think. The qiov holds 256 and bytes 512.
I have no idea how to fix that though.
Thanks,
C.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value
2016-06-14 16:02 ` Cédric Le Goater
@ 2016-06-15 7:57 ` Kevin Wolf
2016-06-15 13:36 ` Cédric Le Goater
0 siblings, 1 reply; 15+ messages in thread
From: Kevin Wolf @ 2016-06-15 7:57 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Eric Blake, Peter Crosthwaite, Max Reitz, qemu-block, qemu-devel
Am 14.06.2016 um 18:02 hat Cédric Le Goater geschrieben:
> On 06/14/2016 10:38 AM, Kevin Wolf wrote:
> > Am 14.06.2016 um 10:02 hat Cédric Le Goater geschrieben:
> >>>> #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.
> >>
> >> That does not fix the assert.
> >>
> >>>> #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}
> >>
> >> So, it seems that the issue is coming from the fact that bdrv_co_pwritev()
> >> does not handle alignments less than BDRV_SECTOR_SIZE :
> >>
> >> /* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
> >> uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
> >>
> >> It calls bdrv_aligned_pwritev() which does the assert :
> >>
> >> assert(!qiov || bytes == qiov->size);
> >
> > Yes, but between these two places, there is code that should actually
> > enforce the right alignment:
> >
> > if ((offset + bytes) & (align - 1)) {
> > ...
> > }
> >
> > You can see in your backtrace that bdrv_aligned_pwritev() gets a
> > different qiov than bdrv_co_pwritev() (which is local_qiov in the latter
> > function).
> >
> > It's just unclear to me why this code extended bytes, but didn't add the
> > tail_buf iovec to local_qiov.
>
> The gdb backtrace is bogus. It does not make sense. May be a gdb issue
> with multithread on jessie.
>
> In the path tracking the tail bytes, we have :
>
> if ((offset + bytes) & (align - 1)) {
> ...
if (!use_local_qiov) {
qemu_iovec_init(&local_qiov, qiov->niov + 1);
qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
use_local_qiov = true;
}
> tail_bytes = (offset + bytes) & (align - 1);
> qemu_iovec_add(&local_qiov, tail_buf + tail_bytes, align - tail_bytes);
>
> bytes = ROUND_UP(bytes, align);
> }
>
> This is where the issue is I think. The qiov holds 256 and bytes 512.
>
> I have no idea how to fix that though.
Added some more context above. qiov->size as passed from the device is
already 256 bytes, which are added to local_qiov with
qemu_iovec_concat(). And then we add another 256 from tail_buf in the
lines that you quoted, so in theory we should end up with a properly
aligned 256 + 256 = 512 byte qiov.
Kevin
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value
2016-06-15 7:57 ` Kevin Wolf
@ 2016-06-15 13:36 ` Cédric Le Goater
0 siblings, 0 replies; 15+ messages in thread
From: Cédric Le Goater @ 2016-06-15 13:36 UTC (permalink / raw)
To: Kevin Wolf
Cc: Eric Blake, Peter Crosthwaite, Max Reitz, qemu-block, qemu-devel
On 06/15/2016 09:57 AM, Kevin Wolf wrote:
> Am 14.06.2016 um 18:02 hat Cédric Le Goater geschrieben:
>> On 06/14/2016 10:38 AM, Kevin Wolf wrote:
>>> Am 14.06.2016 um 10:02 hat Cédric Le Goater geschrieben:
>>>>>> #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.
>>>>
>>>> That does not fix the assert.
>>>>
>>>>>> #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}
>>>>
>>>> So, it seems that the issue is coming from the fact that bdrv_co_pwritev()
>>>> does not handle alignments less than BDRV_SECTOR_SIZE :
>>>>
>>>> /* TODO Lift BDRV_SECTOR_SIZE restriction in BlockDriver interface */
>>>> uint64_t align = MAX(BDRV_SECTOR_SIZE, bs->request_alignment);
>>>>
>>>> It calls bdrv_aligned_pwritev() which does the assert :
>>>>
>>>> assert(!qiov || bytes == qiov->size);
>>>
>>> Yes, but between these two places, there is code that should actually
>>> enforce the right alignment:
>>>
>>> if ((offset + bytes) & (align - 1)) {
>>> ...
>>> }
>>>
>>> You can see in your backtrace that bdrv_aligned_pwritev() gets a
>>> different qiov than bdrv_co_pwritev() (which is local_qiov in the latter
>>> function).
>>>
>>> It's just unclear to me why this code extended bytes, but didn't add the
>>> tail_buf iovec to local_qiov.
>>
>> The gdb backtrace is bogus. It does not make sense. May be a gdb issue
>> with multithread on jessie.
>>
>> In the path tracking the tail bytes, we have :
>>
>> if ((offset + bytes) & (align - 1)) {
>> ...
> if (!use_local_qiov) {
> qemu_iovec_init(&local_qiov, qiov->niov + 1);
> qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
> use_local_qiov = true;
> }
>> tail_bytes = (offset + bytes) & (align - 1);
>> qemu_iovec_add(&local_qiov, tail_buf + tail_bytes, align - tail_bytes);
>>
>> bytes = ROUND_UP(bytes, align);
>> }
>>
>> This is where the issue is I think. The qiov holds 256 and bytes 512.
>>
>> I have no idea how to fix that though.
>
> Added some more context above. qiov->size as passed from the device is
> already 256 bytes, which are added to local_qiov with
> qemu_iovec_concat(). And then we add another 256 from tail_buf in the
> lines that you quoted, so in theory we should end up with a properly
> aligned 256 + 256 = 512 byte qiov.
yes.
It seems that qiov is bogus after the bdrv_aligned_preadv() call. It gets
zeroed most of the time, sometime ->size is 1, and then qemu_iovec_concat()
constructs an awful local_qiov, which brings the assert in bdrv_aligned_pwritev()
How's that possible ? Could it be a serialization issue ?
C.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value
2016-06-13 16:47 ` Eric Blake
2016-06-13 17:43 ` Cédric Le Goater
@ 2016-06-15 17:03 ` Cédric Le Goater
1 sibling, 0 replies; 15+ messages in thread
From: Cédric Le Goater @ 2016-06-15 17:03 UTC (permalink / raw)
To: Eric Blake
Cc: Peter Crosthwaite, Kevin Wolf, Max Reitz, qemu-block, qemu-devel
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-06-15 17:03 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-31 11:36 [Qemu-devel] [PATCH] m25p80: fix test on blk_pread() return value Cédric Le Goater
2016-05-31 14:26 ` Eric Blake
2016-05-31 14:29 ` Cédric Le Goater
2016-05-31 14:36 ` Eric Blake
2016-06-13 16:25 ` Cédric Le Goater
2016-06-13 16:47 ` Eric Blake
2016-06-13 17:43 ` Cédric Le Goater
2016-06-13 18:56 ` Eric Blake
2016-06-14 8:02 ` Cédric Le Goater
2016-06-14 8:38 ` Kevin Wolf
2016-06-14 16:02 ` Cédric Le Goater
2016-06-15 7:57 ` Kevin Wolf
2016-06-15 13:36 ` Cédric Le Goater
2016-06-15 17:03 ` Cédric Le Goater
2016-06-14 8:54 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.