* [PATCH] hw/scsi: Fix sector translation bug in scsi_unmap_complete_noio
@ 2021-05-21 14:28 Kit Westneat
2021-05-21 15:20 ` Paolo Bonzini
0 siblings, 1 reply; 3+ messages in thread
From: Kit Westneat @ 2021-05-21 14:28 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, Kit Westneat
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));
--
2.26.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] hw/scsi: Fix sector translation bug in scsi_unmap_complete_noio
2021-05-21 14:28 [PATCH] hw/scsi: Fix sector translation bug in scsi_unmap_complete_noio Kit Westneat
@ 2021-05-21 15:20 ` Paolo Bonzini
2021-05-24 18:30 ` Kit Westneat
0 siblings, 1 reply; 3+ messages in thread
From: Paolo Bonzini @ 2021-05-21 15:20 UTC (permalink / raw)
To: Kit Westneat, qemu-devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] hw/scsi: Fix sector translation bug in scsi_unmap_complete_noio
2021-05-21 15:20 ` Paolo Bonzini
@ 2021-05-24 18:30 ` Kit Westneat
0 siblings, 0 replies; 3+ messages in thread
From: Kit Westneat @ 2021-05-24 18:30 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2722 bytes --]
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
>
>
[-- Attachment #2: Type: text/html, Size: 3760 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-05-24 18:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-21 14:28 [PATCH] hw/scsi: Fix sector translation bug in scsi_unmap_complete_noio Kit Westneat
2021-05-21 15:20 ` Paolo Bonzini
2021-05-24 18:30 ` Kit Westneat
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.