Page MenuHomeFreeBSD
Differential D26933 Diff 79645 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 192 Lines • ▼ Show 20 Linesvtpci_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) {
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.
rid = PCIR_BAR(1); int table_rid;
sc->vtpci_msix_res = bus_alloc_resource_any(dev,
SYS_RES_MEMORY, &rid, RF_ACTIVE); rid = table_rid = pci_msix_table_bar(dev);
if (table_rid != PCIR_BAR(0)) {
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);
if (rid != table_rid) {
sc->vtpci_msix_pba_res = bus_alloc_resource_any(
dev, SYS_RES_MEMORY, &rid, RF_ACTIVE);
if (sc->vtpci_msix_pba_res != NULL)
msix_load++;
} else
msix_load++;
} }
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 (msix_load != 2) {
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;
if (sc->vtpci_msix_table_res) {
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…
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 21 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