virtio: remove byte conversion function calls in modern pci
ClosedPublic
Actions

Authored by timo.voelker_fh-muenster.de on Tue, Jun 2, 11:39 AM.

Details

Reviewers

ivy
tuexen
adrian
alfredo
bryanv
leonardo.bianconi_eldorado.org.br

Group Reviewers

PowerPC

Commits

rG07b5d1ca52b1: virtio_pci_modern: Remove endianness conversion for config space

Summary

Remove function calls in virtio_pci_modern.c that convert the byte order. This fixes a bug reported here:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=294706

Reading a value from the PCI bus with bus_read_x() (with x > 1) requires a byte conversion on a big-endian system, because the PCI bus uses little endian. However, this conversion is done by the bus_space_read_x() functions, which are used (bus_read_x() are macros defined in sys/sys/bus.h that map to bus_space_read_x()). Thus, another byte conversion in virtio_pci_modern.c is incorrect.

The byte order the PCI device uses is irrelevant. For instance, in a big-endian guest, the VirtIO PCI device uses big endian in legacy mode and little endian in modern mode. However, since the data are transferred over the PCI bus, single values arrive in little endian no matter how the PCI device stores them.

QEMU tends to provide a transitional VirtIO PCI device, which means it can run in both modes, legacy and modern. In a FreeBSD guest, the loader tunable hw.virtio.pci.transitional decides which mode is used. If it is 0, legacy mode is used. If it is 1, modern mode is used. The default value had been 0 until D55894, which changed the default to 1. Since then, modern mode is probably used more often, and that discovered this long-existing bug.

Test Plan

I tested this with a FreeBSD 16-CURRENT guest in three environments and got the same result: modern mode did not work until applying this patch.

1
Host: MacBook with Apple Silicon CPU and macOS (Little Endian).
Hypervisor: QEMU (UTM)
Guest config:
-machine pseries-10.0
(CPU default)
-smp cpus=1,sockets=1,cores=1,threads=1
-device virtio-net-pci,...

2
Host: Raptor Blackbird with Power9 CPU and Linux Debian (Little Endian).
Hypervisor: QEMU (Virt-Manager)
Guest config:
-machine pseries-10.0,usb=off,cap-hpt-max-page-size=4k,dump-guest-core=off,memory-backend=ppc_spapr.ram
-accel kvm
-cpu POWER9
-smp 16,sockets=16,cores=1,threads=1
-device {"driver":"virtio-net-pci",...}

3
Host: Raptor Blackbird with Power9 CPU and FreeBSD 16-CURRENT (Big Endian).
Hypervisor: QEMU (qemu-system-ppc64)
Guest config:
qemu-system-ppc64 -name bsdppc-vm -cpu POWER9 -smp 16 -M pseries-10.0 -m 8G -drive file=/home/msvoelker/bsdppc-vm/disk.qcow2,if=virtio,format=qcow2 -netdev tap,id=net0,ifname=tap0,script=no,downscript=no -device virtio-net-pci,netdev=net0 -nographic

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

timo.voelker_fh-muenster.de created this revision.Tue, Jun 2, 11:39 AM

Herald added a subscriber: imp. · View Herald TranscriptTue, Jun 2, 11:39 AM

timo.voelker_fh-muenster.de requested review of this revision.Tue, Jun 2, 11:39 AM

Neat! So hm what devices are on the virtio pci bus? storage, net, anything else? I'm just trying to imagine the device testing map for this change.

Thanks for chasing this down!

In D57392#1315045, @adrian wrote:

Neat! So hm what devices are on the virtio pci bus? storage, net, anything else? I'm just trying to imagine the device testing map for this change.

Thanks for chasing this down!

I'm not a VirtIO expert. As far as I know, every VirtIO device can use the PCI bus to attach to the system.

In sys/dev/virtio, each subfolder stands for one implemented VirtIO driver implementation (except pci). QEMU can emulate the corresponding device. On my macOS, I get this list

qemu-system-aarch64 -device help | grep virt.\*pci\"
name "virtio-9p-pci", bus PCI, alias "virtio-9p"
name "virtio-blk-pci", bus PCI, alias "virtio-blk"
name "virtio-scsi-pci", bus PCI, alias "virtio-scsi"
name "virtio-net-pci", bus PCI, alias "virtio-net"
name "virtio-keyboard-pci", bus PCI, alias "virtio-keyboard"
name "virtio-mouse-pci", bus PCI, alias "virtio-mouse"
name "virtio-multitouch-pci", bus PCI
name "virtio-serial-pci", bus PCI, alias "virtio-serial"
name "virtio-tablet-pci", bus PCI, alias "virtio-tablet"
name "virtio-gpu-pci", bus PCI, alias "virtio-gpu"
name "virtio-sound-pci", bus PCI, alias "virtio-sound", desc "Virtio Sound"
name "virtio-balloon-pci", bus PCI, alias "virtio-balloon"
name "virtio-crypto-pci", bus PCI
name "virtio-iommu-pci", bus PCI, alias "virtio-iommu"
name "virtio-rng-pci", bus PCI, alias "virtio-rng"

I focused my test on virtio-net-pci but used some other VirtIO devices (mostly virtio-blk-pci) as well. If you have the chance to test other VirtIO devices, that would be great.

In D57392#1315323, @timo.voelker_fh-muenster.de wrote:

In D57392#1315045, @adrian wrote:

Neat! So hm what devices are on the virtio pci bus? storage, net, anything else? I'm just trying to imagine the device testing map for this change.

Thanks for chasing this down!

I'm not a VirtIO expert. As far as I know, every VirtIO device can use the PCI bus to attach to the system.

In sys/dev/virtio, each subfolder stands for one implemented VirtIO driver implementation (except pci). QEMU can emulate the corresponding device. On my macOS, I get this list

qemu-system-aarch64 -device help | grep virt.\*pci\"
name "virtio-9p-pci", bus PCI, alias "virtio-9p"
name "virtio-blk-pci", bus PCI, alias "virtio-blk"
name "virtio-scsi-pci", bus PCI, alias "virtio-scsi"
name "virtio-net-pci", bus PCI, alias "virtio-net"
name "virtio-keyboard-pci", bus PCI, alias "virtio-keyboard"
name "virtio-mouse-pci", bus PCI, alias "virtio-mouse"
name "virtio-multitouch-pci", bus PCI
name "virtio-serial-pci", bus PCI, alias "virtio-serial"
name "virtio-tablet-pci", bus PCI, alias "virtio-tablet"
name "virtio-gpu-pci", bus PCI, alias "virtio-gpu"
name "virtio-sound-pci", bus PCI, alias "virtio-sound", desc "Virtio Sound"
name "virtio-balloon-pci", bus PCI, alias "virtio-balloon"
name "virtio-crypto-pci", bus PCI
name "virtio-iommu-pci", bus PCI, alias "virtio-iommu"
name "virtio-rng-pci", bus PCI, alias "virtio-rng"

I focused my test on virtio-net-pci but used some other VirtIO devices (mostly virtio-blk-pci) as well. If you have the chance to test other VirtIO devices, that would be great.

When using an arm64 VM with UTO on macOS, I get

tuexen@head:~ % dmesg | grep "VirtIO PCI"
virtio_pci0: <VirtIO PCI (modern) Network adapter> port 0x140-0x15f mem 0x1006e000-0x1006efff,0x10058000-0x1005bfff at device 1.0 on pci0
virtio_pci1: <VirtIO PCI (modern) Network adapter> port 0x120-0x13f mem 0x1006d000-0x1006dfff,0x10054000-0x10057fff at device 2.0 on pci0
virtio_pci2: <VirtIO PCI (modern) GPU adapter> mem 0x1006c000-0x1006cfff,0x10050000-0x10053fff at device 3.0 on pci0
virtio_pci3: <VirtIO PCI (modern) Block adapter> mem 0x1006b000-0x1006bfff,0x1004c000-0x1004ffff at device 7.0 on pci0
virtio_pci4: <VirtIO PCI (modern) Console adapter> port 0xc0-0xff mem 0x1006a000-0x1006afff,0x10048000-0x1004bfff at device 8.0 on pci0
virtio_pci5: <VirtIO PCI (modern) Entropy adapter> port 0x100-0x11f mem 0x10069000-0x10069fff,0x10044000-0x10047fff at device 9.0 on pci0
virtio_pci6: <VirtIO PCI (modern) Balloon adapter> port 0x80-0xbf mem 0x10068000-0x10068fff,0x10040000-0x10043fff at device 10.0 on pci0

These work fine with that patch. Which is expected since guest and host are little endian, but it is a data point.

adrian accepted this revision.Mon, Jun 22, 4:19 PM

This revision is now accepted and ready to land.Mon, Jun 22, 4:19 PM

The change itself looks fine but I think the commit message needs rewriting. It is quite verbose on irrelevant details (like how bus_(space_)* foo work under the hood), and talks about data on the PCI bus (which is implementation, not architecture). Also the bug reference should follow the proper format (see tools/tools/git/hooks/prepare-commit-msg). Honestly the commit message would be better if it was entirely replaced with something like:

virtio_pci_modern: Remove endianness conversion for config space

The bus_* functions already handle converting from PCI endianness (i.e.
little-endian) to native endianness when accessing the config space (see
ofw_pcib_bus_get_bus_tag), so converting again with virtio_htogX/virtio_gtohX
undoes any byte-swapping and breaks big-endian systems. They should only be
used for operating on shared memory.

Note part of this reverts commit fb53b42e36a9 ("virtio-modern: fix PCI
common read/write functions on big endian targets").

PR		294706
Fixes:		fb53b42e36a9 ("virtio-modern: fix PCI common read/write functions on big endian targets")
Fixes:		9da9560c4dd3 ("virtio: Add VirtIO PCI modern (V1) support")
MFC after:	1 week

I do wonder about fb53b42e36a9 though. Was that tested, and if so, how did it work? Perhaps some other PCI controller drivers aren't getting the config space endianness right? (I assume you're all testing on systems that are using ofw_pcib?)

In D57392#1323920, @jrtc27 wrote:
The change itself looks fine but I think the commit message needs rewriting. It is quite verbose on irrelevant details (like how bus_(space_)* foo work under the hood), and talks about data on the PCI bus (which is implementation, not architecture). Also the bug reference should follow the proper format (see tools/tools/git/hooks/prepare-commit-msg). Honestly the commit message would be better if it was entirely replaced with something like:
virtio_pci_modern: Remove endianness conversion for config space

The bus_* functions already handle converting from PCI endianness (i.e.
little-endian) to native endianness when accessing the config space (see
ofw_pcib_bus_get_bus_tag), so converting again with virtio_htogX/virtio_gtohX
undoes any byte-swapping and breaks big-endian systems. They should only be
used for operating on shared memory.

Note part of this reverts commit fb53b42e36a9 ("virtio-modern: fix PCI
common read/write functions on big endian targets").

PR		294706
Fixes:		fb53b42e36a9 ("virtio-modern: fix PCI common read/write functions on big endian targets")
Fixes:		9da9560c4dd3 ("virtio: Add VirtIO PCI modern (V1) support")
MFC after:	1 week

I totally agree. The summary I wrote here was not intended as a commit message.

I like your suggested commit message, but I'm struggling with the term config space. The change also affects access to a BAR region. The config space describes a BAR, but is the BAR region part of the config space?

I do wonder about fb53b42e36a9 though. Was that tested, and if so, how did it work? Perhaps some other PCI controller drivers aren't getting the config space endianness right? (I assume you're all testing on systems that are using ofw_pcib?)

I'm wondering about that too. That's one reason why I put some more details in the summary. If I remember correctly, all three of my test systems use ofw_pcib. However, I don't think it's different with another PCI controller because the bus_space API takes care of the conversion. At least this is how I understand this sentence in bus_space(9):

Most of the bus_space functions [those that do not contain stream in their name] imply a host byte-order and a bus byte-order and take care of any translation for the caller.

tuexen accepted this revision.Sun, Jun 28, 10:22 AM

Closed by commit rG07b5d1ca52b1: virtio_pci_modern: Remove endianness conversion for config space (authored by timo.voelker_fh-muenster.de, committed by tuexen). · Explain WhySun, Jun 28, 10:46 AM

This revision was automatically updated to reflect the committed changes.

tuexen added a commit: rG07b5d1ca52b1: virtio_pci_modern: Remove endianness conversion for config space.