Message-ID: <PH0PR12MB5481EE0FE7CE17892E426EB4DC6D9@PH0PR12MB5481.namprd12.prod.outlook.com>
Date:   Thu, 18 Aug 2022 17:20:05 +0000
From:   Parav Pandit <parav@...dia.com>
To:     Jason Wang <jasowang@...hat.com>,
        "Zhu, Lingshan" <lingshan.zhu@...el.com>,
        "Michael S. Tsirkin" <mst@...hat.com>
CC:     "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


> From: Jason Wang <jasowang@...hat.com>
> Sent: Thursday, August 18, 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.
> > 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

Powered by blists - more mailing lists