On Thu, Dec 21, 2017 at 02:42:48PM -0500, Jason Baron wrote: > > > On 12/20/2017 09:33 AM, Yan Vugenfirer wrote: > > > >> On 20 Dec 2017, at 16:31, Michael S. Tsirkin <m...@redhat.com> wrote: > >> > >> On Tue, Dec 19, 2017 at 11:52:39AM -0500, Jason Baron wrote: > >>> > >>> > >>> On 12/19/2017 04:19 AM, Yan Vugenfirer wrote: > >>>> > >>>>> On 18 Dec 2017, at 18:04, Jason Baron via Qemu-devel > >>>>> <qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>> wrote: > >>>>> > >>>>> > >>>>> > >>>>> On 12/18/2017 06:34 AM, Yan Vugenfirer wrote: > >>>>>> > >>>>>>> On 14 Dec 2017, at 21:33, Jason Baron via Qemu-devel > >>>>>>> <qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>> wrote: > >>>>>>> > >>>>>>> Although they can be currently set in linux via 'ethtool -s', this > >>>>>>> requires > >>>>>>> guest changes, and thus it would be nice to extend this > >>>>>>> functionality such > >>>>>>> that it can be configured automatically from the host (as other > >>>>>>> network > >>>>>>> do). > >>>>>>> > >>>>>>> Linkspeed and duplex settings can be set as: > >>>>>>> '-device virtio-net,speed=10000,duplex=full' > >>>>>>> > >>>>>>> where speed is [-1...INT_MAX], and duplex is ["half"|"full"]. > >>>>>>> > >>>>>>> Signed-off-by: Jason Baron <jba...@akamai.com > >>>>>>> <mailto:jba...@akamai.com>> > >>>>>>> Cc: "Michael S. Tsirkin" <m...@redhat.com <mailto:m...@redhat.com>> > >>>>>>> Cc: Jason Wang <jasow...@redhat.com <mailto:jasow...@redhat.com>> > >>>>>>> --- > >>>>>>> hw/net/virtio-net.c | 29 > >>>>>>> +++++++++++++++++++++++++++++ > >>>>>>> include/hw/virtio/virtio-net.h | 3 +++ > >>>>>>> include/standard-headers/linux/virtio_net.h | 4 ++++ > >>>>>>> 3 files changed, 36 insertions(+) > >>>>>>> > >>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >>>>>>> index 38674b0..d63e790 100644 > >>>>>>> --- a/hw/net/virtio-net.c > >>>>>>> +++ b/hw/net/virtio-net.c > >>>>>>> @@ -40,6 +40,12 @@ > >>>>>>> #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE > >>>>>>> #define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE > >>>>>>> > >>>>>>> +/* duplex and speed defines */ > >>>>>>> +#define DUPLEX_UNKNOWN 0xff > >>>>>>> +#define DUPLEX_HALF 0x00 > >>>>>>> +#define DUPLEX_FULL 0x01 > >>>>>>> +#define SPEED_UNKNOWN -1 > >>>>>>> + > >>>>>>> /* > >>>>>>> * Calculate the number of bytes up to and including the given 'field' > >>>>>>> of > >>>>>>> * 'container'. > >>>>>>> @@ -61,6 +67,8 @@ static VirtIOFeature feature_sizes[] = { > >>>>>>> .end = endof(struct virtio_net_config, max_virtqueue_pairs)}, > >>>>>>> {.flags = 1 << VIRTIO_NET_F_MTU, > >>>>>>> .end = endof(struct virtio_net_config, mtu)}, > >>>>>>> + {.flags = 1 << VIRTIO_NET_F_SPEED_DUPLEX, > >>>>>>> + .end = endof(struct virtio_net_config, duplex)}, > >>>>>>> {} > >>>>>>> }; > >>>>>>> > >>>>>>> @@ -88,6 +96,8 @@ static void virtio_net_get_config(VirtIODevice > >>>>>>> *vdev, uint8_t *config) > >>>>>>> virtio_stw_p(vdev, &netcfg.status, n->status); > >>>>>>> virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues); > >>>>>>> virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu); > >>>>>>> + virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed); > >>>>>>> + netcfg.duplex = n->net_conf.duplex; > >>>>>>> memcpy(netcfg.mac, n->mac, ETH_ALEN); > >>>>>>> memcpy(config, &netcfg, n->config_size); > >>>>>>> } > >>>>>>> @@ -1941,6 +1951,23 @@ static void > >>>>>>> virtio_net_device_realize(DeviceState *dev, Error **errp) > >>>>>>> n->host_features |= (0x1 << VIRTIO_NET_F_MTU); > >>>>>>> } > >>>>>>> > >>>>>>> + n->host_features |= (0x1 << VIRTIO_NET_F_SPEED_DUPLEX); > >>>>>>> + if (n->net_conf.duplex_str) { > >>>>>>> + if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) { > >>>>>>> + n->net_conf.duplex = DUPLEX_HALF; > >>>>>>> + } else if (strncmp(n->net_conf.duplex_str, "full", 5) == 0) { > >>>>>>> + n->net_conf.duplex = DUPLEX_FULL; > >>>>>>> + } else { > >>>>>>> + error_setg(errp, "'duplex' must be 'half' or 'full'"); > >>>>>>> + } > >>>>>>> + } else { > >>>>>>> + n->net_conf.duplex = DUPLEX_UNKNOWN; > >>>>>>> + } > >>>>>>> + if (n->net_conf.speed < SPEED_UNKNOWN) { > >>>>>>> + error_setg(errp, "'speed' must be between -1 > >>>>>>> (SPEED_UNKOWN) and " > >>>>>>> + "INT_MAX"); > >>>>>>> + } > >>>>>>> + > >>>>>>> virtio_net_set_config_size(n, n->host_features); > >>>>>>> virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size); > >>>>>>> > >>>>>>> @@ -2160,6 +2187,8 @@ static Property virtio_net_properties[] = { > >>>>>>> DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0), > >>>>>>> DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet, > >>>>>>> mtu_bypass_backend, > >>>>>>> true), > >>>>>>> + DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, > >>>>>>> SPEED_UNKNOWN), > >>>>>> > >>>>>> From Windows guest perspective I prefer to have some reasonable > >>>>>> default (10G for example). > >>>>> > >>>>> > >>>>> hmmm, I didn't want to change/set the default here in case it broke > >>>>> something, but I'm ok setting it to some 'reasonable' value - (10G and > >>>>> duplex?), if the consensus is that that would be safe. > >>>> > >>>> OK from my side. > >>>> Thanks. > >>> > >>> I presume your speaking for windows - i'm wondering if under linux the > >>> virtio device suddenly start showing up as duplex, 10Gbps, will that > >>> break anything? I can speak for the use-cases we have here and that > >>> would certainly be fine for us, but i'm really not sure if that is ok > >>> more generally. > >>> > >>> Thanks, > >>> > >>> -Jason > >> > >> How about not enabling the flag unless the user specified the speed? > >> This way we also do not need an "unknown" value. > > > > This is even better. > > > We may still want "unknown" here as a setting, because otherwise we > can't support dynamically changing the speed later (if we added say a > qmp monitor command), because the flag would not have been set. > > So I am ok with the 10Gps and full duplex defaults, if at least one of > 'speed=' or 'duplex=' is supplied, but I would prefer to leave the > "unknown" values as a setting (to still potentially allow dynamic > updates in this case). > > Thanks, > > -Jason
That's fine. And I really think we should leave defaults to unknown and not 10G. > >> > >>>> > >>>>> > >>>>> Thanks, > >>>>> > >>>>> -Jason > >>>>> > >>>>>> > >>>>>> Thanks, > >>>>>> Yan. > >>>>>> > >>>>>>> + DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str), > >>>>>>> DEFINE_PROP_END_OF_LIST(), > >>>>>>> }; > >>>>>>> > >>>>>>> diff --git a/include/hw/virtio/virtio-net.h > >>>>>>> b/include/hw/virtio/virtio-net.h > >>>>>>> index b81b6a4..af74a94 100644 > >>>>>>> --- a/include/hw/virtio/virtio-net.h > >>>>>>> +++ b/include/hw/virtio/virtio-net.h > >>>>>>> @@ -38,6 +38,9 @@ typedef struct virtio_net_conf > >>>>>>> uint16_t rx_queue_size; > >>>>>>> uint16_t tx_queue_size; > >>>>>>> uint16_t mtu; > >>>>>>> + int32_t speed; > >>>>>>> + char *duplex_str; > >>>>>>> + uint8_t duplex; > >>>>>>> } virtio_net_conf; > >>>>>>> > >>>>>>> /* Maximum packet size we can receive from tap device: header + 64k */ > >>>>>>> diff --git a/include/standard-headers/linux/virtio_net.h > >>>>>>> b/include/standard-headers/linux/virtio_net.h > >>>>>>> index 30ff249..0ff1447 100644 > >>>>>>> --- a/include/standard-headers/linux/virtio_net.h > >>>>>>> +++ b/include/standard-headers/linux/virtio_net.h > >>>>>>> @@ -36,6 +36,7 @@ > >>>>>>> #define VIRTIO_NET_F_GUEST_CSUM1/* Guest handles pkts w/ partial csum > >>>>>>> */ > >>>>>>> #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload > >>>>>>> configuration. */ > >>>>>>> #define VIRTIO_NET_F_MTU3/* Initial MTU advice */ > >>>>>>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4/* Host set linkspeed and duplex > >>>>>>> */ > >>>>>>> #define VIRTIO_NET_F_MAC5/* Host has given MAC address. */ > >>>>>>> #define VIRTIO_NET_F_GUEST_TSO47/* Guest can handle TSOv4 in. */ > >>>>>>> #define VIRTIO_NET_F_GUEST_TSO68/* Guest can handle TSOv6 in. */ > >>>>>>> @@ -76,6 +77,9 @@ struct virtio_net_config { > >>>>>>> uint16_t max_virtqueue_pairs; > >>>>>>> /* Default maximum transmit unit advice */ > >>>>>>> uint16_t mtu; > >>>>>>> +/* Host exported linkspeed and duplex */ > >>>>>>> +uint32_t speed; > >>>>>>> +uint8_t duplex; > >>>>>>> } QEMU_PACKED; > >>>>>>> > >>>>>>> /* > >>>>>>> -- > >>>>>>> 2.6.1 > >