Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value

2016-06-15 Thread Cédric Le Goater
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

2016-06-15 Thread Cédric Le Goater
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

2016-06-15 Thread Kevin Wolf
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

2016-06-14 Thread Cédric Le Goater
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

2016-06-13 Thread Eric Blake
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

2016-06-13 Thread Cédric Le Goater
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:
.
Find the GDB manual and other documentation resources online at:
.
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  0x7fa818e98067 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  0x7fa818e98067 in __GI_raise (sig=sig@entry=6) at 
../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x7fa818e99448 in __GI_abort () at abort.c:89
#2  0x7fa818e91266 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  0x7fa818e91312 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  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
#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
#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
1243assert(!qiov || bytes == qiov->size);
(gdb) p *qiov 
$1 = {iov = 0x7fa81da671d0, niov = 1, nalloc = 1, size = 256}




Re: [Qemu-block] [PATCH] m25p80: fix test on blk_pread() return value

2016-06-13 Thread Eric Blake
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

2016-06-13 Thread Cédric Le Goater
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

2016-05-31 Thread Eric Blake
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

2016-05-31 Thread Cédric Le Goater
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

2016-05-31 Thread Eric Blake
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