Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755458AbdLTOdn convert rfc822-to-8bit (ORCPT ); Wed, 20 Dec 2017 09:33:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59532 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755354AbdLTOdf (ORCPT ); Wed, 20 Dec 2017 09:33:35 -0500 Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: Re: [Qemu-devel] [PATCH 2/2] qemu: add linkspeed and duplex setting to virtio-net From: Yan Vugenfirer In-Reply-To: <20171220162927-mutt-send-email-mst@kernel.org> Date: Wed, 20 Dec 2017 16:33:28 +0200 Cc: Jason Baron , jasowang@redhat.com, qemu-devel@nongnu.org, linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8BIT Message-Id: References: <1513280073-27515-1-git-send-email-jbaron@akamai.com> <9d600f3d-7e8d-153f-fd34-704c9975099b@akamai.com> <21A64922-D22E-4AE1-8B4C-B0D85357240C@redhat.com> <73e8811d-70fd-4920-1cb4-ad1809ebff82@akamai.com> <20171220162927-mutt-send-email-mst@kernel.org> To: "Michael S. Tsirkin" X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Wed, 20 Dec 2017 14:33:35 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7349 Lines: 193 > On 20 Dec 2017, at 16:31, Michael S. Tsirkin 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 >>>> > wrote: >>>> >>>> >>>> >>>> On 12/18/2017 06:34 AM, Yan Vugenfirer wrote: >>>>> >>>>>> On 14 Dec 2017, at 21:33, Jason Baron via Qemu-devel >>>>>> > 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 >>>>> > >>>>>> Cc: "Michael S. Tsirkin" > >>>>>> Cc: Jason Wang > >>>>>> --- >>>>>> 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. > >>> >>>> >>>> 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