Page MenuHomeFreeBSD
Differential D26933 Diff 78704 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 62 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;
struct resource *vtpci_res; struct resource *vtpci_res;
struct resource *vtpci_msix_res; struct resource *vtpci_msix_table_res;
struct resource *vtpci_msix_pba_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
#define VTPCI_FLAG_MSIX 0x4000 #define VTPCI_FLAG_MSIX 0x4000
#define VTPCI_FLAG_SHARED_MSIX 0x8000 #define VTPCI_FLAG_SHARED_MSIX 0x8000
▲ Show 20 Lines • Show All 219 Lines • ▼ Show 20 Lines 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) {
rid = PCIR_BAR(1); rid = pci_msix_table_bar(dev);
sc->vtpci_msix_res = bus_alloc_resource_any(dev, sc->vtpci_msix_table_res = bus_alloc_resource_any(dev,
jhbUnsubmitted
Done
Inline Actions

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.

jhb: I would perhaps move `msix_load` into this block along with the test for it being '2' so that…
khngAuthorUnsubmitted
Done
Inline Actions

I forgot to submit the changes somehow. Now it is marked correctly.

khng: I forgot to submit the changes somehow. Now it is marked correctly.
SYS_RES_MEMORY, &rid, RF_ACTIVE); SYS_RES_MEMORY, &rid, RF_ACTIVE);
rid = pci_msix_pba_bar(dev);
/* Should return NULL if Table BAR and PBA BAR are the same */
sc->vtpci_msix_pba_res = bus_alloc_resource_any(dev,
SYS_RES_MEMORY, &rid, RF_ACTIVE);
KASSERT(sc->vtpci_msix_pba_res != sc->vtpci_msix_table_res,
("%s: Confusing bus_alloc_resource_any results", __func__));
} }
jhbUnsubmitted
Done
Inline Actions

In 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_res == NULL) if (sc->vtpci_msix_table_res == NULL)
jhbUnsubmitted
Done
Inline Actions

If 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;
jhbUnsubmitted
Done
Inline Actions

I 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…
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");
vtpci_set_status(dev, VIRTIO_CONFIG_STATUS_FAILED); vtpci_set_status(dev, VIRTIO_CONFIG_STATUS_FAILED);
Show All 20 Lines if ((child = sc->vtpci_child_dev) != NULL) {
error = device_delete_child(dev, child); error = device_delete_child(dev, child);
if (error) if (error)
return (error); return (error);
sc->vtpci_child_dev = NULL; sc->vtpci_child_dev = NULL;
} }
vtpci_reset(sc); vtpci_reset(sc);
if (sc->vtpci_msix_res != NULL) { if (sc->vtpci_msix_table_res != NULL) {
bus_release_resource(dev, SYS_RES_MEMORY, PCIR_BAR(1), bus_release_resource(dev, SYS_RES_MEMORY,
sc->vtpci_msix_res); rman_get_rid(sc->vtpci_msix_table_res),
sc->vtpci_msix_res = NULL; sc->vtpci_msix_table_res);
sc->vtpci_msix_table_res = NULL;
}
if (sc->vtpci_msix_pba_res != NULL) {
alfredoUnsubmitted
Not Done
Inline Actions

I'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…
khngAuthorUnsubmitted
Not Done
Inline Actions

It 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,
rman_get_rid(sc->vtpci_msix_pba_res),
sc->vtpci_msix_pba_res);
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),
sc->vtpci_res); sc->vtpci_res);
sc->vtpci_res = NULL; sc->vtpci_res = NULL;
} }
▲ Show 20 Lines • Show All 993 Lines • Show Last 20 Lines