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.0 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 AA558C433DB for ; Sun, 28 Feb 2021 21:27:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 70C4864E61 for ; Sun, 28 Feb 2021 21:27:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231161AbhB1V0v (ORCPT ); Sun, 28 Feb 2021 16:26:51 -0500 Received: from us-smtp-delivery-124.mimecast.com ([63.128.21.124]:35478 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230019AbhB1V0r (ORCPT ); Sun, 28 Feb 2021 16:26:47 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1614547518; 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=3yMfUwMSMlLzTtYBioGpjaiH9NykZNmNU2hz5aZaT5Q=; b=Z8SekN/YB+8RH7h/PwsyaX890VyBxQPySf5BvG68OTE1XMmN6Uw/YhhKhj6PUGZNi3EeBu 290V2OceBBqmQrqn5rwqzt4r9q6bCXLXOc+RcKJ1mRd7Cwem6Ec+R4bzilomgBl76zOI67 Z8iYeT8yD2sSZiPmyUCy/yi5iM6mohk= Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-504-xUyjLNxKOdaMLCDyu-XKcQ-1; Sun, 28 Feb 2021 16:25:17 -0500 X-MC-Unique: xUyjLNxKOdaMLCDyu-XKcQ-1 Received: by mail-ed1-f71.google.com with SMTP id i4so7877401edt.11 for ; Sun, 28 Feb 2021 13:25:16 -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=3yMfUwMSMlLzTtYBioGpjaiH9NykZNmNU2hz5aZaT5Q=; b=ejne7V3YrJPA23lzfSgV6d/HacX9lp2joBkHiHef+6tXiTZYhN5rb+zDL9WPgH7+tj OyonddkpDc648YCSZRm60CrZbRJaAhoFGzi8KiS12Y0zpNB9/amwpbS2lP1nOCNAQeXy hudoNJOQOEyAnjRrF9C8o4ISZ2ndspzowMEwZHmVCBbFw7wVuFMs+blCNS+SIvCZCeRe TDbO0oCUtQqVeZCWVHPD63ve3913Y7i8U5myZ9kZksPbC4ydkHXRiASeEy2HsR4Lv6XZ opA/lKmYPJlpycbpz0d9oeHxC7+2xx65Rp7LPn6r7J/+4meOFz+ngM0KpIY2Svz12NSL bVTw== X-Gm-Message-State: AOAM530/4oNOG2FfU62dBw3WuAqlzF3y0jZ8084t/2pOdlTKrpl3hzFz gQ45hdNdhgcTOPsLuzFeHkP0a9gi9tohqthbPpOtGynNDa7KDyzrGSiXPiAIm9wobu9HNoYOA7q Et6dJwvX4StKFI1Vov0ZIFC98 X-Received: by 2002:aa7:c403:: with SMTP id j3mr13466256edq.137.1614547515828; Sun, 28 Feb 2021 13:25:15 -0800 (PST) X-Google-Smtp-Source: ABdhPJwRcQQYvlSJ36Xy6Hj8V4Z8794sGOmtN+bAL8Csgl0n1MU5SKaQ7rs0ucHXpwyqFI/lJoxhzw== X-Received: by 2002:aa7:c403:: with SMTP id j3mr13466237edq.137.1614547515530; Sun, 28 Feb 2021 13:25:15 -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 a14sm12258393edu.13.2021.02.28.13.25.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 28 Feb 2021 13:25:14 -0800 (PST) Date: Sun, 28 Feb 2021 16:25:11 -0500 From: "Michael S. Tsirkin" To: Jason Wang Cc: Cornelia Huck , 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: [virtio-dev] Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero Message-ID: <20210228162306-mutt-send-email-mst@kernel.org> References: <20210223041740-mutt-send-email-mst@kernel.org> <788a0880-0a68-20b7-5bdf-f8150b08276a@redhat.com> <20210223110430.2f098bc0.cohuck@redhat.com> <20210223115833.732d809c.cohuck@redhat.com> <8355f9b3-4cda-cd2e-98df-fed020193008@redhat.com> <20210224121234.0127ae4b.cohuck@redhat.com> <20210225135229-mutt-send-email-mst@kernel.org> <0f8eb381-cc98-9e05-0e35-ccdb1cbd6119@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <0f8eb381-cc98-9e05-0e35-ccdb1cbd6119@redhat.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Feb 26, 2021 at 04:19:16PM +0800, Jason Wang wrote: > > On 2021/2/26 2:53 上午, Michael S. Tsirkin wrote: > > On Thu, Feb 25, 2021 at 12:36:07PM +0800, Jason Wang wrote: > > > On 2021/2/24 7:12 下午, Cornelia Huck wrote: > > > > On Wed, 24 Feb 2021 17:29:07 +0800 > > > > Jason Wang wrote: > > > > > > > > > On 2021/2/23 6:58 下午, Cornelia Huck wrote: > > > > > > On Tue, 23 Feb 2021 18:31:07 +0800 > > > > > > Jason Wang wrote: > > > > > > > On 2021/2/23 6:04 下午, Cornelia Huck wrote: > > > > > > > > On Tue, 23 Feb 2021 17:46:20 +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? > > > > > > > > I think both failing or not accepting the feature can be argued to make > > > > > > > > sense: "the device presented us with a mtu size that does not make > > > > > > > > sense" would point to failing, "we cannot work with the mtu size that > > > > > > > > the device presented us" would point to not negotiating the feature. > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > Before FEATURES_OK is set by the driver, I guess it means "the device > > > > > > > > has offered the feature"; > > > > > > > For me this part is ok since it clarify that it's the driver that set > > > > > > > the bit. > > > > > > > > > > > > > > > > > > > > > > during normal usage, it means "the feature > > > > > > > > has been negotiated". > > > > > > > /? > > > > > > > > > > > > > > It looks to me the feature negotiation is done only after device set > > > > > > > FEATURES_OK, or FEATURES_OK could be read from device status? > > > > > > I'd consider feature negotiation done when the driver reads FEATURES_OK > > > > > > back from the status. > > > > > I agree. > > > > > > > > > > > > > > > > > > (This is a bit fuzzy for legacy mode.) > > > > > > ...because legacy does not have FEATURES_OK. > > > > > > > The problem is the MTU description for example: > > > > > > > > > > > > > > "The following driver-read-only field, mtu only exists if > > > > > > > VIRTIO_NET_F_MTU is set." > > > > > > > > > > > > > > It looks to me need to use "if VIRTIO_NET_F_MTU is set by device". > > > > > > "offered by the device"? I don't think it should 'disappear' from the > > > > > > config space if the driver won't use it. (Same for other config space > > > > > > fields that are tied to feature bits.) > > > > > But what happens if e.g device doesn't offer VIRTIO_NET_F_MTU? It looks > > > > > to according to the spec there will be no mtu field. > > > > I think so, yes. > > > > > > > > > And a more interesting case is VIRTIO_NET_F_MQ is not offered but > > > > > VIRTIO_NET_F_MTU offered. To me, it means we don't have > > > > > max_virtqueue_pairs but it's not how the driver is wrote today. > > > > That would be a bug, but it seems to me that the virtio-net driver > > > > reads max_virtqueue_pairs conditionally and handles absence of the > > > > feature correctly? > > > > > > Yes, see the avove codes: > > > > > >         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); > > >         } > > > > > > So it's probably too late to fix the driver. > > > > > Confused. What is wrong with the above? It never reads the > > field unless the feature has been offered by device. > > > So the spec said: > > " > > The following driver-read-only field, max_virtqueue_pairs only exists if > VIRTIO_NET_F_MQ is set. > > " > > If I read this correctly, there will be no max_virtqueue_pairs field if the > VIRTIO_NET_F_MQ is not offered by device? If yes the offsetof() violates > what spec said. > > Thanks I think that's a misunderstanding. This text was never intended to imply that field offsets change beased on feature bits. We had this pain with legacy and we never wanted to go back there. This merely implies that without VIRTIO_NET_F_MQ the field should not be accessed. Exists in the sense "is accessible to driver". Let's just clarify that in the spec, job done. > > > > > > > > > > > > Otherwise readers (at least for me), may think the MTU is only valid > > > > > > > if driver set the bit. > > > > > > I think it would still be 'valid' in the sense that it exists and has > > > > > > some value in there filled in by the device, but a driver reading it > > > > > > without negotiating the feature would be buggy. (Like in the kernel > > > > > > code above; the kernel not liking the value does not make the field > > > > > > invalid.) > > > > > See Michael's reply, the spec allows read the config before setting > > > > > features. > > > > Yes, the period prior to finishing negotiation is obviously special. > > > > > > > > > > Maybe a statement covering everything would be: > > > > > > > > > > > > "The following driver-read-only field mtu only exists if the device > > > > > > offers VIRTIO_NET_F_MTU and may be read by the driver during feature > > > > > > negotiation and after VIRTIO_NET_F_MTU has been successfully > > > > > > negotiated." > > > > > > > > Should we add a wording clarification to the spec? > > > > > > > I think so. > > > > > > Some clarification would be needed for each field that depends on a > > > > > > feature; that would be quite verbose. Maybe we can get away with a > > > > > > clarifying statement? > > > > > > > > > > > > "Some config space fields may depend on a certain feature. In that > > > > > > case, the field exits if the device has offered the corresponding > > > > > > feature, > > > > > So this implies for !VIRTIO_NET_F_MQ && VIRTIO_NET_F_MTU, the config > > > > > will look like: > > > > > > > > > > struct virtio_net_config { > > > > >         u8 mac[6]; > > > > >         le16 status; > > > > >         le16 mtu; > > > > > }; > > > > > > > > > I agree. > > > > > > So consider it's probably too late to fix the driver which assumes some > > > field are always persent, it looks to me need fix the spec do declare the > > > fields are always existing instead. > > > > > > > > > > > > and may be read by the driver during feature negotiation, and > > > > > > accessed by the driver after the feature has been successfully > > > > > > negotiated. A shorthand for this is a statement that a field only > > > > > > exists if a certain feature bit is set." > > > > > I'm not sure using "shorthand" is good for the spec, at least we can > > > > > limit the its scope only to the configuration space part. > > > > Maybe "a shorthand expression"? > > > > > > So the questions is should we use this for all over the spec or it will be > > > only used in this speicifc part (device configuration). > > > > > > Thanks > > > 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 A12ADC433DB for ; Sun, 28 Feb 2021 21:25:26 +0000 (UTC) Received: from smtp2.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 190BE601FF for ; Sun, 28 Feb 2021 21:25:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 190BE601FF 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 smtp2.osuosl.org (Postfix) with ESMTP id BF0CB4300D; Sun, 28 Feb 2021 21:25:25 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 5Ifg5wA0VURV; Sun, 28 Feb 2021 21:25:24 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp2.osuosl.org (Postfix) with ESMTP id 221134301B; Sun, 28 Feb 2021 21:25:24 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 0095CC000A; Sun, 28 Feb 2021 21:25:23 +0000 (UTC) Received: from smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 67602C0001 for ; Sun, 28 Feb 2021 21:25:22 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 4E88160659 for ; Sun, 28 Feb 2021 21:25:22 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Authentication-Results: smtp3.osuosl.org (amavisd-new); dkim=pass (1024-bit key) header.d=redhat.com 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 EhMZKDYKuFnm for ; Sun, 28 Feb 2021 21:25:21 +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 BD7BE60657 for ; Sun, 28 Feb 2021 21:25:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1614547518; 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=3yMfUwMSMlLzTtYBioGpjaiH9NykZNmNU2hz5aZaT5Q=; b=Z8SekN/YB+8RH7h/PwsyaX890VyBxQPySf5BvG68OTE1XMmN6Uw/YhhKhj6PUGZNi3EeBu 290V2OceBBqmQrqn5rwqzt4r9q6bCXLXOc+RcKJ1mRd7Cwem6Ec+R4bzilomgBl76zOI67 Z8iYeT8yD2sSZiPmyUCy/yi5iM6mohk= Received: from mail-ed1-f70.google.com (mail-ed1-f70.google.com [209.85.208.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-456-TAkh5OaRNEqh3KVdLLLFQw-1; Sun, 28 Feb 2021 16:25:16 -0500 X-MC-Unique: TAkh5OaRNEqh3KVdLLLFQw-1 Received: by mail-ed1-f70.google.com with SMTP id f11so7834716edk.13 for ; Sun, 28 Feb 2021 13:25:16 -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=3yMfUwMSMlLzTtYBioGpjaiH9NykZNmNU2hz5aZaT5Q=; b=d16V9p2LJVuB41O8Qmc9Wd/M5c/r0foGiaJ+q/Jh2RWXsBtmjkAppysqqKqmO5A6kT ux0furfv+wVj1bEbmOUP04lxkO8kq/X2efEHb4AZgHfbE49636Xu+tkJIEFdmz9bWPE2 R++e0jB5GY9NvzDHlPP53q//3yMLQiWRMHFcjfeFAjEJSCHbJmugtRF0t/dA+itKSrQI vo39PBbiztES3OnBzbAPgZ7aRfY0/UXyaXRZ740ay9AsaraF6x17g/vLO6GMw10ARPK7 UyhdOXGhI4IIZ3j+t0lVe4JDrXQvOAX1ZWQ0R7lalGmm86z7lfwmZ+qMS56apDQgAwG0 fSxQ== X-Gm-Message-State: AOAM533T7fSQLwKT0QN/sfmdfQavUABJ+VoasuiMO8V1V9luNU7chQJM 4wvnW4Kzo4kkeGyEGDFAlt0G5jFrCV5aXXDNCuOENROCPpJ6IPOm+0FQe55YQIvRXfActTYXpq+ W9e0hOoPfEOWypnhWDv9AnYhGRBQMjUWLA2/Sv1F+Cg== X-Received: by 2002:aa7:c403:: with SMTP id j3mr13466244edq.137.1614547515721; Sun, 28 Feb 2021 13:25:15 -0800 (PST) X-Google-Smtp-Source: ABdhPJwRcQQYvlSJ36Xy6Hj8V4Z8794sGOmtN+bAL8Csgl0n1MU5SKaQ7rs0ucHXpwyqFI/lJoxhzw== X-Received: by 2002:aa7:c403:: with SMTP id j3mr13466237edq.137.1614547515530; Sun, 28 Feb 2021 13:25:15 -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 a14sm12258393edu.13.2021.02.28.13.25.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 28 Feb 2021 13:25:14 -0800 (PST) Date: Sun, 28 Feb 2021 16:25:11 -0500 From: "Michael S. Tsirkin" To: Jason Wang Subject: Re: [virtio-dev] Re: [PATCH] vdpa/mlx5: set_features should allow reset to zero Message-ID: <20210228162306-mutt-send-email-mst@kernel.org> References: <20210223041740-mutt-send-email-mst@kernel.org> <788a0880-0a68-20b7-5bdf-f8150b08276a@redhat.com> <20210223110430.2f098bc0.cohuck@redhat.com> <20210223115833.732d809c.cohuck@redhat.com> <8355f9b3-4cda-cd2e-98df-fed020193008@redhat.com> <20210224121234.0127ae4b.cohuck@redhat.com> <20210225135229-mutt-send-email-mst@kernel.org> <0f8eb381-cc98-9e05-0e35-ccdb1cbd6119@redhat.com> MIME-Version: 1.0 In-Reply-To: <0f8eb381-cc98-9e05-0e35-ccdb1cbd6119@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, Cornelia Huck , 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" T24gRnJpLCBGZWIgMjYsIDIwMjEgYXQgMDQ6MTk6MTZQTSArMDgwMCwgSmFzb24gV2FuZyB3cm90 ZToKPiAKPiBPbiAyMDIxLzIvMjYgMjo1MyDkuIrljYgsIE1pY2hhZWwgUy4gVHNpcmtpbiB3cm90 ZToKPiA+IE9uIFRodSwgRmViIDI1LCAyMDIxIGF0IDEyOjM2OjA3UE0gKzA4MDAsIEphc29uIFdh bmcgd3JvdGU6Cj4gPiA+IE9uIDIwMjEvMi8yNCA3OjEyIOS4i+WNiCwgQ29ybmVsaWEgSHVjayB3 cm90ZToKPiA+ID4gPiBPbiBXZWQsIDI0IEZlYiAyMDIxIDE3OjI5OjA3ICswODAwCj4gPiA+ID4g SmFzb24gV2FuZyA8amFzb3dhbmdAcmVkaGF0LmNvbT4gd3JvdGU6Cj4gPiA+ID4gCj4gPiA+ID4g PiBPbiAyMDIxLzIvMjMgNjo1OCDkuIvljYgsIENvcm5lbGlhIEh1Y2sgd3JvdGU6Cj4gPiA+ID4g PiA+IE9uIFR1ZSwgMjMgRmViIDIwMjEgMTg6MzE6MDcgKzA4MDAKPiA+ID4gPiA+ID4gSmFzb24g V2FuZyA8amFzb3dhbmdAcmVkaGF0LmNvbT4gd3JvdGU6Cj4gPiA+ID4gPiA+ID4gT24gMjAyMS8y LzIzIDY6MDQg5LiL5Y2ILCBDb3JuZWxpYSBIdWNrIHdyb3RlOgo+ID4gPiA+ID4gPiA+ID4gT24g VHVlLCAyMyBGZWIgMjAyMSAxNzo0NjoyMCArMDgwMAo+ID4gPiA+ID4gPiA+ID4gSmFzb24gV2Fu ZyA8amFzb3dhbmdAcmVkaGF0LmNvbT4gd3JvdGU6Cj4gPiA+ID4gPiA+ID4gPiA+IE9uIDIwMjEv Mi8yMyDkuIvljYg1OjI1LCBNaWNoYWVsIFMuIFRzaXJraW4gd3JvdGU6Cj4gPiA+ID4gPiA+ID4g PiA+ID4gT24gTW9uLCBGZWIgMjIsIDIwMjEgYXQgMDk6MDk6MjhBTSAtMDgwMCwgU2ktV2VpIExp dSB3cm90ZToKPiA+ID4gPiA+ID4gPiA+ID4gPiA+IE9uIDIvMjEvMjAyMSA4OjE0IFBNLCBKYXNv biBXYW5nIHdyb3RlOgo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiBPbiAyMDIxLzIvMTkgNzo1NCDk uIvljYgsIFNpLVdlaSBMaXUgd3JvdGU6Cj4gPiA+ID4gPiA+ID4gPiA+ID4gPiA+ID4gQ29tbWl0 IDQ1MjYzOWE2NGFkOCAoInZkcGE6IG1ha2Ugc3VyZSBzZXRfZmVhdHVyZXMgaXMgaW52b2tlZAo+ ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiA+IGZvciBsZWdhY3kiKSBtYWRlIGFuIGV4Y2VwdGlvbiBm b3IgbGVnYWN5IGd1ZXN0cyB0byByZXNldAo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiA+IGZlYXR1 cmVzIHRvIDAsIHdoZW4gY29uZmlnIHNwYWNlIGlzIGFjY2Vzc2VkIGJlZm9yZSBmZWF0dXJlcwo+ ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiA+IGFyZSBzZXQuIFdlIHNob3VsZCByZWxpZXZlIHRoZSB2 ZXJpZnlfbWluX2ZlYXR1cmVzKCkgY2hlY2sKPiA+ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiBhbmQg YWxsb3cgZmVhdHVyZXMgcmVzZXQgdG8gMCBmb3IgdGhpcyBjYXNlLgo+ID4gPiA+ID4gPiA+ID4g PiA+ID4gPiA+IAo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiA+IEl0J3Mgd29ydGggbm90aW5nIHRo YXQgbm90IGp1c3QgbGVnYWN5IGd1ZXN0cyBjb3VsZCBhY2Nlc3MKPiA+ID4gPiA+ID4gPiA+ID4g PiA+ID4gPiBjb25maWcgc3BhY2UgYmVmb3JlIGZlYXR1cmVzIGFyZSBzZXQuIEZvciBpbnN0YW5j ZSwgd2hlbgo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiA+IGZlYXR1cmUgVklSVElPX05FVF9GX01U VSBpcyBhZHZlcnRpc2VkIHNvbWUgbW9kZXJuIGRyaXZlcgo+ID4gPiA+ID4gPiA+ID4gPiA+ID4g PiA+IHdpbGwgdHJ5IHRvIGFjY2VzcyBhbmQgdmFsaWRhdGUgdGhlIE1UVSBwcmVzZW50IGluIHRo ZSBjb25maWcKPiA+ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiBzcGFjZSBiZWZvcmUgdmlydGlvIGZl YXR1cmVzIGFyZSBzZXQuCj4gPiA+ID4gPiA+ID4gPiA+ID4gPiA+IFRoaXMgbG9va3MgbGlrZSBh IHNwZWMgdmlvbGF0aW9uOgo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiAKPiA+ID4gPiA+ID4gPiA+ ID4gPiA+ID4gIgo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gPiAKPiA+ID4gPiA+ID4gPiA+ID4gPiA+ ID4gVGhlIGZvbGxvd2luZyBkcml2ZXItcmVhZC1vbmx5IGZpZWxkLCBtdHUgb25seSBleGlzdHMg aWYKPiA+ID4gPiA+ID4gPiA+ID4gPiA+ID4gVklSVElPX05FVF9GX01UVSBpcyBzZXQuIFRoaXMg ZmllbGQgc3BlY2lmaWVzIHRoZSBtYXhpbXVtIE1UVSBmb3IgdGhlCj4gPiA+ID4gPiA+ID4gPiA+ ID4gPiA+IGRyaXZlciB0byB1c2UuCj4gPiA+ID4gPiA+ID4gPiA+ID4gPiA+ICIKPiA+ID4gPiA+ ID4gPiA+ID4gPiA+ID4gCj4gPiA+ID4gPiA+ID4gPiA+ID4gPiA+IERvIHdlIHJlYWxseSB3YW50 IHRvIHdvcmthcm91bmQgdGhpcz8KPiA+ID4gPiA+ID4gPiA+ID4gPiA+IElzbid0IHRoZSBjb21t aXQgNDUyNjM5YTY0YWQ4IGl0c2VsZiBpcyBhIHdvcmthcm91bmQgZm9yIGxlZ2FjeSBndWVzdD8K PiA+ID4gPiA+ID4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiA+ID4gPiA+ID4gSSB0aGluayB0aGUg cG9pbnQgaXMsIHNpbmNlIHRoZXJlJ3MgbGVnYWN5IGd1ZXN0IHdlJ2QgaGF2ZSB0byBzdXBwb3J0 LCB0aGlzCj4gPiA+ID4gPiA+ID4gPiA+ID4gPiBob3N0IHNpZGUgd29ya2Fyb3VuZCBpcyB1bmF2 b2lkYWJsZS4gQWx0aG91Z2ggSSBhZ3JlZSB0aGUgdmlvbGF0aW5nIGRyaXZlcgo+ID4gPiA+ID4g PiA+ID4gPiA+ID4gc2hvdWxkIGJlIGZpeGVkICh5ZXMsIGl0J3MgaW4gdG9kYXkncyB1cHN0cmVh bSBrZXJuZWwgd2hpY2ggZXhpc3RzIGZvciBhCj4gPiA+ID4gPiA+ID4gPiA+ID4gPiB3aGlsZSBu b3cpLgo+ID4gPiA+ID4gPiA+ID4gPiA+IE9oICB5b3UgYXJlIHJpZ2h0Ogo+ID4gPiA+ID4gPiA+ ID4gPiA+IAo+ID4gPiA+ID4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiA+ID4gPiA+IHN0YXRpYyBp bnQgdmlydG5ldF92YWxpZGF0ZShzdHJ1Y3QgdmlydGlvX2RldmljZSAqdmRldikKPiA+ID4gPiA+ ID4gPiA+ID4gPiB7Cj4gPiA+ID4gPiA+ID4gPiA+ID4gICAgICAgICAgICAgIGlmICghdmRldi0+ Y29uZmlnLT5nZXQpIHsKPiA+ID4gPiA+ID4gPiA+ID4gPiAgICAgICAgICAgICAgICAgICAgICBk ZXZfZXJyKCZ2ZGV2LT5kZXYsICIlcyBmYWlsdXJlOiBjb25maWcgYWNjZXNzIGRpc2FibGVkXG4i LAo+ID4gPiA+ID4gPiA+ID4gPiA+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgX19mdW5j X18pOwo+ID4gPiA+ID4gPiA+ID4gPiA+ICAgICAgICAgICAgICAgICAgICAgIHJldHVybiAtRUlO VkFMOwo+ID4gPiA+ID4gPiA+ID4gPiA+ICAgICAgICAgICAgICB9Cj4gPiA+ID4gPiA+ID4gPiA+ ID4gCj4gPiA+ID4gPiA+ID4gPiA+ID4gICAgICAgICAgICAgIGlmICghdmlydG5ldF92YWxpZGF0 ZV9mZWF0dXJlcyh2ZGV2KSkKPiA+ID4gPiA+ID4gPiA+ID4gPiAgICAgICAgICAgICAgICAgICAg ICByZXR1cm4gLUVJTlZBTDsKPiA+ID4gPiA+ID4gPiA+ID4gPiAKPiA+ID4gPiA+ID4gPiA+ID4g PiAgICAgICAgICAgICAgaWYgKHZpcnRpb19oYXNfZmVhdHVyZSh2ZGV2LCBWSVJUSU9fTkVUX0Zf TVRVKSkgewo+ID4gPiA+ID4gPiA+ID4gPiA+ICAgICAgICAgICAgICAgICAgICAgIGludCBtdHUg PSB2aXJ0aW9fY3JlYWQxNih2ZGV2LAo+ID4gPiA+ID4gPiA+ID4gPiA+ICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBvZmZzZXRvZihzdHJ1Y3QgdmlydGlvX25l dF9jb25maWcsCj4gPiA+ID4gPiA+ID4gPiA+ID4gICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgIG10dSkpOwo+ID4gPiA+ID4gPiA+ID4gPiA+ICAg ICAgICAgICAgICAgICAgICAgIGlmIChtdHUgPCBNSU5fTVRVKQo+ID4gPiA+ID4gPiA+ID4gPiA+ ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgX192aXJ0aW9fY2xlYXJfYml0KHZkZXYsIFZJ UlRJT19ORVRfRl9NVFUpOwo+ID4gPiA+ID4gPiA+ID4gPiBJIHdvbmRlciB3aHkgbm90IHNpbXBs eSBmYWlsIGhlcmU/Cj4gPiA+ID4gPiA+ID4gPiBJIHRoaW5rIGJvdGggZmFpbGluZyBvciBub3Qg YWNjZXB0aW5nIHRoZSBmZWF0dXJlIGNhbiBiZSBhcmd1ZWQgdG8gbWFrZQo+ID4gPiA+ID4gPiA+ ID4gc2Vuc2U6ICJ0aGUgZGV2aWNlIHByZXNlbnRlZCB1cyB3aXRoIGEgbXR1IHNpemUgdGhhdCBk b2VzIG5vdCBtYWtlCj4gPiA+ID4gPiA+ID4gPiBzZW5zZSIgd291bGQgcG9pbnQgdG8gZmFpbGlu ZywgIndlIGNhbm5vdCB3b3JrIHdpdGggdGhlIG10dSBzaXplIHRoYXQKPiA+ID4gPiA+ID4gPiA+ IHRoZSBkZXZpY2UgcHJlc2VudGVkIHVzIiB3b3VsZCBwb2ludCB0byBub3QgbmVnb3RpYXRpbmcg dGhlIGZlYXR1cmUuCj4gPiA+ID4gPiA+ID4gPiA+ID4gICAgICAgICAgICAgIH0KPiA+ID4gPiA+ ID4gPiA+ID4gPiAKPiA+ID4gPiA+ID4gPiA+ID4gPiAgICAgICAgICAgICAgcmV0dXJuIDA7Cj4g PiA+ID4gPiA+ID4gPiA+ID4gfQo+ID4gPiA+ID4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiA+ID4g PiA+IEFuZCB0aGUgc3BlYyBzYXlzOgo+ID4gPiA+ID4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiA+ ID4gPiA+IAo+ID4gPiA+ID4gPiA+ID4gPiA+IFRoZSBkcml2ZXIgTVVTVCBmb2xsb3cgdGhpcyBz ZXF1ZW5jZSB0byBpbml0aWFsaXplIGEgZGV2aWNlOgo+ID4gPiA+ID4gPiA+ID4gPiA+IDEuIFJl c2V0IHRoZSBkZXZpY2UuCj4gPiA+ID4gPiA+ID4gPiA+ID4gMi4gU2V0IHRoZSBBQ0tOT1dMRURH RSBzdGF0dXMgYml0OiB0aGUgZ3Vlc3QgT1MgaGFzIG5vdGljZWQgdGhlIGRldmljZS4KPiA+ID4g PiA+ID4gPiA+ID4gPiAzLiBTZXQgdGhlIERSSVZFUiBzdGF0dXMgYml0OiB0aGUgZ3Vlc3QgT1Mg a25vd3MgaG93IHRvIGRyaXZlIHRoZSBkZXZpY2UuCj4gPiA+ID4gPiA+ID4gPiA+ID4gNC4gUmVh ZCBkZXZpY2UgZmVhdHVyZSBiaXRzLCBhbmQgd3JpdGUgdGhlIHN1YnNldCBvZiBmZWF0dXJlIGJp dHMgdW5kZXJzdG9vZCBieSB0aGUgT1MgYW5kIGRyaXZlciB0byB0aGUKPiA+ID4gPiA+ID4gPiA+ ID4gPiBkZXZpY2UuIER1cmluZyB0aGlzIHN0ZXAgdGhlIGRyaXZlciBNQVkgcmVhZCAoYnV0IE1V U1QgTk9UIHdyaXRlKSB0aGUgZGV2aWNlLXNwZWNpZmljIGNvbmZpZ3VyYXRpb24KPiA+ID4gPiA+ ID4gPiA+ID4gPiBmaWVsZHMgdG8gY2hlY2sgdGhhdCBpdCBjYW4gc3VwcG9ydCB0aGUgZGV2aWNl IGJlZm9yZSBhY2NlcHRpbmcgaXQuCj4gPiA+ID4gPiA+ID4gPiA+ID4gNS4gU2V0IHRoZSBGRUFU VVJFU19PSyBzdGF0dXMgYml0LiBUaGUgZHJpdmVyIE1VU1QgTk9UIGFjY2VwdCBuZXcgZmVhdHVy ZSBiaXRzIGFmdGVyIHRoaXMgc3RlcC4KPiA+ID4gPiA+ID4gPiA+ID4gPiA2LiBSZS1yZWFkIGRl dmljZSBzdGF0dXMgdG8gZW5zdXJlIHRoZSBGRUFUVVJFU19PSyBiaXQgaXMgc3RpbGwgc2V0OiBv dGhlcndpc2UsIHRoZSBkZXZpY2UgZG9lcyBub3QKPiA+ID4gPiA+ID4gPiA+ID4gPiBzdXBwb3J0 IG91ciBzdWJzZXQgb2YgZmVhdHVyZXMgYW5kIHRoZSBkZXZpY2UgaXMgdW51c2FibGUuCj4gPiA+ ID4gPiA+ID4gPiA+ID4gNy4gUGVyZm9ybSBkZXZpY2Utc3BlY2lmaWMgc2V0dXAsIGluY2x1ZGlu ZyBkaXNjb3Zlcnkgb2YgdmlydHF1ZXVlcyBmb3IgdGhlIGRldmljZSwgb3B0aW9uYWwgcGVyLWJ1 cyBzZXR1cCwKPiA+ID4gPiA+ID4gPiA+ID4gPiByZWFkaW5nIGFuZCBwb3NzaWJseSB3cml0aW5n IHRoZSBkZXZpY2XigJlzIHZpcnRpbyBjb25maWd1cmF0aW9uIHNwYWNlLCBhbmQgcG9wdWxhdGlv biBvZiB2aXJ0cXVldWVzLgo+ID4gPiA+ID4gPiA+ID4gPiA+IDguIFNldCB0aGUgRFJJVkVSX09L IHN0YXR1cyBiaXQuIEF0IHRoaXMgcG9pbnQgdGhlIGRldmljZSBpcyDigJxsaXZl4oCdLgo+ID4g PiA+ID4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiA+ID4gPiA+ IEl0ZW0gNCBvbiB0aGUgbGlzdCBleHBsaWNpdGx5IGFsbG93cyByZWFkaW5nIGNvbmZpZyBzcGFj ZSBiZWZvcmUKPiA+ID4gPiA+ID4gPiA+ID4gPiBGRUFUVVJFU19PSy4KPiA+ID4gPiA+ID4gPiA+ ID4gPiAKPiA+ID4gPiA+ID4gPiA+ID4gPiBJIGNvbmNsdWRlIHRoYXQgVklSVElPX05FVF9GX01U VSBpcyBzZXQgbWVhbnMgInNldCBpbiBkZXZpY2UgZmVhdHVyZXMiLgo+ID4gPiA+ID4gPiA+ID4g PiBTbyB0aGlzIHByb2JhYmx5IG5lZWQgc29tZSBjbGFyaWZpY2F0aW9uLiAiaXMgc2V0IiBpcyB1 c2VkIG1hbnkgdGltZXMgaW4KPiA+ID4gPiA+ID4gPiA+ID4gdGhlIHNwZWMgdGhhdCBoYXMgZGlm ZmVyZW50IGltcGxpY2F0aW9ucy4KPiA+ID4gPiA+ID4gPiA+IEJlZm9yZSBGRUFUVVJFU19PSyBp cyBzZXQgYnkgdGhlIGRyaXZlciwgSSBndWVzcyBpdCBtZWFucyAidGhlIGRldmljZQo+ID4gPiA+ ID4gPiA+ID4gaGFzIG9mZmVyZWQgdGhlIGZlYXR1cmUiOwo+ID4gPiA+ID4gPiA+IEZvciBtZSB0 aGlzIHBhcnQgaXMgb2sgc2luY2UgaXQgY2xhcmlmeSB0aGF0IGl0J3MgdGhlIGRyaXZlciB0aGF0 IHNldAo+ID4gPiA+ID4gPiA+IHRoZSBiaXQuCj4gPiA+ID4gPiA+ID4gCj4gPiA+ID4gPiA+ID4g Cj4gPiA+ID4gPiA+ID4gPiBkdXJpbmcgbm9ybWFsIHVzYWdlLCBpdCBtZWFucyAidGhlIGZlYXR1 cmUKPiA+ID4gPiA+ID4gPiA+IGhhcyBiZWVuIG5lZ290aWF0ZWQiLgo+ID4gPiA+ID4gPiA+IC8/ Cj4gPiA+ID4gPiA+ID4gCj4gPiA+ID4gPiA+ID4gSXQgbG9va3MgdG8gbWUgdGhlIGZlYXR1cmUg bmVnb3RpYXRpb24gaXMgZG9uZSBvbmx5IGFmdGVyIGRldmljZSBzZXQKPiA+ID4gPiA+ID4gPiBG RUFUVVJFU19PSywgb3IgRkVBVFVSRVNfT0sgY291bGQgYmUgcmVhZCBmcm9tIGRldmljZSBzdGF0 dXM/Cj4gPiA+ID4gPiA+IEknZCBjb25zaWRlciBmZWF0dXJlIG5lZ290aWF0aW9uIGRvbmUgd2hl biB0aGUgZHJpdmVyIHJlYWRzIEZFQVRVUkVTX09LCj4gPiA+ID4gPiA+IGJhY2sgZnJvbSB0aGUg c3RhdHVzLgo+ID4gPiA+ID4gSSBhZ3JlZS4KPiA+ID4gPiA+IAo+ID4gPiA+ID4gCj4gPiA+ID4g PiA+ID4gPiAgICAgIChUaGlzIGlzIGEgYml0IGZ1enp5IGZvciBsZWdhY3kgbW9kZS4pCj4gPiA+ ID4gPiA+IC4uLmJlY2F1c2UgbGVnYWN5IGRvZXMgbm90IGhhdmUgRkVBVFVSRVNfT0suCj4gPiA+ ID4gPiA+ID4gVGhlIHByb2JsZW0gaXMgdGhlIE1UVSBkZXNjcmlwdGlvbiBmb3IgZXhhbXBsZToK PiA+ID4gPiA+ID4gPiAKPiA+ID4gPiA+ID4gPiAiVGhlIGZvbGxvd2luZyBkcml2ZXItcmVhZC1v bmx5IGZpZWxkLCBtdHUgb25seSBleGlzdHMgaWYKPiA+ID4gPiA+ID4gPiBWSVJUSU9fTkVUX0Zf TVRVIGlzIHNldC4iCj4gPiA+ID4gPiA+ID4gCj4gPiA+ID4gPiA+ID4gSXQgbG9va3MgdG8gbWUg bmVlZCB0byB1c2UgImlmIFZJUlRJT19ORVRfRl9NVFUgaXMgc2V0IGJ5IGRldmljZSIuCj4gPiA+ ID4gPiA+ICJvZmZlcmVkIGJ5IHRoZSBkZXZpY2UiPyBJIGRvbid0IHRoaW5rIGl0IHNob3VsZCAn ZGlzYXBwZWFyJyBmcm9tIHRoZQo+ID4gPiA+ID4gPiBjb25maWcgc3BhY2UgaWYgdGhlIGRyaXZl ciB3b24ndCB1c2UgaXQuIChTYW1lIGZvciBvdGhlciBjb25maWcgc3BhY2UKPiA+ID4gPiA+ID4g ZmllbGRzIHRoYXQgYXJlIHRpZWQgdG8gZmVhdHVyZSBiaXRzLikKPiA+ID4gPiA+IEJ1dCB3aGF0 IGhhcHBlbnMgaWYgZS5nIGRldmljZSBkb2Vzbid0IG9mZmVyIFZJUlRJT19ORVRfRl9NVFU/IEl0 IGxvb2tzCj4gPiA+ID4gPiB0byBhY2NvcmRpbmcgdG8gdGhlIHNwZWMgdGhlcmUgd2lsbCBiZSBu byBtdHUgZmllbGQuCj4gPiA+ID4gSSB0aGluayBzbywgeWVzLgo+ID4gPiA+IAo+ID4gPiA+ID4g QW5kIGEgbW9yZSBpbnRlcmVzdGluZyBjYXNlIGlzIFZJUlRJT19ORVRfRl9NUSBpcyBub3Qgb2Zm ZXJlZCBidXQKPiA+ID4gPiA+IFZJUlRJT19ORVRfRl9NVFUgb2ZmZXJlZC4gVG8gbWUsIGl0IG1l YW5zIHdlIGRvbid0IGhhdmUKPiA+ID4gPiA+IG1heF92aXJ0cXVldWVfcGFpcnMgYnV0IGl0J3Mg bm90IGhvdyB0aGUgZHJpdmVyIGlzIHdyb3RlIHRvZGF5Lgo+ID4gPiA+IFRoYXQgd291bGQgYmUg YSBidWcsIGJ1dCBpdCBzZWVtcyB0byBtZSB0aGF0IHRoZSB2aXJ0aW8tbmV0IGRyaXZlcgo+ID4g PiA+IHJlYWRzIG1heF92aXJ0cXVldWVfcGFpcnMgY29uZGl0aW9uYWxseSBhbmQgaGFuZGxlcyBh YnNlbmNlIG9mIHRoZQo+ID4gPiA+IGZlYXR1cmUgY29ycmVjdGx5Pwo+ID4gPiAKPiA+ID4gWWVz LCBzZWUgdGhlIGF2b3ZlIGNvZGVzOgo+ID4gPiAKPiA+ID4gIMKgwqDCoMKgwqDCoMKgIGlmICh2 aXJ0aW9faGFzX2ZlYXR1cmUodmRldiwgVklSVElPX05FVF9GX01UVSkpIHsKPiA+ID4gIMKgwqDC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCBpbnQgbXR1ID0gdmlydGlvX2NyZWFkMTYodmRldiwK PiA+ID4gIMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIG9mZnNldG9mKHN0cnVjdCB2aXJ0aW9fbmV0 X2NvbmZpZywKPiA+ID4gIMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg IG10dSkpOwo+ID4gPiAgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIGlmIChtdHUgPCBN SU5fTVRVKQo+ID4gPiAgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg wqDCoCBfX3ZpcnRpb19jbGVhcl9iaXQodmRldiwgVklSVElPX05FVF9GX01UVSk7Cj4gPiA+ICDC oMKgwqDCoMKgwqDCoCB9Cj4gPiA+IAo+ID4gPiBTbyBpdCdzIHByb2JhYmx5IHRvbyBsYXRlIHRv IGZpeCB0aGUgZHJpdmVyLgo+ID4gPiAKPiA+IENvbmZ1c2VkLiBXaGF0IGlzIHdyb25nIHdpdGgg dGhlIGFib3ZlPyBJdCBuZXZlciByZWFkcyB0aGUKPiA+IGZpZWxkIHVubGVzcyB0aGUgZmVhdHVy ZSBoYXMgYmVlbiBvZmZlcmVkIGJ5IGRldmljZS4KPiAKPiAKPiBTbyB0aGUgc3BlYyBzYWlkOgo+ IAo+ICIKPiAKPiBUaGUgZm9sbG93aW5nIGRyaXZlci1yZWFkLW9ubHkgZmllbGQsIG1heF92aXJ0 cXVldWVfcGFpcnMgb25seSBleGlzdHMgaWYKPiBWSVJUSU9fTkVUX0ZfTVEgaXMgc2V0Lgo+IAo+ ICIKPiAKPiBJZiBJIHJlYWQgdGhpcyBjb3JyZWN0bHksIHRoZXJlIHdpbGwgYmUgbm8gbWF4X3Zp cnRxdWV1ZV9wYWlycyBmaWVsZCBpZiB0aGUKPiBWSVJUSU9fTkVUX0ZfTVEgaXMgbm90IG9mZmVy ZWQgYnkgZGV2aWNlPyBJZiB5ZXMgdGhlIG9mZnNldG9mKCkgdmlvbGF0ZXMKPiB3aGF0IHNwZWMg c2FpZC4KPiAKPiBUaGFua3MKCkkgdGhpbmsgdGhhdCdzIGEgbWlzdW5kZXJzdGFuZGluZy4gVGhp cyB0ZXh0IHdhcyBuZXZlciBpbnRlbmRlZCB0bwppbXBseSB0aGF0IGZpZWxkIG9mZnNldHMgY2hh bmdlIGJlYXNlZCBvbiBmZWF0dXJlIGJpdHMuCldlIGhhZCB0aGlzIHBhaW4gd2l0aCBsZWdhY3kg YW5kIHdlIG5ldmVyIHdhbnRlZCB0byBnbyBiYWNrIHRoZXJlLgoKVGhpcyBtZXJlbHkgaW1wbGll cyB0aGF0IHdpdGhvdXQgVklSVElPX05FVF9GX01RIHRoZSBmaWVsZApzaG91bGQgbm90IGJlIGFj Y2Vzc2VkLiBFeGlzdHMgaW4gdGhlIHNlbnNlICJpcyBhY2Nlc3NpYmxlIHRvIGRyaXZlciIuCgpM ZXQncyBqdXN0IGNsYXJpZnkgdGhhdCBpbiB0aGUgc3BlYywgam9iIGRvbmUuCgoKCgo+IAo+ID4g Cj4gPiAKPiA+ID4gPiA+ID4gPiBPdGhlcndpc2UgcmVhZGVycyAoYXQgbGVhc3QgZm9yIG1lKSwg bWF5IHRoaW5rIHRoZSBNVFUgaXMgb25seSB2YWxpZAo+ID4gPiA+ID4gPiA+IGlmIGRyaXZlciBz ZXQgdGhlIGJpdC4KPiA+ID4gPiA+ID4gSSB0aGluayBpdCB3b3VsZCBzdGlsbCBiZSAndmFsaWQn IGluIHRoZSBzZW5zZSB0aGF0IGl0IGV4aXN0cyBhbmQgaGFzCj4gPiA+ID4gPiA+IHNvbWUgdmFs dWUgaW4gdGhlcmUgZmlsbGVkIGluIGJ5IHRoZSBkZXZpY2UsIGJ1dCBhIGRyaXZlciByZWFkaW5n IGl0Cj4gPiA+ID4gPiA+IHdpdGhvdXQgbmVnb3RpYXRpbmcgdGhlIGZlYXR1cmUgd291bGQgYmUg YnVnZ3kuIChMaWtlIGluIHRoZSBrZXJuZWwKPiA+ID4gPiA+ID4gY29kZSBhYm92ZTsgdGhlIGtl cm5lbCBub3QgbGlraW5nIHRoZSB2YWx1ZSBkb2VzIG5vdCBtYWtlIHRoZSBmaWVsZAo+ID4gPiA+ ID4gPiBpbnZhbGlkLikKPiA+ID4gPiA+IFNlZSBNaWNoYWVsJ3MgcmVwbHksIHRoZSBzcGVjIGFs bG93cyByZWFkIHRoZSBjb25maWcgYmVmb3JlIHNldHRpbmcKPiA+ID4gPiA+IGZlYXR1cmVzLgo+ ID4gPiA+IFllcywgdGhlIHBlcmlvZCBwcmlvciB0byBmaW5pc2hpbmcgbmVnb3RpYXRpb24gaXMg b2J2aW91c2x5IHNwZWNpYWwuCj4gPiA+ID4gCj4gPiA+ID4gPiA+IE1heWJlIGEgc3RhdGVtZW50 IGNvdmVyaW5nIGV2ZXJ5dGhpbmcgd291bGQgYmU6Cj4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiAi VGhlIGZvbGxvd2luZyBkcml2ZXItcmVhZC1vbmx5IGZpZWxkIG10dSBvbmx5IGV4aXN0cyBpZiB0 aGUgZGV2aWNlCj4gPiA+ID4gPiA+IG9mZmVycyBWSVJUSU9fTkVUX0ZfTVRVIGFuZCBtYXkgYmUg cmVhZCBieSB0aGUgZHJpdmVyIGR1cmluZyBmZWF0dXJlCj4gPiA+ID4gPiA+IG5lZ290aWF0aW9u IGFuZCBhZnRlciBWSVJUSU9fTkVUX0ZfTVRVIGhhcyBiZWVuIHN1Y2Nlc3NmdWxseQo+ID4gPiA+ ID4gPiBuZWdvdGlhdGVkLiIKPiA+ID4gPiA+ID4gPiA+IFNob3VsZCB3ZSBhZGQgYSB3b3JkaW5n IGNsYXJpZmljYXRpb24gdG8gdGhlIHNwZWM/Cj4gPiA+ID4gPiA+ID4gSSB0aGluayBzby4KPiA+ ID4gPiA+ID4gU29tZSBjbGFyaWZpY2F0aW9uIHdvdWxkIGJlIG5lZWRlZCBmb3IgZWFjaCBmaWVs ZCB0aGF0IGRlcGVuZHMgb24gYQo+ID4gPiA+ID4gPiBmZWF0dXJlOyB0aGF0IHdvdWxkIGJlIHF1 aXRlIHZlcmJvc2UuIE1heWJlIHdlIGNhbiBnZXQgYXdheSB3aXRoIGEKPiA+ID4gPiA+ID4gY2xh cmlmeWluZyBzdGF0ZW1lbnQ/Cj4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiAiU29tZSBjb25maWcg c3BhY2UgZmllbGRzIG1heSBkZXBlbmQgb24gYSBjZXJ0YWluIGZlYXR1cmUuIEluIHRoYXQKPiA+ ID4gPiA+ID4gY2FzZSwgdGhlIGZpZWxkIGV4aXRzIGlmIHRoZSBkZXZpY2UgaGFzIG9mZmVyZWQg dGhlIGNvcnJlc3BvbmRpbmcKPiA+ID4gPiA+ID4gZmVhdHVyZSwKPiA+ID4gPiA+IFNvIHRoaXMg aW1wbGllcyBmb3IgIVZJUlRJT19ORVRfRl9NUSAmJiBWSVJUSU9fTkVUX0ZfTVRVLCB0aGUgY29u ZmlnCj4gPiA+ID4gPiB3aWxsIGxvb2sgbGlrZToKPiA+ID4gPiA+IAo+ID4gPiA+ID4gc3RydWN0 IHZpcnRpb19uZXRfY29uZmlnIHsKPiA+ID4gPiA+ICAgIMKgwqDCoMKgwqDCoMKgIHU4IG1hY1s2 XTsKPiA+ID4gPiA+ICAgIMKgwqDCoMKgwqDCoMKgIGxlMTYgc3RhdHVzOwo+ID4gPiA+ID4gICAg wqDCoMKgwqDCoMKgwqAgbGUxNiBtdHU7Cj4gPiA+ID4gPiB9Owo+ID4gPiA+ID4gCj4gPiA+ID4g SSBhZ3JlZS4KPiA+ID4gCj4gPiA+IFNvIGNvbnNpZGVyIGl0J3MgcHJvYmFibHkgdG9vIGxhdGUg dG8gZml4IHRoZSBkcml2ZXIgd2hpY2ggYXNzdW1lcyBzb21lCj4gPiA+IGZpZWxkIGFyZSBhbHdh eXMgcGVyc2VudCwgaXQgbG9va3MgdG8gbWUgbmVlZCBmaXggdGhlIHNwZWMgZG8gZGVjbGFyZSB0 aGUKPiA+ID4gZmllbGRzIGFyZSBhbHdheXMgZXhpc3RpbmcgaW5zdGVhZC4KPiA+ID4gCj4gPiA+ IAo+ID4gPiA+ID4gPiAgICAgYW5kIG1heSBiZSByZWFkIGJ5IHRoZSBkcml2ZXIgZHVyaW5nIGZl YXR1cmUgbmVnb3RpYXRpb24sIGFuZAo+ID4gPiA+ID4gPiBhY2Nlc3NlZCBieSB0aGUgZHJpdmVy IGFmdGVyIHRoZSBmZWF0dXJlIGhhcyBiZWVuIHN1Y2Nlc3NmdWxseQo+ID4gPiA+ID4gPiBuZWdv dGlhdGVkLiBBIHNob3J0aGFuZCBmb3IgdGhpcyBpcyBhIHN0YXRlbWVudCB0aGF0IGEgZmllbGQg b25seQo+ID4gPiA+ID4gPiBleGlzdHMgaWYgYSBjZXJ0YWluIGZlYXR1cmUgYml0IGlzIHNldC4i Cj4gPiA+ID4gPiBJJ20gbm90IHN1cmUgdXNpbmcgInNob3J0aGFuZCIgaXMgZ29vZCBmb3IgdGhl IHNwZWMsIGF0IGxlYXN0IHdlIGNhbgo+ID4gPiA+ID4gbGltaXQgdGhlIGl0cyBzY29wZSBvbmx5 IHRvIHRoZSBjb25maWd1cmF0aW9uIHNwYWNlIHBhcnQuCj4gPiA+ID4gTWF5YmUgImEgc2hvcnRo YW5kIGV4cHJlc3Npb24iPwo+ID4gPiAKPiA+ID4gU28gdGhlIHF1ZXN0aW9ucyBpcyBzaG91bGQg d2UgdXNlIHRoaXMgZm9yIGFsbCBvdmVyIHRoZSBzcGVjIG9yIGl0IHdpbGwgYmUKPiA+ID4gb25s eSB1c2VkIGluIHRoaXMgc3BlaWNpZmMgcGFydCAoZGV2aWNlIGNvbmZpZ3VyYXRpb24pLgo+ID4g PiAKPiA+ID4gVGhhbmtzCj4gPiA+IAoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX18KVmlydHVhbGl6YXRpb24gbWFpbGluZyBsaXN0ClZpcnR1YWxpemF0aW9u QGxpc3RzLmxpbnV4LWZvdW5kYXRpb24ub3JnCmh0dHBzOi8vbGlzdHMubGludXhmb3VuZGF0aW9u Lm9yZy9tYWlsbWFuL2xpc3RpbmZvL3ZpcnR1YWxpemF0aW9u From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: virtio-dev-return-8062-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 A0F3F986302 for ; Sun, 28 Feb 2021 21:25:22 +0000 (UTC) Date: Sun, 28 Feb 2021 16:25:11 -0500 From: "Michael S. Tsirkin" Message-ID: <20210228162306-mutt-send-email-mst@kernel.org> References: <20210223041740-mutt-send-email-mst@kernel.org> <788a0880-0a68-20b7-5bdf-f8150b08276a@redhat.com> <20210223110430.2f098bc0.cohuck@redhat.com> <20210223115833.732d809c.cohuck@redhat.com> <8355f9b3-4cda-cd2e-98df-fed020193008@redhat.com> <20210224121234.0127ae4b.cohuck@redhat.com> <20210225135229-mutt-send-email-mst@kernel.org> <0f8eb381-cc98-9e05-0e35-ccdb1cbd6119@redhat.com> MIME-Version: 1.0 In-Reply-To: <0f8eb381-cc98-9e05-0e35-ccdb1cbd6119@redhat.com> Subject: Re: [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: Cornelia Huck , 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 Fri, Feb 26, 2021 at 04:19:16PM +0800, Jason Wang wrote: >=20 > On 2021/2/26 2:53 =E4=B8=8A=E5=8D=88, Michael S. Tsirkin wrote: > > On Thu, Feb 25, 2021 at 12:36:07PM +0800, Jason Wang wrote: > > > On 2021/2/24 7:12 =E4=B8=8B=E5=8D=88, Cornelia Huck wrote: > > > > On Wed, 24 Feb 2021 17:29:07 +0800 > > > > Jason Wang wrote: > > > >=20 > > > > > On 2021/2/23 6:58 =E4=B8=8B=E5=8D=88, Cornelia Huck wrote: > > > > > > On Tue, 23 Feb 2021 18:31:07 +0800 > > > > > > Jason Wang wrote: > > > > > > > On 2021/2/23 6:04 =E4=B8=8B=E5=8D=88, Cornelia Huck wrote: > > > > > > > > On Tue, 23 Feb 2021 17:46:20 +0800 > > > > > > > > Jason Wang wrote: > > > > > > > > > On 2021/2/23 =E4=B8=8B=E5=8D=885:25, Michael S. Tsirkin w= rote: > > > > > > > > > > On Mon, Feb 22, 2021 at 09:09:28AM -0800, Si-Wei Liu wr= ote: > > > > > > > > > > > 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 wr= ote: > > > > > > > > > > > > > Commit 452639a64ad8 ("vdpa: make sure set_feature= s is invoked > > > > > > > > > > > > > for legacy") made an exception for legacy guests = to reset > > > > > > > > > > > > > features to 0, when config space is accessed befo= re features > > > > > > > > > > > > > are set. We should relieve the verify_min_feature= s() check > > > > > > > > > > > > > and allow features reset to 0 for this case. > > > > > > > > > > > > >=20 > > > > > > > > > > > > > It's worth noting that not just legacy guests cou= ld access > > > > > > > > > > > > > config space before features are set. For instanc= e, when > > > > > > > > > > > > > feature VIRTIO_NET_F_MTU is advertised some moder= n driver > > > > > > > > > > > > > will try to access and validate the MTU present i= n the config > > > > > > > > > > > > > space before virtio features are set. > > > > > > > > > > > > This looks like a spec violation: > > > > > > > > > > > >=20 > > > > > > > > > > > > " > > > > > > > > > > > >=20 > > > > > > > > > > > > The following driver-read-only field, mtu only exis= ts if > > > > > > > > > > > > VIRTIO_NET_F_MTU is set. This field specifies the m= aximum MTU for the > > > > > > > > > > > > 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 driver > > > > > > > > > > > should be fixed (yes, it's in today's upstream kernel= which exists for 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: c= onfig access disabled\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_config, > > > > > > > > > > = mtu)); > > > > > > > > > > if (mtu < MIN_MTU) > > > > > > > > > > __virtio_clear_bit(vdev, V= IRTIO_NET_F_MTU); > > > > > > > > > I wonder why not simply fail here? > > > > > > > > I think both failing or not accepting the feature can be ar= gued to make > > > > > > > > sense: "the device presented us with a mtu size that does n= ot make > > > > > > > > sense" would point to failing, "we cannot work with the mtu= size that > > > > > > > > the device presented us" would point to not negotiating the= feature. > > > > > > > > > > } > > > > > > > > > >=20 > > > > > > > > > > return 0; > > > > > > > > > > } > > > > > > > > > >=20 > > > > > > > > > > And the spec says: > > > > > > > > > >=20 > > > > > > > > > >=20 > > > > > > > > > > The driver MUST follow this sequence to initialize a de= vice: > > > > > > > > > > 1. Reset the device. > > > > > > > > > > 2. Set the ACKNOWLEDGE status bit: the guest OS has not= iced 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 fe= ature 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 a= ccepting 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 unusab= le. > > > > > > > > > > 7. Perform device-specific setup, including discovery o= f virtqueues for the device, optional per-bus setup, > > > > > > > > > > reading and possibly writing the device=E2=80=99s virti= o configuration space, and population of virtqueues. > > > > > > > > > > 8. Set the DRIVER_OK status bit. At this point the devi= ce is =E2=80=9Clive=E2=80=9D. > > > > > > > > > >=20 > > > > > > > > > >=20 > > > > > > > > > > Item 4 on the list explicitly allows reading config spa= ce before > > > > > > > > > > FEATURES_OK. > > > > > > > > > >=20 > > > > > > > > > > I conclude that VIRTIO_NET_F_MTU is set means "set in d= evice features". > > > > > > > > > So this probably need some clarification. "is set" is use= d many times in > > > > > > > > > the spec that has different implications. > > > > > > > > Before FEATURES_OK is set by the driver, I guess it means "= the device > > > > > > > > has offered the feature"; > > > > > > > For me this part is ok since it clarify that it's the driver = that set > > > > > > > the bit. > > > > > > >=20 > > > > > > >=20 > > > > > > > > during normal usage, it means "the feature > > > > > > > > has been negotiated". > > > > > > > /? > > > > > > >=20 > > > > > > > It looks to me the feature negotiation is done only after dev= ice set > > > > > > > FEATURES_OK, or FEATURES_OK could be read from device status? > > > > > > I'd consider feature negotiation done when the driver reads FEA= TURES_OK > > > > > > back from the status. > > > > > I agree. > > > > >=20 > > > > >=20 > > > > > > > > (This is a bit fuzzy for legacy mode.) > > > > > > ...because legacy does not have FEATURES_OK. > > > > > > > The problem is the MTU description for example: > > > > > > >=20 > > > > > > > "The following driver-read-only field, mtu only exists if > > > > > > > VIRTIO_NET_F_MTU is set." > > > > > > >=20 > > > > > > > It looks to me need to use "if VIRTIO_NET_F_MTU is set by dev= ice". > > > > > > "offered by the device"? I don't think it should 'disappear' fr= om the > > > > > > config space if the driver won't use it. (Same for other config= space > > > > > > fields that are tied to feature bits.) > > > > > But what happens if e.g device doesn't offer VIRTIO_NET_F_MTU? It= looks > > > > > to according to the spec there will be no mtu field. > > > > I think so, yes. > > > >=20 > > > > > And a more interesting case is VIRTIO_NET_F_MQ is not offered but > > > > > VIRTIO_NET_F_MTU offered. To me, it means we don't have > > > > > max_virtqueue_pairs but it's not how the driver is wrote today. > > > > That would be a bug, but it seems to me that the virtio-net driver > > > > reads max_virtqueue_pairs conditionally and handles absence of the > > > > feature correctly? > > >=20 > > > Yes, see the avove codes: > > >=20 > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (virtio_has_feature(vd= ev, VIRTIO_NET_F_MTU)) { > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 int mtu =3D virtio_cread16(vdev, > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 offsetof(struct virtio_net_config, > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 mtu)); > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 if (mtu < MIN_MTU) > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 __= virtio_clear_bit(vdev, VIRTIO_NET_F_MTU); > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > > >=20 > > > So it's probably too late to fix the driver. > > >=20 > > Confused. What is wrong with the above? It never reads the > > field unless the feature has been offered by device. >=20 >=20 > So the spec said: >=20 > " >=20 > The following driver-read-only field, max_virtqueue_pairs only exists if > VIRTIO_NET_F_MQ is set. >=20 > " >=20 > If I read this correctly, there will be no max_virtqueue_pairs field if t= he > VIRTIO_NET_F_MQ is not offered by device? If yes the offsetof() violates > what spec said. >=20 > Thanks I think that's a misunderstanding. This text was never intended to imply that field offsets change beased on feature bits. We had this pain with legacy and we never wanted to go back there. This merely implies that without VIRTIO_NET_F_MQ the field should not be accessed. Exists in the sense "is accessible to driver". Let's just clarify that in the spec, job done. >=20 > >=20 > >=20 > > > > > > > Otherwise readers (at least for me), may think the MTU is onl= y valid > > > > > > > if driver set the bit. > > > > > > I think it would still be 'valid' in the sense that it exists a= nd has > > > > > > some value in there filled in by the device, but a driver readi= ng it > > > > > > without negotiating the feature would be buggy. (Like in the ke= rnel > > > > > > code above; the kernel not liking the value does not make the f= ield > > > > > > invalid.) > > > > > See Michael's reply, the spec allows read the config before setti= ng > > > > > features. > > > > Yes, the period prior to finishing negotiation is obviously special= . > > > >=20 > > > > > > Maybe a statement covering everything would be: > > > > > >=20 > > > > > > "The following driver-read-only field mtu only exists if the de= vice > > > > > > offers VIRTIO_NET_F_MTU and may be read by the driver during fe= ature > > > > > > negotiation and after VIRTIO_NET_F_MTU has been successfully > > > > > > negotiated." > > > > > > > > Should we add a wording clarification to the spec? > > > > > > > I think so. > > > > > > Some clarification would be needed for each field that depends = on a > > > > > > feature; that would be quite verbose. Maybe we can get away wit= h a > > > > > > clarifying statement? > > > > > >=20 > > > > > > "Some config space fields may depend on a certain feature. In t= hat > > > > > > case, the field exits if the device has offered the correspondi= ng > > > > > > feature, > > > > > So this implies for !VIRTIO_NET_F_MQ && VIRTIO_NET_F_MTU, the con= fig > > > > > will look like: > > > > >=20 > > > > > struct virtio_net_config { > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 u8 mac[6]; > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 le16 status; > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 le16 mtu; > > > > > }; > > > > >=20 > > > > I agree. > > >=20 > > > So consider it's probably too late to fix the driver which assumes so= me > > > field are always persent, it looks to me need fix the spec do declare= the > > > fields are always existing instead. > > >=20 > > >=20 > > > > > > and may be read by the driver during feature negotiation, a= nd > > > > > > accessed by the driver after the feature has been successfully > > > > > > negotiated. A shorthand for this is a statement that a field on= ly > > > > > > exists if a certain feature bit is set." > > > > > I'm not sure using "shorthand" is good for the spec, at least we = can > > > > > limit the its scope only to the configuration space part. > > > > Maybe "a shorthand expression"? > > >=20 > > > So the questions is should we use this for all over the spec or it wi= ll be > > > only used in this speicifc part (device configuration). > > >=20 > > > Thanks > > >=20 --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org