Jason Wang
2022-Aug-18 04:18 UTC
[PATCH 2/2] vDPA: conditionally read fields in virtio-net dev
? 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 at 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 at 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 beforeI 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 >> >> >
Parav Pandit
2022-Aug-18 17:20 UTC
[PATCH 2/2] vDPA: conditionally read fields in virtio-net dev
> From: Jason Wang <jasowang at redhat.com> > Sent: Thursday, August 18, 2022 12:19 AM> >>>> On 8/16/2022 10:32 AM, Parav Pandit wrote: > >>>>>> From: Zhu Lingshan <lingshan.zhu at 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. >+1