Date:   Tue, 19 Dec 2017 11:52:39 -0500
From:   Jason Baron <jbaron@...mai.com>
To:     Yan Vugenfirer <yvugenfi@...hat.com>
Cc:     jasowang@...hat.com, mst@...hat.com, qemu-devel@...gnu.org,
        linux-kernel@...r.kernel.org
Subject: Re: [Qemu-devel] [PATCH 2/2] qemu: add linkspeed and duplex setting
 to virtio-net



On 12/19/2017 04:19 AM, Yan Vugenfirer wrote:
> 
>> On 18 Dec 2017, at 18:04, Jason Baron via Qemu-devel
>> <qemu-devel@...gnu.org <mailto:qemu-devel@...gnu.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@...gnu.org <mailto:qemu-devel@...gnu.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 <jbaron@...mai.com
>>>> <mailto:jbaron@...mai.com>>
>>>> Cc: "Michael S. Tsirkin" <mst@...hat.com <mailto:mst@...hat.com>>
>>>> Cc: Jason Wang <jasowang@...hat.com <mailto:jasowang@...hat.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

> 
>>
>> 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
> 

Powered by blists - more mailing lists