Page MenuHomeFreeBSD

virtio_pci: Allow memory space for configuration
AbandonedPublic

Authored by khng on Oct 23 2020, 9:31 AM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 30 2024, 3:53 AM
Unknown Object (File)
Dec 21 2023, 6:30 AM
Unknown Object (File)
Dec 12 2023, 2:02 PM
Unknown Object (File)
Nov 28 2023, 9:55 PM
Unknown Object (File)
Nov 28 2023, 9:36 PM
Unknown Object (File)
Nov 25 2023, 10:04 PM
Unknown Object (File)
Nov 24 2023, 2:12 AM
Unknown Object (File)
Nov 23 2023, 9:35 AM

Details

Summary

For guests running under some kind of VMMs, configuration structure is
available in memory space but not I/O space.

Reported by: Yuan Rui <number201724@me.com>
MFC after: 2 weeks

Test Plan

Ran in FreeBSD guest.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 34368
Build 31480: arc lint + arc unit

Event Timeline

khng requested review of this revision.Oct 23 2020, 9:31 AM
khng created this revision.
bryanv requested changes to this revision.Oct 24 2020, 6:31 PM
bryanv added a subscriber: bryanv.
bryanv added inline comments.
sys/dev/virtio/pci/virtio_pci.c
298

A corresponding update to the detach path needs to be made. What VMM doesn't support I/O Port?

This revision now requires changes to proceed.Oct 24 2020, 6:31 PM
  • Update the virtio-pci BAR0 detach path as well
khng marked an inline comment as done.Oct 24 2020, 6:53 PM
sys/dev/virtio/pci/virtio_pci.c
298

The change was proposed to deal with a modified version of qemu found in VPS vendors. Under the hood, a legacy virtio was presented, but with bar0's type being memory space instead of I/O space. The intention behind was for performance reason from the vendors' point of view.

Below is a line quoted from Virtio 1.1 spec, which people may find non-standard compliant:

Legacy drivers skipped the Device Layout Detection step, assuming legacy device configuration space in BAR0 in I/O space unconditionally.

However, this wasn't a problem as many other OSes' virtio PCI driver will automatically detect the bar type and issue the appropriate R/W command. For example in Linux, the pci_iomap will first find the bar type of the corresponding bar, so the driver itself does not take care of the bar type of bar0. And in Illumos, the situation is similar in rootnex_map(). More importantly, this potentially allows Arm to be supported as Arm is incapable of doing PIO unlike on x86.

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

I dislike putting anything between the return value assignment and the NULL check. Do these before the bus_alloc calls.

  • Cosmetic fixes on vtpci_attach
khng marked an inline comment as done.Oct 26 2020, 5:01 PM
rpokala added inline comments.
sys/dev/virtio/pci/virtio_pci.c
305

I think this could be restructured to be cleaner:

	/*
	 * Most hypervisors export the common configuration structure in IO
	 * space, but some use memory space; try both.
	 */
	for (sc->vtpic_res_type = SYS_RES_IOPORT;
	    sc->vtpic_res_type >= SYS_RES_MEMORY;
	    sc->vtpic_res_type--) {
		rid = PCIR_BAR(0);
		sc->vtpci_res = bus_alloc_resource_any(dev, sc->vtpic_res_type,
		    &rid, RF_ACTIVE);
		if (sc->vtpci_res != NULL)
			break;
	}
	if (sc->vtpci_res == NULL) {
		device_printf(dev, "cannot map I/O nor memory space\n");
		return (ENXIO);
	}
sys/dev/virtio/pci/virtio_pci.c
305

I would like to write it in a slightly different way:

	const int res_types[] = { SYS_RES_IOPORT, SYS_RES_MEMORY };
...
	int i;
...
	/*
	 * Most hypervisors export the common configuration structure in IO
	 * space, but some use memory space; try both.
	 */
	for (i = 0; nitems(res_type); i++) {
		rid = PCIR_BAR(0);
		sc->vtpci_res_type = res_types[i];
		sc->vtpci_res = bus_alloc_resource_any(dev, res_types[i], &rid,
		    RF_ACTIVE);
		if (sc->vtpci_res == NULL) {
			device_printf(dev, "cannot map I/O nor memory space\n");
			return (ENXIO);
		}
	}
  • Code cleanups on vtpci attach trials
khng marked an inline comment as done.Dec 10 2020, 4:32 AM
sys/dev/virtio/pci/virtio_pci.c
305

Well, the final NULL-check has to be outside the loop, or else it will always do it wrong:

  1. If it's using IOPORT, then the first time through the loop, bus_alloc_resource_any() will return non-NULL, so it will not print or return. It will go on to re-run the loop with MEMORY, bus_alloc_resource_any() *will* return NULL, and it will then do the print and return.
  2. If it's using MEMORY, then the first time through the loop, bus_alloc_resource_any() will return NULL, and it will then do the print and return.

Either way, sc->vtpci_res is NULL, the error is printed, and ENXIO is returned. So, you still need to do the break-if-non-NULL inside the loop, and move the NULL-check, print, and return outside the loop.

  • One of the resource allocation checks is mistakenly forgot
khng marked an inline comment as done.Dec 10 2020, 4:57 AM
khng planned changes to this revision.Dec 10 2020, 6:13 AM
  • Fix res_type reference typo

Looks good! Thanks for making the changes. :-)

I would really like to change bus_release_resource() to not need the RID and type (they are stored in the resource already), and it would be nice to not have to do the loop, maybe have a magic PCI_RES_BAR type picks the right type for a BAR for you, but those are all future changes.

can we get it committed and merge before 13.0 release? This patch will support the FreeBSD operating system running on alibabacloud's VM. And need another patch: D26933

I'll try to get this ported and committed to the legacy PCI driver before 13.0

This differential is superseded by D28818.