Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value
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.
Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value
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 0x7fa81c6694ac in bdrv_aligned_pwritev (bs=0x7fa81d4dd050,
>> req=, offset=30878208,
>> bytes=512, qiov=0x7fa7f47fee60, flags=0)
>> at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1243
>> #5 0x7fa81c669ecb 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 0x7fa81c65e367 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 0x7fa81c65e49b in blk_aio_write_entry (opaque=0x7fa7e849aca0)
>> at
>> /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/block-backend.c:977
>> #8 0x7fa81c6c823a in coroutine_trampoline (i0=,
>> i1=)
>> at
>> /home/legoater/work/qemu/qemu-ast2400-mainline.git/util/coroutine-ucontext.c:78
>> #9 0x7fa818ea8f00 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 0x7fa80d5189d0 in ?? ()
>> #11 0x in ?? ()
>> (gdb) up 4
>> #4 0x7fa81c6694ac in bdrv_aligned_pwritev (bs=0x7fa81d4dd050,
>> req=, 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(_qiov, qiov->niov + 1);
> qemu_iovec_concat(_qiov, qiov, 0, qiov->size);
> use_local_qiov = true;
> }
>> tail_bytes = (offset + bytes) & (align - 1);
>> qemu_iovec_add(_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.
Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value
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 0x7fa81c6694ac in bdrv_aligned_pwritev (bs=0x7fa81d4dd050,
> req=, offset=30878208,
> bytes=512, qiov=0x7fa7f47fee60, flags=0)
> at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1243
> #5 0x7fa81c669ecb 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 0x7fa81c65e367 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 0x7fa81c65e49b in blk_aio_write_entry (opaque=0x7fa7e849aca0)
> at
> /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/block-backend.c:977
> #8 0x7fa81c6c823a in coroutine_trampoline (i0=,
> i1=)
> at
> /home/legoater/work/qemu/qemu-ast2400-mainline.git/util/coroutine-ucontext.c:78
> #9 0x7fa818ea8f00 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 0x7fa80d5189d0 in ?? ()
> #11 0x in ?? ()
> (gdb) up 4
> #4 0x7fa81c6694ac in bdrv_aligned_pwritev (bs=0x7fa81d4dd050,
> req=, 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(_qiov, qiov->niov + 1);
qemu_iovec_concat(_qiov, qiov, 0, qiov->size);
use_local_qiov = true;
}
> tail_bytes = (offset + bytes) & (align - 1);
> qemu_iovec_add(_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
Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value
On 06/14/2016 10:38 AM, Kevin Wolf wrote:
> Am 14.06.2016 um 10:02 hat Cédric Le Goater geschrieben:
#4 0x7fa81c6694ac in bdrv_aligned_pwritev (bs=0x7fa81d4dd050,
req=, offset=30878208,
bytes=512, qiov=0x7fa7f47fee60, flags=0)
at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1243
#5 0x7fa81c669ecb 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 0x7fa81c65e367 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 0x7fa81c65e49b in blk_aio_write_entry (opaque=0x7fa7e849aca0)
at
/home/legoater/work/qemu/qemu-ast2400-mainline.git/block/block-backend.c:977
#8 0x7fa81c6c823a in coroutine_trampoline (i0=,
i1=)
at
/home/legoater/work/qemu/qemu-ast2400-mainline.git/util/coroutine-ucontext.c:78
#9 0x7fa818ea8f00 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 0x7fa80d5189d0 in ?? ()
#11 0x in ?? ()
(gdb) up 4
#4 0x7fa81c6694ac in bdrv_aligned_pwritev (bs=0x7fa81d4dd050,
req=, 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(_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.
Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value
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 0x7fa81c6694ac in bdrv_aligned_pwritev (bs=0x7fa81d4dd050,
> req=, offset=30878208,
> bytes=512, qiov=0x7fa7f47fee60, flags=0)
> at /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/io.c:1243
> #5 0x7fa81c669ecb 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 0x7fa81c65e367 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 0x7fa81c65e49b in blk_aio_write_entry (opaque=0x7fa7e849aca0)
> at
> /home/legoater/work/qemu/qemu-ast2400-mainline.git/block/block-backend.c:977
> #8 0x7fa81c6c823a in coroutine_trampoline (i0=,
> i1=)
> at
> /home/legoater/work/qemu/qemu-ast2400-mainline.git/util/coroutine-ucontext.c:78
> #9 0x7fa818ea8f00 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 0x7fa80d5189d0 in ?? ()
> #11 0x in ?? ()
> (gdb) up 4
> #4 0x7fa81c6694ac in bdrv_aligned_pwritev (bs=0x7fa81d4dd050,
> req=, 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
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value
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
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:
Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value
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
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value
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
---
hw/block/m25p80.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> Reviewed-by: Eric Blake
>>>
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.
Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value
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
>>> ---
>>> hw/block/m25p80.c |2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Reviewed-by: Eric Blake
>>
>>>
>>> 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
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value
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
>> ---
>> hw/block/m25p80.c |2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Reviewed-by: Eric Blake
>
>>
>> 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;
>> }
>>
>>
>
Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value
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
> ---
> hw/block/m25p80.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Eric Blake
>
> 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
signature.asc
Description: OpenPGP digital signature
