Changeset View
Standalone View
sys/dev/virtio/pci/virtio_pci.c
Show First 20 Lines • Show All 61 Lines • ▼ Show 20 Lines | |||||
struct vtpci_virtqueue { | struct vtpci_virtqueue { | ||||
struct virtqueue *vtv_vq; | struct virtqueue *vtv_vq; | ||||
int vtv_no_intr; | int vtv_no_intr; | ||||
}; | }; | ||||
struct vtpci_softc { | struct vtpci_softc { | ||||
device_t vtpci_dev; | device_t vtpci_dev; | ||||
int vtpci_res_type; | |||||
struct resource *vtpci_res; | struct resource *vtpci_res; | ||||
struct resource *vtpci_msix_res; | struct resource *vtpci_msix_res; | ||||
uint64_t vtpci_features; | uint64_t vtpci_features; | ||||
uint32_t vtpci_flags; | uint32_t vtpci_flags; | ||||
#define VTPCI_FLAG_NO_MSI 0x0001 | #define VTPCI_FLAG_NO_MSI 0x0001 | ||||
#define VTPCI_FLAG_NO_MSIX 0x0002 | #define VTPCI_FLAG_NO_MSIX 0x0002 | ||||
#define VTPCI_FLAG_LEGACY 0x1000 | #define VTPCI_FLAG_LEGACY 0x1000 | ||||
#define VTPCI_FLAG_MSI 0x2000 | #define VTPCI_FLAG_MSI 0x2000 | ||||
▲ Show 20 Lines • Show All 192 Lines • ▼ Show 20 Lines | vtpci_probe(device_t dev) | ||||
device_set_desc_copy(dev, desc); | device_set_desc_copy(dev, desc); | ||||
return (BUS_PROBE_DEFAULT); | return (BUS_PROBE_DEFAULT); | ||||
} | } | ||||
static int | static int | ||||
vtpci_attach(device_t dev) | vtpci_attach(device_t dev) | ||||
{ | { | ||||
const int res_types[] = { SYS_RES_IOPORT, SYS_RES_MEMORY }; | |||||
struct vtpci_softc *sc; | struct vtpci_softc *sc; | ||||
device_t child; | device_t child; | ||||
int rid; | int rid; | ||||
int i; | |||||
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); | ||||
/* | |||||
* Most hypervisors export the common configuration structure in IO | |||||
bryanv: I dislike putting anything between the return value assignment and the NULL check. Do these… | |||||
* space, but some use memory space; try both. | |||||
*/ | |||||
for (i = 0; nitems(res_types); i++) { | |||||
rid = PCIR_BAR(0); | rid = PCIR_BAR(0); | ||||
sc->vtpci_res = bus_alloc_resource_any(dev, SYS_RES_IOPORT, &rid, | sc->vtpci_res_type = res_types[i]; | ||||
sc->vtpci_res = bus_alloc_resource_any(dev, res_types[i], &rid, | |||||
RF_ACTIVE); | RF_ACTIVE); | ||||
if (sc->vtpci_res != NULL) | |||||
break; | |||||
} | |||||
if (sc->vtpci_res == NULL) { | if (sc->vtpci_res == NULL) { | ||||
device_printf(dev, "cannot map I/O space\n"); | device_printf(dev, "cannot map I/O nor memory space\n"); | ||||
return (ENXIO); | return (ENXIO); | ||||
} | } | ||||
Done Inline ActionsI 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… | |||||
Done Inline ActionsI 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[] = {… | |||||
Done Inline ActionsWell, the final NULL-check has to be outside the loop, or else it will always do it wrong:
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; | ||||
Done Inline ActionsA 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? | |||||
Done Inline ActionsThe 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:
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… | |||||
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); | ||||
} | } | ||||
if (sc->vtpci_msix_res == NULL) | if (sc->vtpci_msix_res == NULL) | ||||
sc->vtpci_flags |= VTPCI_FLAG_NO_MSIX; | sc->vtpci_flags |= VTPCI_FLAG_NO_MSIX; | ||||
Show All 36 Lines | vtpci_detach(device_t dev) | ||||
if (sc->vtpci_msix_res != NULL) { | if (sc->vtpci_msix_res != NULL) { | ||||
bus_release_resource(dev, SYS_RES_MEMORY, PCIR_BAR(1), | bus_release_resource(dev, SYS_RES_MEMORY, PCIR_BAR(1), | ||||
sc->vtpci_msix_res); | sc->vtpci_msix_res); | ||||
sc->vtpci_msix_res = NULL; | sc->vtpci_msix_res = NULL; | ||||
} | } | ||||
if (sc->vtpci_res != NULL) { | if (sc->vtpci_res != NULL) { | ||||
bus_release_resource(dev, SYS_RES_IOPORT, PCIR_BAR(0), | bus_release_resource(dev, sc->vtpci_res_type, PCIR_BAR(0), | ||||
sc->vtpci_res); | sc->vtpci_res); | ||||
sc->vtpci_res = NULL; | sc->vtpci_res = NULL; | ||||
} | } | ||||
return (0); | return (0); | ||||
} | } | ||||
static int | static int | ||||
▲ Show 20 Lines • Show All 989 Lines • Show Last 20 Lines |
I dislike putting anything between the return value assignment and the NULL check. Do these before the bus_alloc calls.