NetBSD Problem Report #57981

From www@netbsd.org  Sat Mar  2 06:11:07 2024
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id ED85F1A923A
	for <gnats-bugs@gnats.NetBSD.org>; Sat,  2 Mar 2024 06:11:06 +0000 (UTC)
Message-Id: <20240302061105.449111A923B@mollari.NetBSD.org>
Date: Sat,  2 Mar 2024 06:11:05 +0000 (UTC)
From: isaki@pastel-flower.jp
Reply-To: isaki@pastel-flower.jp
To: gnats-bugs@NetBSD.org
Subject: ld@virtio confuses F_SEG_MAX negotiation
X-Send-Pr-Version: www-1.0

>Number:         57981
>Category:       kern
>Synopsis:       ld@virtio confuses F_SEG_MAX negotiation
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Mar 02 06:15:00 +0000 2024
>Closed-Date:    Sat Mar 09 11:19:34 +0000 2024
>Last-Modified:  Sun Mar 10 04:40:01 +0000 2024
>Originator:     Tetsuya Isaki
>Release:        NetBSD-current around 20240225
>Organization:
>Environment:
NetBSD 10.99.10 virt68k
>Description:
In ld_virtio_attach(), F_SEG_MAX negotiation is confused.
(Although I cannot find what F_SEG_MAX value means exactly in
virtio 1.0 spec)

According to line 330 and 333, maxnsegs should be the number of
data segments.  If so, the comparison at line 322 is strange.
It compares the number of data segments and the number of non-data
segments.  It should be "if (maxnsegs == 0) then error"?
Or, the name of VIRTIO_BLK_MIN_SEGMENTS = 2 is misleading to me.
I'm not sure but how about VIRTIO_BLK_CTRL_SEGMENTS = 2 ?

src/sys/dev/pci/ld_virtio.c:
   73 /*
   74  * Each block request uses at least two segments - one for the header
   75  * and one for the status.
   76 */
   77 #define VIRTIO_BLK_MIN_SEGMENTS 2
    :
  261 ld_virtio_attach(device_t parent, device_t self, void *aux)
  262 {
    :
  319     if (features & VIRTIO_BLK_F_SEG_MAX) {
  320         maxnsegs = virtio_read_device_config_4(vsc,
  321             VIRTIO_BLK_CONFIG_SEG_MAX);
  322         if (maxnsegs < VIRTIO_BLK_MIN_SEGMENTS) {
  323             aprint_error_dev(sc->sc_dev,
  324                 "Too small SEG_MAX %d minimum is %d\n",
  325                 maxnsegs, VIRTIO_BLK_MIN_SEGMENTS);
  326             maxnsegs = maxxfersize / NBPG;
  327             // goto err;
  328         }
  329     } else
  330         maxnsegs = maxxfersize / NBPG;
  331
  332     /* 2 for the minimum size */
  333     maxnsegs += VIRTIO_BLK_MIN_SEGMENTS;
>How-To-Repeat:
Code review
>Fix:
N/A

>Release-Note:

>Audit-Trail:
From: "Tetsuya Isaki" <isaki@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57981 CVS commit: src/sys/dev/pci
Date: Sat, 9 Mar 2024 11:04:22 +0000

 Module Name:	src
 Committed By:	isaki
 Date:		Sat Mar  9 11:04:22 UTC 2024

 Modified Files:
 	src/sys/dev/pci: ld_virtio.c

 Log Message:
 Modify a confused expression in ld_virtio_attach().
 VIRTIO_BLK_MIN_SEGMENTS should be the total number of non-data segments,
 so I rename it to VIRTIO_BLK_CTRL_SEGMENTS.
 PR kern/57981.


 To generate a diff of this commit:
 cvs rdiff -u -r1.33 -r1.34 src/sys/dev/pci/ld_virtio.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: isaki@NetBSD.org
State-Changed-When: Sat, 09 Mar 2024 11:19:34 +0000
State-Changed-Why:
Committed.


From: Taylor R Campbell <riastradh@NetBSD.org>
To: Tetsuya Isaki <isaki@NetBSD.org>
Cc: gnats-bugs@NetBSD.org, netbsd-bugs@NetBSD.org
Subject: Re: kern/57981: ld@virtio confuses F_SEG_MAX negotiation
Date: Sat, 9 Mar 2024 13:27:44 +0000

 Does this affect netbsd-10 or earlier?  Should it be pulled up?

From: Tetsuya Isaki <isaki@pastel-flower.jp>
To: Taylor R Campbell <riastradh@NetBSD.org>
Cc: gnats-bugs@NetBSD.org, netbsd-bugs@NetBSD.org
Subject: Re: kern/57981: ld@virtio confuses F_SEG_MAX negotiation
Date: Sun, 10 Mar 2024 13:37:55 +0900

 At Sat, 9 Mar 2024 13:27:44 +0000,
 Taylor R Campbell wrote:
 > Does this affect netbsd-10 or earlier?  Should it be pulled up?

 It has been since 2015 (since netbsd-8).
 But this will not occur under normal conditions.
 So I think it's not necessary to pullup (in a hurry).

 Details:
 In the previous code, if the maxnsegs read from a virtio device
 at line 320 is less than 2, it falls back maxnsegs to larger
 value and go.  (By the way, I could not find any description about
 it in the virtio spec 1.0, but strictly speaking, it was a protocol
 violation, I think...)

 When I wrote this PR, I searched which command line option can
 specify the SEG_MAX value in QEMU a little.  But I could not find,
 so I could not fill 'how-to-repeat' field.
 If QEMU has no such option, it will never happen on QEMU.
 (It looks to use 254 (=256-2) as the SEG_MAX)

 I'm writing a virtio device on my emulator now.  So I could make
 it happen intentionally by rewriting the emulator's source code.
 But such small SEG_MAX like 1 or 2 is useless for our ld driver.
 We usually don't do it, too.
 In result, it will only happen on useless devices..

 Thanks,
 ---
 Tetsuya Isaki <isaki@pastel-flower.jp / isaki@NetBSD.org>

>Unformatted:

Home	PR Database Search