Changeset View
Standalone View
usr.sbin/bhyve/pci_virtio_net.c
| Show First 20 Lines • Show All 61 Lines • ▼ Show 20 Lines | |||||
| #include "iov.h" | #include "iov.h" | ||||
| #define VTNET_RINGSZ 1024 | #define VTNET_RINGSZ 1024 | ||||
| #define VTNET_MAXSEGS 256 | #define VTNET_MAXSEGS 256 | ||||
| #define VTNET_MAX_PKT_LEN (65536 + 64) | #define VTNET_MAX_PKT_LEN (65536 + 64) | ||||
| #define VTNET_S_HOSTCAPS \ | #define VTNET_S_HOSTCAPS \ | ||||
rgrimes: I saw that this was refered to by vmaffione, and I know this constant as IPV6 minimum MTU, but… | |||||
Not Done Inline ActionsOk, so we can use the minimum mandated by the virtio standard, which is 68 bytes. vmaffione: Ok, so we can use the minimum mandated by the virtio standard, which is 68 bytes.
https://docs. | |||||
| ( VIRTIO_NET_F_MAC | VIRTIO_NET_F_STATUS | \ | ( VIRTIO_NET_F_MAC | VIRTIO_NET_F_MTU | VIRTIO_NET_F_STATUS | \ | ||||
Not Done Inline ActionsDitto, this does not belong here. rgrimes: Ditto, this does not belong here. | |||||
Not Done Inline ActionsBut since this is a virtio device, we must follow the constraints dictated by the virtio standard, which limits the MTU to 65535. Then of course the kernel can enforce additional constraints. vmaffione: But since this is a virtio device, we must follow the constraints dictated by the virtio… | |||||
| VIRTIO_F_NOTIFY_ON_EMPTY | VIRTIO_RING_F_INDIRECT_DESC) | VIRTIO_F_NOTIFY_ON_EMPTY | VIRTIO_RING_F_INDIRECT_DESC) | ||||
| /* | /* | ||||
| * PCI config-space "registers" | * PCI config-space "registers" | ||||
| */ | */ | ||||
| struct virtio_net_config { | struct virtio_net_config { | ||||
| uint8_t mac[6]; | uint8_t mac[6]; | ||||
| uint16_t status; | uint16_t status; | ||||
| uint16_t max_virtqueue_pairs; | |||||
vmaffioneUnsubmitted Done Inline ActionsIf we add this field we should also export the VIRTIO_NET_F_MQ capability, vmaffione: If we add this field we should also export the `VIRTIO_NET_F_MQ` capability,
and set… | |||||
| uint16_t mtu; | |||||
| } __packed; | } __packed; | ||||
| /* | /* | ||||
| * Queue definitions. | * Queue definitions. | ||||
| */ | */ | ||||
| #define VTNET_RXQ 0 | #define VTNET_RXQ 0 | ||||
| #define VTNET_TXQ 1 | #define VTNET_TXQ 1 | ||||
| #define VTNET_CTLQ 2 /* NB: not yet supported */ | #define VTNET_CTLQ 2 /* NB: not yet supported */ | ||||
| ▲ Show 20 Lines • Show All 439 Lines • ▼ Show 20 Lines | |||||
| #endif | #endif | ||||
| static int | static int | ||||
| pci_vtnet_init(struct vmctx *ctx, struct pci_devinst *pi, char *opts) | pci_vtnet_init(struct vmctx *ctx, struct pci_devinst *pi, char *opts) | ||||
| { | { | ||||
| struct pci_vtnet_softc *sc; | struct pci_vtnet_softc *sc; | ||||
| char tname[MAXCOMLEN + 1]; | char tname[MAXCOMLEN + 1]; | ||||
| int mac_provided; | int mac_provided; | ||||
| uint16_t mtu = ETHERMIN; | |||||
rgrimesUnsubmitted Done Inline ActionsIsnt this going to end up with an MTU of ETHERMIN if it is not specified on command line? If I did my math right this value is 46. rgrimes: Isnt this going to end up with an MTU of ETHERMIN if it is not specified on command line? If I… | |||||
vmaffioneUnsubmitted Done Inline Actions+1, this is a default value, and should be ETHERMTU = 1500. vmaffione: +1, this is a default value, and should be `ETHERMTU` = 1500. | |||||
Done Inline ActionsI would initialize this to ETHERMTU, to keep the compiler happy and (see below). vmaffione: I would initialize this to ETHERMTU, to keep the compiler happy and (see below). | |||||
| /* | /* | ||||
| * Allocate data structures for further virtio initializations. | * Allocate data structures for further virtio initializations. | ||||
| * sc also contains a copy of vtnet_vi_consts, since capabilities | * sc also contains a copy of vtnet_vi_consts, since capabilities | ||||
| * change depending on the backend. | * change depending on the backend. | ||||
| */ | */ | ||||
| sc = calloc(1, sizeof(struct pci_vtnet_softc)); | sc = calloc(1, sizeof(struct pci_vtnet_softc)); | ||||
| sc->vsc_consts = vtnet_vi_consts; | sc->vsc_consts = vtnet_vi_consts; | ||||
| Show All 36 Lines | while (vtopts != NULL) { | ||||
| vtopts = value; | vtopts = value; | ||||
| (void) strsep(&vtopts, ","); | (void) strsep(&vtopts, ","); | ||||
| if (strcmp(key, "mac") == 0) { | if (strcmp(key, "mac") == 0) { | ||||
| err = net_parsemac(value, sc->vsc_config.mac); | err = net_parsemac(value, sc->vsc_config.mac); | ||||
| if (err) | if (err) | ||||
| break; | break; | ||||
| mac_provided = 1; | mac_provided = 1; | ||||
| } else if (strcmp(key, "mtu") == 0) { | |||||
| err = net_parsemtu(value, &mtu); | |||||
| if (err) | |||||
| break; | |||||
| } | } | ||||
| } | } | ||||
| if (err) { | if (err) { | ||||
| free(devname); | free(devname); | ||||
Not Done Inline ActionsDelete the above 4 lines, this is up to the kernel to decide what the limits are on these values. rgrimes: Delete the above 4 lines, this is up to the kernel to decide what the limits are on these… | |||||
| free(sc); | free(sc); | ||||
| return (err); | return (err); | ||||
| } | } | ||||
| err = netbe_init(&sc->vsc_be, devname, pci_vtnet_rx_callback, | err = netbe_init(&sc->vsc_be, devname, pci_vtnet_rx_callback, | ||||
| sc); | sc); | ||||
| free(devname); | free(devname); | ||||
| if (err) { | if (err) { | ||||
| free(sc); | free(sc); | ||||
| return (err); | return (err); | ||||
| } | } | ||||
| sc->vsc_consts.vc_hv_caps |= VIRTIO_NET_F_MRG_RXBUF | | sc->vsc_consts.vc_hv_caps |= VIRTIO_NET_F_MRG_RXBUF | | ||||
| netbe_get_cap(sc->vsc_be); | netbe_get_cap(sc->vsc_be); | ||||
| } | } | ||||
| if (!mac_provided) { | if (!mac_provided) { | ||||
| net_genmac(pi, sc->vsc_config.mac); | net_genmac(pi, sc->vsc_config.mac); | ||||
| } | } | ||||
| sc->vsc_config.mtu = mtu; | |||||
vmaffioneUnsubmitted Done Inline ActionsSet max_virtqueue_pairs to 1. vmaffione: Set `max_virtqueue_pairs` to 1. | |||||
Done Inline ActionsI would initialize this unconditionally, because there is no harm, and having ETHERMTU in here makes more sense than having 0, in any case. vmaffione: I would initialize this unconditionally, because there is no harm, and having ETHERMTU in here… | |||||
Not Done Inline ActionsThe only risk I see is what do old guest OS's do? If they ignore unknown feature bits then I agree with just setting it always. jhb: The only risk I see is what do old guest OS's do? If they ignore unknown feature bits then I… | |||||
Done Inline ActionsI tested Ubuntu 16.04, it ignore unknown bits. But I prefer to enable the VIRTIO_NET_F_MQ flag only if the user provide the appropriate option, as is done in QEMU. afedorov: I tested Ubuntu 16.04, it ignore unknown bits. But I prefer to enable the VIRTIO_NET_F_MQ flag… | |||||
Not Done Inline ActionsYes, they ignore unknown feature bits. They don't read unknown fields. vmaffione: Yes, they ignore unknown feature bits. They don't read unknown fields. | |||||
Done Inline ActionsSorry, of course, I mean the VIRTIO_NET_F_MTU flag. afedorov: Sorry, of course, I mean the VIRTIO_NET_F_MTU flag. | |||||
| /* initialize config space */ | /* initialize config space */ | ||||
| pci_set_cfgdata16(pi, PCIR_DEVICE, VIRTIO_DEV_NET); | pci_set_cfgdata16(pi, PCIR_DEVICE, VIRTIO_DEV_NET); | ||||
| pci_set_cfgdata16(pi, PCIR_VENDOR, VIRTIO_VENDOR); | pci_set_cfgdata16(pi, PCIR_VENDOR, VIRTIO_VENDOR); | ||||
Done Inline Actionss/virtque/virtqueue vmaffione: s/virtque/virtqueue | |||||
| pci_set_cfgdata8(pi, PCIR_CLASS, PCIC_NETWORK); | pci_set_cfgdata8(pi, PCIR_CLASS, PCIC_NETWORK); | ||||
| pci_set_cfgdata16(pi, PCIR_SUBDEV_0, VIRTIO_TYPE_NET); | pci_set_cfgdata16(pi, PCIR_SUBDEV_0, VIRTIO_TYPE_NET); | ||||
| pci_set_cfgdata16(pi, PCIR_SUBVEND_0, VIRTIO_VENDOR); | pci_set_cfgdata16(pi, PCIR_SUBVEND_0, VIRTIO_VENDOR); | ||||
| /* Link is up if we managed to open backend device. */ | /* Link is up if we managed to open backend device. */ | ||||
| sc->vsc_config.status = (opts == NULL || sc->vsc_be); | sc->vsc_config.status = (opts == NULL || sc->vsc_be); | ||||
| vi_softc_linkup(&sc->vsc_vs, &sc->vsc_consts, sc, pi, sc->vsc_queues); | vi_softc_linkup(&sc->vsc_vs, &sc->vsc_consts, sc, pi, sc->vsc_queues); | ||||
| ▲ Show 20 Lines • Show All 97 Lines • Show Last 20 Lines | |||||
I saw that this was refered to by vmaffione, and I know this constant as IPV6 minimum MTU, but it it NOT a link layer function of a network device. Ethernets minimum MTU is much smaller, and again unless there is some great reason to inforce these in the userland it should be the kernel that decides these limits, and it ultimately has to protect itself against out of range values anyway. This does NOT belong in userland code.