From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH v2] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ Date: Tue, 7 Jan 2020 02:06:13 -0500 Message-ID: <20200107020303-mutt-send-email-mst@kernel.org> References: <20200105132120.92370-1-mst@redhat.com> <2d7053b5-295c-4051-a722-7656350bdb74@redhat.com> <20200106074426-mutt-send-email-mst@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: virtualization-bounces@lists.linux-foundation.org Sender: "Virtualization" To: Jason Wang Cc: Alistair Delva , Willem de Bruijn , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, "David S. Miller" List-Id: virtualization@lists.linuxfoundation.org T24gVHVlLCBKYW4gMDcsIDIwMjAgYXQgMTA6Mjk6MDhBTSArMDgwMCwgSmFzb24gV2FuZyB3cm90 ZToKPiAKPiBPbiAyMDIwLzEvNiDkuIvljYg4OjU0LCBNaWNoYWVsIFMuIFRzaXJraW4gd3JvdGU6 Cj4gPiBPbiBNb24sIEphbiAwNiwgMjAyMCBhdCAxMDo0NzozNUFNICswODAwLCBKYXNvbiBXYW5n IHdyb3RlOgo+ID4gPiBPbiAyMDIwLzEvNSDkuIvljYg5OjIyLCBNaWNoYWVsIFMuIFRzaXJraW4g d3JvdGU6Cj4gPiA+ID4gVGhlIG9ubHkgd2F5IGZvciBndWVzdCB0byBjb250cm9sIG9mZmxvYWRz IChhcyBlbmFibGVkIGJ5Cj4gPiA+ID4gVklSVElPX05FVF9GX0NUUkxfR1VFU1RfT0ZGTE9BRFMp IGlzIGJ5IHNlbmRpbmcgY29tbWFuZHMKPiA+ID4gPiB0aHJvdWdoIENUUkxfVlEuIFNvIGl0IGRv ZXMgbm90IG1ha2Ugc2Vuc2UgdG8KPiA+ID4gPiBhY2tub3dsZWRnZSBWSVJUSU9fTkVUX0ZfQ1RS TF9HVUVTVF9PRkZMT0FEUyB3aXRob3V0Cj4gPiA+ID4gVklSVElPX05FVF9GX0NUUkxfVlEuCj4g PiA+ID4gCj4gPiA+ID4gVGhlIHNwZWMgZG9lcyBub3Qgb3V0bGF3IGRldmljZXMgd2l0aCBzdWNo IGEgY29uZmlndXJhdGlvbiwgc28gd2UgaGF2ZQo+ID4gPiA+IHRvIHN1cHBvcnQgaXQuIFNpbXBs eSBjbGVhciBWSVJUSU9fTkVUX0ZfQ1RSTF9HVUVTVF9PRkZMT0FEUy4KPiA+ID4gPiBOb3RlIHRo YXQgTGludXggaXMgc3RpbGwgY3Jhc2hpbmcgaWYgaXQgdHJpZXMgdG8KPiA+ID4gPiBjaGFuZ2Ug dGhlIG9mZmxvYWRzIHdoZW4gdGhlcmUncyBubyBjb250cm9sIHZxLgo+ID4gPiA+IFRoYXQgbmVl ZHMgdG8gYmUgZml4ZWQgYnkgYW5vdGhlciBwYXRjaC4KPiA+ID4gPiAKPiA+ID4gPiBSZXBvcnRl ZC1ieTogQWxpc3RhaXIgRGVsdmEgPGFkZWx2YUBnb29nbGUuY29tPgo+ID4gPiA+IFJlcG9ydGVk LWJ5OiBXaWxsZW0gZGUgQnJ1aWpuIDx3aWxsZW1kZWJydWlqbi5rZXJuZWxAZ21haWwuY29tPgo+ ID4gPiA+IEZpeGVzOiAzZjkzNTIyZmZhYjIgKCJ2aXJ0aW8tbmV0OiBzd2l0Y2ggb2ZmIG9mZmxv YWRzIG9uIGRlbWFuZCBpZiBwb3NzaWJsZSBvbiBYRFAgc2V0IikKPiA+ID4gPiBTaWduZWQtb2Zm LWJ5OiBNaWNoYWVsIFMuIFRzaXJraW4gPG1zdEByZWRoYXQuY29tPgo+ID4gPiA+IC0tLQo+ID4g PiA+IAo+ID4gPiA+IFNhbWUgcGF0Y2ggYXMgdjEgYnV0IHVwZGF0ZSBkb2N1bWVudGF0aW9uIHNv IGl0J3MgY2xlYXIgaXQncyBub3QKPiA+ID4gPiBlbm91Z2ggdG8gZml4IHRoZSBjcmFzaC4KPiA+ ID4gPiAKPiA+ID4gPiAgICBkcml2ZXJzL25ldC92aXJ0aW9fbmV0LmMgfCA5ICsrKysrKysrKwo+ ID4gPiA+ICAgIDEgZmlsZSBjaGFuZ2VkLCA5IGluc2VydGlvbnMoKykKPiA+ID4gPiAKPiA+ID4g PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9uZXQvdmlydGlvX25ldC5jIGIvZHJpdmVycy9uZXQvdmly dGlvX25ldC5jCj4gPiA+ID4gaW5kZXggNGQ3ZDU0MzRjYzVkLi43Yjg4MDViNDdmMGQgMTAwNjQ0 Cj4gPiA+ID4gLS0tIGEvZHJpdmVycy9uZXQvdmlydGlvX25ldC5jCj4gPiA+ID4gKysrIGIvZHJp dmVycy9uZXQvdmlydGlvX25ldC5jCj4gPiA+ID4gQEAgLTI5NzEsNiArMjk3MSwxNSBAQCBzdGF0 aWMgaW50IHZpcnRuZXRfdmFsaWRhdGUoc3RydWN0IHZpcnRpb19kZXZpY2UgKnZkZXYpCj4gPiA+ ID4gICAgCWlmICghdmlydG5ldF92YWxpZGF0ZV9mZWF0dXJlcyh2ZGV2KSkKPiA+ID4gPiAgICAJ CXJldHVybiAtRUlOVkFMOwo+ID4gPiA+ICsJLyogVklSVElPX05FVF9GX0NUUkxfR1VFU1RfT0ZG TE9BRFMgZG9lcyBub3Qgd29yayB3aXRob3V0Cj4gPiA+ID4gKwkgKiBWSVJUSU9fTkVUX0ZfQ1RS TF9WUS4gVW5mb3J0dW5hdGVseSBzcGVjIGZvcmdvdCB0bwo+ID4gPiA+ICsJICogc3BlY2lmeSB0 aGF0IFZJUlRJT19ORVRfRl9DVFJMX0dVRVNUX09GRkxPQURTIGRlcGVuZHMKPiA+ID4gPiArCSAq IG9uIFZJUlRJT19ORVRfRl9DVFJMX1ZRIHNvIGRldmljZXMgY2FuIHNldCB0aGUgbGF0ZXIgYnV0 Cj4gPiA+ID4gKwkgKiBub3QgdGhlIGZvcm1lci4KPiA+ID4gPiArCSAqLwo+ID4gPiA+ICsJaWYg KCF2aXJ0aW9faGFzX2ZlYXR1cmUodmRldiwgVklSVElPX05FVF9GX0NUUkxfVlEpKQo+ID4gPiA+ ICsJCQlfX3ZpcnRpb19jbGVhcl9iaXQodmRldiwgVklSVElPX05FVF9GX0NUUkxfR1VFU1RfT0ZG TE9BRFMpOwo+ID4gPiAKPiA+ID4gSWYgaXQncyBqdXN0IGJlY2F1c2UgYSBidWcgb2Ygc3BlYywg c2hvdWxkIHdlIHNpbXBseSBmaXggdGhlIGJ1ZyBhbmQgZmFpbAo+ID4gPiB0aGUgbmVnb3RpYXRp b24gaW4gdmlydG5ldF92YWxpZGF0ZV9mZWF0dXJlKCk/Cj4gPiBPbmUgbWFuJ3MgYnVnIGlzIGFu b3RoZXIgbWFuJ3MgZmVhdHVyZTogYXJndWFibHkgbGVhdmluZyB0aGUgZmVhdHVyZXMKPiA+IGlu ZGVwZW5kZW50IGluIHRoZSBzcGVjIG1pZ2h0IGFsbG93IHJldXNlIG9mIHRoZSBmZWF0dXJlIGJp dCB3aXRob3V0Cj4gPiBicmVha2luZyBndWVzdHMuCj4gPiAKPiA+IEFuZCBldmVuIGlmIHdlIHNh eSBpdCdzIGEgYnVnIHdlIGNhbid0IHNpbXBseSBmaXggdGhlIGJ1ZyBpbiB0aGUKPiA+IHNwZWM6 IGNoYW5naW5nIHRoZSB0ZXh0IGZvciBhIGZ1dHVyZSB2ZXJzaW9uIGRvZXMgbm90IGNoYW5nZSB0 aGUgZmFjdAo+ID4gdGhhdCBkZXZpY2VzIGJlaGF2aW5nIGFjY29yZGluZyB0byB0aGUgc3BlYyBl eGlzdC4KPiA+IAo+ID4gPiBPdGhlcndpc2UgdGhlcmUgd291bGQgYmUgaW5jb25zaXN0ZW5jeSBp biBoYW5kbGluZyBmZWF0dXJlIGRlcGVuZGVuY2llcyBmb3IKPiA+ID4gY3RybCB2cS4KPiA+ID4g Cj4gPiA+IFRoYW5rcwo+ID4gVGhhdCdzIGEgY29zbWV0aWMgcHJvYmxlbSBBVE0uIEl0IG1pZ2h0 IGJlIGEgZ29vZCBpZGVhIHRvIGdlbmVyYWxseQo+ID4gY2hhbmdlIG91ciBoYW5kbGluZyBvZiBk ZXBlbmRlbmNpZXMsIGFuZCBjbGVhciBmZWF0dXJlIGJpdHMgaW5zdGVhZCBvZgo+ID4gZmFpbGlu ZyBwcm9iZSBvbiBhIG1pc21hdGNoLgo+IAo+IAo+IFNvbWV0aGluZyBsaWtlIEkgcHJvcG9zZWQg aW4gdGhlIHBhc3QgPyBbMV0KPiAKPiBbMV0gaHR0cHM6Ly9sb3JlLmtlcm5lbC5vcmcvcGF0Y2h3 b3JrL3BhdGNoLzUxOTA3NC8KCgpObyB0aGF0IHN0aWxsIGZhaWxzIHByb2JlLgoKSSBhbSBhc2tp bmcgd2hldGhlciBpdCdzIG1vcmUgZnV0dXJlIHByb29mIHRvIGZhaWwgcHJvYmUKb24gZmVhdHVy ZSBjb21iaW5hdGlvbnMgZGlzYWxsb3dlZCBieSBzcGVjLCBvciB0byBjbGVhciBiaXRzCnRvIGdl dCB0byBhbiBleHBlY3RlZCBjb21iaW5hdGlvbi4KCkluIGFueSBjYXNlLCB3ZSBzaG91bGQgcHJv YmFibHkgZG9jdW1lbnQgaW4gdGhlIHNwZWMgaG93CmRyaXZlcnMgYmVoYXZlIG9uIHN1Y2ggY29t YmluYXRpb25zLgoKCj4gCj4gPiAgIEl0J3Mgd29ydGggdGhpbmtpbmcgIC0gYXQgdGhlIHNwZWMg bGV2ZWwgLQo+ID4gaG93IHdlIGNhbiBiZXN0IG1ha2UgdGhlIGNvbmZpZ3VyYXRpb24gZXh0ZW5z aWJsZS4KPiA+IEJ1dCB0aGF0J3Mgbm90IHNvbWV0aGluZyBzcGVjIHNob3VsZCB3b3JyeSBhYm91 dC4KPiA+IAo+ID4gCj4gPiA+ID4gKwo+ID4gPiA+ICAgIAlpZiAodmlydGlvX2hhc19mZWF0dXJl KHZkZXYsIFZJUlRJT19ORVRfRl9NVFUpKSB7Cj4gPiA+ID4gICAgCQlpbnQgbXR1ID0gdmlydGlv X2NyZWFkMTYodmRldiwKPiA+ID4gPiAgICAJCQkJCSBvZmZzZXRvZihzdHJ1Y3QgdmlydGlvX25l dF9jb25maWcsCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f XwpWaXJ0dWFsaXphdGlvbiBtYWlsaW5nIGxpc3QKVmlydHVhbGl6YXRpb25AbGlzdHMubGludXgt Zm91bmRhdGlvbi5vcmcKaHR0cHM6Ly9saXN0cy5saW51eGZvdW5kYXRpb24ub3JnL21haWxtYW4v bGlzdGluZm8vdmlydHVhbGl6YXRpb24= 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=-6.9 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham 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 611BDC33C99 for ; Tue, 7 Jan 2020 07:06:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 29DAF206DB for ; Tue, 7 Jan 2020 07:06:24 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="DTexhIJX" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726558AbgAGHGX (ORCPT ); Tue, 7 Jan 2020 02:06:23 -0500 Received: from us-smtp-1.mimecast.com ([205.139.110.61]:30804 "EHLO us-smtp-delivery-1.mimecast.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1725914AbgAGHGW (ORCPT ); Tue, 7 Jan 2020 02:06:22 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578380780; 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=/efjXzZHBg1iCroxSDJqHPrwvRqUs+JXRk92V6GEKaU=; b=DTexhIJXIOMefzaoInUaxp09cs492MOYlRlr8IHFbB6Aj3wWCEY3SWr/eJa5O4HAdDjLYN QZCZEA4Y6XPDHRNbjURIGkRiIYdsIDfK1LCO8iMYe86STWXZDJdwFLidx5UPbvKTpma2kG ndjJ4lWIpkW34xEqOSdsF16WEHxUYpM= Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-280-Yn_dcOQeO26PnkbIrYzksg-1; Tue, 07 Jan 2020 02:06:19 -0500 X-MC-Unique: Yn_dcOQeO26PnkbIrYzksg-1 Received: by mail-qv1-f70.google.com with SMTP id f16so36240771qvr.7 for ; Mon, 06 Jan 2020 23:06:19 -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=/efjXzZHBg1iCroxSDJqHPrwvRqUs+JXRk92V6GEKaU=; b=t7QdaQ3OwxJIvmsyOoFYRW1MCTFg/IIQJuEXtqXoPesz6RvFLzsSo+y6jzvKEELaGI kFp8460J+nK1lmE/7UqkPTE7MkOq5u3AQcLNJPUJX5ZZgXTDOHvmiQIHubJa+XizHv3S euhCaPOX/X3RoBsSWhQNlN7+NbzT6tTRUDKpuxx5k7cnEnniA9vIKdx7lW8S2B89X+Ei B+Sa/QbcUfne9GUa0V2oEbyecnxnZ/AvofP29Vv3zMDUjisDTwI0xr/V4GDPbCaRnA9Y rAZfGnbCrJxrldRmosGFf0CSGoCNGEj6NaLS+uj2MenSSSZxQvg4D0B46FuLh/BE4zc7 3OYA== X-Gm-Message-State: APjAAAWqMAq4YG228GEWWfmHNnaDsp7O3UauNvBQt/6h5YTTDRiy0cpz 6b1VcLezZYwB+ppUkdOjCXms/mTuM5nTfPw3lGVxhvbb1gpHqXoJ9tBnfD7foJrz9rFJR9zRD4M +DSe/3r8tv2+gD02h0v/6tCOH X-Received: by 2002:ac8:709a:: with SMTP id y26mr78234905qto.304.1578380779300; Mon, 06 Jan 2020 23:06:19 -0800 (PST) X-Google-Smtp-Source: APXvYqyK5Mb+pOAlU+KoIVfxx6YvoZ+HKotsgyRPL144jEGAH30ilnJpURC7Q9CI21MUoelMWJIJHQ== X-Received: by 2002:ac8:709a:: with SMTP id y26mr78234888qto.304.1578380779070; Mon, 06 Jan 2020 23:06:19 -0800 (PST) Received: from redhat.com (bzq-79-183-34-164.red.bezeqint.net. [79.183.34.164]) by smtp.gmail.com with ESMTPSA id o16sm21915243qkj.91.2020.01.06.23.06.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Jan 2020 23:06:18 -0800 (PST) Date: Tue, 7 Jan 2020 02:06:13 -0500 From: "Michael S. Tsirkin" To: Jason Wang Cc: linux-kernel@vger.kernel.org, Alistair Delva , Willem de Bruijn , "David S. Miller" , virtualization@lists.linux-foundation.org, netdev@vger.kernel.org Subject: Re: [PATCH v2] virtio_net: CTRL_GUEST_OFFLOADS depends on CTRL_VQ Message-ID: <20200107020303-mutt-send-email-mst@kernel.org> References: <20200105132120.92370-1-mst@redhat.com> <2d7053b5-295c-4051-a722-7656350bdb74@redhat.com> <20200106074426-mutt-send-email-mst@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 07, 2020 at 10:29:08AM +0800, Jason Wang wrote: > > On 2020/1/6 下午8:54, Michael S. Tsirkin wrote: > > On Mon, Jan 06, 2020 at 10:47:35AM +0800, Jason Wang wrote: > > > On 2020/1/5 下午9:22, Michael S. Tsirkin wrote: > > > > The only way for guest to control offloads (as enabled by > > > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS) is by sending commands > > > > through CTRL_VQ. So it does not make sense to > > > > acknowledge VIRTIO_NET_F_CTRL_GUEST_OFFLOADS without > > > > VIRTIO_NET_F_CTRL_VQ. > > > > > > > > The spec does not outlaw devices with such a configuration, so we have > > > > to support it. Simply clear VIRTIO_NET_F_CTRL_GUEST_OFFLOADS. > > > > Note that Linux is still crashing if it tries to > > > > change the offloads when there's no control vq. > > > > That needs to be fixed by another patch. > > > > > > > > Reported-by: Alistair Delva > > > > Reported-by: Willem de Bruijn > > > > Fixes: 3f93522ffab2 ("virtio-net: switch off offloads on demand if possible on XDP set") > > > > Signed-off-by: Michael S. Tsirkin > > > > --- > > > > > > > > Same patch as v1 but update documentation so it's clear it's not > > > > enough to fix the crash. > > > > > > > > drivers/net/virtio_net.c | 9 +++++++++ > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > index 4d7d5434cc5d..7b8805b47f0d 100644 > > > > --- a/drivers/net/virtio_net.c > > > > +++ b/drivers/net/virtio_net.c > > > > @@ -2971,6 +2971,15 @@ static int virtnet_validate(struct virtio_device *vdev) > > > > if (!virtnet_validate_features(vdev)) > > > > return -EINVAL; > > > > + /* VIRTIO_NET_F_CTRL_GUEST_OFFLOADS does not work without > > > > + * VIRTIO_NET_F_CTRL_VQ. Unfortunately spec forgot to > > > > + * specify that VIRTIO_NET_F_CTRL_GUEST_OFFLOADS depends > > > > + * on VIRTIO_NET_F_CTRL_VQ so devices can set the later but > > > > + * not the former. > > > > + */ > > > > + if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) > > > > + __virtio_clear_bit(vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS); > > > > > > If it's just because a bug of spec, should we simply fix the bug and fail > > > the negotiation in virtnet_validate_feature()? > > One man's bug is another man's feature: arguably leaving the features > > independent in the spec might allow reuse of the feature bit without > > breaking guests. > > > > And even if we say it's a bug we can't simply fix the bug in the > > spec: changing the text for a future version does not change the fact > > that devices behaving according to the spec exist. > > > > > Otherwise there would be inconsistency in handling feature dependencies for > > > ctrl vq. > > > > > > Thanks > > That's a cosmetic problem ATM. It might be a good idea to generally > > change our handling of dependencies, and clear feature bits instead of > > failing probe on a mismatch. > > > Something like I proposed in the past ? [1] > > [1] https://lore.kernel.org/patchwork/patch/519074/ No that still fails probe. I am asking whether it's more future proof to fail probe on feature combinations disallowed by spec, or to clear bits to get to an expected combination. In any case, we should probably document in the spec how drivers behave on such combinations. > > > It's worth thinking - at the spec level - > > how we can best make the configuration extensible. > > But that's not something spec should worry about. > > > > > > > > + > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) { > > > > int mtu = virtio_cread16(vdev, > > > > offsetof(struct virtio_net_config,