Date:   Thu, 18 Aug 2022 14:38:48 +0800
From:   "Zhu, Lingshan" <lingshan.zhu@...el.com>
To:     Jason Wang <jasowang@...hat.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Parav Pandit <parav@...dia.com>
Cc:     "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "xieyongji@...edance.com" <xieyongji@...edance.com>,
        "gautam.dawar@....com" <gautam.dawar@....com>
Subject: Re: [PATCH 2/2] vDPA: conditionally read fields in virtio-net dev



On 8/18/2022 12:18 PM, Jason Wang wrote:
>
> 在 2022/8/17 10:03, Zhu, Lingshan 写道:
>>
>>
>> On 8/17/2022 5:09 AM, Michael S. Tsirkin wrote:
>>> On Tue, Aug 16, 2022 at 09:02:17PM +0000, Parav Pandit wrote:
>>>>> From: Zhu, Lingshan <lingshan.zhu@...el.com>
>>>>> Sent: Tuesday, August 16, 2022 12:19 AM
>>>>>
>>>>>
>>>>> On 8/16/2022 10:32 AM, Parav Pandit wrote:
>>>>>>> From: Zhu Lingshan <lingshan.zhu@...el.com>
>>>>>>> Sent: Monday, August 15, 2022 5:27 AM
>>>>>>>
>>>>>>> Some fields of virtio-net device config space are conditional on 
>>>>>>> the
>>>>>>> feature bits, the spec says:
>>>>>>>
>>>>>>> "The mac address field always exists
>>>>>>> (though is only valid if VIRTIO_NET_F_MAC is set)"
>>>>>>>
>>>>>>> "max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ or
>>>>>>> VIRTIO_NET_F_RSS is set"
>>>>>>>
>>>>>>> "mtu only exists if VIRTIO_NET_F_MTU is set"
>>>>>>>
>>>>>>> so we should read MTU, MAC and MQ in the device config space only
>>>>>>> when these feature bits are offered.
>>>>>> Yes.
>>>>>>
>>>>>>> For MQ, if both VIRTIO_NET_F_MQ and VIRTIO_NET_F_RSS are not set,
>>>>> the
>>>>>>> virtio device should have one queue pair as default value, so when
>>>>>>> userspace querying queue pair numbers, it should return mq=1 
>>>>>>> than zero.
>>>>>> No.
>>>>>> No need to treat mac and max_qps differently.
>>>>>> It is meaningless to differentiate when field exist/not-exists vs 
>>>>>> value
>>>>> valid/not valid.
>>>>> as we discussed before, MQ has a default value 1, to be a 
>>>>> functional virtio-
>>>>> net device, while MAC has no default value, if no VIRTIO_NET_F_MAC 
>>>>> set,
>>>>> the driver should generate a random MAC.
>>>>>>> For MTU, if VIRTIO_NET_F_MTU is not set, we should not read MTU 
>>>>>>> from
>>>>>>> the device config sapce.
>>>>>>> RFC894 <A Standard for the Transmission of IP Datagrams over 
>>>>>>> Ethernet
>>>>>>> Networks> says:"The minimum length of the data field of a packet 
>>>>>>> sent
>>>>>>> Networks> over
>>>>>>> an Ethernet is 1500 octets, thus the maximum length of an IP 
>>>>>>> datagram
>>>>>>> sent over an Ethernet is 1500 octets. Implementations are 
>>>>>>> encouraged
>>>>>>> to support full-length packets"
>>>>>> This line in the RFC 894 of 1984 is wrong.
>>>>>> Errata already exists for it at [1].
>>>>>>
>>>>>> [1] 
>>>>>> https://www.rfc-editor.org/errata_search.php?rfc=894&rec_status=0
>>>>> OK, so I think we should return nothing if _F_MTU not set, like 
>>>>> handling the
>>>>> MAC
>>>>>>> virtio spec says:"The virtio network device is a virtual ethernet
>>>>>>> card", so the default MTU value should be 1500 for virtio-net.
>>>>>>>
>>>>>> Practically I have seen 1500 and highe mtu.
>>>>>> And this derivation is not good of what should be the default mtu 
>>>>>> as above
>>>>> errata exists.
>>>>>> And I see the code below why you need to work so hard to define a 
>>>>>> default
>>>>> value so that _MQ and _MTU can report default values.
>>>>>> There is really no need for this complexity and such a long commit
>>>>> message.
>>>>>> Can we please expose feature bits as-is and report config space 
>>>>>> field which
>>>>> are valid?
>>>>>> User space will be querying both.
>>>>> I think MAC and MTU don't have default values, so return nothing 
>>>>> if the
>>>>> feature bits not set,
>>>>> for MQ, it is still max_vq_paris == 1 by default.
>>>> I have stressed enough to highlight the fact that we don’t want to 
>>>> start digging default/no default, valid/no-valid part of the spec.
>>>> I prefer kernel to reporting fields that _exists_ in the config 
>>>> space and are valid.
>>>> I will let MST to handle the maintenance nightmare that this kind 
>>>> of patch brings in without any visible gain to user 
>>>> space/orchestration apps.
>>>>
>>>> A logic that can be easily build in user space, should be written 
>>>> in user space.
>>>> I conclude my thoughts here for this discussion.
>>>>
>>>> I will let MST to decide how he prefers to proceed.
>>>>
>>>>>>> +    if ((features & BIT_ULL(VIRTIO_NET_F_MTU)) == 0)
>>>>>>> +        val_u16 = 1500;
>>>>>>> +    else
>>>>>>> +        val_u16 = __virtio16_to_cpu(true, config->mtu);
>>>>>>> +
>>>>>> Need to work hard to find default values and that too turned out had
>>>>> errata.
>>>>>> There are more fields that doesn’t have default values.
>>>>>>
>>>>>> There is no point in kernel doing this guess work, that user 
>>>>>> space can figure
>>>>> out of what is valid/invalid.
>>>>> It's not guest work, when guest finds no feature bits set, it can 
>>>>> decide what
>>>>> to do.
>>>> Above code of doing 1500 was probably an honest attempt to find a 
>>>> legitimate default value, and we saw that it doesn’t work.
>>>> This is second example after _MQ that we both agree should not 
>>>> return default.
>>>>
>>>> And there are more fields coming in this area.
>>>> Hence, I prefer to not avoid returning such defaults for MAC, MTU, 
>>>> MQ and rest all fields which doesn’t _exists_.
>>>>
>>>> I will let MST to decide how he prefers to proceed for every field 
>>>> to come next.
>>>> Thanks.
>>>>
>>>
>>> If MTU does not return a value without _F_MTU, and MAC does not return
>>> a value without _F_MAC then IMO yes, number of queues should not return
>>> a value without _F_MQ.
>> sure I can do this, but may I ask whether it is a final decision, I 
>> remember you supported max_queue_paris = 1 without _F_MQ before
>
>
> I think we just need to be consistent:
>
> Either
>
> 1) make field conditional to align with spec
>
> or
>
> 2) always return a value even if the feature is not set
>
> It seems to me 1) is easier.
>
> Thanks
I will pick 1

Thanks
>
>
>>
>> Thanks
>>>
>>>
>>
>

Powered by blists - more mailing lists