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 写道:

quoted

On 8/17/2022 5:09 AM, Michael S. Tsirkin wrote:

quoted

On Tue, Aug 16, 2022 at 09:02:17PM +0000, Parav Pandit wrote:

quoted

quoted

From: Zhu, Lingshan <redacted>
Sent: Tuesday, August 16, 2022 12:19 AM


On 8/16/2022 10:32 AM, Parav Pandit wrote:

quoted

quoted

From: Zhu Lingshan <redacted>
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.

quoted

For MQ, if both VIRTIO_NET_F_MQ and VIRTIO_NET_F_RSS are not set,

the

quoted

quoted

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.

quoted

quoted

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

quoted

quoted

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.

quoted

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.

quoted

There is really no need for this complexity and such a long commit

message.

quoted

Can we please expose feature bits as-is and report config space 
field which

are valid?

quoted

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.

quoted

quoted

quoted

+    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.

quoted

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

quoted

Thanks

quoted