On 04/23/2018 04:14 PM, Karl Beldan wrote: > The logic wants 512-byte aligned blk ops.
The commit you are referencing mentions that the code permits 256, 512,
or 2048-byte alignment, based on the configuration of the hardware it is
emulating. The whole file is hard to read, and I'm not surprised if
what I thought was a patch with no semantic change didn't quite succeed.
> To switch to byte-based block accesses, the fixed commit changed the
> blk read offset,
> PAGE_START(addr) >> 9
> with
> PAGE_START(addr)
> which min alignment, for on-drive OOB, is the min OOB size.
> Consequently the reads are offset by PAGE_START(addr) & 0x1ff.
Do you have a call trace where a caller is passing in an addr that is
not aligned to 512? I agree that the old code was 512-aligned by virtue
of the old interface; but the new code SHOULD be able to call into the
block layer with whatever alignment it needs, where the block layer will
do a larger read to satisfy block constraints, as needed (that is, if a
caller requests a 256-byte read of a device that requires 512-alignment,
blk_pread() will automatically widen out to a 512-byte read).
>
> Fixes: 9fc0d361cc41 ("nand: Switch to byte-based block access")
> Cc: Eric Blake <ebl...@redhat.com>
> Signed-off-by: Karl Beldan <karl.beldan+...@gmail.com>
CC: qemu-sta...@nongnu.org
> ---
> hw/block/nand.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/block/nand.c b/hw/block/nand.c
> index 919cb9b803..ed587f60f0 100644
> --- a/hw/block/nand.c
> +++ b/hw/block/nand.c
> @@ -788,7 +788,7 @@ static void glue(nand_blk_load_,
> PAGE_SIZE)(NANDFlashState *s,
> OOB_SIZE);
> s->ioaddr = s->io + SECTOR_OFFSET(s->addr) + offset;
> } else {
> - if (blk_pread(s->blk, PAGE_START(addr), s->io,
> + if (blk_pread(s->blk, PAGE_START(addr) & ~0x1ff, s->io,
> (PAGE_SECTORS + 2) << BDRV_SECTOR_BITS) < 0) {
I don't like the magic number masking. This should probably be written
QEMU_ALIGN_DOWN(PAGE_START(addr), BDRV_SECTOR_SIZE).
I would like to see some actual numbers from a backtrace to make sure
we're doing this right, but my initial evaluation is done by assuming,
for the sake of argument, that we're using a 256-byte page size (as that
is most likely to be unaligned to a 512-byte boundary). I see that we
can call into nand_blk_load_256() during processing of
NAND_CMD_RANDOMREAD2 from nand_command(), although it gets harder to
audit without an actual call trace whether addr and offset have 512-byte
alignments at that point. But without analysis of whether this is an
actual possibility, let's assume we can somehow trigger a code path that
calls nand_blk_load_256(s, 768, 1).
Before 9fc0d361cc41, this called:
if (blk_read(s->blk, PAGE_START(addr) >> 9,
s->io, (PAGE_SECTORS + 2)) < 0) {
or, tracing macro-expansion
blk_read(s->blk, (PAGE(addr) * (PAGE_SIZE + OOB_SIZE)) >> 9,
s->io, (1 + 2))
blk_read(s->blk, (((addr) >> 8) * (256 + (1 << (PAGE_SHIFT - 5)))) >> 9,
s->io, (1 + 2))
blk_read(s->blk, (((addr) >> 8) * (256 + 8)) >> 9,
s->io, 3)
which, for our given addr of 768, results in
blk_read(s->blk, 792 >> 9, s->io, 3)
or the 1536 bytes starting at offset 512 (which includes offset 792 that
we are interested in).
And indeed, post-patch, it now results in trying to read 1536 bytes, but
starting at offset 792 rather than offset 512.
So it looks like the fix is correct, once it is spelled in a more
obvious way rather than with a magic number, and that the fact that this
has been broken since 2.7 means that this device is not getting much
actual testing :(
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
