Make access to VirtIO configuration with proper endianness on big endian platforms

[afscoelho_gmail.com]

Summary

In legacy VirtIO drivers, the header must be PCI endianness (little) and the device-specific region is encoded in the native endian of the guest.

This patch makes the access (read/write) to VirtIO header using the little endian order. Other read and write access are native endianness. This patch also sets the device's IO region as big endian if we are in a big endian machine, otherwise little endian was assumed.

Test Plan

One can execute a VM instance with QEMU using virtio drivers, like net and block for example:

qemu-system-ppc64 -drive file=disc1.qcow2,if=virtio,format=qcow2 -machine pseries,accel=kvm,cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-hpt-max-page-size=16M -mem-prealloc -mem-path /dev/hugepages -nographic -vga none -smp 20 -m 4G -net tap -netdev tap,id=n1 -device virtio-net-pci,netdev=n1 -global virtio-pci.disable-modern=on

[bdragon]

Ohhhhhh, so THAT is why this has been such a headache?

This is an awesome find.

[jhibbits]

Adding @bryanv since he wrote and maintains the virtio drivers.

[alfredo]

nice job @afscoelho_gmail.com !
I just tried your patch in a image uploaded to a POWER8 instance at https://openpower.ic.unicamp.br/minicloud/ and it worked out of the box.
Please check my comment on the diff.

sys/dev/virtio/pci/virtio_pci.c

194 ↗

(On Diff #67419)

On vtpci_write_header_2 you should change le16toh -> *htole16**, and same for the 32 bit version of this macro. The result will be the same but it explains better the idea of forcing the PCI header to be LE no matter what endianess the "host" is (host=="guest VM" in this case)

[afscoelho_gmail.com]

In D23401#513326, @bdragon wrote:

Ohhhhhh, so THAT is why this has been such a headache?

This is an awesome find.

Thanks @bdragon!

[afscoelho_gmail.com]

In D23401#513986, @alfredo.junior_eldorado.org.br wrote:

nice job @afscoelho_gmail.com !
I just tried your patch in a image uploaded to a POWER8 instance at https://openpower.ic.unicamp.br/minicloud/ and it worked out of the box.
Please check my comment on the diff.

Sure, it is much clearer to use htole{16,32} when writing, thanks @alfredo.junior_eldorado.org.br !

[afscoelho_gmail.com]

Use functions htole16 and htole32 when writing VirtIO header instead of le16toh and le32toh for better comprehension.

[bryanv]

sys/dev/virtio/pci/virtio_pci.c

189 ↗

(On Diff #67519)

Typo: VirtioIO

196 ↗

(On Diff #67519)

Please use the vtpci_write_config macros for consistency with the read in this same block

293 ↗

(On Diff #67519)

The block indent appears off here.

294 ↗

(On Diff #67519)

Typo: configurarion

I would reword the first part of this sentence slightly to better match the wordage from the spec, something like

For legacy VirtIO, the device-specific configuration is guest endian, while the common configuration header is always PCI (little) endian...

[afscoelho_gmail.com]

Use the vtpci_write_config in vtpci_write_header macros for consistency. Typo fixes.

[afscoelho_gmail.com]

Thanks @bryanv for the review.

[bdragon]

sys/dev/virtio/pci/virtio_pci.c

189 ↗

(On Diff #67521)

There are some trailing spaces in this block.

1209 ↗

(On Diff #67521)

some spaces leaked into the indentation on this line.

[afscoelho_gmail.com]

Removing extra white spaces.

[afscoelho_gmail.com]

Removing blank line.

[bdragon]

This is working nicely for me.

[arichardson]

This commit appears to have broken using QEMU MALTA (MIPS64 BE) with -device virtio-rng-pci.
With this change I get an infinite loop on boot that goes away if I revert it:

    at /Users/alex/cheri/cheribsd/sys/dev/virtio/virtqueue.c:600
600             while ((cookie = virtqueue_dequeue(vq, len)) == NULL) {
(gdb) bt
#0  virtqueue_poll (vq=0xc00000000025a000, len=0xc00000000e7ff86c)
    at /Users/alex/cheri/cheribsd/sys/dev/virtio/virtqueue.c:600
#1  0xffffffff803290bc in vtrnd_harvest (sc=<optimized out>, buf=0xc00000000e7ff958, sz=<optimized out>)
    at /Users/alex/cheri/cheribsd/sys/dev/virtio/random/virtio_random.c:237
#2  vtrnd_read (buf=0x0 [,0xfffffffffffff95c-0xfffffffffffff9f9] (sealed), usz=8)
    at /Users/alex/cheri/cheribsd/sys/dev/virtio/random/virtio_random.c:263
#3  0xffffffff803077a0 in random_sources_feed () at /Users/alex/cheri/cheribsd/sys/dev/random/random_harvestq.c:261
#4  random_kthread () at /Users/alex/cheri/cheribsd/sys/dev/random/random_harvestq.c:195
#5  0xffffffff80469838 in fork_exit (callout=0xffffffff803074f8 <random_kthread>, arg=0x9800000006c0a520, 
    frame=0xc00000000e7ffa20) at /Users/alex/cheri/cheribsd/sys/kern/kern_fork.c:1059
#6  0xffffffff807f5f98 in fork_trampoline () at /Users/alex/cheri/cheribsd/sys/mips/mips/swtch.S:84

head/sys/dev/virtio/pci/virtio_pci.c

301

This causes compilation errors on MIPS64 with virtio enabled

[afscoelho_gmail.com]

In D23401#554073, @arichardson wrote:

This commit appears to have broken using QEMU MALTA (MIPS64 BE) with -device virtio-rng-pci.
With this change I get an infinite loop on boot that goes away if I revert it:

I tested -device virtio-rng-pci with qemu-system-ppc64 and had no issues. Some questions:

• Did you use -global virtio-pci.disable-modern=on ?
• If you don't use -device virtio-rng-pci it works?
• What is the compilation error?

[arichardson]

In D23401#554153, @afscoelho_gmail.com wrote:

In D23401#554073, @arichardson wrote:

This commit appears to have broken using QEMU MALTA (MIPS64 BE) with -device virtio-rng-pci.
With this change I get an infinite loop on boot that goes away if I revert it:

I tested -device virtio-rng-pci with qemu-system-ppc64 and had no issues. Some questions:

• Did you use -global virtio-pci.disable-modern=on ?

I just tried and this flag does not seem to make any difference, it also hangs.

• If you don't use -device virtio-rng-pci it works?

Yes, it works in that case. If the device isn't there it doesn't use vtrnd_harvest()

• What is the compilation error?

sys/dev/virtio/pci/virtio_pci.c:301:34: error: use of undeclared identifier 'bs_be_tag'

[fernando.valle_eldorado.org.br]

@arichardson Could you say what is the complete command you use to run this vm on qemu? (./qemu-system-mips64 -M malta (...) what else?)
I reversed this commit and compiled the kernel and would like to test it according to your environment.

[arichardson]

In D23401#555913, @fernando.valle_eldorado.org.br wrote:

@arichardson Could you say what is the complete command you use to run this vm on qemu? (./qemu-system-mips64 -M malta (...) what else?)
I reversed this commit and compiled the kernel and would like to test it according to your environment.

I have the following patch to enable virtio for std.MALTA (uploaded as D25217):

diff --git a/sys/mips/conf/std.MALTA b/sys/mips/conf/std.MALTA
index 26940db1b92f..787b3962caea 100644
--- a/sys/mips/conf/std.MALTA
+++ b/sys/mips/conf/std.MALTA
@@ -55,3 +55,13 @@ device               miibus
 device         bpf
 device         md
 device         uart
+
+# VirtIO support
+device         virtio          # Generic VirtIO bus (required)
+device         virtio_pci      # VirtIO PCI device
+device         vtnet           # VirtIO Ethernet device
+device         virtio_blk      # VirtIO Block device
+device         virtio_mmio     # VirtIO MMIO bus
+device         virtio_random       # VirtIO RNG
+device         virtio_console      # VirtIO Console
+

Then I use
qemu-system-mips64 -M malta -m 2048 -nographic -kernel /home/alr48/cheri/output/freebsd-mips/boot/kernel/kernel -drive file=/home/alr48/cheri/output/freebsd-mips.img,format=raw,index=0,media=disk -nic 'user,id=net0,ipv6=off,smb=/exports/users/alr48/sources -device virtio-rng-pci to boot.

This works for me, but it freezes on startup if I have this change included.
I use the following (probably incorrect) patch to allow it to compile on MIPS since bs_be_tag doesn't exist there:

diff --git a/sys/dev/virtio/pci/virtio_pci.c b/sys/dev/virtio/pci/virtio_pci.c
index aba35eb38ff5..7b48b771c417 100644
--- a/sys/dev/virtio/pci/virtio_pci.c
+++ b/sys/dev/virtio/pci/virtio_pci.c
@@ -297,7 +297,7 @@ vtpci_attach(device_t dev)
  * other parts of this file via functions
  * 'vtpci_[read|write]_header_[2|4]'
  */
-#if _BYTE_ORDER == _BIG_ENDIAN
+#if _BYTE_ORDER == _BIG_ENDIAN && defined(__powerpc__)
        rman_set_bustag(sc->vtpci_res, &bs_be_tag);
 #endif

[arichardson]

In D23401#555947, @arichardson wrote:

In D23401#555913, @fernando.valle_eldorado.org.br wrote:

@arichardson Could you say what is the complete command you use to run this vm on qemu? (./qemu-system-mips64 -M malta (...) what else?)
I reversed this commit and compiled the kernel and would like to test it according to your environment.

I have the following patch to enable virtio for std.MALTA (uploaded as D25217):

diff --git a/sys/mips/conf/std.MALTA b/sys/mips/conf/std.MALTA
index 26940db1b92f..787b3962caea 100644
--- a/sys/mips/conf/std.MALTA
+++ b/sys/mips/conf/std.MALTA
@@ -55,3 +55,13 @@ device               miibus
 device         bpf
 device         md
 device         uart
+
+# VirtIO support
+device         virtio          # Generic VirtIO bus (required)
+device         virtio_pci      # VirtIO PCI device
+device         vtnet           # VirtIO Ethernet device
+device         virtio_blk      # VirtIO Block device
+device         virtio_mmio     # VirtIO MMIO bus
+device         virtio_random       # VirtIO RNG
+device         virtio_console      # VirtIO Console
+

Then I use
qemu-system-mips64 -M malta -m 2048 -nographic -kernel /home/alr48/cheri/output/freebsd-mips/boot/kernel/kernel -drive file=/home/alr48/cheri/output/freebsd-mips.img,format=raw,index=0,media=disk -nic 'user,id=net0,ipv6=off,smb=/exports/users/alr48/sources -device virtio-rng-pci to boot.

This works for me, but it freezes on startup if I have this change included.
I use the following (probably incorrect) patch to allow it to compile on MIPS since bs_be_tag doesn't exist there:

diff --git a/sys/dev/virtio/pci/virtio_pci.c b/sys/dev/virtio/pci/virtio_pci.c
index aba35eb38ff5..7b48b771c417 100644
--- a/sys/dev/virtio/pci/virtio_pci.c
+++ b/sys/dev/virtio/pci/virtio_pci.c
@@ -297,7 +297,7 @@ vtpci_attach(device_t dev)
  * other parts of this file via functions
  * 'vtpci_[read|write]_header_[2|4]'
  */
-#if _BYTE_ORDER == _BIG_ENDIAN
+#if _BYTE_ORDER == _BIG_ENDIAN && defined(__powerpc__)
        rman_set_bustag(sc->vtpci_res, &bs_be_tag);
 #endif

If I do this, it freezes just after

tcp_init: net.inet.tcp.tcbhashsize auto tuned to 32768
Obsolete code will be removed soon: random(9) is the obsolete Park-Miller LCG from 1988

Attaching GDB to QEMU gives the following backtrace:

Program received signal SIGINT, Interrupt.
virtqueue_poll (vq=0xc000000000245000, len=0xc00000000e7a1b7c)
    at /exports/users/alr48/sources/freebsd/sys/dev/virtio/virtqueue.c:601
601		while ((cookie = virtqueue_dequeue(vq, len)) == NULL) {
(gdb) bt
#0  virtqueue_poll (vq=0xc000000000245000, len=0xc00000000e7a1b7c)
    at /exports/users/alr48/sources/freebsd/sys/dev/virtio/virtqueue.c:601
#1  0xffffffff802c896c in vtrnd_harvest (sc=<optimized out>, buf=0xc00000000e7a1c68,
    sz=<optimized out>)
    at /exports/users/alr48/sources/freebsd/sys/dev/virtio/random/virtio_random.c:237
#2  vtrnd_read (buf=0xc00000000e7a1c68, usz=8)
    at /exports/users/alr48/sources/freebsd/sys/dev/virtio/random/virtio_random.c:263
#3  0xffffffff802a6938 in random_sources_feed ()
    at /exports/users/alr48/sources/freebsd/sys/dev/random/random_harvestq.c:252
#4  random_kthread ()
    at /exports/users/alr48/sources/freebsd/sys/dev/random/random_harvestq.c:195
#5  0xffffffff803f2190 in fork_exit (callout=0xffffffff802a6660 <random_kthread>,
    arg=0x0, frame=0xc00000000e7a1d30)
    at /exports/users/alr48/sources/freebsd/sys/kern/kern_fork.c:1052
#6  0xffffffff80713170 in fork_trampoline ()
    at /exports/users/alr48/sources/freebsd/sys/mips/mips/swtch.S:77

(To get GDB to attach I pass -gdb tcp::17923 -S to QEMU and run
/home/alr48/cheri/output/sdk/bin/gdb /home/alr48/cheri/output/freebsd-mips/boot/kernel/kernel '--init-eval-command=set sysroot /home/alr48/cheri/output/freebsd-mips' '--eval-command=break panic' '--eval-command=target remote localhost:17923' --eval-command=continue)