> From: Jason Wang <jasowang@xxxxxxxxxx> > Sent: Thursday, August 18, 2022 12:19 AM > >>>> On 8/16/2022 10:32 AM, Parav Pandit wrote: > >>>>>> From: Zhu Lingshan <lingshan.zhu@xxxxxxxxx> > >>>>>> 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. > +1
