Re: [PATCH 2/2] vDPA: conditionally read fields in virtio-net dev

[Jason Wang <jasowang@redhat.com>]

在 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@intel.com>
>>>> Sent: Tuesday, August 16, 2022 12:19 AM
>>>>
>>>>
>>>> On 8/16/2022 10:32 AM, Parav Pandit wrote:
>>>>>> From: Zhu Lingshan <lingshan.zhu@intel.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


>
> Thanks
>>
>>
>