Changeset View
Changeset View
Standalone View
Standalone View
sys/dev/virtio/pci/virtio_pci.c
| Show First 20 Lines • Show All 272 Lines • ▼ Show 20 Lines | vtpci_probe(device_t dev) | ||||
| return (BUS_PROBE_DEFAULT); | return (BUS_PROBE_DEFAULT); | ||||
| } | } | ||||
| static int | static int | ||||
| vtpci_attach(device_t dev) | vtpci_attach(device_t dev) | ||||
| { | { | ||||
| struct vtpci_softc *sc; | struct vtpci_softc *sc; | ||||
| device_t child; | device_t child; | ||||
| int msix_load; | |||||
| int rid; | int rid; | ||||
| sc = device_get_softc(dev); | sc = device_get_softc(dev); | ||||
| sc->vtpci_dev = dev; | sc->vtpci_dev = dev; | ||||
| msix_load = 0; | |||||
| 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) { | ||||
| /* | /* | ||||
| * We should try harder as the common configuration structure is | * We should try harder as the common configuration structure is | ||||
| * possible to be in memory space. | * possible to be in memory space. | ||||
| */ | */ | ||||
| rid = PCIR_BAR(0); | rid = PCIR_BAR(0); | ||||
| sc->vtpci_res = bus_alloc_resource_any(dev, SYS_RES_MEMORY, &rid, | sc->vtpci_res = bus_alloc_resource_any(dev, SYS_RES_MEMORY, &rid, | ||||
| RF_ACTIVE); | RF_ACTIVE); | ||||
| if (sc->vtpci_res == NULL) { | if (sc->vtpci_res == NULL) { | ||||
| device_printf(dev, "cannot map I/O nor memory space\n"); | device_printf(dev, "cannot map I/O nor memory space\n"); | ||||
| return (ENXIO); | return (ENXIO); | ||||
| } | } | ||||
| } | } | ||||
| 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) { | ||||
| int table_bar; | int table_rid; | ||||
| rid = table_bar = pci_msix_table_bar(dev); | rid = table_rid = pci_msix_table_bar(dev); | ||||
| sc->vtpci_msix_table_res = bus_alloc_resource_any(dev, | if (table_rid != PCIR_BAR(0)) { | ||||
| SYS_RES_MEMORY, &rid, RF_ACTIVE); | sc->vtpci_msix_table_res = bus_alloc_resource_any( | ||||
| dev, SYS_RES_MEMORY, &rid, RF_ACTIVE); | |||||
| if (sc->vtpci_msix_table_res != NULL) | |||||
| msix_load++; | |||||
| } else | |||||
| msix_load++; | |||||
| rid = pci_msix_pba_bar(dev); | rid = pci_msix_pba_bar(dev); | ||||
jhb: I would perhaps move `msix_load` into this block along with the test for it being '2' so that… | |||||
Done Inline ActionsI forgot to submit the changes somehow. Now it is marked correctly. khng: I forgot to submit the changes somehow. Now it is marked correctly. | |||||
| if (rid != table_bar) | if (rid != table_rid) { | ||||
| sc->vtpci_msix_pba_res = bus_alloc_resource_any(dev, | sc->vtpci_msix_pba_res = bus_alloc_resource_any( | ||||
| SYS_RES_MEMORY, &rid, RF_ACTIVE); | dev, SYS_RES_MEMORY, &rid, RF_ACTIVE); | ||||
| if (sc->vtpci_msix_pba_res != NULL) | |||||
| msix_load++; | |||||
| } else | |||||
| msix_load++; | |||||
Done Inline ActionsIn theory the PBA could be back in BAR 0 but the table in BAR 1, so I think you'd have to write this as: if (rid != table_rid && rid != PCI_BAR(0)) jhb: In theory the PBA could be back in BAR 0 but the table in BAR 1, so I think you'd have to write… | |||||
| } | } | ||||
| if (sc->vtpci_msix_table_res == NULL) | if (msix_load != 2) { | ||||
Done Inline ActionsIf for some reason we allocated the table BAR but not the PBA BAR, this flag would be set incorrectly. Also, this will not work if the table or PBA are in BAR 0 since vtpci_res will have already claimed that BAR. I think you need to only allocate the extra BARs if they are both unique and not BAR 0, and you need to only set the flag if you successfully allocated all the BARs you needed to allocate. jhb: If for some reason we allocated the table BAR but not the PBA BAR, this flag would be set… | |||||
| sc->vtpci_flags |= VTPCI_FLAG_NO_MSIX; | sc->vtpci_flags |= VTPCI_FLAG_NO_MSIX; | ||||
| if (sc->vtpci_msix_table_res) { | |||||
Done Inline ActionsI think you can just leave the them allocated and not worry about doing the cleanup until detach. The PCI bus keeps them allocated all the time anyway. jhb: I think you can just leave the them allocated and not worry about doing the cleanup until… | |||||
| bus_release_resource(dev, SYS_RES_MEMORY, | |||||
| rman_get_rid(sc->vtpci_msix_table_res), | |||||
| sc->vtpci_msix_table_res); | |||||
| sc->vtpci_msix_table_res = NULL; | |||||
| } | |||||
| } | |||||
| vtpci_reset(sc); | vtpci_reset(sc); | ||||
| /* Tell the host we've noticed this device. */ | /* Tell the host we've noticed this device. */ | ||||
| vtpci_set_status(dev, VIRTIO_CONFIG_STATUS_ACK); | vtpci_set_status(dev, VIRTIO_CONFIG_STATUS_ACK); | ||||
| if ((child = device_add_child(dev, NULL, -1)) == NULL) { | if ((child = device_add_child(dev, NULL, -1)) == NULL) { | ||||
| device_printf(dev, "cannot create child device\n"); | device_printf(dev, "cannot create child device\n"); | ||||
| Show All 28 Lines | vtpci_detach(device_t dev) | ||||
| if (sc->vtpci_msix_table_res != NULL) { | if (sc->vtpci_msix_table_res != NULL) { | ||||
| bus_release_resource(dev, SYS_RES_MEMORY, | bus_release_resource(dev, SYS_RES_MEMORY, | ||||
| rman_get_rid(sc->vtpci_msix_table_res), | rman_get_rid(sc->vtpci_msix_table_res), | ||||
| sc->vtpci_msix_table_res); | sc->vtpci_msix_table_res); | ||||
| sc->vtpci_msix_table_res = NULL; | sc->vtpci_msix_table_res = NULL; | ||||
| } | } | ||||
| if (sc->vtpci_msix_pba_res != NULL) { | if (sc->vtpci_msix_pba_res != NULL) { | ||||
Not Done Inline ActionsI'm not aware of all details but on line 315 'sc->vtpci_msix_pba_res' is populated with 'bus_alloc_resource_any' result only 'if (rid != table_bar)'. Do you think similar logic is required when 'bus_release_resource' as well? alfredo: I'm not aware of all details but on line 315 'sc->vtpci_msix_pba_res' is populated with… | |||||
Not Done Inline ActionsIt seems sufficient to me, as we never populate sc->vtpci_msix_pba_res otherwise. khng: It seems sufficient to me, as we never populate sc->vtpci_msix_pba_res otherwise. | |||||
| bus_release_resource(dev, SYS_RES_MEMORY, | bus_release_resource(dev, SYS_RES_MEMORY, | ||||
| rman_get_rid(sc->vtpci_msix_pba_res), | rman_get_rid(sc->vtpci_msix_pba_res), | ||||
| sc->vtpci_msix_pba_res); | sc->vtpci_msix_pba_res); | ||||
| sc->vtpci_msix_pba_res = NULL; | sc->vtpci_msix_pba_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, SYS_RES_IOPORT, PCIR_BAR(0), | ||||
| ▲ Show 20 Lines • Show All 997 Lines • Show Last 20 Lines | |||||
I would perhaps move msix_load into this block along with the test for it being '2' so that all of the MSI-X logic is conditional on their being a PCIY_MSIX cap explicitly.