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