NetBSD Problem Report #58680

From sender@gorgnet.net  Thu Sep 19 22:55:52 2024
Return-Path: <sender@gorgnet.net>
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)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256
	 client-signature RSA-PSS (2048 bits) client-digest SHA256)
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 3E0CC1A923D
	for <gnats-bugs@gnats.NetBSD.org>; Thu, 19 Sep 2024 22:55:52 +0000 (UTC)
Message-Id: <20240919225539.2193.qmail@gorgnet.net>
Date: 19 Sep 2024 22:55:39 -0000
From: gorg@gorgnet.net
Reply-To: gorg@gorgnet.net
To: gnats-bugs@NetBSD.org
Cc: christos@NetBSD.org
Subject: [PATCH] Wrong byte order of config space write in big-endian virtio over PCI
X-Send-Pr-Version: 3.95

>Number:         58680
>Category:       kern
>Synopsis:       [PATCH] Wrong byte order of config space write in big-endian virtio over PCI
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    martin
>State:          pending-pullups
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Sep 19 23:00:00 +0000 2024
>Closed-Date:    
>Last-Modified:  Fri Dec 13 17:20:02 +0000 2024
>Originator:     gorg@gorgnet.net
>Release:        NetBSD 10.99.12
>Organization:
>Environment:
System: NetBSD 10.99.12: Wed Sep 18 15:14:20 MDT 2024 evbarm
Architecture: aarch64
Machine: evbarm
>Description:
In virtio_pci.c, the function virtio_pci_bus_space_write_8 writes the first 4
bytes and second 4 bytes of an 8-byte value in the same or swapped order
within the device's config space depending on the endianness of the system it
is compiled for. However, the BUS_ADDR_LO32 and BUS_ADDR_HI32 always produce
the least-significant and most-significant 4 bytes of an 8-byte value,
respectively, regardless of the endianness of the system. Since
virtio_pci_bus_space_write_8 is always used only for writing to little-endian
parts of the config space, the least significant 4 bytes must always be written
to the first 4 bytes in the config space value, and the most significant 4
bytes must always be written to the last 4 bytes of the config space value.
Therefore, swapping the byte order produces incorrect behaviour on big-endian
systems.

The incorrect behaviour seems to have been introduced in revision 1.19 of
virtio_pci.c, which this change would mostly reverse:

http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/pci/virtio_pci.c?rev=1.19&content-type=text/x-cvsweb-markup&only_with_tag=MAIN
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/pci/virtio_pci.c.diff?r1=1.19&r2=1.18&only_with_tag=MAIN

I suspect that this may be related to problem reports #57289 and #57290. This
issue was observed and this fix was made after the VIRTIO_F_ACCESS_PLATFORM
changed was added.

>How-To-Repeat:
One may use qemu to test virtio-over-PCI on a big endian system. For example:

qemu-system-sparc64 ... \
    -device virtio-blk-pci,bus=pciB,drive=... \
    ...

Depending on the memory layout of the particular kernel that is used, the
device may hang or qemu may produce errors about incorrect values being
written while the device fails to work.

>Fix:
Index: sys/dev/pci/virtio_pci.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/virtio_pci.c,v
retrieving revision 1.54
diff -u -r1.54 virtio_pci.c
--- sys/dev/pci/virtio_pci.c	25 Jun 2024 14:55:23 -0000	1.54
+++ sys/dev/pci/virtio_pci.c	19 Sep 2024 22:01:17 -0000
@@ -755,13 +755,8 @@
 virtio_pci_bus_space_write_8(bus_space_tag_t iot, bus_space_handle_t ioh,
     bus_size_t offset, uint64_t value)
 {
-#if _QUAD_HIGHWORD
 	bus_space_write_4(iot, ioh, offset, BUS_ADDR_LO32(value));
 	bus_space_write_4(iot, ioh, offset + 4, BUS_ADDR_HI32(value));
-#else
-	bus_space_write_4(iot, ioh, offset, BUS_ADDR_HI32(value));
-	bus_space_write_4(iot, ioh, offset + 4, BUS_ADDR_LO32(value));
-#endif
 }

 static void

>Release-Note:

>Audit-Trail:

State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Fri, 27 Sep 2024 18:49:06 +0000
State-Changed-Why:
https://mail-index.netbsd.org/source-changes/2024/09/25/msg153546.html

Module Name:    src
Committed By:   christos
Date:           Wed Sep 25 17:12:47 UTC 2024

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

Log Message:
Wrong byte order of config space write in big-endian (gorg)

In virtio_pci.c, the function virtio_pci_bus_space_write_8 writes
the first 4 bytes and second 4 bytes of an 8-byte value in the same
or swapped order within the device's config space depending on the
endianness of the system it is compiled for. However, the BUS_ADDR_LO32
and BUS_ADDR_HI32 always produce the least-significant and
most-significant 4 bytes of an 8-byte value, respectively, regardless
of the endianness of the system. Since virtio_pci_bus_space_write_8
is always used only for writing to little-endian parts of the config
space, the least significant 4 bytes must always be written to the
first 4 bytes in the config space value, and the most significant
4 bytes must always be written to the last 4 bytes of the config
space value.  Therefore, swapping the byte order produces incorrect
behaviour on big-endian systems.

The incorrect behaviour seems to have been introduced in revision
1.19 of virtio_pci.c, which this change would mostly reversed


To generate a diff of this commit:
cvs rdiff -u -r1.54 -r1.55 src/sys/dev/pci/virtio_pci.c


Responsible-Changed-From-To: kern-bug-people->martin
Responsible-Changed-By: riastradh@NetBSD.org
Responsible-Changed-When: Sat, 12 Oct 2024 22:56:08 +0000
Responsible-Changed-Why:
Can I trouble you to try this out on a big-endian system so we can get
it pulled up to 10 if appropriate?


From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/58680 ([PATCH] Wrong byte order of config space write in
 big-endian virtio over PCI)
Date: Sun, 13 Oct 2024 08:33:09 +0200

 On Sat, Oct 12, 2024 at 10:56:08PM +0000, riastradh@NetBSD.org wrote:
 > Can I trouble you to try this out on a big-endian system so we can get
 > it pulled up to 10 if appropriate?

 This can not be tested on big endian hardware, or am I misunderstanding
 something?

 Isn't it a qemu-only problem?

 Martin

State-Changed-From-To: needs-pullups->pending-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Wed, 27 Nov 2024 18:23:50 +0000
State-Changed-Why:
pullup-10 #1018 https://releng.netbsd.org/cgi-bin/req-10.cgi?show=1018
inapplicable <10


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58680 CVS commit: [netbsd-10] src/doc
Date: Fri, 13 Dec 2024 17:15:44 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Fri Dec 13 17:15:43 UTC 2024

 Modified Files:
 	src/doc [netbsd-10]: CHANGES-10.1

 Log Message:
 Typo in PR number for ticket #1018: PR 58680


 To generate a diff of this commit:
 cvs rdiff -u -r1.1.2.75 -r1.1.2.76 src/doc/CHANGES-10.1

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

>Unformatted:

Home	PR Database Search