Re: [PATCH net-next 1/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

From: Jason Baron
Date: Wed Dec 20 2017 - 16:33:04 EST




On 12/20/2017 12:52 PM, Michael S. Tsirkin wrote:
> On Wed, Dec 20, 2017 at 12:07:55PM -0500, Jason Baron wrote:
>>
>>
>> On 12/20/2017 09:57 AM, Michael S. Tsirkin wrote:
>>> On Thu, Dec 14, 2017 at 02:33:53PM -0500, Jason Baron wrote:
>>>> If the hypervisor exports the link and duplex speed, let's use that instead
>>>> of the default unknown speed. The user can still overwrite it later if
>>>> desired via: 'ethtool -s'. This allows the hypervisor to set the default
>>>> link speed and duplex setting without requiring guest changes and is
>>>> consistent with how other network drivers operate. We ran into some cases
>>>> where the guest software was failing due to a lack of linkspeed and had to
>>>> fall back to a fully emulated network device that does export a linkspeed
>>>> and duplex setting.
>>>>
>>>> Implement by adding a new VIRTIO_NET_F_SPEED_DUPLEX feature flag, to
>>>> indicate that a linkspeed and duplex setting are present.
>>>>
>>>> Signed-off-by: Jason Baron <jbaron@xxxxxxxxxx>
>>>> Cc: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
>>>> Cc: Jason Wang <jasowang@xxxxxxxxxx>
>>>> ---
>>>> drivers/net/virtio_net.c | 11 ++++++++++-
>>>> include/uapi/linux/virtio_net.h | 4 ++++
>>>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index 6fb7b65..e7a2ad6 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -2671,6 +2671,14 @@ static int virtnet_probe(struct virtio_device *vdev)
>>>> netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
>>>>
>>>> virtnet_init_settings(dev);
>>>> + if (virtio_has_feature(vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
>>>> + vi->speed = virtio_cread32(vdev,
>>>> + offsetof(struct virtio_net_config,
>>>> + speed));
>>>> + vi->duplex = virtio_cread8(vdev,
>>>> + offsetof(struct virtio_net_config,
>>>> + duplex));
>>>> + }
>>>>
>>>> err = register_netdev(dev);
>>>> if (err) {
>>>
>>> How are we going to validate speed values? Imagine host
>>> using a new 1000Gbit device and exposing that to guest.
>>>
>>> Need to think what do we want guest to do.
>>> I think that ideally we'd say it's a 100Gbit device.
>>>
>>> For duplex, force to one of 3 valid values?
>>
>> So I didn't provide validation here b/c as you point out its not clear
>> how we would validate it. I don't believe h/w drivers do any validation
>> here either.
>
> Right but hardware tends not to change as quickly as the hypervisors :)
> For virtual device drivers, we need some way to handle forward
> compatibility since hypervisors do change quite quickly.
>
>> They simply propagate the value from the the underlying
>> device. So that seemed reasonable to me.
>>
>> Why do you divide by 10 in the above example? Would you propose always
>> dividing what the device reports by 10?
>
> No, that was just an example. I was just suggesting rounding down to
> next valid known speed.

I see, but virtio currently uses ethtool_validate_speed() which allows
arbitrary values up to INT_MAX in units of Mbps. That seems to leave
plenty of headroom. So I could use that function for validation as well
as well as ethtool_validate_duplex() and if they fail fall back to
SPEED_UNKNOWN and DUPLEX_UNKNOWN?

>
>>>
>>>
>>>> @@ -2796,7 +2804,8 @@ static struct virtio_device_id id_table[] = {
>>>> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>>>> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>>>> VIRTIO_NET_F_CTRL_MAC_ADDR, \
>>>> - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>>> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>>>> + VIRTIO_NET_F_SPEED_DUPLEX
>>>>
>>>> static unsigned int features[] = {
>>>> VIRTNET_FEATURES,
>>>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>>>> index fc353b5..acfcf68 100644
>>>> --- a/include/uapi/linux/virtio_net.h
>>>> +++ b/include/uapi/linux/virtio_net.h
>>>> @@ -36,6 +36,7 @@
>>>> #define VIRTIO_NET_F_GUEST_CSUM 1 /* Guest handles pkts w/ partial csum */
>>>> #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
>>>> #define VIRTIO_NET_F_MTU 3 /* Initial MTU advice */
>>>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4 /* Host set linkspeed and duplex */
>>>> #define VIRTIO_NET_F_MAC 5 /* Host has given MAC address. */
>>>> #define VIRTIO_NET_F_GUEST_TSO4 7 /* Guest can handle TSOv4 in. */
>>>> #define VIRTIO_NET_F_GUEST_TSO6 8 /* Guest can handle TSOv6 in. */
>>>
>>> I think I'd prefer a high feature bit - low bits are ones that can
>>> be backported to legacy interfaces, so I think we should hang on to
>>> these for fixing issues that break communication completely (like the
>>> mtu).
>>>
>>
>> So I went with a low bit here b/c in the virtio spec 'section 2.2
>> Feature Bits':
>>
>>
>> 0 to 23
>> Feature bits for the specific device type
>> 24 to 32
>> Feature bits reserved for extensions to the queue and feature
>> negotiation mechanisms
>> 33 and above
>> Feature bits reserved for future extensions.
>>
>> So virtio_net already goes up to 23 (but omits 4 and 6), and I wasn't
>> sure if it was reasonable to use the higher bits. It looks like the code
>> would handle the higher bits ok, so I can try that - bit 33 perhaps ?
>>
>> Thanks,
>>
>> -Jason
>
>
> Transports started from bit 24 and are growing up.
> So I would say devices should start from bit 63 and grow down.
>

Ok, I will use 63.

Thanks,

-Jason