Date:   Tue, 16 Aug 2022 17:09:53 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Parav Pandit <parav@...dia.com>
Cc:     "Zhu, Lingshan" <lingshan.zhu@...el.com>,
        "jasowang@...hat.com" <jasowang@...hat.com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "xieyongji@...edance.com" <xieyongji@...edance.com>,
        "gautam.dawar@....com" <gautam.dawar@....com>
Subject: Re: [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@...el.com>
> > Sent: Tuesday, August 16, 2022 12:19 AM
> > 
> > 
> > On 8/16/2022 10:32 AM, Parav Pandit wrote:
> > >> From: Zhu Lingshan <lingshan.zhu@...el.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

Powered by blists - more mailing lists