Parav Pandit
2022-Aug-16 21:02 UTC
[PATCH 2/2] vDPA: conditionally read fields in virtio-net dev
> 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.
Michael S. Tsirkin
2022-Aug-16 21:09 UTC
[PATCH 2/2] vDPA: conditionally read fields in virtio-net dev
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. -- MST