Page MenuHomeFreeBSD
Differential D26915 Diff 78628 sys/dev/virtio/pci/virtio_pci.c

Changeset View

Standalone View

View Options

sys/dev/virtio/pci/virtio_pci.c

Show First 20 Lines • Show All 281 Lines • ▼ Show 20 Linesvtpci_attach(device_t dev)
sc = device_get_softc(dev); sc = device_get_softc(dev);
sc->vtpci_dev = dev; sc->vtpci_dev = dev;
pci_enable_busmaster(dev); pci_enable_busmaster(dev);
rid = PCIR_BAR(0); rid = PCIR_BAR(0);
sc->vtpci_res = bus_alloc_resource_any(dev, SYS_RES_IOPORT, &rid, sc->vtpci_res = bus_alloc_resource_any(dev, SYS_RES_IOPORT, &rid,
RF_ACTIVE); RF_ACTIVE);
if (sc->vtpci_res == NULL) { if (sc->vtpci_res == NULL) {
bryanvUnsubmitted
Done
Inline Actions

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

bryanv: I dislike putting anything between the return value assignment and the NULL check. Do these…
device_printf(dev, "cannot map I/O space\n"); /*
* We should try harder as the common configuration structure is
* possible to be in memory space.
*/
rid = PCIR_BAR(0);
sc->vtpci_res = bus_alloc_resource_any(dev, SYS_RES_MEMORY, &rid,
bryanvUnsubmitted
Done
Inline Actions

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

bryanv: A corresponding update to the detach path needs to be made. What VMM doesn't support I/O Port?
khngAuthorUnsubmitted
Done
Inline Actions

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.

khng: The change was proposed to deal with a modified version of qemu found in VPS vendors. Under the…
RF_ACTIVE);
if (sc->vtpci_res == NULL) {
device_printf(dev, "cannot map I/O nor memory space\n");
return (ENXIO); return (ENXIO);
}
} }
rpokalaUnsubmitted
Done
Inline Actions

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);
	}
rpokala: I think this could be restructured to be cleaner: ``` /* * Most hypervisors export the…
khngAuthorUnsubmitted
Done
Inline Actions

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);
		}
	}
khng: I would like to write it in a slightly different way: ``` const int res_types[] = {…
rpokalaUnsubmitted
Done
Inline Actions

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.

rpokala: Well, the final NULL-check has to be outside the loop, or else it will always do it wrong: 1.
if (pci_find_cap(dev, PCIY_MSI, NULL) != 0) if (pci_find_cap(dev, PCIY_MSI, NULL) != 0)
sc->vtpci_flags |= VTPCI_FLAG_NO_MSI; sc->vtpci_flags |= VTPCI_FLAG_NO_MSI;
if (pci_find_cap(dev, PCIY_MSIX, NULL) == 0) { if (pci_find_cap(dev, PCIY_MSIX, NULL) == 0) {
rid = PCIR_BAR(1); rid = PCIR_BAR(1);
sc->vtpci_msix_res = bus_alloc_resource_any(dev, sc->vtpci_msix_res = bus_alloc_resource_any(dev,
SYS_RES_MEMORY, &rid, RF_ACTIVE); SYS_RES_MEMORY, &rid, RF_ACTIVE);
▲ Show 20 Lines • Show All 1,046 Lines • Show Last 20 Lines