From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 93879C04FF3 for ; Mon, 24 May 2021 18:32:39 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2394E61414 for ; Mon, 24 May 2021 18:32:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2394E61414 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:37024 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1llFNW-0008H1-6y for qemu-devel@archiver.kernel.org; Mon, 24 May 2021 14:32:38 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:40812) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1llFLx-0005rd-38 for qemu-devel@nongnu.org; Mon, 24 May 2021 14:31:01 -0400 Received: from mail-qv1-xf2f.google.com ([2607:f8b0:4864:20::f2f]:40728) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1llFLu-0001Jg-Jc for qemu-devel@nongnu.org; Mon, 24 May 2021 14:31:00 -0400 Received: by mail-qv1-xf2f.google.com with SMTP id e8so11418433qvp.7 for ; Mon, 24 May 2021 11:30:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=VkUHZouAjxO7+PbfQpHrLl/qWDm2fsDo25zz63I007g=; b=vgC/lhKPLm7NkvhCWYkgs1teBnScLwnoYa+R+Vo9RCGPbWPwLbGUwZKP6Jgi0FzMj+ JYChp5K9+eE53V/XicsoUel7w5aTzdjX3PjFxe+jsM1CkBoO5cbFuq009rHlG4KWGdUK 4rQoYbnJoh77+SoOrf/OyLjIqT49WSrsVPZBjODl5JbXzoCBwA2XXn+z582KEkgtsqIG vVFS+/XCX9XcZZFdETuH/v1blYTFNgiJ5fM8tLjH7j0a95e3Cr8rUbYPZReGV4AtylAQ EX5YUqIxxLpuL40jUF1lDqnF0I0m8IOuvEv9yHeTmDjKXIT6p8AToQ/DTdZFvm6lTcSN WD/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=VkUHZouAjxO7+PbfQpHrLl/qWDm2fsDo25zz63I007g=; b=TATJ5+l2CDxX3YTQTUuZHxDepbplyquA14XuHG3TXiuTN4E5hQqJkOrLqvKICA0GW4 0aqk9w5EW3lXyhSgNSrh3NxBDchDwBV5Kaou5hB0fZhQrmrS0Qb6/Qps0K12eLyx12ps WR4oDWD1ID772dPlD/hZOez6RKUC0lsKC+IBuCY/K+dfYFfCq/EvLFqBPQB1sCngHtfV GISkg+sVkuxVliwQVR0B035nza1J5MzF307gR3gi6hcy7Mwg2uoJF+z7tSzsPNO3q9u5 A/8puq/sMsKafNLqimPD1TR/MXCOSIh5/yh3asoTUtomSB23GVvltneYx6R6xvYhDsda NC2Q== X-Gm-Message-State: AOAM531W4liKANaa+w+im3cg+mzZAJlSYvJxiHDYQqy223JF/Urda29q fnb3wNO+t/YUYgKN2x0Ft/V2ZcQ6GookMAZLmG0= X-Google-Smtp-Source: ABdhPJzc1VMVdd1OkXU0LjNYcrOHeZ5f90No8INcybMVNz9Mb4xxeDEHUa3m9A59ZUJiYXOjdHccuwnUfQy1eLvOgDs= X-Received: by 2002:a0c:f005:: with SMTP id z5mr32392041qvk.33.1621881056882; Mon, 24 May 2021 11:30:56 -0700 (PDT) MIME-Version: 1.0 References: <20210521142829.326217-1-kit.westneat@gmail.com> <56fc3bb8-d6fd-c038-2bcf-8288a5bbacab@redhat.com> In-Reply-To: <56fc3bb8-d6fd-c038-2bcf-8288a5bbacab@redhat.com> From: Kit Westneat Date: Mon, 24 May 2021 14:30:44 -0400 Message-ID: Subject: Re: [PATCH] hw/scsi: Fix sector translation bug in scsi_unmap_complete_noio To: Paolo Bonzini Content-Type: multipart/alternative; boundary="000000000000437ef205c3179c5c" Received-SPF: pass client-ip=2607:f8b0:4864:20::f2f; envelope-from=kit.westneat@gmail.com; helo=mail-qv1-xf2f.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: qemu-devel@nongnu.org Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --000000000000437ef205c3179c5c Content-Type: text/plain; charset="UTF-8" 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 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 > > --- > > 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 > > --000000000000437ef205c3179c5c Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
I started looking into adding tests, but it doesn'= ;t look like blkdebug supports changing the blocksize. I saw that it suppor= ts changing the memory alignment to 4k via the align parameter, but it does= n't implement bdrv_probe_blocksizes so the blocksize stays at 512.
<= /div>

Would it be worth adding a blocksize parameter to = blkdebug? (or maybe log-blksize & phys-blksize?) Or is there another wa= y 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 block= size, but
> scsi_unmap_complete_noio was translating them to 512 block sizes, whic= h was
> causing sense errors in the larger LBAs in devices using a 4k block si= ze.
>
> Resolves: https://gitlab.com/qemu-project/qemu= /-/issues/345
> Signed-off-by: Kit Westneat <kit.westneat@gmail.com>
> ---
>=C2=A0 =C2=A0hw/scsi/scsi-disk.c | 12 +++++++-----
>=C2=A0 =C2=A01 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:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0scsi_check_condition(r, SENSE_CODE(INVALID_F= IELD));
>=C2=A0 =C2=A0}
>=C2=A0 =C2=A0
> +/* sector_num and nb_sectors expected to be in qdev blocksize */
>=C2=A0 =C2=A0static inline bool check_lba_range(SCSIDiskState *s,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 uint64_t sec= tor_num, uint32_t nb_sectors)
>=C2=A0 =C2=A0{
> @@ -1614,11 +1615,12 @@ static void scsi_unmap_complete_noio(UnmapCBDa= ta *data, int ret)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0assert(r->req.aiocb =3D=3D NULL);
>=C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (data->count > 0) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 r->sector =3D ldq_be_p(&data->i= nbuf[0])
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * (s->qdev.blocksize / B= DRV_SECTOR_SIZE);
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 r->sector_count =3D (ldl_be_p(&dat= a->inbuf[8]) & 0xffffffffULL)
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * (s->qdev.blocksize / B= DRV_SECTOR_SIZE);
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!check_lba_range(s, r->sector, r-&= gt;sector_count)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 uint64_t sector_num =3D ldq_be_p(&dat= a->inbuf[0]);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 uint32_t nb_sectors =3D ldl_be_p(&dat= a->inbuf[8]) & 0xffffffffULL;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 r->sector =3D sector_num * (s->qdev= .blocksize / BDRV_SECTOR_SIZE);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 r->sector_count =3D nb_sectors * (s-&g= t;qdev.blocksize / BDRV_SECTOR_SIZE);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!check_lba_range(s, sector_num, nb_se= ctors)) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0block_acct_inval= id(blk_get_stats(s->qdev.conf.blk),
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BLOCK_ACCT_UNMAP);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0scsi_check_condi= tion(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

--000000000000437ef205c3179c5c--