Make access to VirtIO configuration to mips architecture.

[fernando.valle_eldorado.org.br]

Summary

According to the comment reported in review https://reviews.freebsd.org/D23401 (@arichardson ) the kernel was not compiled for mips architecture if it included virtio devices in conf sys/mips/conf/std.MALTA

The problem with mips, which is big-endian, is the different implementation of bus_space.

This patch allows the kernel to be compiled in mips architecture with virtio (tested kernel compiling in mips, powerpc64 and amd64).

Test Plan

Add virtio devices in sys/mips/conf/std.MALTA and compile the kernel:

make buildkernel TARGET=mips TARGET_ARCH=mips64 KERNCONF=MALTA64

Testing in qemu:

qemu-system-mips64 -M malta -m 2048 -nographic -kernel /path/to/kernel -drive file=/path/to/disk-test.img,format=raw,index=0,media=disk -nic user,id=net0,ipv6=off -device virtio-rng-pci

[arichardson]

sys/dev/virtio/pci/virtio_pci.c

307 ↗

(On Diff #73537)

I know there aren't any other supported BE archictectures, but I believe bs_be_tag is only defined for powerpc, so maybe using && defined(__powerpc__) makes more sense?

[jrtc27]

sys/dev/virtio/pci/virtio_pci.c

194–204 ↗

(On Diff #73537)

Surely one of MIPS and PowerPC is wrong here? vtpci_read_config_N should return the same endianness on both, and patching virtio_pci.c is just going to mask the problem.

[arichardson]

I stepped through MIPS init in QEMU and it turns out MALTA ends up with

p sc->vtpci_res->r_bustag
$7 = (bus_space_tag_t) 0xffffffff809aefc0 <gt_pci_space>

This is defined in sys/mips/malta/gt_pci_bus_space.c and uses little endian for all reads/writes.

[afscoelho_gmail.com]

sys/dev/virtio/pci/virtio_pci.c

194–204 ↗

(On Diff #73537)

I think the problem here is that legacy virtio specification mandates that the PCI configuration area must be in LE order, and the rest of pci access must be host endiannes, so in virtio_pci there must be different read modes for these areas. But once virtio_pci uses generic symbols like 'bus_read_2' that have different (and fixed) implementations accordingly with the architecture this prevents an homogeneous treatment in virtio_pci.

[jrtc27]

sys/dev/virtio/pci/virtio_pci.c

194–204 ↗

(On Diff #73537)

I think the problem here is that legacy virtio specification mandates that the PCI configuration area must be in LE order, and the rest of pci access must be host endiannes, so in virtio_pci there must be different read modes for these areas. But once virtio_pci uses generic symbols like 'bus_read_2' that have different (and fixed) implementations accordingly with the architecture this prevents an homogeneous treatment in virtio_pci.

But bus_read_2 needs to have predictable behaviour across all architectures for PCI. Either it should be in native endianness or it should be in PCI endianness. Having it be native endianness on PowerPC but PCI endianness on MIPS is emphatically wrong, regardless of which of the two is the better option. Architecture-specific differences belong in the architecture ports themselves, not littered throughout device drivers across the tree. *Endianness*-specific issues _can_ be though if the device requires something different from what the architecture-independent interface provides.

[alfredo]

sys/dev/virtio/pci/virtio_pci.c

194–204 ↗

(On Diff #73537)

Yes, virtio header must be PCI endian (LE) [1]. So, for MIPS_BE or PowerPC64_BE, a byte swap is expected to occur when vtpci_read_header_* is called.

The fact this change makes MIPS_BE work again, means that virtio driver is incorrectly receiving PCI header data in BE: an undesired conversion is being made by lower layers (bus_read_2); or implementation on QEMU side is incorrectly populating PCI header as BE.

[1] https://ozlabs.org/~rusty/virtio-spec/virtio-0.9.5.pdf - Section 2.2 pg 4

[fernando.valle_eldorado.org.br]

In this way the specificity of the mips is removed, the adjustments of the bus_space in the vtpci_* defines makes the code more generic.

[alfredo]

sys/dev/virtio/pci/virtio_pci.c

183 ↗

(On Diff #74189)

You don't need "#if _BYTE_ORDER == _BIG_ENDIAN" since le16toh and le32toh won't swap bytes if machine is LE already (amd64, for instance). Since virtio-legacy config area is guest endian and pci header is always LE, here is my suggestion:

Read path:

/* virtio-legacy PCI Configuration area is host endianess, no conversion is needed */ 
#define vtpci_read_config_2(sc, o)	bus_read_2((sc)->vtpci_res, (o))
#define vtpci_read_config_4(sc, o)	bus_read_4((sc)->vtpci_res, (o))

/* PCI Header is always read in LE */ 
#define vtpci_read_header_2(sc, o)	le16toh(bus_read_2((sc)->vtpci_res, (o)))
#define vtpci_read_header_4(sc, o)	le32toh(bus_read_4((sc)->vtpci_res, (o)))

Write path:

/* virtio-legacy PCI Configuration area is host endianess, no conversion is needed */ 
#define vtpci_write_config_2(sc, o, v)	bus_write_2((sc)->vtpci_res, (o), (v))
#define vtpci_write_config_4(sc, o, v)	bus_write_4((sc)->vtpci_res, (o), (v))

/* PCI Header is always written in LE */ 
#define vtpci_write_header_2(sc, o, v)  htole16(bus_write_2((sc)->vtpci_res, (o), (v)))
#define vtpci_write_header_4(sc, o, v)  htole32( bus_write_4((sc)->vtpci_res, (o), (v)))

[alfredo]

sys/dev/virtio/pci/virtio_pci.c

183 ↗

(On Diff #74189)

As I cannot edit my previous comment, please ignore my last comment and use this new one after we discussed online:

Se by default bus layer is tagged as LE that makes sense since PCI devices are inherently little-endian.
On FreeBSD/BE systems, bus_read_* and bus_write_* are implicitally converting from LE to BE so upper layer shouldn't care about endianess conversion. However, virtio-legacy specifies that PCI Configuration area should be guest endian, so on BE systems, bus read/write functions are doing and undesired extra swap from BE to LE.

Please add a comment explaning why an extra conversion is needed by virtio driver. Here follows new suggestion:

Read path:

/* virtio-pci specifies that PCI Configuration area is guest endian. However, since PCI devices

are inherently little-endian, on BE systems bus layer is transparently converting it to BE.
For virtio-legacy, this conversion is undesired, an extra byte swap is required to fix it.

*/
#define vtpci_read_config_2(sc, o) le16toh(bus_read_2((sc)->vtpci_res, (o)))
#define vtpci_read_config_4(sc, o) le32toh(bus_read_4((sc)->vtpci_res, (o)))

/* PCI Header LE. On BE systems bus layer is taking care of byte swapping */
#define vtpci_read_header_2(sc, o) bus_read_2((sc)->vtpci_res, (o))
#define vtpci_read_header_4(sc, o) bus_read_4((sc)->vtpci_res, (o))

Write path:

/* virtio-pci specifies that PCI Configuration area is guest endian. However, since PCI devices

are inherently little-endian, on BE systems bus layer is transparently converting it to BE.
For virtio-legacy, this conversion is undesired, an extra byte swap is required to fix it.

*/
#define vtpci_write_config_2(sc, o, v) htole16(bus_write_2((sc)->vtpci_res, (o), (v)))
#define vtpci_write_config_4(sc, o, v) htole32(bus_write_4((sc)->vtpci_res, (o), (v)))

/* PCI Header is LE. On BE systems bus layer is taking care of byte swapping */
#define vtpci_write_header_2(sc, o, v) bus_write_2((sc)->vtpci_res, (o), (v))
#define vtpci_write_header_4(sc, o, v) bus_write_4((sc)->vtpci_res, (o), (v))

[alfredo]

sys/dev/virtio/pci/virtio_pci.c

183 ↗

(On Diff #74189)

#define vtpci_write_config_2(sc, o, v) htole16(bus_write_2((sc)->vtpci_res, (o), (v)))
#define vtpci_write_config_4(sc, o, v) htole32(bus_write_4((sc)->vtpci_res, (o), (v)))

Sorry, write should be like this instead:

#define vtpci_write_config_2(sc, o, v) bus_write_2((sc)->vtpci_res, (o), htole16(v))
#define vtpci_write_config_4(sc, o, v) bus_write_4((sc)->vtpci_res, (o), htole32(v))

[arichardson]

I can confirm that this makes virtio-rng work again for MIPS64. LGTM if you apply @alfredo's suggestions.

[fernando.valle_eldorado.org.br]

@arichardson Which freebsd .img revision are you using?

[arichardson]

In D25416#569712, @fernando.valle_eldorado.org.br wrote:

@arichardson Which freebsd .img revision are you using?

I tested this with your patch applied on top of rS363177 and D25217

[arichardson]

In D25416#569712, @fernando.valle_eldorado.org.br wrote:

@arichardson Which freebsd .img revision are you using?

I can also confirm that it still works with latest HEAD (rS363360)

virtio_pci0: <VirtIO PCI Entropy adapter> irq 0 at device 19.0 on pci0
virtio_pci0: Lazy allocation of 0x20 bytes rid 0x10 type 4 at 0x140
vtrnd0: <VirtIO Entropy Adapter> on virtio_pci0
virtio_pci0: host features: 0x79000000 <EventIdx,RingIndirect,0x8000000,NotifyOnEmpty>
virtio_pci0: negotiated features: 0
random: registering fast source VirtIO Entropy Adapter

[fernando.valle_eldorado.org.br]

'if _BYTE_ORDER' removed, making the code more generic, as suggested in the comments of this revision. Comments were also added explaining why endian conversion is necessary.

[arichardson]

Minor grammar suggestions but otherwise looks good to me. Thanks!

sys/dev/virtio/pci/virtio_pci.c

185–188 ↗

(On Diff #74780)

195 ↗

(On Diff #74780)

[fernando.valle_eldorado.org.br]

Grammar correction suggestions in code comments.

[alfredo]

LGTM

[arichardson]

@alfredo/ @fernando.valle_eldorado.org.br Are you planning to commit this soon?