| Message ID | 20210521142829.326217-1-kit.westneat@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/scsi: Fix sector translation bug in scsi_unmap_complete_noio | expand |
On 21/05/21 16:28, Kit Westneat wrote: > check_lba_range expects sectors to be expressed in original qdev blocksize, but > scsi_unmap_complete_noio was translating them to 512 block sizes, which was > causing sense errors in the larger LBAs in devices using a 4k block size. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/345 > Signed-off-by: Kit Westneat <kit.westneat@gmail.com> > --- > hw/scsi/scsi-disk.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > index 3580e7ee61..e8a547dbb7 100644 > --- a/hw/scsi/scsi-disk.c > +++ b/hw/scsi/scsi-disk.c > @@ -1582,6 +1582,7 @@ invalid_field: > scsi_check_condition(r, SENSE_CODE(INVALID_FIELD)); > } > > +/* sector_num and nb_sectors expected to be in qdev blocksize */ > static inline bool check_lba_range(SCSIDiskState *s, > uint64_t sector_num, uint32_t nb_sectors) > { > @@ -1614,11 +1615,12 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret) > assert(r->req.aiocb == NULL); > > if (data->count > 0) { > - r->sector = ldq_be_p(&data->inbuf[0]) > - * (s->qdev.blocksize / BDRV_SECTOR_SIZE); > - r->sector_count = (ldl_be_p(&data->inbuf[8]) & 0xffffffffULL) > - * (s->qdev.blocksize / BDRV_SECTOR_SIZE); > - if (!check_lba_range(s, r->sector, r->sector_count)) { > + uint64_t sector_num = ldq_be_p(&data->inbuf[0]); > + uint32_t nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL; > + r->sector = sector_num * (s->qdev.blocksize / BDRV_SECTOR_SIZE); > + r->sector_count = nb_sectors * (s->qdev.blocksize / BDRV_SECTOR_SIZE); > + > + if (!check_lba_range(s, sector_num, nb_sectors)) { > block_acct_invalid(blk_get_stats(s->qdev.conf.blk), > BLOCK_ACCT_UNMAP); > scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE)); > Queued, thanks! If you want to produce a testcase for tests/qtest/virtio-scsi-test.c, I won't complain. Paolo
I started looking into adding tests, but it doesn't look like blkdebug supports changing the blocksize. I saw that it supports changing the memory alignment to 4k via the align parameter, but it doesn't implement bdrv_probe_blocksizes so the blocksize stays at 512. Would it be worth adding a blocksize parameter to blkdebug? (or maybe log-blksize & phys-blksize?) Or is there another way to set the blocksize of a qdev? Thanks, Kit On Fri, May 21, 2021 at 11:20 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > On 21/05/21 16:28, Kit Westneat wrote: > > check_lba_range expects sectors to be expressed in original qdev > blocksize, but > > scsi_unmap_complete_noio was translating them to 512 block sizes, which > was > > causing sense errors in the larger LBAs in devices using a 4k block size. > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/345 > > Signed-off-by: Kit Westneat <kit.westneat@gmail.com> > > --- > > hw/scsi/scsi-disk.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c > > index 3580e7ee61..e8a547dbb7 100644 > > --- a/hw/scsi/scsi-disk.c > > +++ b/hw/scsi/scsi-disk.c > > @@ -1582,6 +1582,7 @@ invalid_field: > > scsi_check_condition(r, SENSE_CODE(INVALID_FIELD)); > > } > > > > +/* sector_num and nb_sectors expected to be in qdev blocksize */ > > static inline bool check_lba_range(SCSIDiskState *s, > > uint64_t sector_num, uint32_t > nb_sectors) > > { > > @@ -1614,11 +1615,12 @@ static void scsi_unmap_complete_noio(UnmapCBData > *data, int ret) > > assert(r->req.aiocb == NULL); > > > > if (data->count > 0) { > > - r->sector = ldq_be_p(&data->inbuf[0]) > > - * (s->qdev.blocksize / BDRV_SECTOR_SIZE); > > - r->sector_count = (ldl_be_p(&data->inbuf[8]) & 0xffffffffULL) > > - * (s->qdev.blocksize / BDRV_SECTOR_SIZE); > > - if (!check_lba_range(s, r->sector, r->sector_count)) { > > + uint64_t sector_num = ldq_be_p(&data->inbuf[0]); > > + uint32_t nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL; > > + r->sector = sector_num * (s->qdev.blocksize / BDRV_SECTOR_SIZE); > > + r->sector_count = nb_sectors * (s->qdev.blocksize / > BDRV_SECTOR_SIZE); > > + > > + if (!check_lba_range(s, sector_num, nb_sectors)) { > > block_acct_invalid(blk_get_stats(s->qdev.conf.blk), > > BLOCK_ACCT_UNMAP); > > scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE)); > > > > Queued, thanks! > > If you want to produce a testcase for tests/qtest/virtio-scsi-test.c, I > won't complain. > > Paolo > >
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 3580e7ee61..e8a547dbb7 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -1582,6 +1582,7 @@ invalid_field: scsi_check_condition(r, SENSE_CODE(INVALID_FIELD)); } +/* sector_num and nb_sectors expected to be in qdev blocksize */ static inline bool check_lba_range(SCSIDiskState *s, uint64_t sector_num, uint32_t nb_sectors) { @@ -1614,11 +1615,12 @@ static void scsi_unmap_complete_noio(UnmapCBData *data, int ret) assert(r->req.aiocb == NULL); if (data->count > 0) { - r->sector = ldq_be_p(&data->inbuf[0]) - * (s->qdev.blocksize / BDRV_SECTOR_SIZE); - r->sector_count = (ldl_be_p(&data->inbuf[8]) & 0xffffffffULL) - * (s->qdev.blocksize / BDRV_SECTOR_SIZE); - if (!check_lba_range(s, r->sector, r->sector_count)) { + uint64_t sector_num = ldq_be_p(&data->inbuf[0]); + uint32_t nb_sectors = ldl_be_p(&data->inbuf[8]) & 0xffffffffULL; + r->sector = sector_num * (s->qdev.blocksize / BDRV_SECTOR_SIZE); + r->sector_count = nb_sectors * (s->qdev.blocksize / BDRV_SECTOR_SIZE); + + if (!check_lba_range(s, sector_num, nb_sectors)) { block_acct_invalid(blk_get_stats(s->qdev.conf.blk), BLOCK_ACCT_UNMAP); scsi_check_condition(r, SENSE_CODE(LBA_OUT_OF_RANGE));
check_lba_range expects sectors to be expressed in original qdev blocksize, but scsi_unmap_complete_noio was translating them to 512 block sizes, which was causing sense errors in the larger LBAs in devices using a 4k block size. Resolves: https://gitlab.com/qemu-project/qemu/-/issues/345 Signed-off-by: Kit Westneat <kit.westneat@gmail.com> --- hw/scsi/scsi-disk.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)