From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: virtio-dev-return-8022-cohuck=redhat.com@lists.oasis-open.org Sender: List-Post: List-Help: List-Unsubscribe: List-Subscribe: Received: from lists.oasis-open.org (oasis-open.org [10.110.1.242]) by lists.oasis-open.org (Postfix) with ESMTP id 68FBB986027 for ; Tue, 23 Feb 2021 10:01:41 +0000 (UTC) Date: Tue, 23 Feb 2021 05:01:31 -0500 From: "Michael S. Tsirkin" Message-ID: <20210223045600-mutt-send-email-mst@kernel.org> References: <1613735698-3328-1-git-send-email-si-wei.liu@oracle.com> <605e7d2d-4f27-9688-17a8-d57191752ee7@redhat.com> <20210223041740-mutt-send-email-mst@kernel.org> <788a0880-0a68-20b7-5bdf-f8150b08276a@redhat.com> MIME-Version: 1.0 In-Reply-To: <788a0880-0a68-20b7-5bdf-f8150b08276a@redhat.com> Subject: [virtio-dev] Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable To: Jason Wang Cc: Si-Wei Liu , elic@nvidia.com, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, netdev@vger.kernel.org, virtio-dev@lists.oasis-open.org List-ID: On Tue, Feb 23, 2021 at 05:46:20PM +0800, Jason Wang wrote: >=20 > On 2021/2/23 =E4=B8=8B=E5=8D=885:25, Michael S. Tsirkin wrote: > > On Mon, Feb 22, 2021 at 09:09:28AM -0800, Si-Wei Liu wrote: > > >=20 > > > On 2/21/2021 8:14 PM, Jason Wang wrote: > > > > On 2021/2/19 7:54 =E4=B8=8B=E5=8D=88, Si-Wei Liu wrote: > > > > > Commit 452639a64ad8 ("vdpa: make sure set_features is invoked > > > > > for legacy") made an exception for legacy guests to reset > > > > > features to 0, when config space is accessed before features > > > > > are set. We should relieve the verify_min_features() check > > > > > and allow features reset to 0 for this case. > > > > >=20 > > > > > It's worth noting that not just legacy guests could access > > > > > config space before features are set. For instance, when > > > > > feature VIRTIO_NET_F_MTU is advertised some modern driver > > > > > will try to access and validate the MTU present in the config > > > > > space before virtio features are set. > > > >=20 > > > > This looks like a spec violation: > > > >=20 > > > > " > > > >=20 > > > > The following driver-read-only field, mtu only exists if > > > > VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for t= he > > > > driver to use. > > > > " > > > >=20 > > > > Do we really want to workaround this? > > > Isn't the commit 452639a64ad8 itself is a workaround for legacy guest= ? > > >=20 > > > I think the point is, since there's legacy guest we'd have to support= , this > > > host side workaround is unavoidable. Although I agree the violating d= river > > > should be fixed (yes, it's in today's upstream kernel which exists fo= r a > > > while now). > > Oh you are right: > >=20 > >=20 > > static int virtnet_validate(struct virtio_device *vdev) > > { > > if (!vdev->config->get) { > > dev_err(&vdev->dev, "%s failure: config access disable= d\n", > > __func__); > > return -EINVAL; > > } > >=20 > > if (!virtnet_validate_features(vdev)) > > return -EINVAL; > >=20 > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) { > > int mtu =3D virtio_cread16(vdev, > > offsetof(struct virtio_net_co= nfig, > > mtu)); > > if (mtu < MIN_MTU) > > __virtio_clear_bit(vdev, VIRTIO_NET_F_MTU); >=20 >=20 > I wonder why not simply fail here? Back in 2016 it went like this: =09On Thu, Jun 02, 2016 at 05:10:59PM -0400, Aaron Conole wrote: =09> + if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) { =09> + dev->mtu =3D virtio_cread16(vdev, =09> + offsetof(struct virtio_net_con= fig, =09> + mtu)); =09> + } =09> + =09> if (vi->any_header_sg) =09> dev->needed_headroom =3D vi->hdr_len; =09>=20 =09One comment though: I think we should validate the mtu. =09If it's invalid, clear VIRTIO_NET_F_MTU and ignore. Too late at this point :) I guess it's a way to tell device "I can not live with this MTU", device can fail FEATURES_OK if it wants to. MIN_MTU is an internal linux thing and at the time I felt it's better to try to make progress. >=20 > > } > >=20 > > return 0; > > } > >=20 > > And the spec says: > >=20 > >=20 > > The driver MUST follow this sequence to initialize a device: > > 1. Reset the device. > > 2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device. > > 3. Set the DRIVER status bit: the guest OS knows how to drive the devic= e. > > 4. Read device feature bits, and write the subset of feature bits under= stood by the OS and driver to the > > device. During this step the driver MAY read (but MUST NOT write) the d= evice-specific configuration > > fields to check that it can support the device before accepting it. > > 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new featu= re bits after this step. > > 6. Re-read device status to ensure the FEATURES_OK bit is still set: ot= herwise, the device does not > > support our subset of features and the device is unusable. > > 7. Perform device-specific setup, including discovery of virtqueues for= the device, optional per-bus setup, > > reading and possibly writing the device=E2=80=99s virtio configuration = space, and population of virtqueues. > > 8. Set the DRIVER_OK status bit. At this point the device is =E2=80=9Cl= ive=E2=80=9D. > >=20 > >=20 > > Item 4 on the list explicitly allows reading config space before > > FEATURES_OK. > >=20 > > I conclude that VIRTIO_NET_F_MTU is set means "set in device features". >=20 >=20 > So this probably need some clarification. "is set" is used many times in = the > spec that has different implications. >=20 > Thanks >=20 >=20 > >=20 > > Generally it is worth going over feature dependent config fields > > and checking whether they should be present when device feature is set > > or when feature bit has been negotiated, and making this clear. > >=20 --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.5 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 96BBBC433DB for ; Tue, 23 Feb 2021 10:01:46 +0000 (UTC) Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1F02B64E22 for ; Tue, 23 Feb 2021 10:01:46 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1F02B64E22 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=virtualization-bounces@lists.linux-foundation.org Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id AC7E886578; Tue, 23 Feb 2021 10:01:45 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id L-C33SjdBSOa; Tue, 23 Feb 2021 10:01:42 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id DCE8B86408; Tue, 23 Feb 2021 10:01:42 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id BE963C000B; Tue, 23 Feb 2021 10:01:42 +0000 (UTC) Received: from smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 3C5A0C0001 for ; Tue, 23 Feb 2021 10:01:41 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 13368605BF for ; Tue, 23 Feb 2021 10:01:41 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id g00uiyCCV6tp for ; Tue, 23 Feb 2021 10:01:39 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by smtp3.osuosl.org (Postfix) with ESMTPS id AD462605AB for ; Tue, 23 Feb 2021 10:01:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1614074498; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=fq9fMMA50uXmWN63AH4eeQqewule2njQVz39u52s5qo=; b=XKBHD5wfdkUYl7ZxGFoYTGX9FaBwYju/UX77Xcxxzo+jwkzkNGos/lCP7zCu+mC0iPwCMg 3v7gCvwrdANF42TT6RMh13d/88qf0cR5f+e+hM4JnXHmdmFc4LCEUbyGdbtJjjbsA2a4cl 7iS/EQKXw7fo/4BQfzVRRg93iRheU1c= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-154-KGrES82VNxKMtRJI-uLuTg-1; Tue, 23 Feb 2021 05:01:36 -0500 X-MC-Unique: KGrES82VNxKMtRJI-uLuTg-1 Received: by mail-wr1-f72.google.com with SMTP id u15so7095549wrn.3 for ; Tue, 23 Feb 2021 02:01:36 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=fq9fMMA50uXmWN63AH4eeQqewule2njQVz39u52s5qo=; b=oU78ScbBfywt3SudFQ4GhUY29iIdzHAQHquBFxj/l+s/XgAwHeUbVQQ1QleKzOdk2y h4F3qCEiRdjvkkkwSRXQ99ymNmwmYSCkPGXXqncBzLLlCXmxIx4H84mToQ0wvm86wFGy nQo1KIBn2jfO0EwHzr+QV6v65AzLWSJkX98Dy20pxNOF68w3UxnqG1+Bno8DnOB3diMk BJZ3feyEi6OUgeEz2dNmhYu6un6sTDF92aaF5VEtR+gejavsnAYXiAjZK5pQfwtHIUha 1v6RAJzfnMCEYfe/FkNJTNbH1YVujZWrkuFD3gNPcI0ZDEAvyXPud+UJ0PaAUc2z3Nww e08Q== X-Gm-Message-State: AOAM532w8+NoEZqcx6FgD9IGNv9+GiG8sbzn/IvSeKM9eYwXLeJgU91F vs4dzoHgHqtLQugR9lPCaXKVRx0M8sRrmORBCI4nECmQJZtRRwDN3RmDDt8ISO50Vv3yBHxsxA5 Ud/5Y1ScSQZmm71mOwYVZQiFz0j7576rWp0ZZ7AU9jA== X-Received: by 2002:adf:97d5:: with SMTP id t21mr1510154wrb.139.1614074495014; Tue, 23 Feb 2021 02:01:35 -0800 (PST) X-Google-Smtp-Source: ABdhPJzqzcytawXMdenSafG3a3yvlWPCUdXAY4CQjPGVhSoQqq+a9ZdeDw2NxLx25iW4TucswKLb7Q== X-Received: by 2002:adf:97d5:: with SMTP id t21mr1510136wrb.139.1614074494799; Tue, 23 Feb 2021 02:01:34 -0800 (PST) Received: from redhat.com (bzq-79-180-2-31.red.bezeqint.net. [79.180.2.31]) by smtp.gmail.com with ESMTPSA id z11sm2046114wmi.35.2021.02.23.02.01.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 23 Feb 2021 02:01:33 -0800 (PST) Date: Tue, 23 Feb 2021 05:01:31 -0500 From: "Michael S. Tsirkin" To: Jason Wang Subject: Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero Message-ID: <20210223045600-mutt-send-email-mst@kernel.org> References: <1613735698-3328-1-git-send-email-si-wei.liu@oracle.com> <605e7d2d-4f27-9688-17a8-d57191752ee7@redhat.com> <20210223041740-mutt-send-email-mst@kernel.org> <788a0880-0a68-20b7-5bdf-f8150b08276a@redhat.com> MIME-Version: 1.0 In-Reply-To: <788a0880-0a68-20b7-5bdf-f8150b08276a@redhat.com> Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=mst@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline Cc: virtio-dev@lists.oasis-open.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, Si-Wei Liu , elic@nvidia.com X-BeenThere: virtualization@lists.linux-foundation.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Linux virtualization List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: virtualization-bounces@lists.linux-foundation.org Sender: "Virtualization" T24gVHVlLCBGZWIgMjMsIDIwMjEgYXQgMDU6NDY6MjBQTSArMDgwMCwgSmFzb24gV2FuZyB3cm90 ZToKPiAKPiBPbiAyMDIxLzIvMjMg5LiL5Y2INToyNSwgTWljaGFlbCBTLiBUc2lya2luIHdyb3Rl Ogo+ID4gT24gTW9uLCBGZWIgMjIsIDIwMjEgYXQgMDk6MDk6MjhBTSAtMDgwMCwgU2ktV2VpIExp dSB3cm90ZToKPiA+ID4gCj4gPiA+IE9uIDIvMjEvMjAyMSA4OjE0IFBNLCBKYXNvbiBXYW5nIHdy b3RlOgo+ID4gPiA+IE9uIDIwMjEvMi8xOSA3OjU0IOS4i+WNiCwgU2ktV2VpIExpdSB3cm90ZToK PiA+ID4gPiA+IENvbW1pdCA0NTI2MzlhNjRhZDggKCJ2ZHBhOiBtYWtlIHN1cmUgc2V0X2ZlYXR1 cmVzIGlzIGludm9rZWQKPiA+ID4gPiA+IGZvciBsZWdhY3kiKSBtYWRlIGFuIGV4Y2VwdGlvbiBm b3IgbGVnYWN5IGd1ZXN0cyB0byByZXNldAo+ID4gPiA+ID4gZmVhdHVyZXMgdG8gMCwgd2hlbiBj b25maWcgc3BhY2UgaXMgYWNjZXNzZWQgYmVmb3JlIGZlYXR1cmVzCj4gPiA+ID4gPiBhcmUgc2V0 LiBXZSBzaG91bGQgcmVsaWV2ZSB0aGUgdmVyaWZ5X21pbl9mZWF0dXJlcygpIGNoZWNrCj4gPiA+ ID4gPiBhbmQgYWxsb3cgZmVhdHVyZXMgcmVzZXQgdG8gMCBmb3IgdGhpcyBjYXNlLgo+ID4gPiA+ ID4gCj4gPiA+ID4gPiBJdCdzIHdvcnRoIG5vdGluZyB0aGF0IG5vdCBqdXN0IGxlZ2FjeSBndWVz dHMgY291bGQgYWNjZXNzCj4gPiA+ID4gPiBjb25maWcgc3BhY2UgYmVmb3JlIGZlYXR1cmVzIGFy ZSBzZXQuIEZvciBpbnN0YW5jZSwgd2hlbgo+ID4gPiA+ID4gZmVhdHVyZSBWSVJUSU9fTkVUX0Zf TVRVIGlzIGFkdmVydGlzZWQgc29tZSBtb2Rlcm4gZHJpdmVyCj4gPiA+ID4gPiB3aWxsIHRyeSB0 byBhY2Nlc3MgYW5kIHZhbGlkYXRlIHRoZSBNVFUgcHJlc2VudCBpbiB0aGUgY29uZmlnCj4gPiA+ ID4gPiBzcGFjZSBiZWZvcmUgdmlydGlvIGZlYXR1cmVzIGFyZSBzZXQuCj4gPiA+ID4gCj4gPiA+ ID4gVGhpcyBsb29rcyBsaWtlIGEgc3BlYyB2aW9sYXRpb246Cj4gPiA+ID4gCj4gPiA+ID4gIgo+ ID4gPiA+IAo+ID4gPiA+IFRoZSBmb2xsb3dpbmcgZHJpdmVyLXJlYWQtb25seSBmaWVsZCwgbXR1 IG9ubHkgZXhpc3RzIGlmCj4gPiA+ID4gVklSVElPX05FVF9GX01UVSBpcyBzZXQuIFRoaXMgZmll bGQgc3BlY2lmaWVzIHRoZSBtYXhpbXVtIE1UVSBmb3IgdGhlCj4gPiA+ID4gZHJpdmVyIHRvIHVz ZS4KPiA+ID4gPiAiCj4gPiA+ID4gCj4gPiA+ID4gRG8gd2UgcmVhbGx5IHdhbnQgdG8gd29ya2Fy b3VuZCB0aGlzPwo+ID4gPiBJc24ndCB0aGUgY29tbWl0IDQ1MjYzOWE2NGFkOCBpdHNlbGYgaXMg YSB3b3JrYXJvdW5kIGZvciBsZWdhY3kgZ3Vlc3Q/Cj4gPiA+IAo+ID4gPiBJIHRoaW5rIHRoZSBw b2ludCBpcywgc2luY2UgdGhlcmUncyBsZWdhY3kgZ3Vlc3Qgd2UnZCBoYXZlIHRvIHN1cHBvcnQs IHRoaXMKPiA+ID4gaG9zdCBzaWRlIHdvcmthcm91bmQgaXMgdW5hdm9pZGFibGUuIEFsdGhvdWdo IEkgYWdyZWUgdGhlIHZpb2xhdGluZyBkcml2ZXIKPiA+ID4gc2hvdWxkIGJlIGZpeGVkICh5ZXMs IGl0J3MgaW4gdG9kYXkncyB1cHN0cmVhbSBrZXJuZWwgd2hpY2ggZXhpc3RzIGZvciBhCj4gPiA+ IHdoaWxlIG5vdykuCj4gPiBPaCAgeW91IGFyZSByaWdodDoKPiA+IAo+ID4gCj4gPiBzdGF0aWMg aW50IHZpcnRuZXRfdmFsaWRhdGUoc3RydWN0IHZpcnRpb19kZXZpY2UgKnZkZXYpCj4gPiB7Cj4g PiAgICAgICAgICBpZiAoIXZkZXYtPmNvbmZpZy0+Z2V0KSB7Cj4gPiAgICAgICAgICAgICAgICAg IGRldl9lcnIoJnZkZXYtPmRldiwgIiVzIGZhaWx1cmU6IGNvbmZpZyBhY2Nlc3MgZGlzYWJsZWRc biIsCj4gPiAgICAgICAgICAgICAgICAgICAgICAgICAgX19mdW5jX18pOwo+ID4gICAgICAgICAg ICAgICAgICByZXR1cm4gLUVJTlZBTDsKPiA+ICAgICAgICAgIH0KPiA+IAo+ID4gICAgICAgICAg aWYgKCF2aXJ0bmV0X3ZhbGlkYXRlX2ZlYXR1cmVzKHZkZXYpKQo+ID4gICAgICAgICAgICAgICAg ICByZXR1cm4gLUVJTlZBTDsKPiA+IAo+ID4gICAgICAgICAgaWYgKHZpcnRpb19oYXNfZmVhdHVy ZSh2ZGV2LCBWSVJUSU9fTkVUX0ZfTVRVKSkgewo+ID4gICAgICAgICAgICAgICAgICBpbnQgbXR1 ID0gdmlydGlvX2NyZWFkMTYodmRldiwKPiA+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgIG9mZnNldG9mKHN0cnVjdCB2aXJ0aW9fbmV0X2NvbmZpZywKPiA+ICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIG10dSkpOwo+ID4g ICAgICAgICAgICAgICAgICBpZiAobXR1IDwgTUlOX01UVSkKPiA+ICAgICAgICAgICAgICAgICAg ICAgICAgICBfX3ZpcnRpb19jbGVhcl9iaXQodmRldiwgVklSVElPX05FVF9GX01UVSk7Cj4gCj4g Cj4gSSB3b25kZXIgd2h5IG5vdCBzaW1wbHkgZmFpbCBoZXJlPwoKQmFjayBpbiAyMDE2IGl0IHdl bnQgbGlrZSB0aGlzOgoKCU9uIFRodSwgSnVuIDAyLCAyMDE2IGF0IDA1OjEwOjU5UE0gLTA0MDAs IEFhcm9uIENvbm9sZSB3cm90ZToKCT4gKyAgICAgaWYgKHZpcnRpb19oYXNfZmVhdHVyZSh2ZGV2 LCBWSVJUSU9fTkVUX0ZfTVRVKSkgewoJPiArICAgICAgICAgICAgIGRldi0+bXR1ID0gdmlydGlv X2NyZWFkMTYodmRldiwKCT4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg IG9mZnNldG9mKHN0cnVjdCB2aXJ0aW9fbmV0X2NvbmZpZywKCT4gKyAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIG10dSkpOwoJPiArICAgICB9Cgk+ICsKCT4g ICAgICAgaWYgKHZpLT5hbnlfaGVhZGVyX3NnKQoJPiAgICAgICAgICAgICAgIGRldi0+bmVlZGVk X2hlYWRyb29tID0gdmktPmhkcl9sZW47Cgk+IAoKCU9uZSBjb21tZW50IHRob3VnaDogSSB0aGlu ayB3ZSBzaG91bGQgdmFsaWRhdGUgdGhlIG10dS4KCUlmIGl0J3MgaW52YWxpZCwgY2xlYXIgVklS VElPX05FVF9GX01UVSBhbmQgaWdub3JlLgoKClRvbyBsYXRlIGF0IHRoaXMgcG9pbnQgOikKCkkg Z3Vlc3MgaXQncyBhIHdheSB0byB0ZWxsIGRldmljZSAiSSBjYW4gbm90IGxpdmUgd2l0aCB0aGlz IE1UVSIsCmRldmljZSBjYW4gZmFpbCBGRUFUVVJFU19PSyBpZiBpdCB3YW50cyB0by4gTUlOX01U VQppcyBhbiBpbnRlcm5hbCBsaW51eCB0aGluZyBhbmQgYXQgdGhlIHRpbWUgSSBmZWx0IGl0J3Mg YmV0dGVyIHRvCnRyeSB0byBtYWtlIHByb2dyZXNzLgoKCj4gCj4gPiAgICAgICAgICB9Cj4gPiAK PiA+ICAgICAgICAgIHJldHVybiAwOwo+ID4gfQo+ID4gCj4gPiBBbmQgdGhlIHNwZWMgc2F5czoK PiA+IAo+ID4gCj4gPiBUaGUgZHJpdmVyIE1VU1QgZm9sbG93IHRoaXMgc2VxdWVuY2UgdG8gaW5p dGlhbGl6ZSBhIGRldmljZToKPiA+IDEuIFJlc2V0IHRoZSBkZXZpY2UuCj4gPiAyLiBTZXQgdGhl IEFDS05PV0xFREdFIHN0YXR1cyBiaXQ6IHRoZSBndWVzdCBPUyBoYXMgbm90aWNlZCB0aGUgZGV2 aWNlLgo+ID4gMy4gU2V0IHRoZSBEUklWRVIgc3RhdHVzIGJpdDogdGhlIGd1ZXN0IE9TIGtub3dz IGhvdyB0byBkcml2ZSB0aGUgZGV2aWNlLgo+ID4gNC4gUmVhZCBkZXZpY2UgZmVhdHVyZSBiaXRz LCBhbmQgd3JpdGUgdGhlIHN1YnNldCBvZiBmZWF0dXJlIGJpdHMgdW5kZXJzdG9vZCBieSB0aGUg T1MgYW5kIGRyaXZlciB0byB0aGUKPiA+IGRldmljZS4gRHVyaW5nIHRoaXMgc3RlcCB0aGUgZHJp dmVyIE1BWSByZWFkIChidXQgTVVTVCBOT1Qgd3JpdGUpIHRoZSBkZXZpY2Utc3BlY2lmaWMgY29u ZmlndXJhdGlvbgo+ID4gZmllbGRzIHRvIGNoZWNrIHRoYXQgaXQgY2FuIHN1cHBvcnQgdGhlIGRl dmljZSBiZWZvcmUgYWNjZXB0aW5nIGl0Lgo+ID4gNS4gU2V0IHRoZSBGRUFUVVJFU19PSyBzdGF0 dXMgYml0LiBUaGUgZHJpdmVyIE1VU1QgTk9UIGFjY2VwdCBuZXcgZmVhdHVyZSBiaXRzIGFmdGVy IHRoaXMgc3RlcC4KPiA+IDYuIFJlLXJlYWQgZGV2aWNlIHN0YXR1cyB0byBlbnN1cmUgdGhlIEZF QVRVUkVTX09LIGJpdCBpcyBzdGlsbCBzZXQ6IG90aGVyd2lzZSwgdGhlIGRldmljZSBkb2VzIG5v dAo+ID4gc3VwcG9ydCBvdXIgc3Vic2V0IG9mIGZlYXR1cmVzIGFuZCB0aGUgZGV2aWNlIGlzIHVu dXNhYmxlLgo+ID4gNy4gUGVyZm9ybSBkZXZpY2Utc3BlY2lmaWMgc2V0dXAsIGluY2x1ZGluZyBk aXNjb3Zlcnkgb2YgdmlydHF1ZXVlcyBmb3IgdGhlIGRldmljZSwgb3B0aW9uYWwgcGVyLWJ1cyBz ZXR1cCwKPiA+IHJlYWRpbmcgYW5kIHBvc3NpYmx5IHdyaXRpbmcgdGhlIGRldmljZeKAmXMgdmly dGlvIGNvbmZpZ3VyYXRpb24gc3BhY2UsIGFuZCBwb3B1bGF0aW9uIG9mIHZpcnRxdWV1ZXMuCj4g PiA4LiBTZXQgdGhlIERSSVZFUl9PSyBzdGF0dXMgYml0LiBBdCB0aGlzIHBvaW50IHRoZSBkZXZp Y2UgaXMg4oCcbGl2ZeKAnS4KPiA+IAo+ID4gCj4gPiBJdGVtIDQgb24gdGhlIGxpc3QgZXhwbGlj aXRseSBhbGxvd3MgcmVhZGluZyBjb25maWcgc3BhY2UgYmVmb3JlCj4gPiBGRUFUVVJFU19PSy4K PiA+IAo+ID4gSSBjb25jbHVkZSB0aGF0IFZJUlRJT19ORVRfRl9NVFUgaXMgc2V0IG1lYW5zICJz ZXQgaW4gZGV2aWNlIGZlYXR1cmVzIi4KPiAKPiAKPiBTbyB0aGlzIHByb2JhYmx5IG5lZWQgc29t ZSBjbGFyaWZpY2F0aW9uLiAiaXMgc2V0IiBpcyB1c2VkIG1hbnkgdGltZXMgaW4gdGhlCj4gc3Bl YyB0aGF0IGhhcyBkaWZmZXJlbnQgaW1wbGljYXRpb25zLgo+IAo+IFRoYW5rcwo+IAo+IAo+ID4g Cj4gPiBHZW5lcmFsbHkgaXQgaXMgd29ydGggZ29pbmcgb3ZlciBmZWF0dXJlIGRlcGVuZGVudCBj b25maWcgZmllbGRzCj4gPiBhbmQgY2hlY2tpbmcgd2hldGhlciB0aGV5IHNob3VsZCBiZSBwcmVz ZW50IHdoZW4gZGV2aWNlIGZlYXR1cmUgaXMgc2V0Cj4gPiBvciB3aGVuIGZlYXR1cmUgYml0IGhh cyBiZWVuIG5lZ290aWF0ZWQsIGFuZCBtYWtpbmcgdGhpcyBjbGVhci4KPiA+IAoKX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KVmlydHVhbGl6YXRpb24gbWFp bGluZyBsaXN0ClZpcnR1YWxpemF0aW9uQGxpc3RzLmxpbnV4LWZvdW5kYXRpb24ub3JnCmh0dHBz Oi8vbGlzdHMubGludXhmb3VuZGF0aW9uLm9yZy9tYWlsbWFuL2xpc3RpbmZvL3ZpcnR1YWxpemF0 aW9u From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4DA19C433DB for ; Tue, 23 Feb 2021 10:04:26 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0E95A64E20 for ; Tue, 23 Feb 2021 10:04:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232350AbhBWKEZ (ORCPT ); Tue, 23 Feb 2021 05:04:25 -0500 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:33334 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231867AbhBWKEI (ORCPT ); Tue, 23 Feb 2021 05:04:08 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1614074562; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=fq9fMMA50uXmWN63AH4eeQqewule2njQVz39u52s5qo=; b=deTTz0mD4ZDmqsREN/FhdosC1PWGX2b8AAx2SIndzR/hKJ3XrR6iOgntVEIwJBpEIvwnk0 9uh1kLaEZIYqeRoc1VcCv0no/tQjfvyobKMfHwh79yoEI4nR9TUCUIAekviuQMB/PBVPDW a/aaZwP3LwgRFug1fBQ+6PLXvEmR9ao= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-257-6Xk49179MvGZSEqTLX0u8g-1; Tue, 23 Feb 2021 05:01:36 -0500 X-MC-Unique: 6Xk49179MvGZSEqTLX0u8g-1 Received: by mail-wr1-f71.google.com with SMTP id l10so7123159wry.16 for ; Tue, 23 Feb 2021 02:01:35 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=fq9fMMA50uXmWN63AH4eeQqewule2njQVz39u52s5qo=; b=Yi+kPQJjqHY2/Kofh65Qu+hHvBjLyOzXoY2WdO3UUzIiqV1Se6f7gF1ojdXSaoFeCG JK7DiUYnWlT0WKaGp8cXpm4rJSFuAI5D05oa4WARSv9Kiko7/s+CNYjLdhqMeG6MoaJ7 uUX97fclHEkOnjF8kre/oNdlie0W7B8s58X4hw1uL/lJsDgvMBhdLsuBmraiZZGXugyt b5DxvLrwc6zO9m8joyztkaU0Ije9NmTKbdRcnidn8HaQDoi3aXslndyQyk7R6RL34Nwg HNXmDtlJi8SA491lAgV4mTndsvEJMFBP1s0j7M4f8aOiRxiH2i1fatRexwoXgcceL828 N9BA== X-Gm-Message-State: AOAM532pu5WqLPizFmrTXbCnT0EukwFqoAmTO727aQAtHOu4JtsyQISY WlXKPrkOdQZ2cv/QKg8v//TNsJIQa5UI+A+Iz9u5rhvie1PCY8+EjBFV0HIfL9ySKh1eWR5kIO2 J9H0Ox0roVeFik6sMApstJGPn X-Received: by 2002:adf:97d5:: with SMTP id t21mr1510153wrb.139.1614074495014; Tue, 23 Feb 2021 02:01:35 -0800 (PST) X-Google-Smtp-Source: ABdhPJzqzcytawXMdenSafG3a3yvlWPCUdXAY4CQjPGVhSoQqq+a9ZdeDw2NxLx25iW4TucswKLb7Q== X-Received: by 2002:adf:97d5:: with SMTP id t21mr1510136wrb.139.1614074494799; Tue, 23 Feb 2021 02:01:34 -0800 (PST) Received: from redhat.com (bzq-79-180-2-31.red.bezeqint.net. [79.180.2.31]) by smtp.gmail.com with ESMTPSA id z11sm2046114wmi.35.2021.02.23.02.01.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 23 Feb 2021 02:01:33 -0800 (PST) Date: Tue, 23 Feb 2021 05:01:31 -0500 From: "Michael S. Tsirkin" To: Jason Wang Cc: Si-Wei Liu , elic@nvidia.com, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, netdev@vger.kernel.org, virtio-dev@lists.oasis-open.org Subject: Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero Message-ID: <20210223045600-mutt-send-email-mst@kernel.org> References: <1613735698-3328-1-git-send-email-si-wei.liu@oracle.com> <605e7d2d-4f27-9688-17a8-d57191752ee7@redhat.com> <20210223041740-mutt-send-email-mst@kernel.org> <788a0880-0a68-20b7-5bdf-f8150b08276a@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <788a0880-0a68-20b7-5bdf-f8150b08276a@redhat.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Feb 23, 2021 at 05:46:20PM +0800, Jason Wang wrote: > > On 2021/2/23 下午5:25, Michael S. Tsirkin wrote: > > On Mon, Feb 22, 2021 at 09:09:28AM -0800, Si-Wei Liu wrote: > > > > > > On 2/21/2021 8:14 PM, Jason Wang wrote: > > > > On 2021/2/19 7:54 下午, Si-Wei Liu wrote: > > > > > Commit 452639a64ad8 ("vdpa: make sure set_features is invoked > > > > > for legacy") made an exception for legacy guests to reset > > > > > features to 0, when config space is accessed before features > > > > > are set. We should relieve the verify_min_features() check > > > > > and allow features reset to 0 for this case. > > > > > > > > > > It's worth noting that not just legacy guests could access > > > > > config space before features are set. For instance, when > > > > > feature VIRTIO_NET_F_MTU is advertised some modern driver > > > > > will try to access and validate the MTU present in the config > > > > > space before virtio features are set. > > > > > > > > This looks like a spec violation: > > > > > > > > " > > > > > > > > The following driver-read-only field, mtu only exists if > > > > VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the > > > > driver to use. > > > > " > > > > > > > > Do we really want to workaround this? > > > Isn't the commit 452639a64ad8 itself is a workaround for legacy guest? > > > > > > I think the point is, since there's legacy guest we'd have to support, this > > > host side workaround is unavoidable. Although I agree the violating driver > > > should be fixed (yes, it's in today's upstream kernel which exists for a > > > while now). > > Oh you are right: > > > > > > static int virtnet_validate(struct virtio_device *vdev) > > { > > if (!vdev->config->get) { > > dev_err(&vdev->dev, "%s failure: config access disabled\n", > > __func__); > > return -EINVAL; > > } > > > > if (!virtnet_validate_features(vdev)) > > return -EINVAL; > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) { > > int mtu = virtio_cread16(vdev, > > offsetof(struct virtio_net_config, > > mtu)); > > if (mtu < MIN_MTU) > > __virtio_clear_bit(vdev, VIRTIO_NET_F_MTU); > > > I wonder why not simply fail here? Back in 2016 it went like this: On Thu, Jun 02, 2016 at 05:10:59PM -0400, Aaron Conole wrote: > + if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) { > + dev->mtu = virtio_cread16(vdev, > + offsetof(struct virtio_net_config, > + mtu)); > + } > + > if (vi->any_header_sg) > dev->needed_headroom = vi->hdr_len; > One comment though: I think we should validate the mtu. If it's invalid, clear VIRTIO_NET_F_MTU and ignore. Too late at this point :) I guess it's a way to tell device "I can not live with this MTU", device can fail FEATURES_OK if it wants to. MIN_MTU is an internal linux thing and at the time I felt it's better to try to make progress. > > > } > > > > return 0; > > } > > > > And the spec says: > > > > > > The driver MUST follow this sequence to initialize a device: > > 1. Reset the device. > > 2. Set the ACKNOWLEDGE status bit: the guest OS has noticed the device. > > 3. Set the DRIVER status bit: the guest OS knows how to drive the device. > > 4. Read device feature bits, and write the subset of feature bits understood by the OS and driver to the > > device. During this step the driver MAY read (but MUST NOT write) the device-specific configuration > > fields to check that it can support the device before accepting it. > > 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature bits after this step. > > 6. Re-read device status to ensure the FEATURES_OK bit is still set: otherwise, the device does not > > support our subset of features and the device is unusable. > > 7. Perform device-specific setup, including discovery of virtqueues for the device, optional per-bus setup, > > reading and possibly writing the device’s virtio configuration space, and population of virtqueues. > > 8. Set the DRIVER_OK status bit. At this point the device is “live”. > > > > > > Item 4 on the list explicitly allows reading config space before > > FEATURES_OK. > > > > I conclude that VIRTIO_NET_F_MTU is set means "set in device features". > > > So this probably need some clarification. "is set" is used many times in the > spec that has different implications. > > Thanks > > > > > > Generally it is worth going over feature dependent config fields > > and checking whether they should be present when device feature is set > > or when feature bit has been negotiated, and making this clear. > >